On 2025-01-24 13:35:07 [-1000], Tejun Heo wrote:
> 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?
rcu_access_pointer() is what I was looking for. The __parent pointer is
not dereferenced only compared.
> ...
> > @@ -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.
Okay.
> > 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.
Is it? It checks if the pointer NULL and if so "/" is used otherwise
"kn->name". The __parent pointer itself is not 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?
This would work. kernfs_parent() is the "general purpose" access. It is
used in most places (the kernfs_rename_ns() usage is moved to
kernfs_parent() in the following patch, ended here open coded during the
split, fixed now).
The "!atomic_read(&kn->count)" rule is a special case used only in
kernfs_put() after the counter went to 0 and should not be used (used as
in be valid) anywhere else. This is special because is going away and
__parent can not be renamed/ replaced at this point. One user in total.
The "lockdep_is_held(&kernfs_rename_lock)" rule is only used in
kernfs_get_parent(). One user in total.
Adding these two cases to kernfs_parent() will bloat the code a
little in the debug case (where the check is expanded). Also it will
require to make kernfs_rename_lock global so it be accessed outside of
dir.c.
All in all I don't think it is worth it. If you however prefer it that
way, I sure can update it.
> ...
> > +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.
Right you are. I've seen this flag set and that the root node's flags
were ORed into the child node but I can't find where this does happen.
It does not. I must have seen KERNFS_ACTIVATED then.
> Thanks.
Sebastian
