Re: [PATCH] cgroup: don't queue css_release_work if one already pending

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Hillf Danton]

On Wed, 18 May 2022 09:48:21 -0700 Tadeusz Struk  wrote:
> On 4/22/22 04:05, Michal Koutny wrote:
> > On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo <tj@kernel.org> wrote:
> >> If this is the case, we need to hold an extra reference to be put by the
> >> css_killed_work_fn(), right?

That put could trigger INIT_WORK in css_release() and warning [1]
on init active (active state 0) object OTOH as the same
css->destroy_work is used in both kill and release pathes.

Hillf

[1] https://lore.kernel.org/lkml/000000000000ff747805debce6c6@google.com/
> > 
> > I looked into it a bit more lately and found that there already is such
> > a fuse in kill_css() [1].
> > 
> > At the same type syzbots stack trace demonstrates the fuse is
> > ineffective
> > 
> >> css_release+0xae/0xc0 kernel/cgroup/cgroup.c:5146                    (**)
> >> percpu_ref_put_many include/linux/percpu-refcount.h:322 [inline]
> >> percpu_ref_put include/linux/percpu-refcount.h:338 [inline]
> >> percpu_ref_call_confirm_rcu lib/percpu-refcount.c:162 [inline]        (*)
> >> percpu_ref_switch_to_atomic_rcu+0x5a2/0x5b0 lib/percpu-refcount.c:199
> >> rcu_do_batch+0x4f8/0xbc0 kernel/rcu/tree.c:2485
> >> rcu_core+0x59b/0xe30 kernel/rcu/tree.c:2722
> >> rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2735
> >> __do_softirq+0x27e/0x596 kernel/softirq.c:305
> > 
> > (*) this calls css_killed_ref_fn confirm_switch
> > (**) zero references after confirmed kill?
> > 
> > So, I was also looking at the possible race with css_free_rwork_fn()
> > (from failed css_create()) but that would likely emit a warning from
> > __percpu_ref_exit().
> > 
> > So, I still think there's something fishy (so far possible only via
> > artificial ENOMEM injection) that needs an explanation...
> 
> I can't reliably reproduce this issue on neither mainline nor v5.10, where
> syzbot originally found it. It still triggers for syzbot though.
> 
> -- 
> Thanks,
> Tadeusz

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tadeusz Struk]

On 5/19/22 04:23, Hillf Danton wrote:
> On Wed, 18 May 2022 09:48:21 -0700 Tadeusz Struk  wrote:
>> On 4/22/22 04:05, Michal Koutny wrote:
>>> On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo<tj@kernel.org>  wrote:
>>>> If this is the case, we need to hold an extra reference to be put by the
>>>> css_killed_work_fn(), right?
> That put could trigger INIT_WORK in css_release() and warning [1]
> on init active (active state 0) object OTOH as the same
> css->destroy_work is used in both kill and release pathes.

Will this help if there would be two WQs, one for the css_release path
and one for the rcu_work?

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index adb820e98f24..a4873b33e488 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -124,6 +124,7 @@ DEFINE_PERCPU_RWSEM(cgroup_threadgroup_rwsem);
   * which may lead to deadlock.
   */
  static struct workqueue_struct *cgroup_destroy_wq;
+static struct workqueue_struct *cgroup_destroy_rcu_wq;
  
  /* generate an array of cgroup subsystem pointers */
  #define SUBSYS(_x) [_x ## _cgrp_id] = &_x ## _cgrp_subsys,

-- 
Thanks,
Tadeusz

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tejun Heo]

On Thu, May 19, 2022 at 04:26:51PM -0700, Tadeusz Struk wrote:
> On 5/19/22 04:23, Hillf Danton wrote:
> > On Wed, 18 May 2022 09:48:21 -0700 Tadeusz Struk  wrote:
> > > On 4/22/22 04:05, Michal Koutny wrote:
> > > > On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo<tj@kernel.org>  wrote:
> > > > > If this is the case, we need to hold an extra reference to be put by the
> > > > > css_killed_work_fn(), right?
> > That put could trigger INIT_WORK in css_release() and warning [1]
> > on init active (active state 0) object OTOH as the same
> > css->destroy_work is used in both kill and release pathes.

Hmm... wouldn't the extra reference keep release from happening?

> Will this help if there would be two WQs, one for the css_release path
> and one for the rcu_work?
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index adb820e98f24..a4873b33e488 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -124,6 +124,7 @@ DEFINE_PERCPU_RWSEM(cgroup_threadgroup_rwsem);
>   * which may lead to deadlock.
>   */
>  static struct workqueue_struct *cgroup_destroy_wq;
> +static struct workqueue_struct *cgroup_destroy_rcu_wq;

I don't understand why this would help. Care to elaborate?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tadeusz Struk]

On 5/20/22 01:13, Tejun Heo wrote:
> On Thu, May 19, 2022 at 04:26:51PM -0700, Tadeusz Struk wrote:
>> On 5/19/22 04:23, Hillf Danton wrote:
>>> On Wed, 18 May 2022 09:48:21 -0700 Tadeusz Struk  wrote:
>>>> On 4/22/22 04:05, Michal Koutny wrote:
>>>>> On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo<tj@kernel.org>  wrote:
>>>>>> If this is the case, we need to hold an extra reference to be put by the
>>>>>> css_killed_work_fn(), right?
>>> That put could trigger INIT_WORK in css_release() and warning [1]
>>> on init active (active state 0) object OTOH as the same
>>> css->destroy_work is used in both kill and release paths.
> 
> Hmm... wouldn't the extra reference keep release from happening?
> 
>> Will this help if there would be two WQs, one for the css_release path
>> and one for the rcu_work?
>>
>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>> index adb820e98f24..a4873b33e488 100644
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
>> @@ -124,6 +124,7 @@ DEFINE_PERCPU_RWSEM(cgroup_threadgroup_rwsem);
>>    * which may lead to deadlock.
>>    */
>>   static struct workqueue_struct *cgroup_destroy_wq;
>> +static struct workqueue_struct *cgroup_destroy_rcu_wq;
> 
> I don't understand why this would help. Care to elaborate?

I think it will help to solve the list corruption issue:

list_add corruption. prev->next should be next (ffff8881f705c060), but was ffff888113123870. (prev=ffff888113123870).
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:28!

as this is a result of enqueuing the same css->destroy_work onto the same WQ,
one on the rcu path and one on the css_release path.
I will prototype it today and test with syzbot.

-- 
Thanks,
Tadeusz

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Michal Koutný]

On Fri, May 20, 2022 at 09:38:12AM -0700, Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
> as this is a result of enqueuing the same css->destroy_work onto the same WQ,
> one on the rcu path and one on the css_release path.
> I will prototype it today and test with syzbot.

In my understanding, you'd need two independent work_structs in a css,
not two separate workqueues to put the single entry on.

Michal

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tadeusz Struk]

On 5/20/22 09:42, Michal Koutný wrote:
> On Fri, May 20, 2022 at 09:38:12AM -0700, Tadeusz Struk<tadeusz.struk@linaro.org>  wrote:
>> as this is a result of enqueuing the same css->destroy_work onto the same WQ,
>> one on the rcu path and one on the css_release path.
>> I will prototype it today and test with syzbot.
> In my understanding, you'd need two independent work_structs in a css,
> not two separate workqueues to put the single entry on.

I think either way would work, but two workqueues would be less churn.
I can try both.

-- 
Thanks,
Tadeusz

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Hillf Danton]

On Thu, 19 May 2022 22:13:47 -1000 Tejun Heo  wrote:
> On Thu, May 19, 2022 at 04:26:51PM -0700, Tadeusz Struk wrote:
> > On 5/19/22 04:23, Hillf Danton wrote:
> > > On Wed, 18 May 2022 09:48:21 -0700 Tadeusz Struk  wrote:
> > > > On 4/22/22 04:05, Michal Koutny wrote:
> > > > > On Thu, Apr 21, 2022 at 02:00:56PM -1000, Tejun Heo<tj@kernel.org>  wrote:
> > > > > > If this is the case, we need to hold an extra reference to be put by the
> > > > > > css_killed_work_fn(), right?
> > > 
> > > That put could trigger INIT_WORK in css_release() and warning [1]
> > > on init active (active state 0) object OTOH as the same
> > > css->destroy_work is used in both kill and release pathes.
> 
> Hmm... wouldn't the extra reference keep release from happening?

Hm...Hm... have difficulty following up given the risk of memleak without
release.

> 
> > Will this help if there would be two WQs, one for the css_release path
> > and one for the rcu_work?

Unlikely as adding one more queue does not help INIT_WORK.

Hillf
> > 
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index adb820e98f24..a4873b33e488 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -124,6 +124,7 @@ DEFINE_PERCPU_RWSEM(cgroup_threadgroup_rwsem);
> >   * which may lead to deadlock.
> >   */
> >  static struct workqueue_struct *cgroup_destroy_wq;
> > +static struct workqueue_struct *cgroup_destroy_rcu_wq;
> 
> I don't understand why this would help. Care to elaborate?
> 
> Thanks.
> 
> -- 
> tejun

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tadeusz Struk]

On 5/20/22 09:42, Michal Koutný wrote:
> On Fri, May 20, 2022 at 09:38:12AM -0700, Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
>> as this is a result of enqueuing the same css->destroy_work onto the same WQ,
>> one on the rcu path and one on the css_release path.
>> I will prototype it today and test with syzbot.
> 
> In my understanding, you'd need two independent work_structs in a css,
> not two separate workqueues to put the single entry on.

Yes, separating the css_killed_ref and css_release paths with two separate work_structs
fixes the two syzbot list corruption issues [1]&[2].
I tested it on mainline v5.18.0 and v5.10.117
In case of [2] the mainline triggers an issue, but it is unrelated to list corruption.

[1] https://syzkaller.appspot.com/bug?id=e26e54d6eac9d9fb50b221ec3e4627b327465dbd
[2] https://syzkaller.appspot.com/bug?id=3c7ff113ccb695e839b859da3fc481c36eb1cfd5

I will send a patch soon.

-- 
Thanks,
Tadeusz

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tejun Heo]

On Mon, May 23, 2022 at 12:00:06PM -0700, Tadeusz Struk wrote:
> On 5/20/22 09:42, Michal Koutný wrote:
> > On Fri, May 20, 2022 at 09:38:12AM -0700, Tadeusz Struk <tadeusz.struk@linaro.org> wrote:
> > > as this is a result of enqueuing the same css->destroy_work onto the same WQ,
> > > one on the rcu path and one on the css_release path.
> > > I will prototype it today and test with syzbot.
> > 
> > In my understanding, you'd need two independent work_structs in a css,
> > not two separate workqueues to put the single entry on.
> 
> Yes, separating the css_killed_ref and css_release paths with two separate work_structs
> fixes the two syzbot list corruption issues [1]&[2].
> I tested it on mainline v5.18.0 and v5.10.117
> In case of [2] the mainline triggers an issue, but it is unrelated to list corruption.

Can you try holding an extra ref in the killed path?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tadeusz Struk]

On 5/23/22 12:02, Tejun Heo wrote:
> Can you try holding an extra ref in the killed path?

Yes, I can try that. Should that be with or without the extra work_struct?

-- 
Thanks,
Tadeusz

Re: [PATCH] cgroup: don't queue css_release_work if one already pending

[Tadeusz Struk]

On 5/23/22 12:08, Tadeusz Struk wrote:
> On 5/23/22 12:02, Tejun Heo wrote:
>> Can you try holding an extra ref in the killed path?
> 
> Yes, I can try that. Should that be with or without the extra work_struct?

With this patch:
https://syzkaller.appspot.com/text?tag=Patch&x=1264b23df00000

The issue still triggers:
https://syzkaller.appspot.com/text?tag=CrashReport&x=162b5781f00000

-- 
Thanks,
Tadeusz