netdev - Re: [GIT PULL] Networking for v6.17

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] [thread-next>] [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.