On Tue, Mar 03, 2020 at 04:48:01PM +0000, Bernd Edlinger wrote:
On 3/3/20 4:18 PM, Eric W. Biederman wrote:
Bernd Edlinger bernd.edlinger@hotmail.de 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.
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.
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...
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