Messages in this thread | | | Subject | Re: [GIT PULL] Please pull proc and exec work for 5.7-rc1 | From | Bernd Edlinger <> | Date | Fri, 3 Apr 2020 18:00:12 +0200 |
| |
On 4/2/20 9:52 PM, Linus Torvalds wrote: > 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". >
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@AM6PR03MB5170.eurprd03.prod.outlook.com/
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 >
| |