proc_sys_compare() is the ->d_compare function for /proc/sys.
It uses rcu_dereference() which assumes the RCU read lock is held and
can complain if it isn't.

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().

So this patch calls rcu_read_lock() before accessing the inode (which
seems to be the focus of RCU protection here), and drops it afterwards.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/proc/proc_sysctl.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index cc9d74a06ff0..a4cdc0a189ef 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -917,19 +917,23 @@ static int proc_sys_compare(const struct dentry *dentry,
 {
 	struct ctl_table_header *head;
 	struct inode *inode;
+	int ret;
 
 	/* Although proc doesn't have negative dentries, rcu-walk means
 	 * that inode here can be NULL */
 	/* AV: can it, indeed? */
+	rcu_read_lock();
 	inode = d_inode_rcu(dentry);
-	if (!inode)
-		return 1;
-	if (name->len != len)
-		return 1;
-	if (memcmp(name->name, str, len))
-		return 1;
-	head = rcu_dereference(PROC_I(inode)->sysctl);
-	return !head || !sysctl_is_seen(head);
+	if (!inode ||
+	    name->len != len ||
+	    memcmp(name->name, str, len)) {
+		ret = 1;
+	} else {
+		head = rcu_dereference(PROC_I(inode)->sysctl);
+		ret = !head || !sysctl_is_seen(head);
+	}
+	rcu_read_unlock();
+	return ret;
 }
 
 static const struct dentry_operations proc_sys_dentry_operations = {
-- 
2.49.0

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.

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