Re: [PATCH v4 5/6] kernfs: Use RCU to access kernfs_node::parent.

[Tejun Heo <tj@kernel.org>]

On Fri, Jan 24, 2025 at 06:46:13PM +0100, Sebastian Andrzej Siewior wrote:
...
> +static void *rdt_get_kn_parent_priv(struct kernfs_node *kn)
> +{
> +	guard(rcu)();
> +	return rcu_dereference(kn->__parent)->priv;
> +}
...
> @@ -2429,12 +2435,13 @@ static struct rdtgroup *kernfs_to_rdtgroup(struct kernfs_node *kn)
>  		 * resource. "info" and its subdirectories don't
>  		 * have rdtgroup structures, so return NULL here.
>  		 */
> -		if (kn == kn_info || kn->parent == kn_info)
> +		if (kn == kn_info ||
> +		    rcu_dereference_check(kn->__parent, true) == kn_info)

Why is this safe? What's protecting ->__parent?

...
> @@ -3773,6 +3780,7 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>  		ret = -EPERM;
>  		goto out;
>  	}
> +	parent_kn = rcu_dereference_check(kn->__parent, lockdep_is_held(&rdtgroup_mutex));

Can you please encapsulate the rule in a helper? e.g.

  static rdt_kn_parent(struct kernfs_node *kn)
  {
          return rcu_dereference_check(kn->__parent, lockdep_is_held(&rdtgroup_mutex) + /* whatever other conditions that make accesses safe */);
  }

and then you can use that everywhere e.g.:

  static void *rdt_get_kn_parent_priv(struct krenfs_node *kn)
  {
          guard(rcu)();
          return rdt_kn_parent(kn)->priv;
  }

This way, the rule to access kn->__parent is documented and enforced in a
single place. If the access rules can't be described like this, open coding
exceptions is fine but some documentation would be great.

> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 5a1fea414996e..8e92928c6bca6 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -56,7 +56,7 @@ static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
>  	if (!kn)
>  		return strscpy(buf, "(null)", buflen);
>  
> -	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.

> @@ -295,7 +296,7 @@ struct kernfs_node *kernfs_get_parent(struct kernfs_node *kn)
>  	unsigned long flags;
>  
>  	read_lock_irqsave(&kernfs_rename_lock, flags);
> -	parent = kn->parent;
> +	parent = rcu_dereference_check(kn->__parent, lockdep_is_held(&kernfs_rename_lock));

Ditto, it'd be better to encapsulate the access rules in a helper so that
these aren't open coded differently in different places.

...
> @@ -562,7 +570,7 @@ void kernfs_put(struct kernfs_node *kn)
>  	 * Moving/renaming is always done while holding reference.
>  	 * kn->parent won't change beneath us.
>  	 */
> -	parent = kn->parent;
> +	parent = rcu_dereference_check(kn->__parent, !atomic_read(&kn->count));

And this rule can be encoded in the same accessor function so that the rules
can be documented and enforced from (if possible) a single place.

> @@ -1760,8 +1777,8 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
>  	/* rename_lock protects ->parent and ->name accessors */
>  	write_lock_irq(&kernfs_rename_lock);
>  
> -	old_parent = kn->parent;
> -	kn->parent = new_parent;
> +	old_parent = rcu_dereference_check(kn->__parent, kernfs_root_is_locked(kn));

Another rule here.

> +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?

...
> +static struct cgroup *kn_get_priv(struct kernfs_node *kn)
> +{
> +	return rcu_dereference_check(kn->__parent, kn->flags & KERNFS_ROOT_INVARIANT_PARENT)->priv;
> +}

The flag is a root flag but being tested against a node flags field.

Thanks.

-- 
tejun