linux-kernel - Re: [PATCH v3] kernfs: Use RCU for kernfs

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: <20250116133924.XGY8rIaj@linutronix.de>
Date: Thu, 16 Jan 2025 14:39:24 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Tejun Heo <tj@...nel.org>
Cc: cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
	Michal Koutný <mkoutny@...e.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Hillf Danton <hdanton@...a.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Marco Elver <elver@...gle.com>, Zefan Li <lizefan.x@...edance.com>,
	tglx@...utronix.de,
	syzbot+6ea37e2e6ffccf41a7e6@...kaller.appspotmail.com
Subject: Re: [PATCH v3] kernfs: Use RCU for kernfs_node::name and ::parent
 lookup.

On 2025-01-16 14:27:47 [+0100], To Tejun Heo wrote:
> > > @@ -557,16 +568,18 @@ void kernfs_put(struct kernfs_node *kn)
> > >  	if (!kn || !atomic_dec_and_test(&kn->count))
> > >  		return;
> > >  	root = kernfs_root(kn);
> > > +	guard(rcu)();
> > >   repeat:
> > >  	/*
> > >  	 * Moving/renaming is always done while holding reference.
> > >  	 * kn->parent won't change beneath us.
> > >  	 */
> > > -	parent = kn->parent;
> > > +	parent = rcu_dereference(kn->parent);
> > 
> > I wonder whether it'd be better to encode the reference count rule (ie. add
> > the condition kn->count == 0 to deref_check) in the kn->parent deref
> > accessor. This function doesn't need RCU read lock and holding it makes it
> > more confusing.
> 
> You are saying that we don't need RCU here because if we drop the last
> reference then nobody can rename the node anymore and so parent can't
> change. That sounds right.
> What about using rcu_dereference_protected() instead? Using
> rcu_dereference(x, !atomic_read(&kn->count)) looks odd given that we
> established that the counter is 0. Therefore I would suggest
> rcu_access_pointer() but the reference drop might qualify as "locked".

Ehm or indeed rcu_access_pointer() given that _protected() requires a
second argument…

Sebastian