Centralize ns_common initialization.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
net/core/net_namespace.c | 23 +++--------------------
1 file changed, 3 insertions(+), 20 deletions(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a57b3cda8dbc..897f4927df9e 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -409,7 +409,7 @@ static __net_init int preinit_net(struct net *net, struct user_namespace *user_n
ns_ops = NULL;
#endif
- ret = ns_common_init(&net->ns, ns_ops, false);
+ ret = ns_common_init(&net->ns, ns_ops, true);
if (ret)
return ret;
@@ -597,6 +597,7 @@ struct net *copy_net_ns(unsigned long flags,
net_passive_dec(net);
dec_ucounts:
dec_net_namespaces(ucounts);
+ ns_free_inum(&net->ns);
return ERR_PTR(rv);
}
return net;
@@ -718,6 +719,7 @@ static void cleanup_net(struct work_struct *work)
#endif
put_user_ns(net->user_ns);
net_passive_dec(net);
+ ns_free_inum(&net->ns);
}
WRITE_ONCE(cleanup_net_task, NULL);
}
@@ -831,31 +833,12 @@ static void net_ns_net_debugfs(struct net *net)
static __net_init int net_ns_net_init(struct net *net)
{
- int ret = 0;
-
- if (net == &init_net)
- net->ns.inum = PROC_NET_INIT_INO;
- else
- ret = proc_alloc_inum(&to_ns_common(net)->inum);
- if (ret)
- return ret;
-
net_ns_net_debugfs(net);
return 0;
}
-static __net_exit void net_ns_net_exit(struct net *net)
-{
- /*
- * Initial network namespace doesn't exit so we don't need any
- * special checks here.
- */
- ns_free_inum(&net->ns);
-}
-
static struct pernet_operations __net_initdata net_ns_ops = {
.init = net_ns_net_init,
- .exit = net_ns_net_exit,
};
static const struct nla_policy rtnl_net_policy[NETNSA_MAX + 1] = {
--
2.47.3On Wed 17-09-25 12:28:06, Christian Brauner wrote: > Centralize ns_common initialization. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > net/core/net_namespace.c | 23 +++-------------------- > 1 file changed, 3 insertions(+), 20 deletions(-) > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c > index a57b3cda8dbc..897f4927df9e 100644 > --- a/net/core/net_namespace.c > +++ b/net/core/net_namespace.c > @@ -409,7 +409,7 @@ static __net_init int preinit_net(struct net *net, struct user_namespace *user_n > ns_ops = NULL; > #endif > > - ret = ns_common_init(&net->ns, ns_ops, false); > + ret = ns_common_init(&net->ns, ns_ops, true); > if (ret) > return ret; > > @@ -597,6 +597,7 @@ struct net *copy_net_ns(unsigned long flags, > net_passive_dec(net); > dec_ucounts: > dec_net_namespaces(ucounts); > + ns_free_inum(&net->ns); This looks like a wrong place to put it? dec_ucounts also gets called when we failed to create 'net' and thus net == NULL. > return ERR_PTR(rv); > } > return net; > @@ -718,6 +719,7 @@ static void cleanup_net(struct work_struct *work) > #endif > put_user_ns(net->user_ns); > net_passive_dec(net); > + ns_free_inum(&net->ns); The calling of ns_free_inum() after we've dropped our reference (net_passive_dec()) looks suspicious. Given how 'net' freeing works I don't think this can lead to actual UAF issues but it is in my opinion a bad coding pattern and for no good reason AFAICT. > } > WRITE_ONCE(cleanup_net_task, NULL); > } Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
On Thu, Sep 18, 2025 at 11:42:38AM +0200, Jan Kara wrote:
> On Wed 17-09-25 12:28:06, Christian Brauner wrote:
> > Centralize ns_common initialization.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> > net/core/net_namespace.c | 23 +++--------------------
> > 1 file changed, 3 insertions(+), 20 deletions(-)
> >
> > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> > index a57b3cda8dbc..897f4927df9e 100644
> > --- a/net/core/net_namespace.c
> > +++ b/net/core/net_namespace.c
> > @@ -409,7 +409,7 @@ static __net_init int preinit_net(struct net *net, struct user_namespace *user_n
> > ns_ops = NULL;
> > #endif
> >
> > - ret = ns_common_init(&net->ns, ns_ops, false);
> > + ret = ns_common_init(&net->ns, ns_ops, true);
> > if (ret)
> > return ret;
> >
> > @@ -597,6 +597,7 @@ struct net *copy_net_ns(unsigned long flags,
> > net_passive_dec(net);
> > dec_ucounts:
> > dec_net_namespaces(ucounts);
> > + ns_free_inum(&net->ns);
>
> This looks like a wrong place to put it? dec_ucounts also gets called when we
> failed to create 'net' and thus net == NULL.
>
> > return ERR_PTR(rv);
> > }
> > return net;
> > @@ -718,6 +719,7 @@ static void cleanup_net(struct work_struct *work)
> > #endif
> > put_user_ns(net->user_ns);
> > net_passive_dec(net);
> > + ns_free_inum(&net->ns);
>
> The calling of ns_free_inum() after we've dropped our reference
> (net_passive_dec()) looks suspicious. Given how 'net' freeing works I don't
> think this can lead to actual UAF issues but it is in my opinion a bad
> coding pattern and for no good reason AFAICT.
All good points. I can't say I'm fond of the complexity in this specific
instance in general.
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 897f4927df9e..9df236811454 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -590,6 +590,7 @@ struct net *copy_net_ns(unsigned long flags,
if (rv < 0) {
put_userns:
+ ns_free_inum(&net->ns);
#ifdef CONFIG_KEYS
key_remove_domain(net->key_domain);
#endif
@@ -597,7 +598,6 @@ struct net *copy_net_ns(unsigned long flags,
net_passive_dec(net);
dec_ucounts:
dec_net_namespaces(ucounts);
- ns_free_inum(&net->ns);
return ERR_PTR(rv);
}
return net;
@@ -713,13 +713,13 @@ static void cleanup_net(struct work_struct *work)
/* Finally it is safe to free my network namespace structure */
list_for_each_entry_safe(net, tmp, &net_exit_list, exit_list) {
list_del_init(&net->exit_list);
+ ns_free_inum(&net->ns);
dec_net_namespaces(net->ucounts);
#ifdef CONFIG_KEYS
key_remove_domain(net->key_domain);
#endif
put_user_ns(net->user_ns);
net_passive_dec(net);
- ns_free_inum(&net->ns);
}
WRITE_ONCE(cleanup_net_task, NULL);
}
On Fri 19-09-25 10:08:33, Christian Brauner wrote:
> On Thu, Sep 18, 2025 at 11:42:38AM +0200, Jan Kara wrote:
> > On Wed 17-09-25 12:28:06, Christian Brauner wrote:
> > > Centralize ns_common initialization.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > ---
> > > net/core/net_namespace.c | 23 +++--------------------
> > > 1 file changed, 3 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> > > index a57b3cda8dbc..897f4927df9e 100644
> > > --- a/net/core/net_namespace.c
> > > +++ b/net/core/net_namespace.c
> > > @@ -409,7 +409,7 @@ static __net_init int preinit_net(struct net *net, struct user_namespace *user_n
> > > ns_ops = NULL;
> > > #endif
> > >
> > > - ret = ns_common_init(&net->ns, ns_ops, false);
> > > + ret = ns_common_init(&net->ns, ns_ops, true);
> > > if (ret)
> > > return ret;
> > >
> > > @@ -597,6 +597,7 @@ struct net *copy_net_ns(unsigned long flags,
> > > net_passive_dec(net);
> > > dec_ucounts:
> > > dec_net_namespaces(ucounts);
> > > + ns_free_inum(&net->ns);
> >
> > This looks like a wrong place to put it? dec_ucounts also gets called when we
> > failed to create 'net' and thus net == NULL.
> >
> > > return ERR_PTR(rv);
> > > }
> > > return net;
> > > @@ -718,6 +719,7 @@ static void cleanup_net(struct work_struct *work)
> > > #endif
> > > put_user_ns(net->user_ns);
> > > net_passive_dec(net);
> > > + ns_free_inum(&net->ns);
> >
> > The calling of ns_free_inum() after we've dropped our reference
> > (net_passive_dec()) looks suspicious. Given how 'net' freeing works I don't
> > think this can lead to actual UAF issues but it is in my opinion a bad
> > coding pattern and for no good reason AFAICT.
>
> All good points. I can't say I'm fond of the complexity in this specific
> instance in general.
Agreed. The changes look good to me now. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 897f4927df9e..9df236811454 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -590,6 +590,7 @@ struct net *copy_net_ns(unsigned long flags,
>
> if (rv < 0) {
> put_userns:
> + ns_free_inum(&net->ns);
> #ifdef CONFIG_KEYS
> key_remove_domain(net->key_domain);
> #endif
> @@ -597,7 +598,6 @@ struct net *copy_net_ns(unsigned long flags,
> net_passive_dec(net);
> dec_ucounts:
> dec_net_namespaces(ucounts);
> - ns_free_inum(&net->ns);
> return ERR_PTR(rv);
> }
> return net;
> @@ -713,13 +713,13 @@ static void cleanup_net(struct work_struct *work)
> /* Finally it is safe to free my network namespace structure */
> list_for_each_entry_safe(net, tmp, &net_exit_list, exit_list) {
> list_del_init(&net->exit_list);
> + ns_free_inum(&net->ns);
> dec_net_namespaces(net->ucounts);
> #ifdef CONFIG_KEYS
> key_remove_domain(net->key_domain);
> #endif
> put_user_ns(net->user_ns);
> net_passive_dec(net);
> - ns_free_inum(&net->ns);
> }
> WRITE_ONCE(cleanup_net_task, NULL);
> }
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
© 2016 - 2025 Red Hat, Inc.