lkml.org 
[lkml]   [2020]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [GIT PULL] Please pull proc and exec work for 5.7-rc1
    On Thu, Apr 2, 2020 at 12:31 PM Bernd Edlinger
    <bernd.edlinger@hotmail.de> wrote:
    >
    > This is at least what is my impression how the existing mutexes are used,
    > a mutex called "cred_guard_mutex" is a not very good self explaining name,
    > in my opinion, it is totally unclear what it does "guard", and why.

    Oh, I absolutely agree that cred_guard_mutex is a horrible lock.

    It actually _used_ to be a lot more understandable, and the name used
    to make more sense in the context it was used.

    See commit

    a2a8474c3fff ("exec: do not sleep in TASK_TRACED under ->cred_guard_mutex")

    for when it changed from "somewhat understandable" to "really hard to follow".

    Don't get me wrong - that commit has a very good reason for it, but it
    does make the locking really hard to understand.

    It all used to be in one function - do_execve() - and it was holding
    the lock over a fairly obvious range, starting at

    bprm->cred = prepare_exec_creds();

    and ending at basically "we're done with execve()".

    So basically, cred_guard_mutex ends up being the thing that is held
    all the way from the "before execve looks at the old creds" to "execve
    is done, and has changed the creds".

    The reason it's needed is exactly that there are some nasty situations
    where execve() itself does things with creds to determine that the new
    creds are ok. And it uses the old creds to do that, but it also uses
    the task->flags and task->ptrace.

    So think of cred_guard_mutex as a lock around not just the creds, but
    the combination of creds and the task flags/ptrace.

    Anybody who changes the task ptrace setting needs to serialize with
    execve(). Or anybody who tests for "dumpable()", for example.

    If *all* you care about is just the creds, then you don't need it.
    It's really only users that do more checks than just credentials.
    "dumpable()" is I think the common one.

    And that's why cred_guard_mutex has that big range - it starts when we
    read the original creds (because it will use those creds to determine
    how the *new* creds will affect dumpability etc), and it ends when it
    has updated not only to the new creds, but it has set all those other
    flags too.

    So I'm not at all against splitting the lock up, and trying to make it
    more directed and specific.

    My complaints were about how the new lock wasn't much better. It was
    still completely incomprehensible, the conditional unlocking was hard
    to follow, and it really wasn't obvious that the converted users were
    fine.

    See?

    Linus

    \
     
     \ /
      Last update: 2020-04-02 21:53    [W:4.873 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site