[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250801-luftschicht-pochen-b060b1536fe1@brauner>
Date: Fri, 1 Aug 2025 15:32:54 +0200
From: Christian Brauner <brauner@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org, pabeni@...hat.com
Subject: Re: [GIT PULL] Networking for v6.17
On Wed, Jul 30, 2025 at 09:20:46AM -0700, Linus Torvalds wrote:
> On Sat, 26 Jul 2025 at 18:35, Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > Networking changes for 6.17.
>
> So while merging this, there was a trivial conflict with commit
> 9b0240b3ccc3 ("netns: use stable inode number for initial mount ns")
> from the vfs side (acked by networking people).
>
> And the conflict wasn't hard to resolve, but while looking at it, I
> got very unhappy with that conflicting commit from the vfs tree.
>
> Christian - when the "use stable inode number" code triggers, it
> bypasses ns_alloc_inum() entirely. Fine - except that function *also*
> does that
>
> WRITE_ONCE(ns->stashed, NULL);
>
> so now ns->stashed isn't initialized any more.
>
> Now, that shouldn't matter here because this only triggers for
> 'init_net' that is a global data structure and thus initialized to all
> zeroes anyway, but it makes me very unhappy about that pattern that
> ends up being about allocating the pid, but also almost incidentally
> initializing that 'stashed' entry.
>
> I ended up re-organizing the net_ns_net_init() code a bit (because it
> now does that debugfs setup on success, so the old "return 0" didn't
> work), and I think the merge is fine, but I think this "don't call
> ns_alloc_inum()" pattern is wrong.
>
> IOW, I don't think this is a bug, but I think it's not great.
I think we should not be initializing ns->stashed in ns_alloc_inum().
The function name is already wrong for that purpose:
static inline int ns_alloc_inum(struct ns_common *ns)
{
WRITE_ONCE(ns->stashed, NULL);
return proc_alloc_inum(&ns->inum);
}
That was done a long time ago via atomic_long_set() and I just changed
it to WRITE_ONCE() when I reworked both nsfs and pidfs.
We let all callers initialize the fields of struct ns_common embedded in
their respective namespace types already. I see no reason to not just do
the same thing for ns->stashed and drop that implicit initialization
from ns_alloc_inum().
But aside from that I think my patch should have probably been:
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 1b6f3826dd0e..5c39fb544f93 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -815,7 +815,6 @@ static __net_init int net_ns_net_init(struct net *net)
#ifdef CONFIG_NET_NS
net->ns.ops = &netns_operations;
#endif
- net->ns.inum = PROC_NET_INIT_INO;
if (net != &init_net) {
int ret = ns_alloc_inum(&net->ns);
if (ret)
@@ -1283,6 +1282,8 @@ void __init net_ns_init(void)
init_net.key_domain = &init_net_key_domain;
#endif
preinit_net(&init_net, &init_user_ns);
+ init_net.ns.inum = PROC_NET_INIT_INO;
+ init_net.ns.stashed = NULL;
down_write(&pernet_ops_rwsem);
if (setup_net(&init_net))
so the setup for the initial network namespce happens right where it is
explicitly initialized.
Powered by blists - more mailing lists