cgroup: fixed the cset refcnt leak when fork() failed

[PATCH] cgroup: fixed the cset refcnt leak when fork() failed

Posted by Zou Cao 2 years, 8 months ago

TeamID: B1486294

when fork, cset will be increased by commit "ef2c41cf38a7", the refcnt will
be decrease by child exit, but when failed in fork(), this refcnt will
be lost decrease in cgroup_cancel_fork as follow:

copy_process
     |
cgroup_can_fork    //  increase the css refcount
  ......
  spin_lock_irq(&css_set_lock);
  cset = task_css_setcurrent);
  get_css_set(cset);
  spin_unlock_irq&css_set_lock);
  ......
     |
goto cgroup_cancel_fork    // if failed in  copy_process
     |
cgroup_cancel_fork  // lost the decrease refcount if flag not CLONE_INTO_CGROUP

Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
Signed-off-by: Zou Cao <zoucao@kuaishou.com>
---
 kernel/cgroup/cgroup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index d18c2ef..5ecd706 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6284,6 +6284,11 @@ void cgroup_cancel_fork(struct task_struct *child,
 		if (ss->cancel_fork)
 			ss->cancel_fork(child, kargs->cset);
 
+	if (!(kargs->flags & CLONE_INTO_CGROUP) &&
+			kargs->cset) {
+		put_css_set(kargs->cset);
+	}
+
 	cgroup_css_set_put_fork(kargs);
 }
 
-- 
1.8.3.1

Re: [PATCH] cgroup: fixed the cset refcnt leak when fork() failed

Posted by Tejun Heo 2 years, 8 months ago

On Mon, Jun 05, 2023 at 09:04:44PM +0800, Zou Cao wrote:
> TeamID: B1486294
> 
> when fork, cset will be increased by commit "ef2c41cf38a7", the refcnt will
> be decrease by child exit, but when failed in fork(), this refcnt will
> be lost decrease in cgroup_cancel_fork as follow:
> 
> copy_process
>      |
> cgroup_can_fork    //  increase the css refcount
>   ......
>   spin_lock_irq(&css_set_lock);
>   cset = task_css_setcurrent);
>   get_css_set(cset);
>   spin_unlock_irq&css_set_lock);
>   ......
>      |
> goto cgroup_cancel_fork    // if failed in  copy_process
>      |
> cgroup_cancel_fork  // lost the decrease refcount if flag not CLONE_INTO_CGROUP
> 
> Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
> Signed-off-by: Zou Cao <zoucao@kuaishou.com>

Is this the same bug fixed by the following commit?

 https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/commit/?h=for-6.4-fixes&id=2bd110339288c18823dcace602b63b0d8627e520

Thanks.

-- 
tejun

Re: [PATCH] cgroup: fixed the cset refcnt leak when fork() failed

Posted by Waiman Long 2 years, 8 months ago

On 6/5/23 17:38, Tejun Heo wrote:
> On Mon, Jun 05, 2023 at 09:04:44PM +0800, Zou Cao wrote:
>> TeamID: B1486294
>>
>> when fork, cset will be increased by commit "ef2c41cf38a7", the refcnt will
>> be decrease by child exit, but when failed in fork(), this refcnt will
>> be lost decrease in cgroup_cancel_fork as follow:
>>
>> copy_process
>>       |
>> cgroup_can_fork    //  increase the css refcount
>>    ......
>>    spin_lock_irq(&css_set_lock);
>>    cset = task_css_setcurrent);
>>    get_css_set(cset);
>>    spin_unlock_irq&css_set_lock);
>>    ......
>>       |
>> goto cgroup_cancel_fork    // if failed in  copy_process
>>       |
>> cgroup_cancel_fork  // lost the decrease refcount if flag not CLONE_INTO_CGROUP
>>
>> Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
>> Signed-off-by: Zou Cao <zoucao@kuaishou.com>
> Is this the same bug fixed by the following commit?
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/commit/?h=for-6.4-fixes&id=2bd110339288c18823dcace602b63b0d8627e520

I believe it is the same bug that this patch is trying to fix. I missed 
the part kargs->cset is cleared in cgroup_post_fork() so that the put 
can be done solely in cgroup_css_set_put_fork(). That is definitely the 
cleaner approach.

Cheers,
Longman

Re: [PATCH] cgroup: fixed the cset refcnt leak when fork() failed

Posted by Waiman Long 2 years, 8 months ago

On 6/5/23 09:04, Zou Cao wrote:
> TeamID: B1486294
What is this?
>
> when fork, cset will be increased by commit "ef2c41cf38a7", the refcnt will
> be decrease by child exit, but when failed in fork(), this refcnt will
> be lost decrease in cgroup_cancel_fork as follow:
>
> copy_process
>       |
> cgroup_can_fork    //  increase the css refcount
>    ......
>    spin_lock_irq(&css_set_lock);
>    cset = task_css_setcurrent);
>    get_css_set(cset);
>    spin_unlock_irq&css_set_lock);
>    ......

The code quoted above is actually in cgroup_css_set_fork(). You may want 
to list it as well.

I believe the problem is in

         if (!(kargs->flags & CLONE_INTO_CGROUP)) {
                 kargs->cset = cset;
                 return 0;
         }

When CLONE_INTO_CGROUP isn't set, a reference to the current cset is taken.

>       |
> goto cgroup_cancel_fork    // if failed in  copy_process
>       |
> cgroup_cancel_fork  // lost the decrease refcount if flag not CLONE_INTO_CGROUP
>
> Fixes: ef2c41cf38a7 ("clone3: allow spawning processes into cgroups")
> Signed-off-by: Zou Cao <zoucao@kuaishou.com>
> ---
>   kernel/cgroup/cgroup.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index d18c2ef..5ecd706 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -6284,6 +6284,11 @@ void cgroup_cancel_fork(struct task_struct *child,
>   		if (ss->cancel_fork)
>   			ss->cancel_fork(child, kargs->cset);
>   
> +	if (!(kargs->flags & CLONE_INTO_CGROUP) &&
> +			kargs->cset) {
> +		put_css_set(kargs->cset);
> +	}
> +
I believe the out_revert error path of cgroup_can_fork() has a similar 
issue. Perhaps you may want to put the put_css_set() call in 
cgroup_css_set_put_fork().
>   	cgroup_css_set_put_fork(kargs);
>   }
>   
Cheers,
Longman

Re: [PATCH] cgroup: fixed the cset refcnt leak when fork() failed

Posted by Waiman Long 2 years, 8 months ago

On 6/5/23 10:12, Waiman Long wrote:
>> kernel/cgroup/cgroup.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>> index d18c2ef..5ecd706 100644
>> --- a/kernel/cgroup/cgroup.c
>> +++ b/kernel/cgroup/cgroup.c
>> @@ -6284,6 +6284,11 @@ void cgroup_cancel_fork(struct task_struct 
>> *child,
>>           if (ss->cancel_fork)
>>               ss->cancel_fork(child, kargs->cset);
>>   +    if (!(kargs->flags & CLONE_INTO_CGROUP) &&
>> +            kargs->cset) {
>> +        put_css_set(kargs->cset);
>> +    }
>> +
> I believe the out_revert error path of cgroup_can_fork() has a similar 
> issue. Perhaps you may want to put the put_css_set() call in 
> cgroup_css_set_put_fork().

Sorry, it should not be done in cgroup_css_set_put_fork(). As the 
increase in reference is properly matched in css_post_fork(). That means 
similar change will be needed in cgroup_can_fork().

Cheers,
Longman