From: Bernd Edlinger <bernd.edlinger@hotmail.de>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
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>,
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: Wed, 16 Jun 2021 23:31:21 +0200 [thread overview]
Message-ID: <AM8PR10MB47083E11E2B39ACBDF396954E40F9@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <AS8PR10MB47120E7A195A593C1377172CE4309@AS8PR10MB4712.EURPRD10.PROD.OUTLOOK.COM>
On 6/15/21 4:26 PM, Bernd Edlinger wrote:
> Thanks for your review.
>
> On 6/14/21 6:42 PM, Eric W. Biederman wrote:
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>
>>> 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.
>>
>> Userspace processes hang waiting for each other. Not a proper kernel
>> deadlock. Annoying but not horrible. Definitely worth fixing if we can.
>>
>
> I wonder if I am used a wrong term in the title.
> Do you have a suggestion for better wording?
>
>>> 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.
>>
>> Except you are not detecting this situation. Testing for t->ptrace
>> finds tasks that have completed their ptrace attach and no longer need
>> the cred_gaurd_mutex.
>>
>
> The first phase of de_thread needs co-operation from a user task,
> if and only if any task t except the thread leader has t->ptrace.
> Taking tasks from RUNNING->EXIT_ZOMBIE only needs co-operation from kernel code,
Aehm, sorry, that is not correct, what I said here.
I totally overlooked ptrace(PTRACE_SEIZE, pid, 0L, PTRACE_O_TRACEEXIT)
and unfortunately this also prevents even the thread leader to enter the
EXIT_ZOMBIE state because do_exit does:
ptrace_event(PTRACE_EVENT_EXIT, code);
unfortunately this sends an event to the tracer, and waits not only for
the tracer to call waitpid, but also needs a PTRACE_CONT before do_exit
can call exit_notify which does tsk->exit_state = EXIT_ZOMBIE.
So unfortunately this breaks my patch, so I have to withdraw it for now,
since I see no way how to fix it.
I will clean-up my previous patch which changes the ptrace API to return
an error if an unsafe execve is detected, and send it to this list.
Thanks
Bernd.
next prev parent reply other threads:[~2021-06-16 21:31 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
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 [this message]
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=AM8PR10MB47083E11E2B39ACBDF396954E40F9@AM8PR10MB4708.EURPRD10.PROD.OUTLOOK.COM \
--to=bernd.edlinger@hotmail.de \
--cc=adobriyan@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=areber@redhat.com \
--cc=axboe@kernel.dk \
--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.