Messages in this thread | | | Date | Thu, 18 Oct 2012 11:56:34 +0900 | From | "Jun'ichi Nomura" <> | Subject | Re: [PATCH] Fix use-after-free of q->root_blkg and q->root_rl.blkg |
| |
On 10/17/12 22:47, Vivek Goyal wrote: > On Wed, Oct 17, 2012 at 09:02:22AM +0900, Jun'ichi Nomura wrote: >> On 10/17/12 08:20, Tejun Heo wrote: >>>>>> - if (ent == &q->root_blkg->q_node) >>>>>> + if (q->root_blkg && ent == &q->root_blkg->q_node) >>>>> >>>>> Can we fix it little differently. Little earlier in the code, we check for >>>>> if q->blkg_list is empty, then all the groups are gone, and there are >>>>> no more request lists hence and return NULL. >>>>> >>>>> Current code: >>>>> if (rl == &q->root_rl) { >>>>> ent = &q->blkg_list; >>>>> >>>>> Modified code: >>>>> if (rl == &q->root_rl) { >>>>> ent = &q->blkg_list; >>>>> /* There are no more block groups, hence no request lists */ >>>>> if (list_empty(ent)) >>>>> return NULL; >>>>> } >>> >>> Do we need this at all? q->root_blkg being NULL is completely fine >>> there and the comparison would work as expected, no? >> >> Hmm? >> >> If list_empty(ent) and q->root_blkg == NULL, >> >>> /* walk to the next list_head, skip root blkcg */ >>> ent = ent->next; >> >> ent is &q->blkg_list again. >> >>> if (ent == &q->root_blkg->q_node) >> >> So ent is not &q->root_blkg->q_node. > > If q->root_blkg is NULL, will it not lead to NULL pointer dereference. > (q->root_blkg->q_node).
It's not dereferenced.
>>> ent = ent->next; >>> if (ent == &q->blkg_list) >>> return NULL; >> >> And we return NULL here. >> >> Ah, yes. You are correct. >> We can do without the above hunk. > > I would rather prefer to check for this boundary condition early and > return instead of letting it fall through all these conditions and > then figure out yes we have no next rl. IMO, code becomes easier to > understand if nothing else. Otherwise one needs a step by step > explanation as above to show that case of q->root_blkg is covered.
I have same opinion as yours that it's good for readability.
-- Jun'ichi Nomura, NEC Corporation
| |