Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1

[Bernd Edlinger]

On 4/2/20 9:52 PM, Linus Torvalds wrote:
> On Thu, Apr 2, 2020 at 12:31 PM Bernd Edlinger
> <bernd.edlinger@xxxxxxxxxx> 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".
> 

Ah, yes, there it was introduced.

That fixed only the case of a single-threaded process doing execve,
but missed to fix the case of a multi-threaded process doing execve,
and the other threads racing with the execve.  That is what happened
on my laptop, again and again, when I tried to fix a bug in the
gcc testsuite, that is while I wanted to track down another bug,
that is why the gcc testsuite left loads of temp-files in /tmp,
until I decided to go on a little bug-hunt in the linux kernel
instead :-/

And I had no idea what was happening at all.  But that way this bug
bit me again and again, until I realized the nature of the strace
problem, when I was really baffled.

Before I considered a linux patch for that I tried to fix it in the
strace code instead, and in fact I had tried two approaches,
one is wait in a signal handler, that did not work.
The second one is use another thread that does the wait, and that
did only work when I disable the PTRACE_O_TRACEEXIT flags.

I posted the two patches on lkml, just for reference.
Maybe you are amused by those patches. I consider that a craziness myself,
but it was indeed able to avoid the deadlock, with a user space change alone:

https://lore.kernel.org/lkml/AM6PR03MB5170D68B5010FCA627A603F8E4E60@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/


so that is more or less for your amusement, sincerely I would not propose
that as the way to fix the strace deadlock.


Bernd.

> 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
>