| 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
| ||
|
Message-ID: <20251113-hochphase-sprossen-2ff6e1f23b5b@brauner>
Date: Thu, 13 Nov 2025 14:05:00 +0100
From: Christian Brauner <brauner@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: syzbot <syzbot+0b2e79f91ff6579bfa5b@...kaller.appspotmail.com>,
akpm@...ux-foundation.org, bpf@...r.kernel.org, bsegall@...gle.com, david@...hat.com,
dietmar.eggemann@....com, jsavitz@...hat.com, juri.lelli@...hat.com, kartikey406@...il.com,
kees@...nel.org, liam.howlett@...cle.com, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-mm@...ck.org, linux-security-module@...r.kernel.org,
lorenzo.stoakes@...cle.com, mgorman@...e.de, mhocko@...e.com, mingo@...hat.com,
mjguzik@...il.com, oleg@...hat.com, paul@...l-moore.com, peterz@...radead.org,
rostedt@...dmis.org, rppt@...nel.org, sergeh@...nel.org, surenb@...gle.com,
syzkaller-bugs@...glegroups.com, vbabka@...e.cz, vincent.guittot@...aro.org,
viro@...iv.linux.org.uk, vschneid@...hat.com,
syzbot+0a8655a80e189278487e@...kaller.appspotmail.com
Subject: Re: [PATCH] nsproxy: fix free_nsproxy() and simplify
create_new_namespaces()
On Thu, Nov 13, 2025 at 12:19:40PM +0100, Jan Kara wrote:
> 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@...kaller.appspotmail.com
> > Reported-by: syzbot+0a8655a80e189278487e@...kaller.appspotmail.com
> > Link: https://lore.kernel.org/690bfb9e.050a0220.2e3c35.0013.GAE@google.com
> > Signed-off-by: Christian Brauner <brauner@...nel.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()?
Good idea, I'll rename to that!
>
> Otherwise the patch looks correct to me. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.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@...e.com>
> SUSE Labs, CR
Powered by blists - more mailing lists