Re: Use after free in cgroup_migrate_execute

Re: Use after free in cgroup_migrate_execute

[Tejun Heo]

On Wed, Mar 02, 2022 at 08:42:32PM +0530, Mukesh Ojha wrote:
> Hi ,
> 
> We are facing one issue like below in cgroup .
> Not able to find which race could lead to this.
> Any idea, would be helpful.
> 
> [136233.086904][ T1457] ------------[ cut here ]------------
> *[136233.086912][ T1457] refcount_t: addition on 0; use-after-free.*
> [136233.086943][ T1457] WARNING: CPU: 4 PID: 1457 at lib/refcount.c:25
> cgroup_migrate_execute+0x188/0x528
> [136233.087527][ T1457] CPU: 4 PID: 1457 Comm: PERFD-SERVER Tainted: G
> S      WC O      5.10.66 #1
> [136233.087532][ T1457] pstate: 62400085 (nZCv daIf +PAN -UAO +TCO BTYPE=--)
> [136233.087536][ T1457] pc : cgroup_migrate_execute+0x188/0x528
> [136233.087539][ T1457] lr : cgroup_migrate_execute+0x188/0x528
> [136233.087541][ T1457] sp : ffffffc01ff23a60
> [136233.087543][ T1457] x29: ffffffc01ff23a60 x28: 00000000c0000000
> [136233.087547][ T1457] x27: ffffffffffffeaa8 x26: ffffff88cbc55668
> [136233.087551][ T1457] x25: ffffff878424d458 x24: ffffff891fdd5e00
> [136233.087557][ T1457] x23: ffffff88cbc55600 x22: ffffff8784d673d8
> [136233.087565][ T1457] x21: ffffff88cbc55758 x20: ffffffc01ff23b20
> [136233.087572][ T1457] x19: ffffffc01ff23b00 x18: ffffffc019475068
> [136233.087580][ T1457] x17: 0000000000000000 x16: 0000000000162ba8
> [136233.087587][ T1457] x15: 0000000000000004 x14: 000000000000407f
> [136233.087594][ T1457] x13: ffffff8ae5d48be8 x12: 00000000ffffffff
> [136233.087602][ T1457] x11: ffffff8785a79f98 x10: 0000000000000002
> [136233.087609][ T1457] x9 : 759287265d79e000 x8 : 759287265d79e000
> [136233.087616][ T1457] x7 : 206e6f206e6f6974 x6 : ffffffd7616121b4
> [136233.087623][ T1457] x5 : ffffffffffffffff x4 : 0000000000000000
> [136233.087629][ T1457] x3 : ffffffd7635ce996 x2 : 0000000000000000
> [136233.087633][ T1457] x1 : ffffffd7635ce996 x0 : 000000000000002a
> [136233.087636][ T1457] Call trace:
> [136233.087640][ T1457]  cgroup_migrate_execute+0x188/0x528
> [136233.087643][ T1457]  cgroup_migrate+0xb4/0xe4
> [136233.087646][ T1457]  cgroup_attach_task+0x128/0x20c
> [136233.087650][ T1457]  __cgroup1_procs_write+0x1d8/0x290
> [136233.087653][ T1457]  cgroup1_procs_write+0x18/0x28
> [136233.087656][ T1457]  cgroup_file_write+0xa4/0x544
> [136233.087661][ T1457]  kernfs_fop_write_iter+0x1b0/0x2f8
> [136233.087665][ T1457]  vfs_write+0x300/0x37c
> [136233.087668][ T1457]  ksys_write+0x84/0x12c
> [136233.087672][ T1457]  __arm64_sys_write+0x20/0x30
> [136233.087676][ T1457]  el0_svc_common+0xdc/0x294
> [136233.087681][ T1457]  el0_svc+0x38/0x9c
> [136233.087684][ T1457]  el0_sync_handler+0x8c/0xf0
> [136233.087688][ T1457]  el0_sync+0x1b4/0x1c0
> [136233.087690][ T1457] ---[ end trace 9e592742965258ba ]---
> [136233.087693][ T1457] ------------[ cut here ]------------
> *[136233.087695][ T1457] refcount_t: saturated; leaking memory.*

Looks like the target css_set ref underglowed but you have five taint flags
set and this isn't even the first warning message. Any chance you can
reproduce this in a cleaner environment?

Thanks.

-- 
tejun

Re: Use after free in cgroup_migrate_execute

[Mukesh Ojha]

+@paulmc

It would be difficult to reproduce this with clean kernel.

However, i have put trace_printk on the cset object and found that task 
reference is dropped from rcu_do_batch() as below and later trying to 
get same cset reference resulted in the warning and later BUG_ON().

rcuop/0-16    [002]   255.430731: bprint: 
put_css_set_locked:cset:refzero:put_css_set_locked:cset:0xffffff80274eb800:

Callers 
(cgroup_free<-delayed_put_task_struct<-rcu_do_batch<-nocb_cb_wait<-rcu_nocb_cb_kthread<-kthread<-ret_from_fork)

PERFD-SERVER-1387  [002]   255.432631: bprint: 
get_css_set: get_css_set: 0xffffff80274eb800: 
Callers:(cgroup_migrate_execute<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_write)

PERFD-SERVER-1387  [002]   255.456360: bprint: 
get_css_set: get_css_set: 0xffffff80274eb800: 
Callers:(cgroup_migrate_execute<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_write)


Thanks,
-Mukesh

On 3/2/2022 9:15 PM, Tejun Heo wrote:
> On Wed, Mar 02, 2022 at 08:42:32PM +0530, Mukesh Ojha wrote:
>> Hi ,
>>
>> We are facing one issue like below in cgroup .
>> Not able to find which race could lead to this.
>> Any idea, would be helpful.
>>
>> [136233.086904][ T1457] ------------[ cut here ]------------
>> *[136233.086912][ T1457] refcount_t: addition on 0; use-after-free.*
>> [136233.086943][ T1457] WARNING: CPU: 4 PID: 1457 at lib/refcount.c:25
>> cgroup_migrate_execute+0x188/0x528
>> [136233.087527][ T1457] CPU: 4 PID: 1457 Comm: PERFD-SERVER Tainted: G
>> S      WC O      5.10.66 #1
>> [136233.087532][ T1457] pstate: 62400085 (nZCv daIf +PAN -UAO +TCO BTYPE=--)
>> [136233.087536][ T1457] pc : cgroup_migrate_execute+0x188/0x528
>> [136233.087539][ T1457] lr : cgroup_migrate_execute+0x188/0x528
>> [136233.087541][ T1457] sp : ffffffc01ff23a60
>> [136233.087543][ T1457] x29: ffffffc01ff23a60 x28: 00000000c0000000
>> [136233.087547][ T1457] x27: ffffffffffffeaa8 x26: ffffff88cbc55668
>> [136233.087551][ T1457] x25: ffffff878424d458 x24: ffffff891fdd5e00
>> [136233.087557][ T1457] x23: ffffff88cbc55600 x22: ffffff8784d673d8
>> [136233.087565][ T1457] x21: ffffff88cbc55758 x20: ffffffc01ff23b20
>> [136233.087572][ T1457] x19: ffffffc01ff23b00 x18: ffffffc019475068
>> [136233.087580][ T1457] x17: 0000000000000000 x16: 0000000000162ba8
>> [136233.087587][ T1457] x15: 0000000000000004 x14: 000000000000407f
>> [136233.087594][ T1457] x13: ffffff8ae5d48be8 x12: 00000000ffffffff
>> [136233.087602][ T1457] x11: ffffff8785a79f98 x10: 0000000000000002
>> [136233.087609][ T1457] x9 : 759287265d79e000 x8 : 759287265d79e000
>> [136233.087616][ T1457] x7 : 206e6f206e6f6974 x6 : ffffffd7616121b4
>> [136233.087623][ T1457] x5 : ffffffffffffffff x4 : 0000000000000000
>> [136233.087629][ T1457] x3 : ffffffd7635ce996 x2 : 0000000000000000
>> [136233.087633][ T1457] x1 : ffffffd7635ce996 x0 : 000000000000002a
>> [136233.087636][ T1457] Call trace:
>> [136233.087640][ T1457]  cgroup_migrate_execute+0x188/0x528
>> [136233.087643][ T1457]  cgroup_migrate+0xb4/0xe4
>> [136233.087646][ T1457]  cgroup_attach_task+0x128/0x20c
>> [136233.087650][ T1457]  __cgroup1_procs_write+0x1d8/0x290
>> [136233.087653][ T1457]  cgroup1_procs_write+0x18/0x28
>> [136233.087656][ T1457]  cgroup_file_write+0xa4/0x544
>> [136233.087661][ T1457]  kernfs_fop_write_iter+0x1b0/0x2f8
>> [136233.087665][ T1457]  vfs_write+0x300/0x37c
>> [136233.087668][ T1457]  ksys_write+0x84/0x12c
>> [136233.087672][ T1457]  __arm64_sys_write+0x20/0x30
>> [136233.087676][ T1457]  el0_svc_common+0xdc/0x294
>> [136233.087681][ T1457]  el0_svc+0x38/0x9c
>> [136233.087684][ T1457]  el0_sync_handler+0x8c/0xf0
>> [136233.087688][ T1457]  el0_sync+0x1b4/0x1c0
>> [136233.087690][ T1457] ---[ end trace 9e592742965258ba ]---
>> [136233.087693][ T1457] ------------[ cut here ]------------
>> *[136233.087695][ T1457] refcount_t: saturated; leaking memory.*
> 
> Looks like the target css_set ref underglowed but you have five taint flags
> set and this isn't even the first warning message. Any chance you can
> reproduce this in a cleaner environment?
> 
> Thanks.
> 

Re: Use after free in cgroup_migrate_execute

[Paul E. McKenney]

On Mon, May 16, 2022 at 08:24:44PM +0530, Mukesh Ojha wrote:
> +@paulmc
> 
> It would be difficult to reproduce this with clean kernel.
> 
> However, i have put trace_printk on the cset object and found that task
> reference is dropped from rcu_do_batch() as below and later trying to get
> same cset reference resulted in the warning and later BUG_ON().
> 
> rcuop/0-16    [002]   255.430731: bprint:
> put_css_set_locked:cset:refzero:put_css_set_locked:cset:0xffffff80274eb800:
> 
> Callers (cgroup_free<-delayed_put_task_struct<-rcu_do_batch<-nocb_cb_wait<-rcu_nocb_cb_kthread<-kthread<-ret_from_fork)
> 
> PERFD-SERVER-1387  [002]   255.432631: bprint: get_css_set: get_css_set:
> 0xffffff80274eb800: Callers:(cgroup_migrate_execute<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_write)
> 
> PERFD-SERVER-1387  [002]   255.456360: bprint: get_css_set: get_css_set:
> 0xffffff80274eb800: Callers:(cgroup_migrate_execute<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_write)

If you can make this happen on small systems, you might try building with
CONFIG_RCU_STRICT_GRACE_PERIOD=y, which will make RCU grace periods finish
more quickly, possibly increasing the probability of hitting this problem.

							Thanx, Paul

> Thanks,
> -Mukesh
> 
> On 3/2/2022 9:15 PM, Tejun Heo wrote:
> > On Wed, Mar 02, 2022 at 08:42:32PM +0530, Mukesh Ojha wrote:
> > > Hi ,
> > > 
> > > We are facing one issue like below in cgroup .
> > > Not able to find which race could lead to this.
> > > Any idea, would be helpful.
> > > 
> > > [136233.086904][ T1457] ------------[ cut here ]------------
> > > *[136233.086912][ T1457] refcount_t: addition on 0; use-after-free.*
> > > [136233.086943][ T1457] WARNING: CPU: 4 PID: 1457 at lib/refcount.c:25
> > > cgroup_migrate_execute+0x188/0x528
> > > [136233.087527][ T1457] CPU: 4 PID: 1457 Comm: PERFD-SERVER Tainted: G
> > > S      WC O      5.10.66 #1
> > > [136233.087532][ T1457] pstate: 62400085 (nZCv daIf +PAN -UAO +TCO BTYPE=--)
> > > [136233.087536][ T1457] pc : cgroup_migrate_execute+0x188/0x528
> > > [136233.087539][ T1457] lr : cgroup_migrate_execute+0x188/0x528
> > > [136233.087541][ T1457] sp : ffffffc01ff23a60
> > > [136233.087543][ T1457] x29: ffffffc01ff23a60 x28: 00000000c0000000
> > > [136233.087547][ T1457] x27: ffffffffffffeaa8 x26: ffffff88cbc55668
> > > [136233.087551][ T1457] x25: ffffff878424d458 x24: ffffff891fdd5e00
> > > [136233.087557][ T1457] x23: ffffff88cbc55600 x22: ffffff8784d673d8
> > > [136233.087565][ T1457] x21: ffffff88cbc55758 x20: ffffffc01ff23b20
> > > [136233.087572][ T1457] x19: ffffffc01ff23b00 x18: ffffffc019475068
> > > [136233.087580][ T1457] x17: 0000000000000000 x16: 0000000000162ba8
> > > [136233.087587][ T1457] x15: 0000000000000004 x14: 000000000000407f
> > > [136233.087594][ T1457] x13: ffffff8ae5d48be8 x12: 00000000ffffffff
> > > [136233.087602][ T1457] x11: ffffff8785a79f98 x10: 0000000000000002
> > > [136233.087609][ T1457] x9 : 759287265d79e000 x8 : 759287265d79e000
> > > [136233.087616][ T1457] x7 : 206e6f206e6f6974 x6 : ffffffd7616121b4
> > > [136233.087623][ T1457] x5 : ffffffffffffffff x4 : 0000000000000000
> > > [136233.087629][ T1457] x3 : ffffffd7635ce996 x2 : 0000000000000000
> > > [136233.087633][ T1457] x1 : ffffffd7635ce996 x0 : 000000000000002a
> > > [136233.087636][ T1457] Call trace:
> > > [136233.087640][ T1457]  cgroup_migrate_execute+0x188/0x528
> > > [136233.087643][ T1457]  cgroup_migrate+0xb4/0xe4
> > > [136233.087646][ T1457]  cgroup_attach_task+0x128/0x20c
> > > [136233.087650][ T1457]  __cgroup1_procs_write+0x1d8/0x290
> > > [136233.087653][ T1457]  cgroup1_procs_write+0x18/0x28
> > > [136233.087656][ T1457]  cgroup_file_write+0xa4/0x544
> > > [136233.087661][ T1457]  kernfs_fop_write_iter+0x1b0/0x2f8
> > > [136233.087665][ T1457]  vfs_write+0x300/0x37c
> > > [136233.087668][ T1457]  ksys_write+0x84/0x12c
> > > [136233.087672][ T1457]  __arm64_sys_write+0x20/0x30
> > > [136233.087676][ T1457]  el0_svc_common+0xdc/0x294
> > > [136233.087681][ T1457]  el0_svc+0x38/0x9c
> > > [136233.087684][ T1457]  el0_sync_handler+0x8c/0xf0
> > > [136233.087688][ T1457]  el0_sync+0x1b4/0x1c0
> > > [136233.087690][ T1457] ---[ end trace 9e592742965258ba ]---
> > > [136233.087693][ T1457] ------------[ cut here ]------------
> > > *[136233.087695][ T1457] refcount_t: saturated; leaking memory.*
> > 
> > Looks like the target css_set ref underglowed but you have five taint flags
> > set and this isn't even the first warning message. Any chance you can
> > reproduce this in a cleaner environment?
> > 
> > Thanks.
> > 

Re: Use after free in cgroup_migrate_execute

[Mukesh Ojha]

+shisiyuan <shisiyuan19870131@gmail.com>

Hi Tejun/Paul,


Following patch is trying to fix the problem, which i reported earlier 
in this thread.

https://lore.kernel.org/lkml/1654187688-27411-1-git-send-email-shisiyuan@xiaomi.com/#t


So, here is the issue.

Userspace want to migrate one group task from one CSS to another CSS.

Two process(P1, P2) which belongs to same process group but having
different cset C1 and C2 respectively. But here, finally P1, P2 need
to get migrated to C2 css.

1.

For P1,
cgroup_migrate_add_src<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_writ

It takes reference of src cset(c1) and add itself to mgctx 
preloaded_src_cset list.
list_add_tail(&src_cset->mg_preload_node, &mgctx->preloaded_src_csets);

2.For P2, Since P2 is already there in C2 and finally it has to go to C2 
css only. It takes reference of src_cset(C2) and add itself to 
           mgctx->preloaded_src_csets);

cgroup_migrate_add_src<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_writ

3. Now in a scenario while iterating through list preloaded_src_csets in 
cgroup_migrate_prepare_dst(),

We got dst_cset(C2) for a given src_cset (C1) for process P1 and since 
they are not equal. And here comes the main action, P1 will observe 
dst_cset->mg_preload_node (C2) as non empty and it is due 
src_cset->mg_preload_node(P2/C2) added itself to mgctx->preloaded_src_csets
both are same address and same cset P1 will mistakenly drop reference
to dst_cset and makes the dest cset(C2) reference out of order. In next 
iteration for p2, both src and destination cset are same so, it drop 
their references respectively.

4. Now, there comes a window there is no RCU lock and below path runs 
put reference on the task P2 and that eventually put reference on 
css(C2) and it get freed up as it reference broken due to extra mistaken
put_css_set in step 3.

rcuop/1-28    [005] 308947.813116: bprint: 
put_css_set_locked: cset:refzero:put_css_set_locked: 
0xffffff87f3e92800x: ref 0, Callers: 
(put_css_set<-__put_task_struct<-delayed_put_task_struct<-rcu_do_batch<-nocb_cb_wait<-rcu_nocb_cb_kthread<-kthread)

5. As the migration was going on, Now we hit two warnings b2b and a 
later BUG_ON which is shared in initial mail of this thread.

Let me know, if i am unable to explain any of the steps clearly.



-Mukesh


On 5/17/2022 2:04 AM, Paul E. McKenney wrote:
> On Mon, May 16, 2022 at 08:24:44PM +0530, Mukesh Ojha wrote:
>> +@paulmc
>>
>> It would be difficult to reproduce this with clean kernel.
>>
>> However, i have put trace_printk on the cset object and found that task
>> reference is dropped from rcu_do_batch() as below and later trying to get
>> same cset reference resulted in the warning and later BUG_ON().
>>
>> rcuop/0-16    [002]   255.430731: bprint:
>> put_css_set_locked:cset:refzero:put_css_set_locked:cset:0xffffff80274eb800:
>>
>> Callers (cgroup_free<-delayed_put_task_struct<-rcu_do_batch<-nocb_cb_wait<-rcu_nocb_cb_kthread<-kthread<-ret_from_fork)
>>
>> PERFD-SERVER-1387  [002]   255.432631: bprint: get_css_set: get_css_set:
>> 0xffffff80274eb800: Callers:(cgroup_migrate_execute<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_write)
>>
>> PERFD-SERVER-1387  [002]   255.456360: bprint: get_css_set: get_css_set:
>> 0xffffff80274eb800: Callers:(cgroup_migrate_execute<-cgroup_attach_task<-__cgroup1_procs_write<-cgroup1_procs_write<-cgroup_file_write<-kernfs_fop_write_iter<-vfs_write)
> 
> If you can make this happen on small systems, you might try building with
> CONFIG_RCU_STRICT_GRACE_PERIOD=y, which will make RCU grace periods finish
> more quickly, possibly increasing the probability of hitting this problem.
> 
> 							Thanx, Paul
> 
>> Thanks,
>> -Mukesh
>>
>> On 3/2/2022 9:15 PM, Tejun Heo wrote:
>>> On Wed, Mar 02, 2022 at 08:42:32PM +0530, Mukesh Ojha wrote:
>>>> Hi ,
>>>>
>>>> We are facing one issue like below in cgroup .
>>>> Not able to find which race could lead to this.
>>>> Any idea, would be helpful.
>>>>
>>>> [136233.086904][ T1457] ------------[ cut here ]------------
>>>> *[136233.086912][ T1457] refcount_t: addition on 0; use-after-free.*
>>>> [136233.086943][ T1457] WARNING: CPU: 4 PID: 1457 at lib/refcount.c:25
>>>> cgroup_migrate_execute+0x188/0x528
>>>> [136233.087527][ T1457] CPU: 4 PID: 1457 Comm: PERFD-SERVER Tainted: G
>>>> S      WC O      5.10.66 #1
>>>> [136233.087532][ T1457] pstate: 62400085 (nZCv daIf +PAN -UAO +TCO BTYPE=--)
>>>> [136233.087536][ T1457] pc : cgroup_migrate_execute+0x188/0x528
>>>> [136233.087539][ T1457] lr : cgroup_migrate_execute+0x188/0x528
>>>> [136233.087541][ T1457] sp : ffffffc01ff23a60
>>>> [136233.087543][ T1457] x29: ffffffc01ff23a60 x28: 00000000c0000000
>>>> [136233.087547][ T1457] x27: ffffffffffffeaa8 x26: ffffff88cbc55668
>>>> [136233.087551][ T1457] x25: ffffff878424d458 x24: ffffff891fdd5e00
>>>> [136233.087557][ T1457] x23: ffffff88cbc55600 x22: ffffff8784d673d8
>>>> [136233.087565][ T1457] x21: ffffff88cbc55758 x20: ffffffc01ff23b20
>>>> [136233.087572][ T1457] x19: ffffffc01ff23b00 x18: ffffffc019475068
>>>> [136233.087580][ T1457] x17: 0000000000000000 x16: 0000000000162ba8
>>>> [136233.087587][ T1457] x15: 0000000000000004 x14: 000000000000407f
>>>> [136233.087594][ T1457] x13: ffffff8ae5d48be8 x12: 00000000ffffffff
>>>> [136233.087602][ T1457] x11: ffffff8785a79f98 x10: 0000000000000002
>>>> [136233.087609][ T1457] x9 : 759287265d79e000 x8 : 759287265d79e000
>>>> [136233.087616][ T1457] x7 : 206e6f206e6f6974 x6 : ffffffd7616121b4
>>>> [136233.087623][ T1457] x5 : ffffffffffffffff x4 : 0000000000000000
>>>> [136233.087629][ T1457] x3 : ffffffd7635ce996 x2 : 0000000000000000
>>>> [136233.087633][ T1457] x1 : ffffffd7635ce996 x0 : 000000000000002a
>>>> [136233.087636][ T1457] Call trace:
>>>> [136233.087640][ T1457]  cgroup_migrate_execute+0x188/0x528
>>>> [136233.087643][ T1457]  cgroup_migrate+0xb4/0xe4
>>>> [136233.087646][ T1457]  cgroup_attach_task+0x128/0x20c
>>>> [136233.087650][ T1457]  __cgroup1_procs_write+0x1d8/0x290
>>>> [136233.087653][ T1457]  cgroup1_procs_write+0x18/0x28
>>>> [136233.087656][ T1457]  cgroup_file_write+0xa4/0x544
>>>> [136233.087661][ T1457]  kernfs_fop_write_iter+0x1b0/0x2f8
>>>> [136233.087665][ T1457]  vfs_write+0x300/0x37c
>>>> [136233.087668][ T1457]  ksys_write+0x84/0x12c
>>>> [136233.087672][ T1457]  __arm64_sys_write+0x20/0x30
>>>> [136233.087676][ T1457]  el0_svc_common+0xdc/0x294
>>>> [136233.087681][ T1457]  el0_svc+0x38/0x9c
>>>> [136233.087684][ T1457]  el0_sync_handler+0x8c/0xf0
>>>> [136233.087688][ T1457]  el0_sync+0x1b4/0x1c0
>>>> [136233.087690][ T1457] ---[ end trace 9e592742965258ba ]---
>>>> [136233.087693][ T1457] ------------[ cut here ]------------
>>>> *[136233.087695][ T1457] refcount_t: saturated; leaking memory.*
>>>
>>> Looks like the target css_set ref underglowed but you have five taint flags
>>> set and this isn't even the first warning message. Any chance you can
>>> reproduce this in a cleaner environment?
>>>
>>> Thanks.
>>>

[PATCH cgroup/for-5.19-fixes] cgroup: Use separate src/dst nodes when preloading css_sets for migration

[Tejun Heo]

Each cset (css_set) is pinned by its tasks. When we're moving tasks around
across csets for a migration, we need to hold the source and destination
csets to ensure that they don't go away while we're moving tasks about. This
is done by linking cset->mg_preload_node on either the
mgctx->preloaded_dst_csets or mgctx->preloaded_dst_csets list. Using the
same cset->mg_preload_node for both the src and dst lists was deemed okay as
a cset can't be both the source and destination at the same time.

Unfortunately, this overloading becomes problematic when multiple tasks are
involved in a migration and some of them are identity noop migrations while
others are actually moving across cgroups. For example, this can happen with
the following sequence on cgroup1:

 #1> mkdir -p /sys/fs/cgroup/misc/a/b
 #2> echo $$ > /sys/fs/cgroup/misc/a/cgroup.procs
 #3> RUN_A_COMMAND_WHICH_CREATES_MULTIPLE_THREADS &
 #4> PID=$!
 #5> echo $PID > /sys/fs/cgroup/misc/a/b/tasks
 #6> echo $PID > /sys/fs/cgroup/misc/a/cgroup.procs

#5 moves the group leader thread into a/b and then #6 moves all threads of
the process including the group leader back into a. In this final migration,
non-leader threads would be doing identity migration while the group leader
is doing an actual one.

After #3, let's say the whole process was in cset A, and that after #4, the
leader moves to cset B. Then, during #6, the following happens:

 1. cgroup_migrate_add_src() is called on B for the leader.

 2. cgroup_migrate_add_src() is called on A for the other threads.

 3. cgroup_migrate_prepare_dst() is called. It scans the src list.

 3. It notices that B wants to migrate to A, so it tries to A to the dst
    list but realizes that its ->mg_preload_node is already busy.

 4. and then it notices A wants to migrate to A as it's an identity
    migration, it culls it by list_del_init()'ing its ->mg_preload_node and
    putting references accordingly.

 5. The rest of migration takes place with B on the src list but nothing on
    the dst list.

This means that A isn't held while migration is in progress. If all tasks
leave A before the migration finishes and the incoming task pins it, the
cset will be destroyed leading to use-after-free.

This is caused by overloading cset->mg_preload_node for both src and dst
preload lists. We wanted to exclude the cset from the src list but ended up
inadvertently excluding it from the dst list too.

This patch fixes the issue by separating out cset->mg_preload_node into
->mg_src_preload_node and ->mg_dst_preload_node, so that the src and dst
preloadings don't interfere with each other.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Mukesh Ojha <quic_mojha@quicinc.com>
Reported-by: shisiyuan <shisiyuan19870131@gmail.com>
Link: http://lkml.kernel.org/r/1654187688-27411-1-git-send-email-shisiyuan@xiaomi.com
Link: https://www.spinics.net/lists/cgroups/msg33313.html
Fixes: f817de98513d ("cgroup: prepare migration path for unified hierarchy")
Cc: stable@vger.kernel.org # v3.16+
---
Hello,

Shisiyuan and Mukesh, can you guys please confirm that this fixes the
problem you're seeing? This is a different approach from Shisiyuan's patch
which most likely would work too but this is a bit more straightforward.

Thanks.

 include/linux/cgroup-defs.h |    3 ++-
 kernel/cgroup/cgroup.c      |   37 +++++++++++++++++++++++--------------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 1bfcfb1af3524..d4427d0a0e187 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -264,7 +264,8 @@ struct css_set {
 	 * List of csets participating in the on-going migration either as
 	 * source or destination.  Protected by cgroup_mutex.
 	 */
-	struct list_head mg_preload_node;
+	struct list_head mg_src_preload_node;
+	struct list_head mg_dst_preload_node;
 	struct list_head mg_node;
 
 	/*
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1779ccddb734d..13c8e91d78620 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -765,7 +765,8 @@ struct css_set init_css_set = {
 	.task_iters		= LIST_HEAD_INIT(init_css_set.task_iters),
 	.threaded_csets		= LIST_HEAD_INIT(init_css_set.threaded_csets),
 	.cgrp_links		= LIST_HEAD_INIT(init_css_set.cgrp_links),
-	.mg_preload_node	= LIST_HEAD_INIT(init_css_set.mg_preload_node),
+	.mg_src_preload_node	= LIST_HEAD_INIT(init_css_set.mg_src_preload_node),
+	.mg_dst_preload_node	= LIST_HEAD_INIT(init_css_set.mg_dst_preload_node),
 	.mg_node		= LIST_HEAD_INIT(init_css_set.mg_node),
 
 	/*
@@ -1240,7 +1241,8 @@ static struct css_set *find_css_set(struct css_set *old_cset,
 	INIT_LIST_HEAD(&cset->threaded_csets);
 	INIT_HLIST_NODE(&cset->hlist);
 	INIT_LIST_HEAD(&cset->cgrp_links);
-	INIT_LIST_HEAD(&cset->mg_preload_node);
+	INIT_LIST_HEAD(&cset->mg_src_preload_node);
+	INIT_LIST_HEAD(&cset->mg_dst_preload_node);
 	INIT_LIST_HEAD(&cset->mg_node);
 
 	/* Copy the set of subsystem state objects generated in
@@ -2597,21 +2599,27 @@ int cgroup_migrate_vet_dst(struct cgroup *dst_cgrp)
  */
 void cgroup_migrate_finish(struct cgroup_mgctx *mgctx)
 {
-	LIST_HEAD(preloaded);
 	struct css_set *cset, *tmp_cset;
 
 	lockdep_assert_held(&cgroup_mutex);
 
 	spin_lock_irq(&css_set_lock);
 
-	list_splice_tail_init(&mgctx->preloaded_src_csets, &preloaded);
-	list_splice_tail_init(&mgctx->preloaded_dst_csets, &preloaded);
+	list_for_each_entry_safe(cset, tmp_cset, &mgctx->preloaded_src_csets,
+				 mg_src_preload_node) {
+		cset->mg_src_cgrp = NULL;
+		cset->mg_dst_cgrp = NULL;
+		cset->mg_dst_cset = NULL;
+		list_del_init(&cset->mg_src_preload_node);
+		put_css_set_locked(cset);
+	}
 
-	list_for_each_entry_safe(cset, tmp_cset, &preloaded, mg_preload_node) {
+	list_for_each_entry_safe(cset, tmp_cset, &mgctx->preloaded_dst_csets,
+				 mg_dst_preload_node) {
 		cset->mg_src_cgrp = NULL;
 		cset->mg_dst_cgrp = NULL;
 		cset->mg_dst_cset = NULL;
-		list_del_init(&cset->mg_preload_node);
+		list_del_init(&cset->mg_dst_preload_node);
 		put_css_set_locked(cset);
 	}
 
@@ -2651,7 +2659,7 @@ void cgroup_migrate_add_src(struct css_set *src_cset,
 	if (src_cset->dead)
 		return;
 
-	if (!list_empty(&src_cset->mg_preload_node))
+	if (!list_empty(&src_cset->mg_src_preload_node))
 		return;
 
 	src_cgrp = cset_cgroup_from_root(src_cset, dst_cgrp->root);
@@ -2664,7 +2672,7 @@ void cgroup_migrate_add_src(struct css_set *src_cset,
 	src_cset->mg_src_cgrp = src_cgrp;
 	src_cset->mg_dst_cgrp = dst_cgrp;
 	get_css_set(src_cset);
-	list_add_tail(&src_cset->mg_preload_node, &mgctx->preloaded_src_csets);
+	list_add_tail(&src_cset->mg_src_preload_node, &mgctx->preloaded_src_csets);
 }
 
 /**
@@ -2689,7 +2697,7 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
 
 	/* look up the dst cset for each src cset and link it to src */
 	list_for_each_entry_safe(src_cset, tmp_cset, &mgctx->preloaded_src_csets,
-				 mg_preload_node) {
+				 mg_src_preload_node) {
 		struct css_set *dst_cset;
 		struct cgroup_subsys *ss;
 		int ssid;
@@ -2708,7 +2716,7 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
 		if (src_cset == dst_cset) {
 			src_cset->mg_src_cgrp = NULL;
 			src_cset->mg_dst_cgrp = NULL;
-			list_del_init(&src_cset->mg_preload_node);
+			list_del_init(&src_cset->mg_src_preload_node);
 			put_css_set(src_cset);
 			put_css_set(dst_cset);
 			continue;
@@ -2716,8 +2724,8 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
 
 		src_cset->mg_dst_cset = dst_cset;
 
-		if (list_empty(&dst_cset->mg_preload_node))
-			list_add_tail(&dst_cset->mg_preload_node,
+		if (list_empty(&dst_cset->mg_dst_preload_node))
+			list_add_tail(&dst_cset->mg_dst_preload_node,
 				      &mgctx->preloaded_dst_csets);
 		else
 			put_css_set(dst_cset);
@@ -2963,7 +2971,8 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
 		goto out_finish;
 
 	spin_lock_irq(&css_set_lock);
-	list_for_each_entry(src_cset, &mgctx.preloaded_src_csets, mg_preload_node) {
+	list_for_each_entry(src_cset, &mgctx.preloaded_src_csets,
+			    mg_src_preload_node) {
 		struct task_struct *task, *ntask;
 
 		/* all tasks in src_csets need to be migrated */

Re: [PATCH cgroup/for-5.19-fixes] cgroup: Use separate src/dst nodes when preloading css_sets for migration

[Christian Brauner]

On Mon, Jun 13, 2022 at 12:19:50PM -1000, Tejun Heo wrote:
> Each cset (css_set) is pinned by its tasks. When we're moving tasks around
> across csets for a migration, we need to hold the source and destination
> csets to ensure that they don't go away while we're moving tasks about. This
> is done by linking cset->mg_preload_node on either the
> mgctx->preloaded_dst_csets or mgctx->preloaded_dst_csets list. Using the

Nit: This probably wants to be 
"mgctx->preloaded_src_csets or mgctx->preloaded_dst_csets lists"
it currentl has "mgctx->preloaded_dst_csets" twice. :)

> same cset->mg_preload_node for both the src and dst lists was deemed okay as
> a cset can't be both the source and destination at the same time.
> 
> Unfortunately, this overloading becomes problematic when multiple tasks are
> involved in a migration and some of them are identity noop migrations while
> others are actually moving across cgroups. For example, this can happen with
> the following sequence on cgroup1:
> 
>  #1> mkdir -p /sys/fs/cgroup/misc/a/b
>  #2> echo $$ > /sys/fs/cgroup/misc/a/cgroup.procs
>  #3> RUN_A_COMMAND_WHICH_CREATES_MULTIPLE_THREADS &
>  #4> PID=$!
>  #5> echo $PID > /sys/fs/cgroup/misc/a/b/tasks
>  #6> echo $PID > /sys/fs/cgroup/misc/a/cgroup.procs
> 
> #5 moves the group leader thread into a/b and then #6 moves all threads of
> the process including the group leader back into a. In this final migration,
> non-leader threads would be doing identity migration while the group leader
> is doing an actual one.
> 
> After #3, let's say the whole process was in cset A, and that after #4, the
> leader moves to cset B. Then, during #6, the following happens:
> 
>  1. cgroup_migrate_add_src() is called on B for the leader.
> 
>  2. cgroup_migrate_add_src() is called on A for the other threads.
> 
>  3. cgroup_migrate_prepare_dst() is called. It scans the src list.
> 
>  3. It notices that B wants to migrate to A, so it tries to A to the dst
>     list but realizes that its ->mg_preload_node is already busy.
> 
>  4. and then it notices A wants to migrate to A as it's an identity
>     migration, it culls it by list_del_init()'ing its ->mg_preload_node and
>     putting references accordingly.
> 
>  5. The rest of migration takes place with B on the src list but nothing on
>     the dst list.
> 
> This means that A isn't held while migration is in progress. If all tasks
> leave A before the migration finishes and the incoming task pins it, the
> cset will be destroyed leading to use-after-free.
> 
> This is caused by overloading cset->mg_preload_node for both src and dst
> preload lists. We wanted to exclude the cset from the src list but ended up
> inadvertently excluding it from the dst list too.
> 
> This patch fixes the issue by separating out cset->mg_preload_node into
> ->mg_src_preload_node and ->mg_dst_preload_node, so that the src and dst
> preloadings don't interfere with each other.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Reported-by: shisiyuan <shisiyuan19870131@gmail.com>
> Link: http://lkml.kernel.org/r/1654187688-27411-1-git-send-email-shisiyuan@xiaomi.com
> Link: https://www.spinics.net/lists/cgroups/msg33313.html
> Fixes: f817de98513d ("cgroup: prepare migration path for unified hierarchy")
> Cc: stable@vger.kernel.org # v3.16+
> ---
> Hello,
> 
> Shisiyuan and Mukesh, can you guys please confirm that this fixes the
> problem you're seeing? This is a different approach from Shisiyuan's patch
> which most likely would work too but this is a bit more straightforward.
> 
> Thanks.
> 
>  include/linux/cgroup-defs.h |    3 ++-
>  kernel/cgroup/cgroup.c      |   37 +++++++++++++++++++++++--------------
>  2 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
> index 1bfcfb1af3524..d4427d0a0e187 100644
> --- a/include/linux/cgroup-defs.h
> +++ b/include/linux/cgroup-defs.h
> @@ -264,7 +264,8 @@ struct css_set {
>  	 * List of csets participating in the on-going migration either as
>  	 * source or destination.  Protected by cgroup_mutex.
>  	 */
> -	struct list_head mg_preload_node;
> +	struct list_head mg_src_preload_node;
> +	struct list_head mg_dst_preload_node;
>  	struct list_head mg_node;
>  
>  	/*
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 1779ccddb734d..13c8e91d78620 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -765,7 +765,8 @@ struct css_set init_css_set = {
>  	.task_iters		= LIST_HEAD_INIT(init_css_set.task_iters),
>  	.threaded_csets		= LIST_HEAD_INIT(init_css_set.threaded_csets),
>  	.cgrp_links		= LIST_HEAD_INIT(init_css_set.cgrp_links),
> -	.mg_preload_node	= LIST_HEAD_INIT(init_css_set.mg_preload_node),
> +	.mg_src_preload_node	= LIST_HEAD_INIT(init_css_set.mg_src_preload_node),
> +	.mg_dst_preload_node	= LIST_HEAD_INIT(init_css_set.mg_dst_preload_node),
>  	.mg_node		= LIST_HEAD_INIT(init_css_set.mg_node),
>  
>  	/*
> @@ -1240,7 +1241,8 @@ static struct css_set *find_css_set(struct css_set *old_cset,
>  	INIT_LIST_HEAD(&cset->threaded_csets);
>  	INIT_HLIST_NODE(&cset->hlist);
>  	INIT_LIST_HEAD(&cset->cgrp_links);
> -	INIT_LIST_HEAD(&cset->mg_preload_node);
> +	INIT_LIST_HEAD(&cset->mg_src_preload_node);
> +	INIT_LIST_HEAD(&cset->mg_dst_preload_node);
>  	INIT_LIST_HEAD(&cset->mg_node);
>  
>  	/* Copy the set of subsystem state objects generated in
> @@ -2597,21 +2599,27 @@ int cgroup_migrate_vet_dst(struct cgroup *dst_cgrp)
>   */
>  void cgroup_migrate_finish(struct cgroup_mgctx *mgctx)
>  {
> -	LIST_HEAD(preloaded);
>  	struct css_set *cset, *tmp_cset;
>  
>  	lockdep_assert_held(&cgroup_mutex);
>  
>  	spin_lock_irq(&css_set_lock);
>  
> -	list_splice_tail_init(&mgctx->preloaded_src_csets, &preloaded);
> -	list_splice_tail_init(&mgctx->preloaded_dst_csets, &preloaded);
> +	list_for_each_entry_safe(cset, tmp_cset, &mgctx->preloaded_src_csets,
> +				 mg_src_preload_node) {
> +		cset->mg_src_cgrp = NULL;
> +		cset->mg_dst_cgrp = NULL;
> +		cset->mg_dst_cset = NULL;
> +		list_del_init(&cset->mg_src_preload_node);
> +		put_css_set_locked(cset);
> +	}
>  
> -	list_for_each_entry_safe(cset, tmp_cset, &preloaded, mg_preload_node) {
> +	list_for_each_entry_safe(cset, tmp_cset, &mgctx->preloaded_dst_csets,
> +				 mg_dst_preload_node) {
>  		cset->mg_src_cgrp = NULL;
>  		cset->mg_dst_cgrp = NULL;
>  		cset->mg_dst_cset = NULL;
> -		list_del_init(&cset->mg_preload_node);
> +		list_del_init(&cset->mg_dst_preload_node);
>  		put_css_set_locked(cset);
>  	}
>  
> @@ -2651,7 +2659,7 @@ void cgroup_migrate_add_src(struct css_set *src_cset,
>  	if (src_cset->dead)
>  		return;
>  
> -	if (!list_empty(&src_cset->mg_preload_node))
> +	if (!list_empty(&src_cset->mg_src_preload_node))
>  		return;
>  
>  	src_cgrp = cset_cgroup_from_root(src_cset, dst_cgrp->root);
> @@ -2664,7 +2672,7 @@ void cgroup_migrate_add_src(struct css_set *src_cset,
>  	src_cset->mg_src_cgrp = src_cgrp;
>  	src_cset->mg_dst_cgrp = dst_cgrp;
>  	get_css_set(src_cset);
> -	list_add_tail(&src_cset->mg_preload_node, &mgctx->preloaded_src_csets);
> +	list_add_tail(&src_cset->mg_src_preload_node, &mgctx->preloaded_src_csets);
>  }
>  
>  /**
> @@ -2689,7 +2697,7 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
>  
>  	/* look up the dst cset for each src cset and link it to src */
>  	list_for_each_entry_safe(src_cset, tmp_cset, &mgctx->preloaded_src_csets,
> -				 mg_preload_node) {
> +				 mg_src_preload_node) {
>  		struct css_set *dst_cset;
>  		struct cgroup_subsys *ss;
>  		int ssid;
> @@ -2708,7 +2716,7 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
>  		if (src_cset == dst_cset) {
>  			src_cset->mg_src_cgrp = NULL;
>  			src_cset->mg_dst_cgrp = NULL;
> -			list_del_init(&src_cset->mg_preload_node);
> +			list_del_init(&src_cset->mg_src_preload_node);
>  			put_css_set(src_cset);
>  			put_css_set(dst_cset);
>  			continue;
> @@ -2716,8 +2724,8 @@ int cgroup_migrate_prepare_dst(struct cgroup_mgctx *mgctx)
>  
>  		src_cset->mg_dst_cset = dst_cset;
>  
> -		if (list_empty(&dst_cset->mg_preload_node))
> -			list_add_tail(&dst_cset->mg_preload_node,
> +		if (list_empty(&dst_cset->mg_dst_preload_node))
> +			list_add_tail(&dst_cset->mg_dst_preload_node,
>  				      &mgctx->preloaded_dst_csets);
>  		else
>  			put_css_set(dst_cset);
> @@ -2963,7 +2971,8 @@ static int cgroup_update_dfl_csses(struct cgroup *cgrp)
>  		goto out_finish;
>  
>  	spin_lock_irq(&css_set_lock);
> -	list_for_each_entry(src_cset, &mgctx.preloaded_src_csets, mg_preload_node) {
> +	list_for_each_entry(src_cset, &mgctx.preloaded_src_csets,
> +			    mg_src_preload_node) {
>  		struct task_struct *task, *ntask;
>  
>  		/* all tasks in src_csets need to be migrated */

Re: [PATCH cgroup/for-5.19-fixes] cgroup: Use separate src/dst nodes when preloading css_sets for migration

[Tejun Heo]

On Mon, Jun 13, 2022 at 12:19:50PM -1000, Tejun Heo wrote:
> Each cset (css_set) is pinned by its tasks. When we're moving tasks around
> across csets for a migration, we need to hold the source and destination
> csets to ensure that they don't go away while we're moving tasks about. This
> is done by linking cset->mg_preload_node on either the
> mgctx->preloaded_dst_csets or mgctx->preloaded_dst_csets list. Using the
> same cset->mg_preload_node for both the src and dst lists was deemed okay as
> a cset can't be both the source and destination at the same time.

Applied to cgroup/for-5.19-fixes w/ patch description fixed.

Thanks.

-- 
tejun