psi: Fix uaf issue when psi trigger is destroyed while being polled

kernel/cgroup/cgroup.c +6 -0

index cafb8c114a21..93b51a2104f7 100644
@@ -3642,6 +3642,12 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
 	cgroup_get(cgrp);
 	cgroup_kn_unlock(of->kn);
 
+	/* Allow only one trigger per file descriptor */
+	if (ctx->psi.trigger) {
+		cgroup_put(cgrp);
+		return -EBUSY;
+	}
+
 	psi = cgroup_ino(cgrp) == 1 ? &psi_system : &cgrp->psi;
 	new = psi_trigger_create(psi, buf, nbytes, res);
 	if (IS_ERR(new)) {

kernel/cgroup/cgroup.c +5 -5

@@ -1305,14 +1287,24 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
 
 	buf[buf_size - 1] = '\0';
 
-	new = psi_trigger_create(&psi_system, buf, nbytes, res);
-	if (IS_ERR(new))
-		return PTR_ERR(new);
-
 	seq = file->private_data;
+
 	/* Take seq->lock to protect seq->private from concurrent writes */
 	mutex_lock(&seq->lock);
-	psi_trigger_replace(&seq->private, new);
+
+	/* Allow only one trigger per file descriptor */
+	if (seq->private) {
+		mutex_unlock(&seq->lock);

The write here needs to use smp_store_release(), since it is paired with the concurrent READ_ONCE() in psi_trigger_poll().

Quoted 1 line

+ seq->private = new;

Likewise here.

Quoted 3 lines · Eric Biggers

The write here needs to use smp_store_release(), since it is paired with the concurrent READ_ONCE() in psi_trigger_poll().

A smp_store_release() doesn't make sense pairing with a READ_ONCE().

Any memory ordering that the smp_store_release() does on the writing side is entirely irrelevant, since the READ_ONCE() doesn't imply any ordering on the reading side. Ordering one but not the other is nonsensical.

So the proper pattern is to use a WRITE_ONCE() to pair with a READ_ONCE() (when you don't care about memory ordering, or you handle it explicitly), or a smp_load_acquire() with a smp_store_release() (in which case writes before the smp_store_release() on the writing side will be ordered wrt accesses after smp_load_acquire() on the reading side).

Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings)

But in this case, I do think that since there's some setup involved with the trigger pointer, the proper serialization is to use smp_store_release() to set the pointer, and then smp_load_acquire() on the reading side.

Or just use the RCU primitives - they are even better optimized, and handle exactly that case, and can be more efficient on some architectures if release->acquire isn't already cheap.

That said, we've pretty much always accepted that normal word writes are not going to tear, so we *have* also accepted just

- do any normal store of a value on the write side

- do a READ_ONCE() on the reading side

where the reading side doesn't actually care *what* value it gets, it only cares that the value it gets is *stable* (ie no compiler reloads that might show up as two different values on the reading side).

Of course, that has the same issue as WRITE_ONCE/READ_ONCE - you need to worry about memory ordering separately.

Quoted 3 lines

Quoted 1 line

+ seq->private = new;

Likewise here.

Yeah, same deal, except here you can't even use the RCU ones, because 'seq->private' isn't annotated for RCU.

Or you'd do the casting, of course.

Linus

Thanks for the explanation!
So, it sounds like the best (semantically correct) option I have here is smp_store_release() to set the pointer, and then smp_load_acquire() to read it. Is my understanding correct?

Quoted 3 lines · Eric Biggers

The write here needs to use smp_store_release(), since it is paired with the concurrent READ_ONCE() in psi_trigger_poll().

A smp_store_release() doesn't make sense pairing with a READ_ONCE().

Any memory ordering that the smp_store_release() does on the writing side is entirely irrelevant, since the READ_ONCE() doesn't imply any ordering on the reading side. Ordering one but not the other is nonsensical.

So the proper pattern is to use a WRITE_ONCE() to pair with a READ_ONCE() (when you don't care about memory ordering, or you handle it explicitly), or a smp_load_acquire() with a smp_store_release() (in which case writes before the smp_store_release() on the writing side will be ordered wrt accesses after smp_load_acquire() on the reading side).

Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings)

But in this case, I do think that since there's some setup involved with the trigger pointer, the proper serialization is to use smp_store_release() to set the pointer, and then smp_load_acquire() on the reading side.

Or just use the RCU primitives - they are even better optimized, and handle exactly that case, and can be more efficient on some architectures if release->acquire isn't already cheap.

That said, we've pretty much always accepted that normal word writes are not going to tear, so we *have* also accepted just

- do any normal store of a value on the write side

- do a READ_ONCE() on the reading side

where the reading side doesn't actually care *what* value it gets, it only cares that the value it gets is *stable* (ie no compiler reloads that might show up as two different values on the reading side).

Of course, that has the same issue as WRITE_ONCE/READ_ONCE - you need to worry about memory ordering separately.

Quoted 3 lines

Quoted 1 line

+ seq->private = new;

Likewise here.

Yeah, same deal, except here you can't even use the RCU ones, because 'seq->private' isn't annotated for RCU.

Or you'd do the casting, of course.

Quoted 49 lines · Linus Torvalds

Quoted 3 lines · Eric Biggers

The write here needs to use smp_store_release(), since it is paired with the concurrent READ_ONCE() in psi_trigger_poll().

A smp_store_release() doesn't make sense pairing with a READ_ONCE().

Any memory ordering that the smp_store_release() does on the writing side is entirely irrelevant, since the READ_ONCE() doesn't imply any ordering on the reading side. Ordering one but not the other is nonsensical.

So the proper pattern is to use a WRITE_ONCE() to pair with a READ_ONCE() (when you don't care about memory ordering, or you handle it explicitly), or a smp_load_acquire() with a smp_store_release() (in which case writes before the smp_store_release() on the writing side will be ordered wrt accesses after smp_load_acquire() on the reading side).

Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings)

But in this case, I do think that since there's some setup involved with the trigger pointer, the proper serialization is to use smp_store_release() to set the pointer, and then smp_load_acquire() on the reading side.

Or just use the RCU primitives - they are even better optimized, and handle exactly that case, and can be more efficient on some architectures if release->acquire isn't already cheap.

That said, we've pretty much always accepted that normal word writes are not going to tear, so we *have* also accepted just

- do any normal store of a value on the write side

- do a READ_ONCE() on the reading side

where the reading side doesn't actually care *what* value it gets, it only cares that the value it gets is *stable* (ie no compiler reloads that might show up as two different values on the reading side).

Of course, that has the same issue as WRITE_ONCE/READ_ONCE - you need to worry about memory ordering separately.

Quoted 9 lines

Quoted 3 lines

Quoted 1 line

+ seq->private = new;

Likewise here.

Yeah, same deal, except here you can't even use the RCU ones, because 'seq->private' isn't annotated for RCU.

Or you'd do the casting, of course.

This is yet another case of "one time init". There have been long discussions on this topic before:
* https://lore.kernel.org/linux-fsdevel/[email protected]/T/#u
* https://lore.kernel.org/lkml/[email protected]/T/#u
* https://lwn.net/Articles/827180/

I even attempted to document the best practices:
* https://lore.kernel.org/linux-fsdevel/[email protected]/T/#u

However, no one could agree on whether READ_ONCE() or smp_load_acquire() should be used. smp_load_acquire() is always correct, so it remains my preference. However, READ_ONCE() is correct in some cases, and some people (including the primary LKMM maintainer) insist that it be used in all such cases, as well as in rcu_dereference() even though this places difficult-to-understand constraints on how rcu_dereference() can be used.

My preference is that smp_load_acquire() be used. But be aware that this risks the READ_ONCE() people coming out of the woodwork and arguing for READ_ONCE().

- Eric

This is yet another case of "one time init".

Quoted 2 lines · Eric Biggers

This is yet another case of "one time init".

Ehh. It's somewhat debatable.

For a flag that sets a value once, the rules are somewhat different. In that case, people may simply not care about memory ordering at all, because all they care about is the actual flag value, and - thanks to the one-time behavior - basically whether some transition had happened or not. That's not all that unusual.

But when you fetch a pointer, things are at least conceptually slightly different.

Of course, you may use the existence of the pointer itself as a flag (ie just a "NULL or not"), in which case it's the same as any other one-time flag thing.

But if you use it to dereference something, then _by_definition_ you're not just fetching a one-time flag - even if the pointer is only set once. At that point, at a minimum, you require that that thing has been initialized.

Now, it's then absolutely true that the stuff behind the pointer may then have other reasons not to care about memory ordering again, and you may be able to avoid memory ordering even then. If you're just switching the pointer around between different objects that has been statically allocated and initialized, then there is no memory ordering required, for example. You might be back to the "I just want one or the other of these two pointers".

But if you have something that was initialized before the pointer was assigned, you really do hit the problem we had on alpha, where even if you order the pointer write side accesses, the dereferencing of the pointer may not be ordered on the read side.

Now, alpha is basically dead, and we probably don't really care. Even on alpha, the whole "data dependency isn't a memory ordering" is almost impossible to trigger.

And in fact, to avoid too much pain we ended up saying "screw alpha" and added a memory barrier to READ_ONCE(), so it turns out that smp_store_release -> READ_ONCE() does work because we just couldn't be bothered to try something more proper.

So yeah, READ_ONCE() ends up making the "access through a pointer" thing safe, but that's less of a "it should be safe" and more of a "we can't waste time dealing with braindamage on platforms that don't matter".

In general, I think the rule should be that READ_ONCE() is for things that simply don't care about memory ordering at all (or do whatever ordering they want explicitly). And yes, one such very common case is the "one-way flag" where once a certain state has been reached, it's idempotent.

Of course, then we have the fact that READ_ONCE() can be more efficient than "smp_load_acquire()" on some platforms, so if something is *hugely* performance-critical, you might use READ_ONCE() even if it's not really technically the right thing.

So it's complicated.

A lot of READ_ONCE() users exist just for historical reasons because they predated smp_store_release/smp_load_acquire. They may well have been using ACCESS_ONCE() long ago.

And some are there because it's a very critical piece of code, and it's very intentional.

But if you don't have some huge reasons, I really would prefer people use "smp_store_release -> smp_load_acquire" as a very clear "handoff" event.

Linus

Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings)

Of course, in practice, for pointers, the whole "dereference off a pointer" on the read side *does* imply a barrier in all relevant situations. So yes, a smp_store_release() -> READ_ONCE() does work in practice, although it's technically wrong (in particular, it's wrong on alpha, because of the completely broken memory ordering that alpha has that doesn't even honor data dependencies as read-side orderings)

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Fixes 0e94682b73bf ("psi: introduce psi monitor")

Cc [email protected]

Reported-by [email protected]

Analyzed-by Eric Biggers <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Signed-off-by Suren Baghdasaryan <[email protected]>