Re: [PATCH v3] kernfs: Use RCU for kernfs_node::name and ::parent lookup.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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@linutronix.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