linux-kernel - Re: psi_trigger_poll() is completely broken

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 5 Jan 2022 11:07:13 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Eric Biggers <ebiggers@...nel.org>, Tejun Heo <tj@...nel.org>,
        Zefan Li <lizefan.x@...edance.com>
Cc:     Johannes Weiner <hannes@...xchg.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Ingo Molnar <mingo@...hat.com>,
        Hillf Danton <hdanton@...a.com>,
        syzbot <syzbot+cdb5dd11c97cc532efad@...kaller.appspotmail.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: psi_trigger_poll() is completely broken

On Wed, Jan 5, 2022 at 10:50 AM Linus Torvalds
<torvalds@...ux-foundation.org> 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