linux-kernel - Re: [PATCH] nsproxy: use put_nsproxy() in switch_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201118131341.jxmqocn2vr3hyftt@wittgenstein>
Date:   Wed, 18 Nov 2020 14:13:41 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Hui Su <sh_def@....com>
Cc:     serge@...lyn.com, avagin@...nvz.org, 0x7f454c46@...il.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nsproxy: use put_nsproxy() in switch_task_namespaces()

On Mon, Nov 16, 2020 at 02:00:54AM +0800, Hui Su wrote:
> Use put_nsproxy() instead of '
> if (atomic_dec_and_test(&ns->count)) {
> 	free_nsproxy(ns);
> }' in switch_task_namespaces().
> 
> and remove the whitespace by the way.
> 
> Signed-off-by: Hui Su <sh_def@....com>
> ---

Acked-by: Christian Brauner <christian.brauner@...ntu.com>
Thanks! I'm picking this up now.

I'll add to the commit message though sm like:

"We already have a dedicated helper that handles reference count
checking so stop open-coding the reference count check in
switch_task_namespaces() and use the dedicated put_nsproxy() helper
instead."

(Looking at put_nsproxy() I feel we should probably start mandating that
all free()/put() helpers should gracefully handle when passed NULL to
avoid:
if (ns)
	put()
calls. But that's independent of this fix.)

>  kernel/nsproxy.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index 12dd41b39a7f..3ebfd090398a 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -173,7 +173,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
>  	 * it along with CLONE_NEWIPC.
>  	 */
>  	if ((flags & (CLONE_NEWIPC | CLONE_SYSVSEM)) ==
> -		(CLONE_NEWIPC | CLONE_SYSVSEM)) 
> +		(CLONE_NEWIPC | CLONE_SYSVSEM))
>  		return -EINVAL;
>  
>  	new_ns = create_new_namespaces(flags, tsk, user_ns, tsk->fs);
> @@ -250,8 +250,8 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
>  	p->nsproxy = new;
>  	task_unlock(p);
>  
> -	if (ns && atomic_dec_and_test(&ns->count))
> -		free_nsproxy(ns);
> +	if (ns)
> +		put_nsproxy(ns);
>  }
>  
>  void exit_task_namespaces(struct task_struct *p)
> -- 
> 2.29.0
> 
>