[Devel] [PATCH vz10 11/24] ve/kernfs: hold the iterated tree's rwsem in the perms seq_file

[Pavel Tikhomirov]

This is exactly the same to parralel patch:

"[PATCH vz10 v4 3/9] fs/kernfs, ve: lock the walked tree rwsem in kernfs_perms_start"
CC 

It seems better for me as in it we've also dropped excess root_for_sem variable.

On 7/6/26 12:59, Konstantin Khorenko wrote:
> kernfs_perms_start() iterates the perms tree passed as @root (for sysfs
> that is sysfs_root_kn), walking it with kernfs_next_recursive() ->
> kernfs_parent(), and kernfs_perms_show() walks the parent chain of each
> node likewise. kernfs_parent() is rcu_dereference_check(kn->__parent,
> kernfs_root_is_locked(kn) || ...) and expects that node's kernfs rwsem
> (or rename_lock, or RCU) to be held.
> 
> But the code took down_read() on kernfs_root(of->kn)->kernfs_rwsem, i.e.
> the rwsem of the *control file*, not of the tree being walked. The perms
> tree can live in a different kernfs instance than the control file - the
> sysfs perms are shown via a cgroupfs file - so this is the wrong lock. On
> a lockdep kernel it trips:
> 
>   WARNING: suspicious RCU usage
>   fs/kernfs/kernfs-internal.h:131 suspicious rcu_dereference_check() usage!
>   kernfs_perms_show <- seq_read_iter <- vfs_read
> 
> and it is a real (if narrow) race: a concurrent sysfs add/remove/reparent
> under the sysfs rwsem is not excluded by holding the cgroupfs rwsem.
> 
> Lock the rwsem of the iterated tree (kernfs_root(@root)) instead. This is
> the lock kernfs_parent() expects and it excludes concurrent mutation of
> the tree being walked. kernfs_perms_stop() takes @root too so it releases
> the same rwsem.
> 
> Fixes: d0cb91d2dec2 ("ve/kernfs: fix kernfs locking in the control per-VE nodes visibility code")
> Feature: sysfs: per-CT entries visibility and permissions configuration
> https://virtuozzo.atlassian.net/browse/VSTOR-137234
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> ---
>  fs/kernfs/ve.c            | 21 ++++++++++++---------
>  fs/sysfs/ve.c             |  2 +-
>  include/linux/kernfs-ve.h |  2 +-
>  3 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/kernfs/ve.c b/fs/kernfs/ve.c
> index e83ee5bec3bf..a89abc1f4040 100644
> --- a/fs/kernfs/ve.c
> +++ b/fs/kernfs/ve.c
> @@ -163,11 +163,18 @@ void *kernfs_perms_start(struct seq_file *m, loff_t *ppos,
>  			 struct kernfs_node *root, struct kmapset_key *key)
>  {
>  	struct ve_struct *ve = css_to_ve(seq_css(m));
> -	struct kernfs_open_file *of = m->private;
> -	struct kernfs_node *kn = of->kn;
> -	struct kernfs_root *root_for_sem = kernfs_root(kn);
> +	struct kernfs_root *root_for_sem = kernfs_root(root);
> +	struct kernfs_node *kn;
>  	loff_t pos = *ppos;
>  
> +	/*
> +	 * We iterate the perms tree (@root), so hold that tree's
> +	 * kernfs_rwsem: it guards kernfs_parent()/kernfs_next_recursive()
> +	 * against a concurrent reparent and is the lock kernfs_parent()
> +	 * expects to be held. @root may live in a different kernfs instance
> +	 * than the control file (of->kn) - e.g. sysfs perms are shown via a
> +	 * cgroupfs file - so locking of->kn's root would be the wrong lock.
> +	 */
>  	down_read(&root_for_sem->kernfs_rwsem);
>  	for (kn = root; kn; kn = kernfs_next_recursive(kn)) {
>  		if (kernfs_perms_shown(ve, kn, key) && !pos--)
> @@ -190,13 +197,9 @@ void *kernfs_perms_next(struct seq_file *m, void *v, loff_t *ppos,
>  	return kn;
>  }
>  
> -void kernfs_perms_stop(struct seq_file *m, void *v)
> +void kernfs_perms_stop(struct seq_file *m, void *v, struct kernfs_node *root)
>  {
> -	struct kernfs_open_file *of = m->private;
> -	struct kernfs_node *kn = of->kn;
> -	struct kernfs_root *root = kernfs_root(kn);
> -
> -	up_read(&root->kernfs_rwsem);
> +	up_read(&kernfs_root(root)->kernfs_rwsem);
>  }
>  
>  int kernfs_perms_show(struct seq_file *m, void *v, struct kmapset_key *key)
> diff --git a/fs/sysfs/ve.c b/fs/sysfs/ve.c
> index eb941a1d5dd5..144a901547c0 100644
> --- a/fs/sysfs/ve.c
> +++ b/fs/sysfs/ve.c
> @@ -55,7 +55,7 @@ static void *sysfs_perms_next(struct seq_file *m, void *v, loff_t *ppos)
>  
>  static void sysfs_perms_stop(struct seq_file *m, void *v)
>  {
> -	kernfs_perms_stop(m, v);
> +	kernfs_perms_stop(m, v, sysfs_root_kn);
>  	mutex_unlock(&sysfs_perms_mutex);
>  }
>  
> diff --git a/include/linux/kernfs-ve.h b/include/linux/kernfs-ve.h
> index 2cb905918393..25837ff79e5b 100644
> --- a/include/linux/kernfs-ve.h
> +++ b/include/linux/kernfs-ve.h
> @@ -24,7 +24,7 @@ void *kernfs_perms_start(struct seq_file *m, loff_t *ppos,
>  			 struct kernfs_node *root, struct kmapset_key *key);
>  void *kernfs_perms_next(struct seq_file *m, void *v, loff_t *ppos,
>  			      struct kmapset_key *key);
> -void kernfs_perms_stop(struct seq_file *m, void *v);
> +void kernfs_perms_stop(struct seq_file *m, void *v, struct kernfs_node *root);
>  
>  int kernfs_perms_show(struct seq_file *m, void *v, struct kmapset_key *key);
>  

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.