Hello,
On Mon, Jan 27, 2025 at 05:25:43PM +0100, Sebastian Andrzej Siewior wrote:
> > > - 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.
Ah, ignore me. I was misreading.
> > > +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.
Hmm... maybe other people have different preferences here but I much prefer
documenting and enforcing RCU deref rules in a single place. It only adds
debug annotations that go away in prod builds while clarifying subtleties.
The trade-off seems pretty one-sided to me.
Thanks.
--
tejun
