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

[Tejun Heo <tj@kernel.org>]

Hello, sorry about the delay.

Generally looks good to me but I have some rcu deref accessor related
comments.

On Thu, Nov 21, 2024 at 06:52:50PM +0100, Sebastian Andrzej Siewior wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
...
> @@ -1312,6 +1314,11 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
>  		ret = -EINVAL;
>  		goto out_region;
>  	}
> +	kn_name = kstrdup(rdt_kn_get_name(rdtgrp->kn), GFP_KERNEL);

Shouldn't this be freed somewhere?

> +	if (!kn_name) {
> +		ret = -ENOMEM;
> +		goto out_cstates;
> +	}
>  
>  	plr->thread_done = 0;
>  
...
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
...
> @@ -533,7 +543,8 @@ static void kernfs_free_rcu(struct rcu_head *rcu)
>  {
>  	struct kernfs_node *kn = container_of(rcu, struct kernfs_node, rcu);
>  
> -	kfree_const(kn->name);
> +	/* If the whole node goes away, the name can't be used outside */
> +	kfree_const(rcu_dereference_check(kn->name, true));

rcu_access_pointer()?

> @@ -557,16 +568,18 @@ void kernfs_put(struct kernfs_node *kn)
>  	if (!kn || !atomic_dec_and_test(&kn->count))
>  		return;
>  	root = kernfs_root(kn);
> +	guard(rcu)();
>   repeat:
>  	/*
>  	 * Moving/renaming is always done while holding reference.
>  	 * kn->parent won't change beneath us.
>  	 */
> -	parent = kn->parent;
> +	parent = rcu_dereference(kn->parent);

I wonder whether it'd be better to encode the reference count rule (ie. add
the condition kn->count == 0 to deref_check) in the kn->parent deref
accessor. This function doesn't need RCU read lock and holding it makes it
more confusing.

> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 8502ef68459b9..05f7b30283150 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -911,9 +911,11 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  	/* kick fsnotify */
>  
>  	down_read(&root->kernfs_supers_rwsem);
> +	down_read(&root->kernfs_rwsem);

Why is this addition necessary? Hmm... was the code previously broken w.r.t.
renaming? Can this be RCU?

> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index 1358c21837f1a..db71faba3bb53 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -145,8 +145,10 @@ static struct dentry *kernfs_fh_to_parent(struct super_block *sb,
>  static struct dentry *kernfs_get_parent_dentry(struct dentry *child)
>  {
>  	struct kernfs_node *kn = kernfs_dentry_node(child);
> +	struct kernfs_root *root = kernfs_root(kn);
>  
> -	return d_obtain_alias(kernfs_get_inode(child->d_sb, kn->parent));
> +	guard(rwsem_read)(&root->kernfs_rwsem);
> +	return d_obtain_alias(kernfs_get_inode(child->d_sb, kernfs_rcu_get_parent(kn)));

Ditto.

> @@ -186,10 +188,10 @@ static struct kernfs_node *find_next_ancestor(struct kernfs_node *child,
>  		return NULL;
>  	}
>  
> -	while (child->parent != parent) {
> -		if (!child->parent)
> +	while (kernfs_rcu_get_parent(child) != parent) {
> +		child = kernfs_rcu_get_parent(child);
> +		if (!child)

I think kernfs_rcu_get_parent() name is a bit confusing given that it allows
derefing without RCU if the rwsem is locked. Maybe just kernfs_get_parent()
or kernfs_parent()?

> @@ -216,6 +219,9 @@ struct dentry *kernfs_node_dentry(struct kernfs_node *kn,
>  	if (!kn->parent)
>  		return dentry;
>  
> +	root = kernfs_root(kn);
> +	guard(rwsem_read)(&root->kernfs_rwsem);

Here too, it's a bit confusing that it's adding new locking. Was the code
broken before? If so, it'd be clearer if the fixes were in their own patch.

> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index d1995e2d6c943..e9bfe3e80809d 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -19,13 +19,19 @@
>  
>  #include "sysfs.h"
>  
> +static struct kobject *sysfs_file_kobj(struct kernfs_node *kn)
> +{
> +	guard(rcu)();
> +	return rcu_dereference(kn->parent)->priv;
> +}

I wonder whether it'd be better to rename kn->parent to something like
kn->__parent (or maybe some other suffix) to clarify that the field is not
to be deref'ed directly and kernfs_parent() helper is made available to the
users. That way, users can benefit from the additional conditions in
rcu_dereference_check().

> diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
> index e28d5f0d20ed0..202e329759b12 100644
> --- a/kernel/cgroup/cgroup-v1.c
> +++ b/kernel/cgroup/cgroup-v1.c
> @@ -844,7 +844,7 @@ static int cgroup1_rename(struct kernfs_node *kn, struct kernfs_node *new_parent
>  
>  	if (kernfs_type(kn) != KERNFS_DIR)
>  		return -ENOTDIR;
> -	if (kn->parent != new_parent)
> +	if (rcu_dereference_check(kn->parent, true) != new_parent)
>  		return -EIO;

This isn't being derefed, rcu_access_pointer()?

>  	/*
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 044c7ba1cc482..d11d05a53783c 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -633,9 +633,15 @@ int cgroup_task_count(const struct cgroup *cgrp)
>  	return count;
>  }
>  
> +static struct cgroup *cg_get_parent_priv(struct kernfs_node *kn)
> +{
> +	/* The parent can not be changed */
> +	return rcu_dereference_check(kn->parent, true)->priv;
> +}

e.g. Here, it'd be a lot better if kernfs provided helper can be used so
that deref condition check can be preserved.

Thanks.

-- 
tejun