Bernd Edlinger [off-list ref] writes:
This fixes a deadlock in the tracer when tracing a multi-threaded
application that calls execve while more than one thread are running.
I observed that when running strace on the gcc test suite, it always
blocks after a while, when expect calls execve, because other threads
have to be terminated. They send ptrace events, but the strace is no
longer able to respond, since it is blocked in vm_access.
The deadlock is always happening when strace needs to access the
tracees process mmap, while another thread in the tracee starts to
execve a child process, but that cannot continue until the
PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
A couple of things.
Why do we think it is safe to change the behavior exposed to userspace?
Not the deadlock but all of the times the current code would not
deadlock?
Especially given that this is a small window it might be hard for people
to track down and report so we need a strong argument that this won't
break existing userspace before we just change things.
Usually surveying all of the users of a system call that we can find
and checking to see if they might be affected by the change in behavior
is difficult enough that we usually opt for not being lazy and
preserving the behavior.
This patch is up to two changes in behavior now, that could potentially
affect a whole array of programs. Adding linux-api so that this change
in behavior can be documented if/when this change goes through.
If you can split the documentation and test fixes out into separate
patches that would help reviewing this code, or please make it explicit
that the your are changing documentation about behavior that is changing
with this patch.
Eric
quoted hunk
diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c
new file mode 100644
index 0000000..6d8a048
--- /dev/null
+++ b/tools/testing/selftests/ptrace/vmaccess.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020 Bernd Edlinger <bernd.edlinger@hotmail.de>
+ * All rights reserved.
+ *
+ * Check whether /proc/$pid/mem can be accessed without causing deadlocks
+ * when de_thread is blocked with ->cred_guard_mutex held.
+ */
+
+#include "../kselftest_harness.h"
+#include <stdio.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <signal.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+
+static void *thread(void *arg)
+{
+ ptrace(PTRACE_TRACEME, 0, 0L, 0L);
+ return NULL;
+}
+
+TEST(vmaccess)
+{
+ int f, pid = fork();
+ char mm[64];
+
+ if (!pid) {
+ pthread_t pt;
+
+ pthread_create(&pt, NULL, thread, NULL);
+ pthread_join(pt, NULL);
+ execlp("true", "true", NULL);
+ }
+
+ sleep(1);
+ sprintf(mm, "/proc/%d/mem", pid);
+ f = open(mm, O_RDONLY);
+ ASSERT_LE(0, f);
+ close(f);
+ f = kill(pid, SIGCONT);
+ ASSERT_EQ(0, f);
+}
+
+TEST(attach)
+{
+ int f, pid = fork();
+
+ if (!pid) {
+ pthread_t pt;
+
+ pthread_create(&pt, NULL, thread, NULL);
+ pthread_join(pt, NULL);
+ execlp("true", "true", NULL);
+ }
+
+ sleep(1);
+ f = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
To be meaningful this code needs to learn to loop when
ptrace returns -EAGAIN.
Because that is pretty much what any self respecting user space
process will do.
At which point I am not certain we can say that the behavior has
sufficiently improved not to be a deadlock.
+ ASSERT_EQ(EAGAIN, errno);
+ ASSERT_EQ(f, -1);
+ f = kill(pid, SIGCONT);
+ ASSERT_EQ(0, f);
+}
+
+TEST_HARNESS_MAIN
Eric
On 3/3/20 4:18 PM, Eric W. Biederman wrote:
Bernd Edlinger [off-list ref] writes:
quoted
This fixes a deadlock in the tracer when tracing a multi-threaded
application that calls execve while more than one thread are running.
I observed that when running strace on the gcc test suite, it always
blocks after a while, when expect calls execve, because other threads
have to be terminated. They send ptrace events, but the strace is no
longer able to respond, since it is blocked in vm_access.
The deadlock is always happening when strace needs to access the
tracees process mmap, while another thread in the tracee starts to
execve a child process, but that cannot continue until the
PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
A couple of things.
Why do we think it is safe to change the behavior exposed to userspace?
Not the deadlock but all of the times the current code would not
deadlock?
Especially given that this is a small window it might be hard for people
to track down and report so we need a strong argument that this won't
break existing userspace before we just change things.
Hmm, I tend to agree.
Usually surveying all of the users of a system call that we can find
and checking to see if they might be affected by the change in behavior
is difficult enough that we usually opt for not being lazy and
preserving the behavior.
This patch is up to two changes in behavior now, that could potentially
affect a whole array of programs. Adding linux-api so that this change
in behavior can be documented if/when this change goes through.
One is PTRACE_ACCESS possibly returning EAGAIN, yes.
We could try to restrict that behavior change to when any
thread is ptraced when execve starts, can't be too complicated.
But the other is only SYS_seccomp returning EAGAIN, when a different
thread of the current process is calling execve at the same time.
I would consider it completely impossible to have any user-visual effect,
since de_thread is just terminating all threads, including the thread
where the -EAGAIN was returned, so we will never know what happened.
If you can split the documentation and test fixes out into separate
patches that would help reviewing this code, or please make it explicit
that the your are changing documentation about behavior that is changing
with this patch.
I am not sure if I have touched the right user documentation.
I only saw a document referring to a non-existent "current->cred_replace_mutex"
I haven't digged the git history, but that must be pre-historic IMHO.
It appears to me that is some developer documentation, but it's nevertheless
worth to keep up to date when the code changes.
So where would I add the possibility for PTRACE_ATTACH to return -EAGAIN ?
Bernd.
Eric
quoted
diff --git a/tools/testing/selftests/ptrace/vmaccess.c b/tools/testing/selftests/ptrace/vmaccess.c
new file mode 100644
index 0000000..6d8a048
--- /dev/null
+++ b/tools/testing/selftests/ptrace/vmaccess.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020 Bernd Edlinger <bernd.edlinger@hotmail.de>
+ * All rights reserved.
+ *
+ * Check whether /proc/$pid/mem can be accessed without causing deadlocks
+ * when de_thread is blocked with ->cred_guard_mutex held.
+ */
+
+#include "../kselftest_harness.h"
+#include <stdio.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <signal.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+
+static void *thread(void *arg)
+{
+ ptrace(PTRACE_TRACEME, 0, 0L, 0L);
+ return NULL;
+}
+
+TEST(vmaccess)
+{
+ int f, pid = fork();
+ char mm[64];
+
+ if (!pid) {
+ pthread_t pt;
+
+ pthread_create(&pt, NULL, thread, NULL);
+ pthread_join(pt, NULL);
+ execlp("true", "true", NULL);
+ }
+
+ sleep(1);
+ sprintf(mm, "/proc/%d/mem", pid);
+ f = open(mm, O_RDONLY);
+ ASSERT_LE(0, f);
+ close(f);
+ f = kill(pid, SIGCONT);
+ ASSERT_EQ(0, f);
+}
+
+TEST(attach)
+{
+ int f, pid = fork();
+
+ if (!pid) {
+ pthread_t pt;
+
+ pthread_create(&pt, NULL, thread, NULL);
+ pthread_join(pt, NULL);
+ execlp("true", "true", NULL);
+ }
+
+ sleep(1);
+ f = ptrace(PTRACE_ATTACH, pid, 0L, 0L);
To be meaningful this code needs to learn to loop when
ptrace returns -EAGAIN.
Because that is pretty much what any self respecting user space
process will do.
At which point I am not certain we can say that the behavior has
sufficiently improved not to be a deadlock.
In this special dead-duck test it won't work, but it would
still be lots more transparent what is going on, since previously
you had two zombie process, and no way to even output debug
messages, which also all self respecting user space processes
should do.
So yes, I can at least give a good example and re-try it several
times together with wait4 which a tracer is expected to do.
Bernd.
quoted
+ ASSERT_EQ(EAGAIN, errno);
+ ASSERT_EQ(f, -1);
+ f = kill(pid, SIGCONT);
+ ASSERT_EQ(0, f);
+}
+
+TEST_HARNESS_MAIN
Eric
On Tue, Mar 03, 2020 at 04:48:01PM +0000, Bernd Edlinger wrote:
On 3/3/20 4:18 PM, Eric W. Biederman wrote:
quoted
Bernd Edlinger [off-list ref] writes:
quoted
This fixes a deadlock in the tracer when tracing a multi-threaded
application that calls execve while more than one thread are running.
I observed that when running strace on the gcc test suite, it always
blocks after a while, when expect calls execve, because other threads
have to be terminated. They send ptrace events, but the strace is no
longer able to respond, since it is blocked in vm_access.
The deadlock is always happening when strace needs to access the
tracees process mmap, while another thread in the tracee starts to
execve a child process, but that cannot continue until the
PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
A couple of things.
Why do we think it is safe to change the behavior exposed to userspace?
Not the deadlock but all of the times the current code would not
deadlock?
Especially given that this is a small window it might be hard for people
to track down and report so we need a strong argument that this won't
break existing userspace before we just change things.
Hmm, I tend to agree.
quoted
Usually surveying all of the users of a system call that we can find
and checking to see if they might be affected by the change in behavior
is difficult enough that we usually opt for not being lazy and
preserving the behavior.
This patch is up to two changes in behavior now, that could potentially
affect a whole array of programs. Adding linux-api so that this change
in behavior can be documented if/when this change goes through.
One is PTRACE_ACCESS possibly returning EAGAIN, yes.
We could try to restrict that behavior change to when any
thread is ptraced when execve starts, can't be too complicated.
But the other is only SYS_seccomp returning EAGAIN, when a different
thread of the current process is calling execve at the same time.
I would consider it completely impossible to have any user-visual effect,
since de_thread is just terminating all threads, including the thread
where the -EAGAIN was returned, so we will never know what happened.
I think if we risk a user-space facing change we should try the simple
thing first before making the fix more convoluted? But it's a tough
call...
quoted
If you can split the documentation and test fixes out into separate
patches that would help reviewing this code, or please make it explicit
that the your are changing documentation about behavior that is changing
with this patch.
I am not sure if I have touched the right user documentation.
I only saw a document referring to a non-existent "current->cred_replace_mutex"
I haven't digged the git history, but that must be pre-historic IMHO.
It appears to me that is some developer documentation, but it's nevertheless
worth to keep up to date when the code changes.
So where would I add the possibility for PTRACE_ATTACH to return -EAGAIN ?
Since that would be a potentially user-visible change it would make the
most sense to add it to man ptrace(2) if/when we land this change.
For developers, placing a comment in kernel/ptrace.c:ptrace_attach()
would make the most sense? We already have something about exec
protection in there.
Christian
On Tue, Mar 03, 2020 at 06:01:11PM +0100, Christian Brauner wrote:
On Tue, Mar 03, 2020 at 04:48:01PM +0000, Bernd Edlinger wrote:
quoted
On 3/3/20 4:18 PM, Eric W. Biederman wrote:
quoted
Bernd Edlinger [off-list ref] writes:
quoted
This fixes a deadlock in the tracer when tracing a multi-threaded
application that calls execve while more than one thread are running.
I observed that when running strace on the gcc test suite, it always
blocks after a while, when expect calls execve, because other threads
have to be terminated. They send ptrace events, but the strace is no
longer able to respond, since it is blocked in vm_access.
The deadlock is always happening when strace needs to access the
tracees process mmap, while another thread in the tracee starts to
execve a child process, but that cannot continue until the
PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
A couple of things.
Why do we think it is safe to change the behavior exposed to userspace?
Not the deadlock but all of the times the current code would not
deadlock?
Especially given that this is a small window it might be hard for people
to track down and report so we need a strong argument that this won't
break existing userspace before we just change things.
Hmm, I tend to agree.
quoted
Usually surveying all of the users of a system call that we can find
and checking to see if they might be affected by the change in behavior
is difficult enough that we usually opt for not being lazy and
preserving the behavior.
This patch is up to two changes in behavior now, that could potentially
affect a whole array of programs. Adding linux-api so that this change
in behavior can be documented if/when this change goes through.
One is PTRACE_ACCESS possibly returning EAGAIN, yes.
We could try to restrict that behavior change to when any
thread is ptraced when execve starts, can't be too complicated.
But the other is only SYS_seccomp returning EAGAIN, when a different
thread of the current process is calling execve at the same time.
I would consider it completely impossible to have any user-visual effect,
since de_thread is just terminating all threads, including the thread
where the -EAGAIN was returned, so we will never know what happened.
I think if we risk a user-space facing change we should try the simple
thing first before making the fix more convoluted? But it's a tough
call...
Actually, to get a _rough_ estimate of the possible impact I would
recommend you run the criu test suite (and possible the strace
test-suite) on a kernel with and without your fix. That's what I tend to
do when I touch code I fear will have impact on APIs that very deeply
touch core kernel. Criu's test-suite makes heavy use of ptrace and
usually runs into a bunch of interesting (exec) races too, and does have
tests for handling zombies processes etc. pp.
Should be relatively simple: create a vm and then criu build-dependencies,
git clone criu; cd criu; make; cd test; ./zdtm.py run -a --keep-going
If your system doesn't support Selinux properly, you need to disable it
when running the tests and you also need to make sure that you're using
python3 or change the shebang in zdtm.py to python3.
Just a recommendation.
Christian
On Tue, Mar 03, 2020 at 09:18:44AM -0600, Eric W. Biederman wrote:
Bernd Edlinger [off-list ref] writes:
quoted
This fixes a deadlock in the tracer when tracing a multi-threaded
application that calls execve while more than one thread are running.
I observed that when running strace on the gcc test suite, it always
blocks after a while, when expect calls execve, because other threads
have to be terminated. They send ptrace events, but the strace is no
longer able to respond, since it is blocked in vm_access.
The deadlock is always happening when strace needs to access the
tracees process mmap, while another thread in the tracee starts to
execve a child process, but that cannot continue until the
PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
A couple of things.
Why do we think it is safe to change the behavior exposed to userspace?
Not the deadlock but all of the times the current code would not
deadlock?
Especially given that this is a small window it might be hard for people
to track down and report so we need a strong argument that this won't
break existing userspace before we just change things.
Usually surveying all of the users of a system call that we can find
and checking to see if they might be affected by the change in behavior
is difficult enough that we usually opt for not being lazy and
preserving the behavior.
This patch is up to two changes in behavior now, that could potentially
affect a whole array of programs. Adding linux-api so that this change
in behavior can be documented if/when this change goes through.
If you can split the documentation and test fixes out into separate
patches that would help reviewing this code, or please make it explicit
that the your are changing documentation about behavior that is changing
with this patch.
Agreed. I think it'd be good to do it in three patches:
1. unrelated documentation update
2. fix + documentation changes specific to the fix
3. test(s)
Christian