Re: [PATCH] sched: fix NULL pointer issue in pick_next_entity()

[Yafang Shao]

Hi Peter,

Thanks for your response!

> CFS, CFQ is a block IO scheduler.
Sorry for the mistake.  It should be CFS.

> And how would that happen? We only call pick_next_entity(.curr=NULL)
> when we _know_ cfs_rq->nr_running.

It crashed my machine when I did hadoop test, and after I made this
change it works now.
On SMP system, cfs_rq->nr_running isn't protected well, although we
_know_ cfs_rq->nr_running,
but it is modified by other thread running on other CPU and the
sched_entity is set NULL as well.
Then this thread broken here as accessed the NULL pointer here.

> You diff is broken, this is very much not the clear_buddies() function.
You're right. it is in pick_next_entity().

2017-08-01 16:38 GMT+08:00 Peter Zijlstra <peterz@xxxxxxxxxxxxx>:
> On Tue, Aug 01, 2017 at 06:01:56PM +0800, Yafang Shao wrote:
>> When we select CFQ as the scheduler, in function pick_next_task_fair
>
> CFS, CFQ is a block IO scheduler.
>
>> it will pass NULL as the 2nd argument to pick_next_entity:
>>     pick_next_entity(cfs_rq, NULL);
>>
>> And once __pick_first_entity() is called, it could return NULL as well.
>>
>> So in function pick_next_entity(), the local variable 'left' and 'curr'
>> could both be NULL, then this will cause NULL pointer issue.
>>
>> In order to fix this issue, we just need return NULL under the condition
>> that both 'left' and 'curr' are NULL, meaning that no entity available.
>
> And how would that happen? We only call pick_next_entity(.curr=NULL)
> when we _know_ cfs_rq->nr_running.
>
>> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
>> ---
>>  kernel/sched/fair.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index c95880e..e64c359 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -3903,6 +3903,8 @@ static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se)
>
> You diff is broken, this is very much not the clear_buddies() function.
>
>>       struct sched_entity *left = __pick_first_entity(cfs_rq);
>>       struct sched_entity *se;
>>
>> +     if (!left && !curr)
>> +             return NULL;
>>       /*
>>        * If curr is set we have to see if its left of the leftmost entity
>>        * still in the tree, provided there was anything in the tree at all.
>> --
>> 1.8.3.1
>>