linux-kernel - Re: [PATCH] proc_sysctl: Fix up ->is

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <174978225309.608730.8864073362569294982@noble.neil.brown.name>
Date: Fri, 13 Jun 2025 12:37:33 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Kees Cook" <kees@...nel.org>, "Joel Granados" <joel.granados@...nel.org>,
 linux-fsdevel@...r.kernel.org, "LKML" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] proc_sysctl: Fix up ->is_seen() handling

On Fri, 13 Jun 2025, Al Viro wrote:
> On Fri, Jun 13, 2025 at 02:54:21AM +0100, Al Viro wrote:
> > On Fri, Jun 13, 2025 at 10:37:58AM +1000, NeilBrown wrote:
> > > 
> > > Some sysctl tables can provide an is_seen() function which reports if
> > > the sysctl should be visible to the current process.  This is currently
> > > used to cause d_compare to fail for invisible sysctls.
> > > 
> > > This technique might have worked in 2.6.26 when it was implemented, but
> > > it cannot work now.  In particular if ->d_compare always fails for a
> > > particular name, then d_alloc_parallel() will always create a new dentry
> > > and pass it to lookup() resulting in a new inode for every lookup.  I
> > > tested this by changing sysctl_is_seen() to always return 0.  When
> > > all sysctls were still visible and repeated lookups (ls -li) reported
> > > different inode numbers.
> > 
> > What do you mean, "name"?
> 
> The whole fucking point of that thing is that /proc/sys/net contents for
> processes in different netns is not the same.  And such processes should
> not screw each other into the ground by doing lookups in that area.
> 
> Yes, it means multiple children of the same dentry having the same name
> *and* staying hashed at the same time.
> 

If two threads in the same namespace look up the same name at the same
time (which previously didn't exist), they will both enter
d_alloc_parallel() where neither will notice the other, so both will
create and install d_in_lookup() dentries, and then both will call
->lookup, creating two identical inodes.

I suspect that isn't fatal, but it does seem odd.

Maybe proc_sys_compare should return 0 for d_in_lookup() (aka !inode)
dentries, and then proc_sys_revalidate() can perform the is_seen test
and return -EAGAIN if needed, and __lookup_slow() and others could
interpret that as meaning to "goto again" without calling
d_invalidate().

Maybe.

NeilBrown