On Wed, Jan 5, 2022 at 10:50 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> That would require that 'psi_trigger_replace()' serialize with the
> waitqueue lock (easy)
I take the "easy" back. The other side of that serialization would
require that the poll() side also re-lookup the trigger pointer under
that same lock.
And you can't do that with the waitqueue lock, because 'poll_wait()'
does the add_wait_queue() internally, and that will take the waitqueue
lock. So you can't take and hold the waitqueue lock in the caller in
poll, it would just deadlock.
And not holding the lock over the call would mean that you'd have a
small race between adding a new poll waiter, and checking that the
trigger is still the same one.
We could use another lock - the code in kernel/sched/psi.c already does
mutex_lock(&seq->lock);
psi_trigger_replace(&seq->private, new);
mutex_unlock(&seq->lock);
and could use that same lock around the poll sequence too.
But the cgroup_pressure_write() code doesn't even do that, and
concurrent writes aren't serialized at all (much less concurrent
poll() calls).
Side note: it looks like concurrent writes in the
cgroup_pressure_write() is literally broken. Because
psi_trigger_replace() is *not* handling concurrency, and does that
struct psi_trigger *old = *trigger_ptr;
....
if (old)
kref_put(&old->refcount, psi_trigger_destroy);
assuming that the caller holds some lock that makes '*trigger_ptr' a
stable thing.
Again, kernel/sched/psi.c itself does that already, but the cgroup
code doesn't seem to.
So the bugs in this area go deeper than "just" poll(). The whole
psi_trigger_replace() thing is literally broken even ignoring the
poll() interactions.
Whoever came up with that stupid "replace existing trigger with a
write()" model should feel bad. It's garbage, and it's actively buggy
in multiple ways.
Linus
