linux-kernel - Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw

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: <875z5h4b7a.fsf@x220.int.ebiederm.org>
Date:   Fri, 04 Dec 2020 13:34:33 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Bernd Edlinger <bernd.edlinger@...mail.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Jann Horn <jannh@...gle.com>,
        Vasiliy Kulikov <segoon@...nwall.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Oleg Nesterov <oleg@...hat.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Sargun Dhillon <sargun@...gun.me>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Arnd Bergmann <arnd@...db.de>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Waiman Long <longman@...hat.com>,
        Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [PATCH 3/3] exec: Transform exec_update_mutex into a rw_semaphore

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Fri, Dec 4, 2020 at 8:08 AM Bernd Edlinger <bernd.edlinger@...mail.de> wrote:
>>
>> >
>> > -static void kcmp_unlock(struct mutex *m1, struct mutex *m2)
>> > +static void kcmp_unlock(struct rw_semaphore *l1, struct rw_semaphore *l2)
>> >  {
>> > -     if (likely(m2 != m1))
>> > -             mutex_unlock(m2);
>> > -     mutex_unlock(m1);
>> > +     if (likely(l2 != l1))
>>
>> is this still necessary ?
>>
>> > +             up_read(l2);
>> > +     up_read(l1);
>> >  }
>> >
>> > -static int kcmp_lock(struct mutex *m1, struct mutex *m2)
>> > +static int kcmp_lock(struct rw_semaphore *l1, struct rw_semaphore *l2)
>> >  {
>> >       int err;
>> >
>> > -     if (m2 > m1)
>> > -             swap(m1, m2);
>> > +     if (l2 > l1)
>> > +             swap(l1, l2);
>>
>> and this is probably also no longer necessary?
>
> These are still necessary, because even a recursive read lock can
> still block on a writer trying to come in between the two read locks
> due to fairness guarantees.
>
> So taking the same read lock twice is still a source of possible deadlocks.
>
> For the same reason, read locks still have ABBA deadlock and need to
> be taken in order.
>
> So switching from a mutex to a rwlock doesn't really change the
> locking rules in this respect.

Thinking about the specific case of down_read on two instances of
exec_update_lock.  If there are two instances of kcmp being called with
the sames two pids, but in opposite order running, and the tasks of that
both of those pids refer to both exec, you could definitely get a
deadlock.

So yes.  We definitely need to keep the swap as well.

> In fact, I'm not convinced this change even fixes the deadlock that
> syzbot reported, for the same reason: it just requires a write lock in
> between two read locks to deadlock.

>From a deadlock perspective the change is strictly better than what we
have today.  The readers will no longer block on each other.

For the specific case that syzbot reported it is readers who were
blocking on each other so that specific case if fixed.

On the write side of exec_update_lock we have:

cred_guard_mutex -> exec_update_lock

Which means that to get an ABBA deadlock cred_guard_mutex would need to
be involved and it is only acquired in 3 places: ptrace_attach,
do_seccomp, and proc_pid_attr_write.  In none of the 3 from the syscall
entry point until the code that takes cred_guard_mutex can I find
anything that takes any locks.  Perhaps there is something in io_uring I
did not completely trace that write path.

So given that the exec path would need to be involved, and the exec path
takes exec_update_lock pretty much at the top level.  I am not seeing
how there is any room for deadlocks after this change.

Am I missing something?

Eric