| Message ID | 87blmy6zay.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Trivial cleanups for exec | expand |
On Fri, May 08, 2020 at 01:45:25PM -0500, Eric W. Biederman wrote: > > Oleg modified the code that did > "mutex_lock_interruptible(¤t->cred_guard_mutex)" to return > -ERESTARTNOINTR instead of -EINTR, so that userspace will never see a > failure to grab the mutex. > > Slightly earlier Liam R. Howlett defined mutex_lock_killable for > exactly the same situation but it does it a little more cleanly. > > Switch the code to mutex_lock_killable so that it is clearer what the > code is doing. > > Ref: ad776537cc6b ("Add mutex_lock_killable") > Ref: 793285fcafce ("cred_guard_mutex: do not return -EINTR to user-space") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Fri, May 8, 2020 at 11:48 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > > Oleg modified the code that did > "mutex_lock_interruptible(¤t->cred_guard_mutex)" to return > -ERESTARTNOINTR instead of -EINTR, so that userspace will never see a > failure to grab the mutex. > > Slightly earlier Liam R. Howlett defined mutex_lock_killable for > exactly the same situation but it does it a little more cleanly. What what what? None of this makes sense. Your commit message is completely wrong, and the patch is utter shite. mutex_lock_interruptible() and mutex_lock_killable() are completely different operations, and the difference has absolutely nothing to do with -ERESTARTNOINTR or -EINTR. mutex_lock_interruptible() is interrupted by any signal. mutex_lock_killable() is - surprise surprise - only interrupted by SIGKILL (in theory any fatal signal, but we never actually implemented that logic, so it's only interruptible by the known-to-always-be-fatal SIGKILL). > Switch the code to mutex_lock_killable so that it is clearer what the > code is doing. This nonsensical patch makes me worry about all your other patches. The explanation is wrong, the patch is wrong, and it changes things to be fundamentally broken. Before this, ^C would break out of a blocked execve()/ptrace() situation. After this patch, you need special tools to do so. This patch is completely wrong. And Kees, what the heck is that "Reviewed-by" for? Worthless review too. Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Fri, May 8, 2020 at 11:48 AM Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> >> Oleg modified the code that did >> "mutex_lock_interruptible(¤t->cred_guard_mutex)" to return >> -ERESTARTNOINTR instead of -EINTR, so that userspace will never see a >> failure to grab the mutex. >> >> Slightly earlier Liam R. Howlett defined mutex_lock_killable for >> exactly the same situation but it does it a little more cleanly. > > What what what? > > None of this makes sense. Your commit message is completely wrong, and > the patch is utter shite. > > mutex_lock_interruptible() and mutex_lock_killable() are completely > different operations, and the difference has absolutely nothing to do > with -ERESTARTNOINTR or -EINTR. > > mutex_lock_interruptible() is interrupted by any signal. > > mutex_lock_killable() is - surprise surprise - only interrupted by > SIGKILL (in theory any fatal signal, but we never actually implemented > that logic, so it's only interruptible by the known-to-always-be-fatal > SIGKILL). > >> Switch the code to mutex_lock_killable so that it is clearer what the >> code is doing. > > This nonsensical patch makes me worry about all your other patches. > The explanation is wrong, the patch is wrong, and it changes things to > be fundamentally broken. > > Before this, ^C would break out of a blocked execve()/ptrace() > situation. After this patch, you need special tools to do so. > > This patch is completely wrong. Sigh. Brain fart on my part. You are correct. I saw the restart, and totally forgot that it allows the handling of a signal before restarting the system call. Except for the handling of the signal in userspace it is the same as mutex_lock_killable but that is a big big big if. My apologies. I will drop this patch. Eric
On Sat, May 09, 2020 at 12:18:06PM -0700, Linus Torvalds wrote: > On Fri, May 8, 2020 at 11:48 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > > > > > Oleg modified the code that did > > "mutex_lock_interruptible(¤t->cred_guard_mutex)" to return > > -ERESTARTNOINTR instead of -EINTR, so that userspace will never see a > > failure to grab the mutex. > > > > Slightly earlier Liam R. Howlett defined mutex_lock_killable for > > exactly the same situation but it does it a little more cleanly. > > mutex_lock_interruptible() and mutex_lock_killable() are completely > different operations, and the difference has absolutely nothing to do > with -ERESTARTNOINTR or -EINTR. > > [...] > > And Kees, what the heck is that "Reviewed-by" for? Worthless review too. Yeah, I messed that up; apologies. And I know exactly where my brain misfired on this one. On a related note, I must stop doing code reviews on Friday night. :)
diff --git a/fs/exec.c b/fs/exec.c index 82106241ed53..11a5c073aa35 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1493,8 +1493,9 @@ EXPORT_SYMBOL(finalize_exec); */ static int prepare_bprm_creds(struct linux_binprm *bprm) { - if (mutex_lock_interruptible(¤t->signal->cred_guard_mutex)) - return -ERESTARTNOINTR; + int retval = mutex_lock_killable(¤t->signal->cred_guard_mutex); + if (retval) + return retval; bprm->cred = prepare_exec_creds(); if (likely(bprm->cred)) diff --git a/kernel/ptrace.c b/kernel/ptrace.c index 43d6179508d6..1876b3392488 100644 --- a/kernel/ptrace.c +++ b/kernel/ptrace.c @@ -391,8 +391,8 @@ static int ptrace_attach(struct task_struct *task, long request, * SUID, SGID and LSM creds get determined differently * under ptrace. */ - retval = -ERESTARTNOINTR; - if (mutex_lock_interruptible(&task->signal->cred_guard_mutex)) + retval = mutex_lock_killable(&task->signal->cred_guard_mutex); + if (retval) goto out; task_lock(task);
Oleg modified the code that did "mutex_lock_interruptible(¤t->cred_guard_mutex)" to return -ERESTARTNOINTR instead of -EINTR, so that userspace will never see a failure to grab the mutex. Slightly earlier Liam R. Howlett defined mutex_lock_killable for exactly the same situation but it does it a little more cleanly. Switch the code to mutex_lock_killable so that it is clearer what the code is doing. Ref: ad776537cc6b ("Add mutex_lock_killable") Ref: 793285fcafce ("cred_guard_mutex: do not return -EINTR to user-space") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 5 +++-- kernel/ptrace.c | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-)