Re: [PATCH] sched/fair: do not announce throttled next buddy in dequeue_task_fair

[Konstantin Khlebnikov]

On 11.07.2016 15:12, Xunlei Pang wrote:
> On 2016/07/11 at 17:54, Wanpeng Li wrote:
> > Hi Konstantin, Xunlei,
> > 2016-07-11 16:42 GMT+08:00 Xunlei Pang <xpang@xxxxxxxxxx>:
> > > On 2016/07/11 at 16:22, Xunlei Pang wrote:
> > > > On 2016/07/11 at 15:25, Wanpeng Li wrote:
> > > > > 2016-06-16 20:57 GMT+08:00 Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>:
> > > > > > Hierarchy could be already throttled at this point. Throttled next
> > > > > > buddy could trigger null pointer dereference in pick_next_task_fair().
> > > > > There is cfs_rq->next check in pick_next_entity(), so how can null
> > > > > pointer dereference happen?
> > > > I guess it's the following code leading to a NULL se returned:
> > > s/NULL/empty-entity cfs_rq se/
> > >
> > > > pick_next_entity():
> > > >      if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
> >              ^^^^^^^^^^^^^
> > I think this will return false.
>
> With the wrong throttled_hierarchy(), I think this can happen. But after we have the
> corrected throttled_hierarchy() patch, I can't see how it is possible.
>
> dequeue_task_fair():
>      if (task_sleep && parent_entity(se))
>          set_next_buddy(parent_entity(se));
>
> How does dequeue_task_fair() with DEQUEUE_SLEEP set(true task_sleep) happen to a throttled hierarchy?
> IOW, a task belongs to a throttled hierarchy is running?
>
> Maybe Konstantin knows the reason.

This function (dequeue_task_fair) check throttling but at point it could skip several
levels and announce as next buddy actually throttled entry.
Probably this bug hadn't happened but this's really hard to prove that this is impossible.
->set_curr_task(), PI-boost or some tricky migration in balancer could break this easily.

--
Konstantin