Message-ID: <20250127162543.Vr347xPN@linutronix.de>
Date: Mon, 27 Jan 2025 17:25:43 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Tejun Heo <tj@...nel.org>
Cc: cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
	Michal Koutný <mkoutny@...e.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Hillf Danton <hdanton@...a.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Marco Elver <elver@...gle.com>, Zefan Li <lizefan.x@...edance.com>,
	tglx@...utronix.de
Subject: Re: [PATCH v4 5/6] kernfs: Use RCU to access kernfs_node::parent.

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

Powered by blists - more mailing lists