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

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

[Aboorva Devarajan]

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
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@linux.ibm.com>
---
 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;
 	} else {
-		struct rq *rq = rq_of(cfs_rq);
-
 		prev = rq->tmp_alone_branch;
 	}
 
-- 
2.43.5

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

[Vincent Guittot]

On Tue, 4 Mar 2025 at 18:00, Aboorva Devarajan <aboorvad@linux.ibm.com> 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
> 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@linux.ibm.com>
> ---
>  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

>         } else {
> -               struct rq *rq = rq_of(cfs_rq);
> -
>                 prev = rq->tmp_alone_branch;
>         }
>
> --
> 2.43.5
>

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

[Dietmar Eggemann]

On 05/03/2025 09:21, Vincent Guittot wrote:
> On Tue, 4 Mar 2025 at 18:00, Aboorva Devarajan <aboorvad@linux.ibm.com> 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
...

>> 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@linux.ibm.com>
>> ---
>>  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
>>

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

[Aboorva Devarajan]

On Wed, 2025-03-05 at 09:21 +0100, Vincent Guittot wrote:
> On Tue, 4 Mar 2025 at 18:00, Aboorva Devarajan <aboorvad@linux.ibm.com> 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
> > 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@linux.ibm.com>
> > ---
> >  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

Hi Vincent,

Thanks for the comments, yes indeed `rq->tmp_alone_branch` canpoint to `rq->leaf_cfs_rq_list`

I overlooked this and assumed that as long as we have at least one cfs_rq, tmp_alone_branch
would always point to a valid list node (cfs_rq->leaf_cfs_rq_list).

I will send a v2 with the changes to carry out the correctness check unconditionally.


> >         } else {
> > -               struct rq *rq = rq_of(cfs_rq);
> > -
> >                 prev = rq->tmp_alone_branch;
> >         }
> > 
> > --
> > 2.43.5
> > 

- Aboorva

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@linux.ibm.com> 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@linux.ibm.com/

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@linux.ibm.com>
> > > ---
> > >  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

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@linux.ibm.com> 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,

Yes, you are right, I meant that we will still have invalid pointers and use it 
silently in the vanilla kernel, but it won't always lead to a crash.

The crash in this specific case happens if `prev_cfs_rq->tg` points to a memory
location that cannot be de-referenced. Otherwise, the function de-references and
uses memory locations that are not valid but did not cause a visible failure so far.

Here are more details on what I meant by reordering the runqueue:

With the system and kernel configuration, I encountered the crash while trying
to reorder the runqueue structure, here is the minimal change that caused the
crash on top of v6.14-rc5 kernel:

---
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c8512a9fb022..597c1e6a9b5d 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1143,8 +1143,8 @@ struct rq {
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
        /* list of leaf cfs_rq on this CPU: */
+       struct list_head        *tmp_alone_branch;
        struct list_head        leaf_cfs_rq_list;
-       struct list_head        *tmp_alone_branch;
 #endif /* CONFIG_FAIR_GROUP_SCHED */
---

Here is the crash signature:

[    1.114431][  T552] Kernel attempted to read user page (10000012f) - exploit attempt? (uid: 0)
[    1.114440][  T552] BUG: Unable to handle kernel data access on read at 0x10000012f
[    1.114446][  T552] Faulting instruction address: 0xc0000000001c1044
[    1.116344][  T241] pstore: backend (nvram) writing error (-1)
[    1.116351][  T241] 
[    1.116354][  T552] Oops: Kernel access of bad area, sig: 11 [#2]
[    1.116354][  T241] note: kworker/44:0[241] exited with irqs disabled
[    1.116356][  T552] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
[    1.116368][  T552] Modules linked in: autofs4
[    1.116374][  T552] CPU: 73 UID: 0 PID: 552 Comm: kworker/73:1 Tainted: G      D            6.14.0-rc5-dirty #193
[    1.116381][  T552] Tainted: [D]=DIE
[    1.116384][  T552] Hardware name: IBM,9080-HEX POWER10 (architected) 0x800200 0xf000006 of:IBM,FW1060.00 (NH1060_012) hv:phyp pSeries
[    1.116391][  T552] NIP:  c0000000001c1044 LR: c0000000001c0f98 CTR: c000000000027ad4
[    1.116396][  T552] REGS: c000000076627870 TRAP: 0300   Tainted: G      D             (6.14.0-rc5-dirty)
[    1.116401][  T552] MSR:  800000000280b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE>  CR: 48008202  XER: 20040154
...
[    1.116467][  T552] NIP [c0000000001c1044] sched_balance_update_blocked_averages+0x35c/0x88c
[    1.116474][  T552] LR [c0000000001c0f98] sched_balance_update_blocked_averages+0x2b0/0x88c

~~~~

Before reordering, struct rq and cfs_rq had the following memory layout
(snippet from pahole):

struct rq {
	...                                                 (offset  bytes)
	struct list_head           leaf_cfs_rq_list;     /*  4048    16 */
	struct list_head *         tmp_alone_branch;     /*  4064     8 */
	unsigned int               nr_uninterruptible;   /*  4072     4 */
	...
}
struct cfs_rq {
	...
	struct list_head           leaf_cfs_rq_list;     /*   456    16 */
	struct task_group *        tg;                   /*   472     8 */
	...
}

In child_cfs_rq_on_list(), `prev_cfs_rq` is computed using the `container_of`
macro:

prev_cfs_rq = container_of(prev, struct cfs_rq, leaf_cfs_rq_list);

Since `prev == &rq->leaf_cfs_rq_list`, this results in:

prev_cfs_rq = rq->leaf_cfs_rq_list -  456 (offset of leaf_cfs_rq_list in cfs_rq)

Then, `prev_cfs_rq->tg` is accessed at an offset of 472 bytes from base cfs_rq:

prev_cfs_rq->tg = (rq->leaf_cfs_rq_list - 456) + 472
                 = rq->leaf_cfs_rq_list + 16
                 = rq->tmp_alone_branch

Since `tmp_alone_branch` is always at this point a valid pointer, dereferencing
`prev_cfs_rq->tg->parent` doesn't cause a crash, even though it is not
a valid task_group pointer.

~~~~

After reordering, the layout of `struct rq` changed as follows:

struct rq {
	...                                                (offset  bytes)
	struct list_head *         tmp_alone_branch;     /*  4048     8 */ -> this is shuffled up
	struct list_head           leaf_cfs_rq_list;     /*  4056    16 */
	unsigned int               nr_uninterruptible;   /*  4072     4 */
	...
}

The layout of `struct cfs_rq` is unchanged.

Now, when the same pointer arithmetic is performed:

prev_cfs_rq = rq->leaf_cfs_rq_list - 456

prev_cfs_rq->tg = (rq->leaf_cfs_rq_list - 456) + 472
                 = rq->leaf_cfs_rq_list + 16
                 = rq->nr_uninterruptible        # now this mem location corresponds to nr_uninterruptible.

Since nr_uninterruptible is an integer rather than a de-referenceable pointer, I presume because of
this, de-referencing parent from prev_cfs_rq->tg results in a crash. Otherwise,
incorrect pointers are silently used without a visible failure.


But looks like a patch similar to this is merged yesterday [1], so this can
be ignored :)


[1] https://lore.kernel.org/all/174119292742.14745.16827644501260146974.tip-bot2@tip-bot2/

Thanks,
Aboorva


> > > ...
> > > 
> 

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

[Dietmar Eggemann]

On 06/03/2025 05:48, Aboorva Devarajan wrote:
> 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@linux.ibm.com> wrote:

[...]

>>>> 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,
> 
> Yes, you are right, I meant that we will still have invalid pointers and use it 
> silently in the vanilla kernel, but it won't always lead to a crash.
> 
> The crash in this specific case happens if `prev_cfs_rq->tg` points to a memory
> location that cannot be de-referenced. Otherwise, the function de-references and
> uses memory locations that are not valid but did not cause a visible failure so far.
> 
> Here are more details on what I meant by reordering the runqueue:
> 
> With the system and kernel configuration, I encountered the crash while trying
> to reorder the runqueue structure, here is the minimal change that caused the
> crash on top of v6.14-rc5 kernel:

Ah, OK. You changed the code locally. Somehow I thought you referred to
a change which is already in mainline (or tip/sched/core) and I was
wondering which one it would be.

[...]

> But looks like a patch similar to this is merged yesterday [1], so this can
> be ignored :)
> 
> 
> [1] https://lore.kernel.org/all/174119292742.14745.16827644501260146974.tip-bot2@tip-bot2/

Ah, OK, same idea though.

BTW, the:

 	} else {
 		prev = rq->tmp_alone_branch;
 	}

path is taken when dealing with CONFIG_CFS_BANDWIDTH and throttling
scenarios so this is important to cover as well.