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

Jann Horn [off-list ref] writes:

On Sun, Mar 8, 2020 at 10:41 PM Eric W. Biederman [off-list ref] wrote:

quoted

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.

The plan is to switch the users of cred_guard_mutex to
exec_udpate_mutex one by one.  This lets us move forward while still
being careful and not introducing any regressions.

[...]

quoted

@@ -1034,6 +1035,11 @@ static int exec_mmap(struct mm_struct *mm)
                        return -EINTR;
                }
        }
+
+       ret = mutex_lock_killable(&tsk->signal->exec_update_mutex);
+       if (ret)
+               return ret;

We're already holding the old mmap_sem, and now nest the
exec_update_mutex inside it; but then while still holding the
exec_update_mutex, we do mmput(), which can e.g. end up in ksm_exit(),
which can do down_write(&mm->mmap_sem) from __ksm_exit(). So I think
at least lockdep will be unhappy, and I'm not sure whether it's an
actual problem or not.

Good point.  I should double check the lock ordering here with mmap_sem.
It doesn't look like mmput takes mmap_sem, but still there might be a
lock inversion of some kind here.  At least as far as lockdep is
concerned and we don't want anything like that.

Eric