linux-kernel - Re: [PATCH v5 5/6] kernfs: Use RCU to access 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: <20250129202614.eK7sf6Jt@linutronix.de>
Date: Wed, 29 Jan 2025 21:26:14 +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>, tglx@...utronix.de
Subject: Re: [PATCH v5 5/6] kernfs: Use RCU to access kernfs_node::parent.

On 2025-01-29 06:54:33 [-1000], Tejun Heo wrote:
> Hello,
> 
> On Wed, Jan 29, 2025 at 02:23:11PM +0100, Sebastian Andrzej Siewior wrote:
> > > > @@ -64,9 +64,9 @@ static size_t kernfs_depth(struct kernfs_node *from, struct kernfs_node *to)
> > > >  {
> > > >  	size_t depth = 0;
> > > >  
> > > > -	while (to->parent && to != from) {
> > > > +	while (rcu_dereference(to->__parent) && to != from) {
> > > 
> > > Why not use kernfs_parent() here and other places?
> > 
> > Because it is from within RCU section and the other checks are not
> > required. If you prefer this instead, I sure can update it.
> 
> Hmm... I would have gone with using the same accessor everywhere but am not
> sure how strongly I feel about it. I don't think it's useful to worry about
> the overhead of the extra lockdep annotations in debug builds. Ignoring that
> and just considering code readability, what would you do?

It is your call. I would prefer to open code that part that we do only
rely on RCU here but sure understand that you don't care about the
details and want to have only one accessor.

> > > > @@ -226,6 +227,7 @@ int kernfs_path_from_node(struct kernfs_node *to, struct kernfs_node *from,
> > > >  	unsigned long flags;
> > > >  	int ret;
> > > >  
> > > > +	guard(rcu)();
> > > 
> > > Doesn't irqsave imply rcu?
…
> > Also, rcu_dereference() will complain about missing RCU annotation. On
> > PREEMPT_RT rcu_dereference_sched() will complain because irqsave (in
> > this case) will not disable interrupts.
> 
> You know this a lot better than I do. If it's necessary for RT builds, it's
> not redundant.

Good.

> Thanks.

Sebastian