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
