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] [day] [month] [year] [list]

Message-ID: <20241125180226.Qo_rHBoM@linutronix.de>
Date: Mon, 25 Nov 2024 19:02:26 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Michal Koutný <mkoutny@...e.com>
Cc: cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
	"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>, Tejun Heo <tj@...nel.org>,
	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 2024-11-25 15:49:34 [+0100], Michal Koutný wrote:
> Hello.
Hi Michal,

> On Thu, Nov 21, 2024 at 06:52:50PM GMT, Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> 
> > - kernfs_rename_ns() is only using kernfs_rename_lock if the parents are
> >   different. All users users use either RCU or kernfs_rwsem.
> > - kernfs_fop_readdir() drops kernfs_root::kernfs_rwsem while holding a
> >   reference to name and invoking dir_emit(). This has been changed and
> >   lock is held.
> > - kernfs_notify_workfn() access kernfs_node::name without any
> >   protection. Added kernfs_root::kernfs_rwsem for the iteration.
> > - kernfs_get_parent_dentry() acquires now kernfs_root::kernfs_rwsem
> >   while accessing the parent node.
> > - kernfs_node_dentry() acquires now kernfs_root::kernfs_rwsem while
> >   parent is accessed and the name looked up.
> 
> Why is the kernfs_root::kernfs_rwsem newly R-taken? Shouldn't be RCU
> read section sufficient for those users?

Those users. If I skip/ left something out, please poke.

kernfs_notify_workfn(). There is ilookup() -> wait_on_inode() which can
sleep.
kernfs_get_parent_dentry(). There is kernfs_get_inode() -> iget_locked()
which can sleep.
kernfs_node_dentry(). There is lookup_positive_unlocked() ->
lookup_one_unlocked() -> lookup_slow() which might sleep.

Assuming the parent can't vanish in these cases, name could during the
invocation. I can't keep the RCU read section open while there is a
sleep within the call chain. Therefore I added the lock so the
rcu_dereference.*() is quiet.

> (Perhaps it's related to second observation I have -- why there is
> sometimes kernfs_rcu_get_parent() whereas there are other call sites
> with mere rcu_dereference(kn->parent)?)

rcu_dereference() is used where I was sure that there is always a RCU
read section. I have kernfs_rcu_get_parent() when there is either a RCU
read section or the kernfs_rwsem (or just the lock). 

Sebastian