Re: [PATCH] Fix use-after-free of q->root_blkg and q->root_rl.blkg

["Jun'ichi Nomura" <j-nomura@ce.jp.nec.com>]

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