linux-kernel - Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface for name lookups

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241113120706.rotCvUqt@linutronix.de>
Date: Wed, 13 Nov 2024 13:07:06 +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,
	syzbot+6ea37e2e6ffccf41a7e6@...kaller.appspotmail.com
Subject: Re: [PATCH v2 2/2] cgroup, kernfs: Move cgroup to the RCU interface
 for name lookups

On 2024-11-13 08:43:32 [+0100], To Tejun Heo wrote:
> On 2024-11-12 08:59:16 [-1000], Tejun Heo wrote:
> > Hello,
> 
> Hi,
> 
> > On Tue, Nov 12, 2024 at 04:52:39PM +0100, Sebastian Andrzej Siewior wrote:
> > ...
> > >  /**
> > > - * pr_cont_kernfs_name - pr_cont name of a kernfs_node
> > > + * pr_cont_kernfs_name_rcu - pr_cont name of a kernfs_node
> > >   * @kn: kernfs_node of interest
> > >   *
> > > - * This function can be called from any context.
> > > + * This function can be called from any context. The root node must be with
> > > + * KERNFS_ROOT_SAME_PARENT.
> > >   */
> > > -void pr_cont_kernfs_name(struct kernfs_node *kn)
> > > +void pr_cont_kernfs_name_rcu(struct kernfs_node *kn)
> > 
> > Having to split the interface all the way up isn't great. While there are
> > also downsides, I wonder whether a better approach here is just making the
> > backend function (kernfs_path_from_node()) automatically use RCU locking if
> > the flag is set rather than propagating the difference by splitting the
> > interface. The distinction doesn't mean anything to most users after all.
> 
> Indeed.

Now I see what the problems are. If we merge both into one, then I get
this:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {
|         struct kernfs_root *root;
|         bool rcu_lookup;
|
|         if (!kn)
|                 return strscpy(buf, "(null)", buflen);
|
|         root = kernfs_root(kn);

This is the tricky part. For KERNFS_ROOT_INVARIANT_PARENT I don't worry
that the parent goes away and I need it to get a reference to the
kernfs_root node. For the !KERNFS_ROOT_INVARIANT_PARENT I need the lock
for kernfs_root() so I put the guard/ lock at the top.

I think that is why you suggested the two functions (or this is what I
understood). Looking at the remaining bits:

|         rcu_lookup = root->flags & KERNFS_ROOT_INVARIANT_PARENT;
|         if (rcu_lookup) {
|                 guard(rcu)();
|                 return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
|         }
|         guard(read_lock_irqsave)(&kernfs_rename_lock);
|         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }

This could collapse into the RCU version because read_lock_irqsave()
implies RCU protection. And since ->name is always RCU assigned/
deallocated I don't really need the lock here, RCU would be enough.
Except for the parent. The kn->parent does not matter here (it should be
always be != NULL if assigned), the problematic part is kernfs_root()
which checks the parent for the root node.

To make this simple I could avoid kernfs_root lookup and just have:
| int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
| {       
|         if (!kn)
|                 return strscpy(buf, "(null)", buflen);
|         
|         guard(rcu)();
|         return strscpy(buf, kn->parent ? rcu_dereference(kn->name) : "/", buflen);
| }                             

That is the easy part. kernfs_path_from_node() is different as it
requires the parent pointer. In order to distinguish the RCU from the
non-RCU version I need kernfs_root for the flag and depending on it, the
lock so the parent does not go away.

Would it work to add the pointer to kernfs_root into kernfs_node? This
would shrink kernfs_elem_dir by a pointer but the union would remain the
same size due to kernfs_elem_attr so the struct would grow.

> > Thanks.

Sebastian