Hello,

On Mon, Jan 27, 2025 at 05:25:43PM +0100, Sebastian Andrzej Siewior wrote:

quoted

quoted

-	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.

quoted

quoted

+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