linux-kernel - Re: [PATCH v4 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: <Z5fJ1m9ve8edEH1F@slm.duckdns.org>
Date: Mon, 27 Jan 2025 08:00:54 -1000
From: Tejun Heo <tj@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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
Subject: Re: [PATCH v4 5/6] kernfs: Use RCU to access kernfs_node::parent.

Hello,

On Mon, Jan 27, 2025 at 05:25:43PM +0100, Sebastian Andrzej Siewior wrote:
> > > -	return strscpy(buf, kn->parent ? kn->name : "/", buflen);
> > > +	return strscpy(buf, rcu_access_pointer(kn->__parent) ? kn->name : "/", buflen);
> > 
> > rcu_access_pointer() is for when only the pointer value is used without
> > dereferencing it. Here, the poiner is being dereferenced.
> 
> Is it? It checks if the pointer NULL and if so "/" is used otherwise
> "kn->name". The __parent pointer itself is not dereferenced. 

Ah, ignore me. I was misreading.

> > > +static inline struct kernfs_node *kernfs_parent(const struct kernfs_node *kn)
> > > +{
> > > +	return rcu_dereference_check(kn->__parent, kernfs_root_is_locked(kn));
> > > +}
> > 
> > AFAICS, all rules can be put into this helper, no?
> 
> This would work. kernfs_parent() is the "general purpose" access. It is
> used in most places (the kernfs_rename_ns() usage is moved to
> kernfs_parent() in the following patch, ended here open coded during the
> split, fixed now).
> 
> The "!atomic_read(&kn->count)" rule is a special case used only in
> kernfs_put() after the counter went to 0 and should not be used (used as
> in be valid) anywhere else. This is special because is going away and
> __parent can not be renamed/ replaced at this point. One user in total.
> 
> The "lockdep_is_held(&kernfs_rename_lock)" rule is only used in
> kernfs_get_parent(). One user in total.
> 
> Adding these two cases to kernfs_parent() will bloat the code a
> little in the debug case (where the check is expanded). Also it will
> require to make kernfs_rename_lock global so it be accessed outside of
> dir.c.
> All in all I don't think it is worth it. If you however prefer it that
> way, I sure can update it.

Hmm... maybe other people have different preferences here but I much prefer
documenting and enforcing RCU deref rules in a single place. It only adds
debug annotations that go away in prod builds while clarifying subtleties.
The trade-off seems pretty one-sided to me.

Thanks.

-- 
tejun