From: Andrew Morton <akpm@linux-foundation.org>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
Alexey Dobriyan <adobriyan@gmail.com>,
Oleg Nesterov <oleg@redhat.com>,
Kees Cook <keescook@chromium.org>,
Andy Lutomirski <luto@amacapital.net>,
Will Drewry <wad@chromium.org>, Shuah Khan <shuah@kernel.org>,
Christian Brauner <christian.brauner@ubuntu.com>,
Michal Hocko <mhocko@suse.com>, Serge Hallyn <serge@hallyn.com>,
James Morris <jamorris@linux.microsoft.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
Charles Haithcock <chaithco@redhat.com>,
Suren Baghdasaryan <surenb@google.com>,
Yafang Shao <laoar.shao@gmail.com>, Helge Deller <deller@gmx.de>,
YiFei Zhu <yifeifz2@illinois.edu>,
Adrian Reber <areber@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
Jens Axboe <axboe@kernel.dk>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
linux-kselftest@vger.kernel.org,
"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH v9] exec: Fix dead-lock in de_thread with ptrace_attach
Date: Fri, 11 Jun 2021 16:16:55 -0700 [thread overview]
Message-ID: <20210611161655.0a3076495e59add166bac58a@linux-foundation.org> (raw)
In-Reply-To: <AM8PR10MB470896FBC519ABCC20486958E4349@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM>
On Fri, 11 Jun 2021 17:55:09 +0200 Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> This introduces signal->unsafe_execve_in_progress,
> which is used to fix the case when at least one of the
> sibling threads is traced, and therefore the trace
> process may dead-lock in ptrace_attach, but de_thread
> will need to wait for the tracer to continue execution.
>
> The solution is to detect this situation and allow
> ptrace_attach to continue, while de_thread() is still
> waiting for traced zombies to be eventually released.
> When the current thread changed the ptrace status from
> non-traced to traced, we can simply abort the whole
> execve and restart it by returning -ERESTARTSYS.
> This needs to be done before changing the thread leader,
> because the PTRACE_EVENT_EXEC needs to know the old
> thread pid.
>
> Although it is technically after the point of no return,
> we just have to reset bprm->point_of_no_return here,
> since at this time only the other threads have received
> a fatal signal, not the current thread.
>
> >From the user's point of view the whole execve was
> simply delayed until after the ptrace_attach.
>
> Other threads die quickly since the cred_guard_mutex
> is released, but a deadly signal is already pending.
> In case the mutex_lock_killable misses the signal,
> ->unsafe_execve_in_progress makes sure they release
> the mutex immediately and return with -ERESTARTNOINTR.
>
> This means there is no API change, unlike the previous
> version of this patch which was discussed here:
>
> https://lore.kernel.org/lkml/b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de/
>
> See tools/testing/selftests/ptrace/vmaccess.c
> for a test case that gets fixed by this change.
>
> Note that since the test case was originally designed to
> test the ptrace_attach returning an error in this situation,
> the test expectation needed to be adjusted, to allow the
> API to succeed at the first attempt.
>
err, sorry. I replied to the v8 patch, not to v9.
--- a/fs/exec.c~exec-fix-dead-lock-in-de_thread-with-ptrace_attach-v9
+++ a/fs/exec.c
@@ -1056,29 +1056,31 @@ static int de_thread(struct task_struct
return -EAGAIN;
}
- while_each_thread(tsk, t) {
- if (unlikely(t->ptrace) && t != tsk->group_leader)
- sig->unsafe_execve_in_progress = true;
- }
-
sig->group_exit_task = tsk;
sig->notify_count = zap_other_threads(tsk);
if (!thread_group_leader(tsk))
sig->notify_count--;
- spin_unlock_irq(lock);
- if (unlikely(sig->unsafe_execve_in_progress))
+ while_each_thread(tsk, t) {
+ if (unlikely(t->ptrace) && t != tsk->group_leader)
+ sig->unsafe_execve_in_progress = true;
+ }
+
+ if (unlikely(sig->unsafe_execve_in_progress)) {
+ spin_unlock_irq(lock);
mutex_unlock(&sig->cred_guard_mutex);
+ spin_lock_irq(lock);
+ }
- for (;;) {
- set_current_state(TASK_KILLABLE);
- if (!sig->notify_count)
- break;
+ while (sig->notify_count) {
+ __set_current_state(TASK_KILLABLE);
+ spin_unlock_irq(lock);
schedule();
if (__fatal_signal_pending(tsk))
goto killed;
+ spin_lock_irq(lock);
}
- __set_current_state(TASK_RUNNING);
+ spin_unlock_irq(lock);
if (unlikely(sig->unsafe_execve_in_progress)) {
if (mutex_lock_killable(&sig->cred_guard_mutex))
_
next prev parent reply other threads:[~2021-06-11 23:16 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-10 7:31 [PATCHv8] exec: Fix dead-lock in de_thread with ptrace_attach Bernd Edlinger
2021-06-10 21:36 ` Andrew Morton
2021-06-11 4:42 ` Bernd Edlinger
2021-06-11 15:55 ` [PATCH v9] " Bernd Edlinger
2021-06-11 23:16 ` Andrew Morton [this message]
2021-06-12 5:22 ` Bernd Edlinger
2021-06-12 7:02 ` Greg KH
2021-06-14 16:42 ` Eric W. Biederman
2021-06-15 14:26 ` Bernd Edlinger
2021-06-16 21:31 ` Bernd Edlinger
2021-06-22 5:10 ` Bernd Edlinger
2021-06-12 19:44 ` [PATCHv8] " Eric W. Biederman
2021-06-11 7:54 ` Bernd Edlinger
2021-06-11 23:15 ` Andrew Morton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210611161655.0a3076495e59add166bac58a@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=adobriyan@gmail.com \
--cc=areber@redhat.com \
--cc=axboe@kernel.dk \
--cc=bernd.edlinger@hotmail.de \
--cc=chaithco@redhat.com \
--cc=christian.brauner@ubuntu.com \
--cc=deller@gmx.de \
--cc=ebiederm@xmission.com \
--cc=jamorris@linux.microsoft.com \
--cc=keescook@chromium.org \
--cc=laoar.shao@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=mhocko@suse.com \
--cc=oleg@redhat.com \
--cc=serge@hallyn.com \
--cc=shuah@kernel.org \
--cc=stable@vger.kernel.org \
--cc=surenb@google.com \
--cc=tglx@linutronix.de \
--cc=viro@zeniv.linux.org.uk \
--cc=wad@chromium.org \
--cc=yifeifz2@illinois.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.