Re: psi_trigger_poll() is completely broken

[Linus Torvalds]

On Wed, Jan 5, 2022 at 7:21 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote:
>
> [changed subject line to hopefully get people to stop ignoring this]
>
> Please see my message below where I explained the problem in detail.  Any
> response from the maintainers of kernel/sched/psi.c?  There are a lot of you:

Ok, this one is clearly a kernel/sched/psi.c bug, since the lifetime
isn't even maintained by the fiel reference.

I think the proper thing to do is to move the whole "get kref to
trigger pointer" in the open/close code, and keep the ref around that
way.

The natural thing to do would be to look up the trigger at open time,
save the pointer in seq->private, and release it at close time.

Sadly, right now the code actually uses that 'seq->private' as an
indirect rcu-pointer to the trigger data, instead of as the trigger
data itself. And that seems very much on purpose and inherent to that
'psi_write()' model, where it changes the trigger pointer very much on
purpose.

So I agree 100% - the PSI code is fundamentally broken. psi_write()
seems to be literally _designed_ to do the wrong thing.

I don't know who - if anybody - uses this. My preference would be to
just disable the completely broken poll support.

Another alternative is to just make 'psi_write()' return -EBUSY if
there are existing poll waiters (ie t->event_wait not being empty.  At
least then the open file would keep the kref to the trigger.

That would require that 'psi_trigger_replace()' serialize with the
waitqueue lock (easy), but all callers would also have to check the
return value of it

The cgroup code does

        psi_trigger_replace(&of->priv, NULL);

in the release function, but I guess that might work since at release
time there shouldn't be any pending polls anyway.

But it would also mean that anybody who can open the file for reading
(so that they can poll it) would be able to keep people from changing
it.

But yes, I think that unless we get some reply from the PSI
maintainers, we will have to just disable polling entirely.

I hope there are no users that would break, but considering that the
current code is clearly too broken to live, this may be one of those
"we have no choice" cases.

                         Linus