Re: [PATCH] nsproxy: fix free_nsproxy() and simplify create_new_namespaces()

[Jan Kara <jack@suse.cz>]

On Tue 11-11-25 22:29:44, Christian Brauner wrote:
> Make it possible to handle NULL being passed to the reference count
> helpers instead of forcing the caller to handle this. Afterwards we can
> nicely allow a cleanup guard to handle nsproxy freeing.
> 
> Active reference count handling is not done in nsproxy_free() but rather
> in free_nsproxy() as nsproxy_free() is also called from setns() failure
> paths where a new nsproxy has been prepared but has not been marked as
> active via switch_task_namespaces().
> 
> Fixes: 3c9820d5c64a ("ns: add active reference count")
> Reported-by: syzbot+0b2e79f91ff6579bfa5b@syzkaller.appspotmail.com
> Reported-by: syzbot+0a8655a80e189278487e@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/690bfb9e.050a0220.2e3c35.0013.GAE@google.com
> Signed-off-by: Christian Brauner <brauner@kernel.org>

I believe having free_nsproxy() and nsproxy_free() functions with
the same signature and slightly different semantics is making things too
easy to get wrong. Maybe call free_nsproxy() say deactivate_nsproxy()?

Otherwise the patch looks correct to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/ns_common.h |  11 ++--
>  kernel/nsproxy.c          | 107 +++++++++++++++-----------------------
>  2 files changed, 48 insertions(+), 70 deletions(-)
> 
> diff --git a/include/linux/ns_common.h b/include/linux/ns_common.h
> index 136f6a322e53..825f5865bfc5 100644
> --- a/include/linux/ns_common.h
> +++ b/include/linux/ns_common.h
> @@ -114,11 +114,14 @@ static __always_inline __must_check bool __ns_ref_dec_and_lock(struct ns_common
>  }
>  
>  #define ns_ref_read(__ns) __ns_ref_read(to_ns_common((__ns)))
> -#define ns_ref_inc(__ns) __ns_ref_inc(to_ns_common((__ns)))
> -#define ns_ref_get(__ns) __ns_ref_get(to_ns_common((__ns)))
> -#define ns_ref_put(__ns) __ns_ref_put(to_ns_common((__ns)))
> +#define ns_ref_inc(__ns) \
> +	do { if (__ns) __ns_ref_inc(to_ns_common((__ns))); } while (0)
> +#define ns_ref_get(__ns) \
> +	((__ns) ? __ns_ref_get(to_ns_common((__ns))) : false)
> +#define ns_ref_put(__ns) \
> +	((__ns) ? __ns_ref_put(to_ns_common((__ns))) : false)
>  #define ns_ref_put_and_lock(__ns, __ns_lock) \
> -	__ns_ref_dec_and_lock(to_ns_common((__ns)), __ns_lock)
> +	((__ns) ? __ns_ref_dec_and_lock(to_ns_common((__ns)), __ns_lock) : false)
>  
>  #define ns_ref_active_read(__ns) \
>  	((__ns) ? __ns_ref_active_read(to_ns_common(__ns)) : 0)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index 94c2cfe0afa1..2c94452dc793 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -60,6 +60,27 @@ static inline struct nsproxy *create_nsproxy(void)
>  	return nsproxy;
>  }
>  
> +static inline void nsproxy_free(struct nsproxy *ns)
> +{
> +	put_mnt_ns(ns->mnt_ns);
> +	put_uts_ns(ns->uts_ns);
> +	put_ipc_ns(ns->ipc_ns);
> +	put_pid_ns(ns->pid_ns_for_children);
> +	put_time_ns(ns->time_ns);
> +	put_time_ns(ns->time_ns_for_children);
> +	put_cgroup_ns(ns->cgroup_ns);
> +	put_net(ns->net_ns);
> +	kmem_cache_free(nsproxy_cachep, ns);
> +}
> +
> +DEFINE_FREE(nsproxy_free, struct nsproxy *, if (_T) nsproxy_free(_T))
> +
> +void free_nsproxy(struct nsproxy *ns)
> +{
> +	nsproxy_ns_active_put(ns);
> +	nsproxy_free(ns);
> +}
> +
>  /*
>   * Create new nsproxy and all of its the associated namespaces.
>   * Return the newly created nsproxy.  Do not attach this to the task,
> @@ -69,76 +90,45 @@ static struct nsproxy *create_new_namespaces(u64 flags,
>  	struct task_struct *tsk, struct user_namespace *user_ns,
>  	struct fs_struct *new_fs)
>  {
> -	struct nsproxy *new_nsp;
> -	int err;
> +	struct nsproxy *new_nsp __free(nsproxy_free) = NULL;
>  
>  	new_nsp = create_nsproxy();
>  	if (!new_nsp)
>  		return ERR_PTR(-ENOMEM);
>  
>  	new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);
> -	if (IS_ERR(new_nsp->mnt_ns)) {
> -		err = PTR_ERR(new_nsp->mnt_ns);
> -		goto out_ns;
> -	}
> +	if (IS_ERR(new_nsp->mnt_ns))
> +		return ERR_CAST(new_nsp->mnt_ns);
>  
>  	new_nsp->uts_ns = copy_utsname(flags, user_ns, tsk->nsproxy->uts_ns);
> -	if (IS_ERR(new_nsp->uts_ns)) {
> -		err = PTR_ERR(new_nsp->uts_ns);
> -		goto out_uts;
> -	}
> +	if (IS_ERR(new_nsp->uts_ns))
> +		return ERR_CAST(new_nsp->uts_ns);
>  
>  	new_nsp->ipc_ns = copy_ipcs(flags, user_ns, tsk->nsproxy->ipc_ns);
> -	if (IS_ERR(new_nsp->ipc_ns)) {
> -		err = PTR_ERR(new_nsp->ipc_ns);
> -		goto out_ipc;
> -	}
> +	if (IS_ERR(new_nsp->ipc_ns))
> +		return ERR_CAST(new_nsp->ipc_ns);
>  
> -	new_nsp->pid_ns_for_children =
> -		copy_pid_ns(flags, user_ns, tsk->nsproxy->pid_ns_for_children);
> -	if (IS_ERR(new_nsp->pid_ns_for_children)) {
> -		err = PTR_ERR(new_nsp->pid_ns_for_children);
> -		goto out_pid;
> -	}
> +	new_nsp->pid_ns_for_children = copy_pid_ns(flags, user_ns,
> +						   tsk->nsproxy->pid_ns_for_children);
> +	if (IS_ERR(new_nsp->pid_ns_for_children))
> +		return ERR_CAST(new_nsp->pid_ns_for_children);
>  
>  	new_nsp->cgroup_ns = copy_cgroup_ns(flags, user_ns,
>  					    tsk->nsproxy->cgroup_ns);
> -	if (IS_ERR(new_nsp->cgroup_ns)) {
> -		err = PTR_ERR(new_nsp->cgroup_ns);
> -		goto out_cgroup;
> -	}
> +	if (IS_ERR(new_nsp->cgroup_ns))
> +		return ERR_CAST(new_nsp->cgroup_ns);
>  
>  	new_nsp->net_ns = copy_net_ns(flags, user_ns, tsk->nsproxy->net_ns);
> -	if (IS_ERR(new_nsp->net_ns)) {
> -		err = PTR_ERR(new_nsp->net_ns);
> -		goto out_net;
> -	}
> +	if (IS_ERR(new_nsp->net_ns))
> +		return ERR_CAST(new_nsp->net_ns);
>  
>  	new_nsp->time_ns_for_children = copy_time_ns(flags, user_ns,
> -					tsk->nsproxy->time_ns_for_children);
> -	if (IS_ERR(new_nsp->time_ns_for_children)) {
> -		err = PTR_ERR(new_nsp->time_ns_for_children);
> -		goto out_time;
> -	}
> +						     tsk->nsproxy->time_ns_for_children);
> +	if (IS_ERR(new_nsp->time_ns_for_children))
> +		return ERR_CAST(new_nsp->time_ns_for_children);
>  	new_nsp->time_ns = get_time_ns(tsk->nsproxy->time_ns);
>  
> -	return new_nsp;
> -
> -out_time:
> -	put_net(new_nsp->net_ns);
> -out_net:
> -	put_cgroup_ns(new_nsp->cgroup_ns);
> -out_cgroup:
> -	put_pid_ns(new_nsp->pid_ns_for_children);
> -out_pid:
> -	put_ipc_ns(new_nsp->ipc_ns);
> -out_ipc:
> -	put_uts_ns(new_nsp->uts_ns);
> -out_uts:
> -	put_mnt_ns(new_nsp->mnt_ns);
> -out_ns:
> -	kmem_cache_free(nsproxy_cachep, new_nsp);
> -	return ERR_PTR(err);
> +	return no_free_ptr(new_nsp);
>  }
>  
>  /*
> @@ -185,21 +175,6 @@ int copy_namespaces(u64 flags, struct task_struct *tsk)
>  	return 0;
>  }
>  
> -void free_nsproxy(struct nsproxy *ns)
> -{
> -	nsproxy_ns_active_put(ns);
> -
> -	put_mnt_ns(ns->mnt_ns);
> -	put_uts_ns(ns->uts_ns);
> -	put_ipc_ns(ns->ipc_ns);
> -	put_pid_ns(ns->pid_ns_for_children);
> -	put_time_ns(ns->time_ns);
> -	put_time_ns(ns->time_ns_for_children);
> -	put_cgroup_ns(ns->cgroup_ns);
> -	put_net(ns->net_ns);
> -	kmem_cache_free(nsproxy_cachep, ns);
> -}
> -
>  /*
>   * Called from unshare. Unshare all the namespaces part of nsproxy.
>   * On success, returns the new nsproxy.
> @@ -338,7 +313,7 @@ static void put_nsset(struct nsset *nsset)
>  	if (nsset->fs && (flags & CLONE_NEWNS) && (flags & ~CLONE_NEWNS))
>  		free_fs_struct(nsset->fs);
>  	if (nsset->nsproxy)
> -		free_nsproxy(nsset->nsproxy);
> +		nsproxy_free(nsset->nsproxy);
>  }
>  
>  static int prepare_nsset(unsigned flags, struct nsset *nsset)
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR