linux-kernel - Re: possible deadlock in proc_pid

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: <87eenmgbxr.fsf@x220.int.ebiederm.org>
Date:   Mon, 31 Aug 2020 08:52:16 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     peterz@...radead.org
Cc:     syzbot <syzbot+db9cdf3dd1f64252c6ef@...kaller.appspotmail.com>,
        adobriyan@...il.com, akpm@...ux-foundation.org, avagin@...il.com,
        christian@...uner.io, gladkov.alexey@...il.com,
        keescook@...omium.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        walken@...gle.com, Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Miklos Szeredi <miklos@...redi.hu>, jannh@...gle.com
Subject: Re: possible deadlock in proc_pid_syscall (2)

peterz@...radead.org writes:

> On Sun, Aug 30, 2020 at 07:31:39AM -0500, Eric W. Biederman wrote:
>
>> I am thinking that for cases where we want to do significant work it
>> might be better to ask the process to pause at someplace safe (probably
>> get_signal) and then do all of the work when we know nothing is changing
>> in the process.
>> 
>> I don't really like the idea of checking and then checking again.  We
>> might have to do it but it feels like the model is wrong somewhere.
>> 
>> Given that this is tricky to hit in practice, and given that I am
>> already working the general problem of how to sort out the locking I am
>> going to work this with the rest of the thorny issues of in exec.  This
>> feels like a case where the proper solution is that we simply need
>> something better than a mutex.
>
> One possible alternative would be something RCU-like, surround the thing
> with get_task_cred() / put_cred() and then have commit_creds() wait for
> the usage of the old creds to drop to 0 before continuing.
>
> (Also, get_cred_rcu() is disgusting for casting away const)
>
> But this could be complete garbage, I'm not much familiar with any of
> thise code.

This looks like an area of code that will take a couple of passes to get
100% right.

Usually changing creds happens atomically, and separately from
everything else so we simply don't care if there a race,  Either the old
creds or the new creds are valid.

With exec the situation is trickier as several things in addition to the
cred are changing at the same time.  So a lock is needed.

Now that it is separated from the cred_guard_mutex, probably the easiest
solution is to make exec_update_mutex a sleeping reader writer lock.
There are fewer cases that matter as such a lock would only block on
exec (the writer).

I don't understand perf well enough to do much without carefully
studying the code.

Eric