Re: [PATCH 2/2] exec: Add a exec_update_mutex to replace cred_guard_mutex

[Eric W. Biederman]

Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes:

> Am 06.03.20 um 06:17 schrieb Eric W. Biederman:
>> Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes:
>> 
>>> On 3/5/20 10:16 PM, Eric W. Biederman wrote:
>>>>
>>>> The cred_guard_mutex is problematic.  The cred_guard_mutex is held
>>>> over the userspace accesses as the arguments from userspace are read.
>>>> The cred_guard_mutex is held of PTRACE_EVENT_EXIT as the the other
>>>> threads are killed.  The cred_guard_mutex is held over
>>>> "put_user(0, tsk->clear_child_tid)" in exit_mm().
>>>>
>>>> Any of those can result in deadlock, as the cred_guard_mutex is held
>>>> over a possible indefinite userspace waits for userspace.
>>>>
>>>> Add exec_update_mutex that is only held over exec updating process
>>>> with the new contents of exec, so that code that needs not to be
>>>> confused by exec changing the mm and the cred in ways that can not
>>>> happen during ordinary execution of a process can take.
>>>>
>>>> The plan is to switch the users of cred_guard_mutex to
>>>> exed_udpate_mutex one by one.  This lets us move forward while still
>>>> being careful and not introducing any regressions.
>>>>
>>>> Link: https://lore.kernel.org/lkml/20160921152946.GA24210@xxxxxxxxxxxxxx/
>>>> Link: https://lore.kernel.org/lkml/AM6PR03MB5170B06F3A2B75EFB98D071AE4E60@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
>>>> Link: https://lore.kernel.org/linux-fsdevel/20161102181806.GB1112@xxxxxxxxxx/
>>>> Link: https://lore.kernel.org/lkml/20160923095031.GA14923@xxxxxxxxxx/
>>>> Link: https://lore.kernel.org/lkml/20170213141452.GA30203@xxxxxxxxxx/
>>>> Ref: 45c1a159b85b ("Add PTRACE_O_TRACEVFORKDONE and PTRACE_O_TRACEEXIT facilities.")
>>>> Ref: 456f17cd1a28 ("[PATCH] user-vm-unlock-2.5.31-A2")
>>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>>>> ---
>>>>  fs/exec.c                    | 4 ++++
>>>>  include/linux/sched/signal.h | 9 ++++++++-
>>>>  kernel/fork.c                | 1 +
>>>>  3 files changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/exec.c b/fs/exec.c
>>>> index c243f9660d46..ad7b518f906d 100644
>>>> --- a/fs/exec.c
>>>> +++ b/fs/exec.c
>>>> @@ -1182,6 +1182,7 @@ static int de_thread(struct linux_binprm *bprm, struct task_struct *tsk)
>>>>  		release_task(leader);
>>>>  	}
>>>>  
>>>> +	mutex_lock(&current->signal->exec_update_mutex);
>
> And by the way, could you make this mutex_lock_killable?

For some reason when I first read this suggestion I thought making this
mutex_lock_killable would cause me to rework the logic of when I
set unrecoverable and when I unlock the mutex.  I blame a tired brain.
If a process has received a fatal signal none of that matters.

So yes I will do that just to make things robust in case we miss
something that would still make it possible to deadlock in with the new
mutex.

I am a little worried that the new mutex might still cover a little too
much.  But past a certain point I we are not being able to make this
code perfect in the first change.  The best we can do is to be careful
and avoid regressions.  Whatever slips through we can fix when we spot
the problem.

Eric