Re: [PATCH 4/4] kernfs: Remove kernfs_rwsem from dentry revalidation

[Sandeep Dhavale]

Hi Shakeel and Christian,

On Thu, Aug 20, 2026 at 10:05 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
>
> kernfs_dop_revalidate() takes kernfs_rwsem for read once per path component
> of every walk into a kernfs mount. Linux rwsems do not permit reader lock
> stealing once a writer is queued, so a single writer parks the whole
> incoming reader stream in uninterruptible sleep, stalling cgroup-polling
> daemons for minutes.
>
> Nothing the callback reads requires the semaphore. kn->active is an
> atomic_t that kernfs_find_and_get_node_by_id() already tests through
> __kernfs_active(); kn->__parent and kn->name are RCU pointers whose old
> values are freed only after a grace period; kn->ns is now compared rather
> than dereferenced; parent->dir.rev was annotated earlier in this series.
>
> What the semaphore does provide is a coherent snapshot, and that is not
> needed. ->d_revalidate() answers a question about a single instant, and the
> answer is already stale when it returns: a rename landing just after
> up_read() gives the same outcome as one observed mid-read. A lockless
> reader can only return "valid" for the (parent, name, namespace) triple
> identifying the dentry it was handed, and that triple was true when the
> dentry was instantiated, so it reports a genuine past state exactly as the
> locked version did. Removal is backstopped by kernfs_get_active() failing
> in the subsequent open().
>
> Take an RCU read lock instead. kernfs_parent() and kernfs_rcu_name() work
> unchanged: the condition in their rcu_dereference_check() is an alternative
> to holding the RCU read lock, not an extra requirement. The negative dentry
> path needs nothing, as @dir pins the parent. The namespace check can use
> @parent directly once the preceding check establishes it equals
> kernfs_parent(kn), so the kn_parent local and its NULL test go away.
>
> kernfs_ns_enabled() reads @parent->flags, which KERNFS_ACTIVATED and
> KERNFS_REMOVING update as a plain read-modify-write under kernfs_rwsem.
> Those bits are not read here and KERNFS_NS cannot change once the directory
> has children, so mark the read data_race() rather than READ_ONCE(), which
> would not silence KCSAN against the unmarked writers anyway.
>
> kernfs_iop_permission() still forces every walk out of RCU-walk before
> children are revalidated, so lifting the LOOKUP_RCU bail here would have no
> observable effect; it is left to the series fixing that path.
>
> Readers walking cgroupfs and sysfs against concurrent cgroup and netdev
> churn: kernfs_rwsem read acquisitions drop from 48,593,360 to 1,280,280,
> and kernfs_dop_revalidate() no longer appears among its contention sites.
>
> Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> ---
>  fs/kernfs/dir.c | 49 ++++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 29 deletions(-)
>
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 27949b0e027c..cd7a8ff8b6b2 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -1171,9 +1171,8 @@ struct kernfs_node *kernfs_create_empty_dir(struct kernfs_node *parent,
>  static int kernfs_dop_revalidate(struct inode *dir, const struct qstr *name,
>                                  struct dentry *dentry, unsigned int flags)
>  {
> -       struct kernfs_node *kn, *kn_parent;
>         struct kernfs_node *parent = dir->i_private;
> -       struct kernfs_root *root;
> +       struct kernfs_node *kn;
>         const char *kn_name;
>
>         if (flags & LOOKUP_RCU)
> @@ -1191,49 +1190,41 @@ static int kernfs_dop_revalidate(struct inode *dir, const struct qstr *name,
>                  * changes and the lookup re-done so that a new positive
>                  * dentry can be properly created.
>                  */
> -               root = kernfs_root(parent);
> -               down_read(&root->kernfs_rwsem);
> -               if (kernfs_dir_changed(parent, dentry)) {
> -                       up_read(&root->kernfs_rwsem);
> -                       return 0;
> -               }
> -               up_read(&root->kernfs_rwsem);
> -
> -               /* The kernfs parent node hasn't changed, leave the
> -                * dentry negative and return success.
> -                */
> -               return 1;
> +               return !kernfs_dir_changed(parent, dentry);
>         }
>
>         kn = kernfs_dentry_node(dentry);
> -       root = kernfs_root(kn);
> -       down_read(&root->kernfs_rwsem);
> +
> +       guard(rcu)();
>
>         /* The kernfs node has been deactivated */
> -       if (!kernfs_active(kn))
> -               goto out_bad;
> +       if (!__kernfs_active(kn))
> +               return 0;
>

I have couple of reports on android kernel based on 6.12 with callstack

[   41.137176][ T5373] pc : kernfs_dop_revalidate+0x28/0x108
[   41.137180][ T5373] lr : path_openat+0x7ec/0x1b04
[   41.137183][ T5373] sp : ffffffc0b02fbba0
[   41.137184][ T5373] x29: ffffffc0b02fbba0 x28: ffffffc0b02fbda4
x27: ffffff8917f90000
[   41.137187][ T5373] x26: 0000000000000030 x25: ffffff8918985860
x24: 0000000000028000
[   41.137189][ T5373] x23: ffffff8918985040 x22: 0000000000000040
x21: 0000000000000000
[   41.137192][ T5373] x20: ffffff8918985040 x19: ffffff894321d000
x18: ffffffdc3adec8c0
[   41.137194][ T5373] x17: 000000004495e485 x16: 000000004495e485
x15: ffffffffff000000
[   41.137197][ T5373] x14: 7d6770f3f4cee991 x13: 0000000000737365
x12: 0000000000737365
[   41.137199][ T5373] x11: ffffff8918985088 x10: 0000000000000018 x9
: 0000000100000000
[   41.137202][ T5373] x8 : ffffff89dc6c3d30 x7 : 705f636f6c6c6100 x6
: 0080808080808080
[   41.137204][ T5373] x5 : ffffffc0b02fbad0 x4 : 0000000000000000 x3
: 0000000000000000
[   41.137207][ T5373] x2 : 0000000000000000 x1 : 0000000000000100 x0
: ffffff8918985040
[   41.137210][ T5373] Call trace:
[   41.137211][ T5373]  kernfs_dop_revalidate+0x28/0x108
[   41.137213][ T5373]  path_openat+0x7ec/0x1b04
[   41.137215][ T5373]  do_filp_open+0xac/0x130
[   41.137216][ T5373]  do_sys_openat2+0x140/0x21c
[   41.137219][ T5373]  __arm64_sys_openat+0x70/0x9c
[   41.137222][ T5373]  invoke_syscall+0x58/0xf0
[   41.137224][ T5373]  do_el0_svc+0x64/0xe0
[   41.137226][ T5373]  el0_svc+0x4c/0xdc
[   41.137228][ T5373]  el0t_64_sync_handler+0x20/0xf4
[   41.137230][ T5373]  el0t_64_sync+0x1bc/0x1c0

After investigation it seems there is race where
kernfs_dop_revalidate() crashed dereferencing kn->__parent in
kernfs_root(kn) at offset 0x8).
Although your patch eliminates kernfs_root(kn), the underlying race
where kn == NULL still exists in this patch and will now crash in
__kernfs_active(kn) at offset 0x4.

Scenario seems to be during lockless VFS path lookup (lookup_fast()),
cached dentries are revalidated without holding dentry locks:

  1. lookup_fast() finds a cached positive dentry where dentry->d_inode != NULL.
  2. In kernfs_dop_revalidate(), d_really_is_negative(dentry)
evaluates to false, so it skips the negative dentry branch.
  3. Concurrently, slab memory reclaim under memory pressure evicts
unused dentries:
    shrink_node() -> shrink_slab() -> super_cache_scan()
      -> prune_dcache_sb() -> shrink_dentry_list()
        -> dentry_unlink_inode()
             -> __d_clear_type_and_inode(dentry)
                  -> dentry->d_inode = NULL;

  4. kernfs_dop_revalidate() now runs:
    kn = kernfs_dentry_node(dentry);
  kernfs_dentry_node() tests if (d_really_is_negative(dentry)) return NULL;.
  Because dentry->d_inode was cleared to NULL by
dentry_unlink_inode(), kernfs_dentry_node(dentry) returns NULL

While crafting fix for 6.12, I found this series reworking
kernfs_dop_revalidate().
Just to be clear, your series is not introducing the problem.

But perhaps we can improve this with a check like below?

  -    /* The kernfs node has been deactivated */
  -    if (!__kernfs_active(kn))
  +    /* The kernfs node is unlinked or deactivated */
  +    if (!kn || !__kernfs_active(kn))
             return 0;

Let me know your thoughts or if you want a separate patch

Thanks,
Sandeep.