Re: [PATCH 1/1] sched/fair: Fix invalid pointer dereference in child_cfs_rq_on_list()

[Aboorva Devarajan]

On Wed, 2025-03-05 at 10:23 +0100, Dietmar Eggemann wrote:
> On 05/03/2025 09:21, Vincent Guittot wrote:
> > On Tue, 4 Mar 2025 at 18:00, Aboorva Devarajan <aboorvad@xxxxxxxxxxxxx> wrote:
> > > 
> > > In child_cfs_rq_on_list(), leaf_cfs_rq_list.prev is expected to point to
> > > a valid cfs_rq->leaf_cfs_rq_list in the hierarchy. However, when accessed
> > > from the first node in a list, leaf_cfs_rq_list.prev can incorrectly point
> > > back to the list head (rq->leaf_cfs_rq_list) instead of another
> > > cfs_rq->leaf_cfs_rq_list.
> > > 
> > > The function does not handle this case, leading to incorrect pointer
> > > calculations and unintended memory accesses, which can result in a kernel
> > > crash.
> > > 
> > > A recent attempt to reorder fields in struct rq exposed this issue by
> > > modifying memory offsets and affecting how pointer computations are
> > > resolved. While the problem existed before, it was previously masked by
> > > specific field arrangement. The reordering caused erroneous pointer
> > > accesses, leading to a NULL dereference and a crash, as seen in the
> 
> I'm running tip/sched/core on arm64 and I still only see the wrong
> pointer for 'prev_cfs_rq->tg->parent' in the 'prev ==
> &rq->leaf_cfs_rq_list' case?
> 
> ...
> cpu=5 prev_cfs_rq->tg=ffff00097efb63a0 parent=0000000000000010
> cfs_rq->tg=ffff000802084000
> ...

Hi Dietmar,

Thanks a lot for testing this,

I have sent an updated patch (v2) to fix this:
https://lore.kernel.org/all/20250305100854.318599-1-aboorvad@xxxxxxxxxxxxx/

Can you try and see if v2 works for you?

> 
> > > following trace:
> > > 
> > > [    2.152852] Call Trace:
> > > [    2.152855] __update_blocked_fair+0x45c/0x6a0 (unreliable)
> > > [    2.152862] sched_balance_update_blocked_averages+0x11c/0x24c
> > > [    2.152869] sched_balance_softirq+0x60/0x9c
> > > [    2.152876] handle_softirqs+0x148/0x3b4
> > > [    2.152884] do_softirq_own_stack+0x40/0x54
> > > [    2.152891] __irq_exit_rcu+0x18c/0x1b4
> > > [    2.152897] irq_exit+0x20/0x38
> > > [    2.152903] timer_interrupt+0x174/0x30c
> > > [    2.152910] decrementer_common_virt+0x28c/0x290
> > > [    2.059873] systemd[1]: Hostname set to ...
> > > [    2.152682] BUG: Unable to handle kernel data access on read at 0x100000125
> > > [    2.152717] Faulting instruction address: 0xc0000000001c0270
> > > [    2.152724] Oops: Kernel access of bad area, sig: 7 [#1]
> > > ..
> > > 
> > > To fix this, introduce a check to detect when prev points to the list head
> > > (&rq->leaf_cfs_rq_list). If this condition is met, return early to prevent
> > > the use of an invalid prev_cfs_rq.
> > > 
> > > Fixes: fdaba61ef8a2 ("sched/fair: Ensure that the CFS parent is added after unthrottling")
> > > Signed-off-by: Aboorva Devarajan <aboorvad@xxxxxxxxxxxxx>
> > > ---
> > >  kernel/sched/fair.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 1c0ef435a7aa..a4daa7a9af0b 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -4045,12 +4045,15 @@ static inline bool child_cfs_rq_on_list(struct cfs_rq *cfs_rq)
> > >  {
> > >         struct cfs_rq *prev_cfs_rq;
> > >         struct list_head *prev;
> > > +       struct rq *rq;
> > > +
> > > +       rq = rq_of(cfs_rq);
> > > 
> > >         if (cfs_rq->on_list) {
> > >                 prev = cfs_rq->leaf_cfs_rq_list.prev;
> > > +               if (prev == &rq->leaf_cfs_rq_list)
> > > +                       return false;
> > 
> > what about the else case below , prev can also point to rq->leaf_cfs_rq_list
> 
> Should be the same issue IMHO. I'm not seeing it on my machine  during
> startup or while doing simple taskgroup tests though, 'cfs_rq->on_list'
> is always 1 so far.
> 
> > >         } else {
> > > -               struct rq *rq = rq_of(cfs_rq);
> > > -
> > >                 prev = rq->tmp_alone_branch;
> > >         }
> > > 
> > > --
> > > 2.43.5
> > > 
> 

Regards,
Aboorva