linux-kernel - Re: [PATCH 2/2] fs/proc: take rcu_read_lock() in proc_sys

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: <174970262010.608730.16666030974664097741@noble.neil.brown.name>
Date: Thu, 12 Jun 2025 14:30:20 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "David Howells" <dhowells@...hat.com>, "Tyler Hicks" <code@...icks.com>,
 "Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
 "Miklos Szeredi" <miklos@...redi.hu>, "Amir Goldstein" <amir73il@...il.com>,
 "Kees Cook" <kees@...nel.org>, "Joel Granados" <joel.granados@...nel.org>,
 "Namjae Jeon" <linkinjeon@...nel.org>, "Steve French" <smfrench@...il.com>,
 "Sergey Senozhatsky" <senozhatsky@...omium.org>, netfs@...ts.linux.dev,
 linux-kernel@...r.kernel.org, ecryptfs@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-nfs@...r.kernel.org,
 linux-unionfs@...r.kernel.org, linux-cifs@...r.kernel.org
Subject: Re: [PATCH 2/2] fs/proc: take rcu_read_lock() in proc_sys_compare()

On Thu, 12 Jun 2025, Al Viro wrote:
> On Thu, Jun 12, 2025 at 08:57:03AM +1000, NeilBrown wrote:
> 
> > However there is no guarantee that this lock is held by d_same_name()
> > (the caller of ->d_compare).  In particularly d_alloc_parallel() calls
> > d_same_name() after rcu_read_unlock().
> 
> d_alloc_parallel() calls d_same_name() with dentry being pinned;
> if it's positive, nothing's going to happen to its inode,
> rcu_read_lock() or not.  It can go from negative to positive,
> but that's it.
> 
> Why is it needed?  We do care about possibly NULL inode (basically,
> when RCU dcache lookup runs into a dentry getting evicted right
> under it), but that's not relevant here.
> 

Maybe it isn't needed.  Maybe I could fix the warning by removing the
rcu_dereference() (and the RCU_INIT_POINTER() in inode.c).  But then I
might have to pretend that I understand the code - and it makes no
sense.

If a second d_alloc_parallel() is called while there is already a
d_in_lookup() dentry, then ->d_compare will return 1 so a second
d_in_lookup() will be created and ->lookup will be called twice
(possibly concurrently) and both will be added to the dcache.  Probably
not harmful but not really wanted.

And I'm having trouble seeing how sysctl_is_seen() is useful.  If it
reports that the sysctl is not visible to this process, it'll just
create a new dentry/inode which is that same as any other that would be
created... 

NeilBrown