Possible bug introduced in commit 9b84cca

DISCUSSION Denys Vlasenko 5 participants 15 messages Dec 28, 2011 – Jan 4, 2012 ptrace support

lore

Share & tools

lore

Denys Vlasenko

author

2011-12-28 18:56 UTC #0 lore raw

Hi Tejun, Oleg,

Apologies if you are already informed about this bug by people who originally discovered it.

Looks like after commit 9b84cca, waitpid under strace sometimes returns bogus ECHILD while child does exist.

I did not yet confirm that the bug appeared exactly at this commit - Łukasz says that.

I confirmed that bug exists on kernels 3.1.6 (in Fedora) and 3.1.0-rc4 (vanilla).

We have a testcase which spawns N threads, each of them performs an infinite loop "fork, exit in child, waitpid in parent for the child". When straced, sometimes waitpid returns ECHILD. In fact, there is no need to run many threads - I just saw it happening with single thread on 4-CPU machine when I ran "strace -otestcase1.LOG -f ./testcase1 1". This machine uses 3.1.0-rc4.

Please find testcase attached.

Also please find testcase1.LOG attached.
The key part is here:

931   clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xf763dbd8) = 1048
1048  exit_group(42)                    = ?
931   waitpid(1048,  <unfinished ...>
1048  +++ exited with 42 +++
931   <... waitpid resumed> 0xf763d3a0, 0) = -1 ECHILD (No child processes)

To complicate matters, this is observed only under development version of strace. Old (released) versions of strace do not let ptraced processes to die - they detach from them when they think they are going to die (such as when they enter _exit() or receive a "deadly" signal). Which is a aesthetically horrible and logically buggy (racy) hack, so we are removing it from strace. Łukasz says that old strace versions (ones which still use the hack) don't trigger the bug.

For testing, I will send you strace source tree and pre-compiled strace binary in a separate email. Alternatively, pull latest strace git and "autoreconf -fvi && ./configure && make" it.

Signature · 1 line

vda

#include <errno.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/sysinfo.h>
#include <sys/wait.h>
#include <stdio.h>
#include <pthread.h>

void* worker(void *arg)

{
	while (1) {
		pid_t p = fork();
		if (-1 == p) { /* error */
			perror("fork");
			_exit(EXIT_FAILURE);
		}
		if (0 == p) { /* child */
			_exit(42);
		}
		/* parent */
		int stat_loc;
		int s = waitpid(p, &stat_loc, 0);
		if (-1 == s) {
			perror("waitpid");
			_exit(EXIT_FAILURE);
		}
	}
}

int main(int argc, char **argv)

{
	int pool_size = get_nprocs() * 4;

	if (argv[1])
		pool_size = atoi(argv[1]);
	printf("Poolsize: %d\n", pool_size);

	pthread_t thread_id;
	int i;
	for (i = 0; i != pool_size; ++i) {
		if (pthread_create(&thread_id, NULL, worker, NULL) != 0) {
			perror("pthread_create");
			_exit(EXIT_FAILURE);
		}
	}

	/* Prevent exiting: wait for last thread (forever) */
	void *retval;
	pthread_join(thread_id, &retval);

	return 43;
}

Denys Vlasenko

2011-12-28 21:07 UTC #1 lore raw

Quoted 5 lines · Denys Vlasenko

Hi Tejun, Oleg,
Apologies if you are already informed about this bug by people who originally discovered it.

Quoted 8 lines

Looks like after commit 9b84cca, waitpid under strace sometimes returns bogus ECHILD while child does exist.
I did not yet confirm that the bug appeared exactly at this commit - Łukasz says that.
I confirmed that bug exists on kernels 3.1.6 (in Fedora) and 3.1.0-rc4 (vanilla).

I tested it under a few more Fedora kernels.
On kernel-PAE-2.6.39-1.fc16.i686 bug does not trigger. On next Fedora kernel, kernel-PAE-3.0-0.rc1.git0.2.fc16.i686, it triggers.

And indeed, these kernels differ in kernel/exit.c::wait_consider_task() only by commit 9b84cca.

Oleg, if you need to test it, I have the setup on my machine.

Quoted 3 lines · Łukasz Michalik

The bug only happens on recent linux. I've bisected the kernel and found that it was introduced by 9b84cca2564b9 [2] in linux.git, so pretty much everything since 3.0-rc2 is affected.

Łukasz, looks like 3.0-rc1 is affected too.

Signature · 1 line

vda

Łukasz Michalik

2011-12-28 21:33 UTC #2 lore raw

Quoted 7 lines · Denys Vlasenko

Quoted 3 lines · Łukasz Michalik
The bug only happens on recent linux. I've bisected the kernel and found that it was introduced by 9b84cca2564b9 [2] in linux.git, so pretty much everything since 3.0-rc2 is affected.
Łukasz, looks like 3.0-rc1 is affected too.

Yeah, my git foo failed me back then. 3.0-rc1 is the the first upstream tag that contains that change.

Signature · 2 lines

Pozdrawiam,
Łukasz P. Michalik

Oleg Nesterov

maintainer

2011-12-29 11:38 UTC #3 lore raw

In reply to Denys Vlasenko

Quoted 7 lines · Denys Vlasenko

Looks like after commit 9b84cca, waitpid under strace sometimes returns bogus ECHILD while child does exist.
I did not yet confirm that the bug appeared exactly at this commit - Łukasz says that.

Quoted 7 lines

I confirmed that bug exists on kernels 3.1.6 (in Fedora) and 3.1.0-rc4 (vanilla).
We have a testcase which spawns N threads, each of them performs an infinite loop "fork, exit in child, waitpid in parent for the child". When straced, sometimes waitpid returns ECHILD.

You mean, the natural parent gets ECHILD, not strace?

Quoted 7 lines

The key part is here:

931   clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xf763dbd8) = 1048
1048  exit_group(42)                    = ?
931   waitpid(1048,  <unfinished ...>
1048  +++ exited with 42 +++
931   <... waitpid resumed> 0xf763d3a0, 0) = -1 ECHILD (No child processes)

Argh. I seem to understand

I didn't check, but I think the offending commit is 823b018e5b1196d8 "job control: Small reorganization of wait_consider_task()".

ptracer sees EXIT_ZOMBIE and temporary sets EXIT_DEAD, this fools the ->real_parent.

I need to think. The fix should be simple, but perhaps it is the time to kill EXIT_DEAD altogether. I'll try to make the patch after vacation. In the next year ;)

Thanks a lot Denys!

Oleg.

Oleg Nesterov

maintainer

2011-12-29 12:10 UTC #4 lore raw

Quoted 32 lines · Oleg Nesterov

Quoted 7 lines · Denys Vlasenko
Looks like after commit 9b84cca, waitpid under strace sometimes returns bogus ECHILD while child does exist.
I did not yet confirm that the bug appeared exactly at this commit - Łukasz says that.
Quoted 7 lines
I confirmed that bug exists on kernels 3.1.6 (in Fedora) and 3.1.0-rc4 (vanilla).
We have a testcase which spawns N threads, each of them performs an infinite loop "fork, exit in child, waitpid in parent for the child". When straced, sometimes waitpid returns ECHILD.
You mean, the natural parent gets ECHILD, not strace?
Quoted 7 lines
The key part is here:
931   clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xf763dbd8) = 1048
1048  exit_group(42)                    = ?
931   waitpid(1048,  <unfinished ...>
1048  +++ exited with 42 +++
931   <... waitpid resumed> 0xf763d3a0, 0) = -1 ECHILD (No child processes)
Argh. I seem to understand
I didn't check, but I think the offending commit is 823b018e5b1196d8 "job control: Small reorganization of wait_consider_task()".

Quoted 8 lines

ptracer sees EXIT_ZOMBIE and temporary sets EXIT_DEAD, this fools the ->real_parent.
I need to think. The fix should be simple, but perhaps it is the time to kill EXIT_DEAD altogether. I'll try to make the patch after vacation. In the next year ;)
Thanks a lot Denys!

I've made the simple test-case, it triggers the bug.

Oleg.

#include <sys/ptrace.h>
#include <sys/wait.h>
#include <unistd.h>
#include <stdio.h>
#include <assert.h>

int main(void)

{
	int pid, status;

	pid = fork();
	if (!pid) {
		for (;;) {
			if (!fork())
				return 0x23;
			assert(waitpid(-1, &status, 0) > 0);
			assert(status == 0x2300);
		}
	}

	assert(ptrace(PTRACE_ATTACH, pid, 0,0) == 0);
	assert(waitpid(-1, NULL, 0) == pid);

	assert(ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_TRACEFORK) == 0);

	for (;;) {
		ptrace(PTRACE_CONT, pid, 0, 0);
		pid = waitpid(-1, NULL, 0);
		assert(pid > 0);
	}

	return 0;
}

Oleg Nesterov

maintainer

2012-01-03 14:35 UTC #5 lore raw

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov
Quoted 1 line · Denys Vlasenko

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

931   clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xf763dbd8) = 1048
1048  exit_group(42)                    = ?
931   waitpid(1048,  <unfinished ...>
1048  +++ exited with 42 +++
931   <... waitpid resumed> 0xf763d3a0, 0) = -1 ECHILD (No child processes)

I've made the simple test-case, it triggers the bug.

I am going to send the hack^Wpatch below to Linus as a temporary workaround for 3.2.

Any chance you can test it?

I did the testing too with my test-case, but it would be nice to know if it works for you.

Thanks,

Oleg.

kernel/exit.c renamed from kernel/exit.c~ +4 -1

@@ -1540,8 +1540,11 @@ static int wait_consider_task(struct wai
 	}
 
 	/* dead body doesn't have much to contribute */
-	if (p->exit_state == EXIT_DEAD)
+	if (unlikely(p->exit_state == EXIT_DEAD)) {
+		if (likely(!ptrace) && unlikely(ptrace_reparented(p)))
+			wo->notask_error = 0;
 		return 0;
+	}
 
 	/* slay zombie? */
 	if (p->exit_state == EXIT_ZOMBIE) {

Oleg Nesterov

maintainer

2012-01-03 15:49 UTC #6 lore raw

ptrace fixes for 3.2

Quoted 3 lines · Oleg Nesterov

I am going to send the hack^Wpatch below to Linus as a temporary workaround for 3.2.

The same patch with the changelog.

Denys, Tejun, any chance you can review it before I send it to Linus ?

Also, I am going to send this one:

	http://marc.info/?l=linux-kernel&m=131705871825598&w=2

could you please take a look?

Oleg.

------------------------------------------------------------------------------
[PATCH] ptrace: partially fix the do_wait(WEXITED) vs EXIT_DEAD->EXIT_ZOMBIE race

Test-case:

	int main(void)
	{
		int pid, status;

		pid = fork();
		if (!pid) {
			for (;;) {
				if (!fork())
					return 0;
				if (waitpid(-1, &status, 0) < 0) {
					printf("ERR!! wait: %m\n");
					return 0;
				}
			}
		}

		assert(ptrace(PTRACE_ATTACH, pid, 0,0) == 0);
		assert(waitpid(-1, NULL, 0) == pid);

		assert(ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_TRACEFORK) == 0);

		do {
			ptrace(PTRACE_CONT, pid, 0, 0);
			pid = waitpid(-1, NULL, 0);
		} while (pid > 0);

		return 1;
	}

It fails because ->real_parent sees its child in EXIT_DEAD state while the tracer is going to change the state back to EXIT_ZOMBIE in wait_task_zombie().

The offending commit is 823b018e which moved the EXIT_DEAD check, but in fact we should not blame it. The original code was not correct as well because it didn't take ptrace_reparented() into account and because we can't really trust ->ptrace.

This patch adds the additional check to close this particular race but it doesn't solve the whole problem. We simply can't rely on ->ptrace in this case, it can be cleared if the tracer is multithreaded by the exiting ->parent.

I think we should kill EXIT_DEAD altogether, we should always remove the soon-to-be-reaped child from ->children or at least we should never do the DEAD->ZOMBIE transition. But this is too complex for 3.2.

Also, I think wait_consider_task() needs more fixes. I do not think we should clear ->notask_error without WEXITED in this case, but this is what we do in the EXIT_ZOMBIE case.

Reported-by Denys Vlasenko <[email protected]>

Cc v3.0.. <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/exit.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

kernel/exit.c +8 -1

index d0b7d98..e6e01b9 100644
@@ -1540,8 +1540,15 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 	}
 
 	/* dead body doesn't have much to contribute */
-	if (p->exit_state == EXIT_DEAD)
+	if (unlikely(p->exit_state == EXIT_DEAD)) {
+		/*
+		 * But do not ignore this task until the tracer does
+		 * wait_task_zombie()->do_notify_parent().
+		 */
+		if (likely(!ptrace) && unlikely(ptrace_reparented(p)))
+			wo->notask_error = 0;
 		return 0;
+	}
 
 	/* slay zombie? */
 	if (p->exit_state == EXIT_ZOMBIE) {

Signature · 1 line

1.5.5.1

Tejun Heo

Acked-by

2012-01-03 16:30 UTC #7 lore raw

Re: ptrace fixes for 3.2

Hello, Oleg.

Quoted 3 lines · Oleg Nesterov

It fails because ->real_parent sees its child in EXIT_DEAD state while the tracer is going to change the state back to EXIT_ZOMBIE in wait_task_zombie().

Argh.... EXIT_ZOMBIE -> DEAD -> ZOMBIE dancing in wait_task_zombie() is just nasty. Didn't realize it was doing that. :(

Quoted 9 lines

The offending commit is 823b018e which moved the EXIT_DEAD check, but in fact we should not blame it. The original code was not correct as well because it didn't take ptrace_reparented() into account and because we can't really trust ->ptrace.
This patch adds the additional check to close this particular race but it doesn't solve the whole problem. We simply can't rely on ->ptrace in this case, it can be cleared if the tracer is multithreaded by the exiting ->parent.

I'm not following this part. Can you please explain it in a bit more detail?

Quoted 4 lines

I think we should kill EXIT_DEAD altogether, we should always remove the soon-to-be-reaped child from ->children or at least we should never do the DEAD->ZOMBIE transition. But this is too complex for 3.2.

Agreed. Removing the reverse transition shouldn't be too difficult and can be done without affecting fast non-ptrace path. ie. if the child is ptraced, drop readlock, grab writelock, recheck, buffer states to copy out to userland, detach and transit to DEAD if necessary.

Quoted 3 lines

Also, I think wait_consider_task() needs more fixes. I do not think we should clear ->notask_error without WEXITED in this case, but this is what we do in the EXIT_ZOMBIE case.

Hmmm... I'm not sure about that. Why do you think so?

Quoted 3 lines

Reported-by Denys Vlasenko <[email protected]>
Cc v3.0.. <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>

Anyways, the fix looks good to me.

Acked-by Tejun Heo <[email protected]>

Thank you.

Signature · 1 line

tejun

Denys Vlasenko

2012-01-03 16:52 UTC #13 lore raw

Re: ptrace fixes for 3.2

In reply to Oleg Nesterov

Quoted 8 lines · Oleg Nesterov

Quoted 3 lines · Oleg Nesterov
I am going to send the hack^Wpatch below to Linus as a temporary workaround for 3.2.
The same patch with the changelog.
Denys, Tejun, any chance you can review it before I send it to Linus ?

Testing it on top of Fedora 16's kernel-3.1.6-1.fc16:

Confirmed that before patch both tests fail (in about a minute of run time at worst), and with patch they do not fail.

Signature · 1 line

vda

Oleg Nesterov

maintainer

2012-01-03 17:15 UTC #8 lore raw

Re: ptrace fixes for 3.2

In reply to Tejun Heo

Hi Tejun,

Quoted 8 lines · Tejun Heo

Quoted 3 lines · Oleg Nesterov
It fails because ->real_parent sees its child in EXIT_DEAD state while the tracer is going to change the state back to EXIT_ZOMBIE in wait_task_zombie().
Argh.... EXIT_ZOMBIE -> DEAD -> ZOMBIE dancing in wait_task_zombie() is just nasty. Didn't realize it was doing that. :(

We both missed this ;)

Quoted 5 lines

Quoted 5 lines
The offending commit is 823b018e which moved the EXIT_DEAD check, but in fact we should not blame it. The original code was not correct as well because it didn't take ptrace_reparented() into account and because we can't really trust ->ptrace.

Quoted 7 lines

Quoted 4 lines
This patch adds the additional check to close this particular race but it doesn't solve the whole problem. We simply can't rely on ->ptrace in this case, it can be cleared if the tracer is multithreaded by the exiting ->parent.
I'm not following this part. Can you please explain it in a bit more detail?

Before 823b018e the code was:

	if (!ptrace && p->ptrace) {
		wo->notask_error = 0;
		return 0;
	}

	if (p->exit_state == EXIT_DEAD)
		return 0;

There are 2 problems:

1. it is not correct to clear ->notask_error unless this child is ptrace_reparented(). Nobody will wakeup us if EXIT_DEAD was set by our sub-thread.

2. We can not rely on ->ptrace to detect this case.

Suppose that the tracer is multithreaded, it has two threads T1 and T2, T1 traces our child.

- T2 does do_wait(WEXITED), sets EXIT_DEAD, drops tasklist_lock.

- T1 exits and does __ptrace_detach(), this means __ptrace_unlink() and nothing more.

- Now, real_parent does do_wait() and sees the EXIT_DEAD child but ->ptrace = 0.

- finally T2 sets EXIT_DEAD but it is too late,

The patch doesn't solve the 2nd (btw very old) problem. Fortunately this race is very unlikely.

Quoted 10 lines

Quoted 4 lines
I think we should kill EXIT_DEAD altogether, we should always remove the soon-to-be-reaped child from ->children or at least we should never do the DEAD->ZOMBIE transition. But this is too complex for 3.2.
Agreed. Removing the reverse transition shouldn't be too difficult and can be done without affecting fast non-ptrace path. ie. if the child is ptraced, drop readlock, grab writelock, recheck, buffer states to copy out to userland, detach and transit to DEAD if necessary.

Yes.

Quoted 5 lines

Quoted 3 lines
Also, I think wait_consider_task() needs more fixes. I do not think we should clear ->notask_error without WEXITED in this case, but this is what we do in the EXIT_ZOMBIE case.
Hmmm... I'm not sure about that. Why do you think so?

I am not sure too. But why do_wait() should sleep if it is called without WEXITED (lets ignore WCONTINUED) and the child is ZOMBIE? I think it should return -ECHILD, like it does if the child is not traced.

IOW. Suppose we have a single EXIT_ZOMBIE child. If it is not traced, do_wait(WSTOPPED) returns -ECHILD. If the child is traced (by another process) do_wait() sleeps until detach just to return the same error. This looks a bit strange.

Quoted 1 line

Acked-by Tejun Heo <[email protected]>

Great, thanks.

Oleg.

Tejun Heo

2012-01-03 19:18 UTC #9 lore raw

Re: ptrace fixes for 3.2

Hello,

Quoted 23 lines · Oleg Nesterov

Quoted 5 lines
Quoted 5 lines
The offending commit is 823b018e which moved the EXIT_DEAD check, but in fact we should not blame it. The original code was not correct as well because it didn't take ptrace_reparented() into account and because we can't really trust ->ptrace.
Quoted 7 lines
Quoted 4 lines
This patch adds the additional check to close this particular race but it doesn't solve the whole problem. We simply can't rely on ->ptrace in this case, it can be cleared if the tracer is multithreaded by the exiting ->parent.
I'm not following this part. Can you please explain it in a bit more detail?
Before 823b018e the code was:
	if (!ptrace && p->ptrace) {
		wo->notask_error = 0;
		return 0;
	}

	if (p->exit_state == EXIT_DEAD)
		return 0;

Quoted 5 lines

There are 2 problems:
1. it is not correct to clear ->notask_error unless this child is ptrace_reparented(). Nobody will wakeup us if EXIT_DEAD was set by our sub-thread.

Yeah, if that happens between normal and ptrace wait_consider_task() calls.

Quoted 8 lines

2. We can not rely on ->ptrace to detect this case.
Suppose that the tracer is multithreaded, it has two threads T1 and T2, T1 traces our child.
- T2 does do_wait(WEXITED), sets EXIT_DEAD, drops tasklist_lock.

Quoted 7 lines

- T1 exits and does __ptrace_detach(), this means __ptrace_unlink() and nothing more.
- Now, real_parent does do_wait() and sees the EXIT_DEAD child but ->ptrace = 0.
- finally T2 sets EXIT_DEAD but it is too late,

You mean EXIT_ZOMBIE here, right? So, to rephrase, ZOMBIE -> DEAD -> ZOMBIE dancing may race against tracer detaching. Urgh... why do we even allow tasks which aren't the direct tracer to wait for ptraced children anyway when ptrace ops are restricted to the ptracing task?

Quoted 11 lines

Quoted 5 lines
Quoted 3 lines
Also, I think wait_consider_task() needs more fixes. I do not think we should clear ->notask_error without WEXITED in this case, but this is what we do in the EXIT_ZOMBIE case.
Hmmm... I'm not sure about that. Why do you think so?
I am not sure too. But why do_wait() should sleep if it is called without WEXITED (lets ignore WCONTINUED) and the child is ZOMBIE? I think it should return -ECHILD, like it does if the child is not traced.

Quoted 4 lines

IOW. Suppose we have a single EXIT_ZOMBIE child. If it is not traced, do_wait(WSTOPPED) returns -ECHILD. If the child is traced (by another process) do_wait() sleeps until detach just to return the same error. This looks a bit strange.

I don't think it really matter either way. To me, both behaviors seem understandable and I don't think the current behavior would cause any real problem.

Thanks a lot for the explanation.

Signature · 1 line

tejun

Łukasz Michalik

2012-01-04 09:00 UTC #14 lore raw

Re: ptrace fixes for 3.2

In reply to Denys Vlasenko

Quoted 6 lines · Denys Vlasenko

Quoted 5 lines · Oleg Nesterov
Quoted 3 lines · Oleg Nesterov
I am going to send the hack^Wpatch below to Linus as a temporary workaround for 3.2.

Quoted 9 lines

Quoted 3 lines
The same patch with the changelog.
Denys, Tejun, any chance you can review it before I send it to Linus ?
Testing it on top of Fedora 16's kernel-3.1.6-1.fc16:
Confirmed that before patch both tests fail (in about a minute of run time at worst), and with patch they do not fail.

Tested on top vanilla's HEAD and stable 3.1.7, with both testcases and both strace and sydbox. Confirming same as Denys.

Signature · 2 lines

Pozdrawiam,
Łukasz P. Michalik

Oleg Nesterov

maintainer

2012-01-04 11:41 UTC #10 lore raw

Re: ptrace fixes for 3.2

In reply to Tejun Heo

Hi,

Quoted 14 lines · Tejun Heo

Quoted 11 lines · Oleg Nesterov
2. We can not rely on ->ptrace to detect this case.
Suppose that the tracer is multithreaded, it has two threads T1 and T2, T1 traces our child.
- T2 does do_wait(WEXITED), sets EXIT_DEAD, drops tasklist_lock.
- T1 exits and does __ptrace_detach(), this means __ptrace_unlink() and nothing more.

Quoted 6 lines

Quoted 4 lines
- Now, real_parent does do_wait() and sees the EXIT_DEAD child but ->ptrace = 0.
- finally T2 sets EXIT_DEAD but it is too late,
You mean EXIT_ZOMBIE here, right?

Yes, typo...

Quoted 4 lines

So, to rephrase, ZOMBIE -> DEAD ->
ZOMBIE dancing may race against tracer detaching. Urgh... why do we even allow tasks which aren't the direct tracer to wait for ptraced children anyway when ptrace ops are restricted to the ptracing task?

If only I knew ;) I am not even sure this is by design... OTOH, it is not clear how we should restrict do_wait() if the tracee is not ptrace_reparented().

Anyway we can't change this.

Quoted 5 lines

Quoted 5 lines
I am not sure too. But why do_wait() should sleep if it is called without WEXITED (lets ignore WCONTINUED) and the child is ZOMBIE? I think it should return -ECHILD, like it does if the child is not traced.

Quoted 8 lines

Quoted 4 lines
IOW. Suppose we have a single EXIT_ZOMBIE child. If it is not traced, do_wait(WSTOPPED) returns -ECHILD. If the child is traced (by another process) do_wait() sleeps until detach just to return the same error. This looks a bit strange.
I don't think it really matter either way. To me, both behaviors seem understandable and I don't think the current behavior would cause any real problem.

Yes I agree this is minor, surely not a bug.

OK, in any case this has nothing to do with this fix, I'll remove this sentence from the changelog to avoid the confusion.

Tejun, do you see any problem with the 2nd fix below?

Oleg.

------------------------------------------------------------------------------
Can't understand how did we miss this, but WARN_ON_ONCE(!ptrace) in do_signal_stop() is not right. Debugger can resume the stopped task, and it can clone the _untraced_ thread running in the stopped group.

And even if the new thread is auto-attached, we have the problems with JOBCTL_STOP_SIGMASK.

I do not want to discuss the "proper" solution here. I think the necessary changes are simple, but I do not think this is the 3.1 material, and 3.1 needs some trivial fix.

What do you think about the patch below? I am going to send it to Linus unless you see something wrong.

And I'd like to explain why I prefer to add the (temporary) hack into __ptrace_unlink() instead of adding

	if (unlikely(ptrace) && current->ptrace) {
		...
		child->jobctl = current->jobctl & JOBCTL_STOP_SIGMASK;
		...
	}

into ptrace_init_task(). I think that jobctl & JOBCTL_STOP_SIGMASK" should be cleanuped. Look, once we set JOBCTL_STOP_SIGMASK we never clear it. Yes, this doesn't really matter but this can hide an error, for example this can "fool" the WARN_ON(!signr) in do_jobctl_trap(). Imho, at least SIGCONT should clear this part of ->jobctl. IOW, it should be non-zero only if/when it has effect.

That is why I do not want to change ptrace_init_task() until we decide what should we do with CLONE_THREAD && SIGNAL_STOP_STOPPED, to avoid the bogus (jobctl & JOBCTL_STOP_SIGMASK) != 0. IOW I prefer the change in __ptrace_unlink() to catch the other possible problems like this.

Also, I'd like to defend 6634ae10
"ptrace_init_task: initialize child->jobctl explicitly" which can be blamed for the 2nd problem. Afaics, this change is really needed and it fixes the bug. The changelog says "Currently this is harmless" but this is not right, dup_task_struct() can happen between SIGSTOP and SIGCONT, in this case the child can have the wrong JOBCTL_STOP_PENDING.

So, what do you think?

Oleg.

-------------------------------------------------------------------------------
[PATCH for 3.1] ptrace_unlink: ensure JOBCTL_STOP_SIGMASK is not zero

This is the temporary simple fix for 3.1, we need more changes in this area.

1. do_signal_stop() assumes that the running untraced thread in the stopped thread group is not possible. This was our goal but it is not yet achieved: a stopped-but-resumed tracee can clone the running thread which can initiate another group-stop.

Remove WARN_ON_ONCE(!current->ptrace).

2. A new thread always starts with ->jobctl = 0. If it is auto-attached and this group is stopped, __ptrace_unlink() sets JOBCTL_STOP_PENDING but JOBCTL_STOP_SIGMASK part is zero, this triggers WANR_ON(!signr) in do_jobctl_trap() if another debugger attaches.

Change __ptrace_unlink() to set the artificial SIGSTOP for report.

Alternatively we could change ptrace_init_task() to copy signr from current, but this means we can copy it for no reason and hide the possible similar problems.

Signed-off-by Oleg Nesterov <[email protected]>

---

 kernel/signal.c |    2 --
 kernel/ptrace.c |   13 ++++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

kernel/signal.c renamed from kernel/signal.c~2_traced_stop_and_clone +0 -2

@@ -1986,8 +1986,6 @@ static bool do_signal_stop(int signr)
 		 */
 		if (!(sig->flags & SIGNAL_STOP_STOPPED))
 			sig->group_exit_code = signr;
-		else
-			WARN_ON_ONCE(!current->ptrace);
 
 		sig->group_stop_count = 0;

kernel/ptrace.c renamed from kernel/ptrace.c~2_traced_stop_and_clone +12 -1

@@ -96,9 +96,20 @@ void __ptrace_unlink(struct task_struct 
 	 */
 	if (!(child->flags & PF_EXITING) &&
 	    (child->signal->flags & SIGNAL_STOP_STOPPED ||
-	     child->signal->group_stop_count))
+	     child->signal->group_stop_count)) {
 		child->jobctl |= JOBCTL_STOP_PENDING;
 
+		/*
+		 * This is only possible if this thread was cloned by the
+		 * traced task running in the stopped group, set the signal
+		 * for the future reports.
+		 * FIXME: we should change ptrace_init_task() to handle this
+		 * case.
+		 */
+		if (!(child->jobctl & JOBCTL_STOP_SIGMASK))
+			child->jobctl |= SIGSTOP;
+	}
+
 	/*
 	 * If transition to TASK_STOPPED is pending or in TASK_TRACED, kick
 	 * @child in the butt.  Note that @resume should be used iff @child

Tejun Heo

Acked-by

2012-01-04 15:31 UTC #11 lore raw

Re: ptrace fixes for 3.2

Hello,

Quoted 4 lines · Oleg Nesterov

Can't understand how did we miss this, but WARN_ON_ONCE(!ptrace) in do_signal_stop() is not right. Debugger can resume the stopped task, and it can clone the _untraced_ thread running in the stopped group.

Right, we should be setting JOBCTL_STOP_PENDING for newly cloned tasks if sigstop is in effect.

Quoted 6 lines

And even if the new thread is auto-attached, we have the problems with JOBCTL_STOP_SIGMASK.
I do not want to discuss the "proper" solution here. I think the necessary changes are simple, but I do not think this is the 3.1 material, and 3.1 needs some trivial fix.

Yeah, sure thing.

Quoted 12 lines

What do you think about the patch below? I am going to send it to Linus unless you see something wrong.
And I'd like to explain why I prefer to add the (temporary) hack into __ptrace_unlink() instead of adding
	if (unlikely(ptrace) && current->ptrace) {
		...
		child->jobctl = current->jobctl & JOBCTL_STOP_SIGMASK;
		...
	}

Quoted 6 lines

into ptrace_init_task(). I think that jobctl & JOBCTL_STOP_SIGMASK" should be cleanuped. Look, once we set JOBCTL_STOP_SIGMASK we never clear it. Yes, this doesn't really matter but this can hide an error, for example this can "fool" the WARN_ON(!signr) in do_jobctl_trap(). Imho, at least SIGCONT should clear this part of ->jobctl. IOW, it should be non-zero only if/when it has effect.

I don't recall the details but there was somewhing convoluted about clearing the signal number. There's some non-obvious case where the signr is used after what appears to be the apparent lifespan. I *think* we already talked about this but could be imagining things. But, yeah, sure, if we can do it without complicating stuff, why not?

Quoted 5 lines

That is why I do not want to change ptrace_init_task() until we decide what should we do with CLONE_THREAD && SIGNAL_STOP_STOPPED, to avoid the bogus (jobctl & JOBCTL_STOP_SIGMASK) != 0. IOW I prefer the change in __ptrace_unlink() to catch the other possible problems like this.

As long as it gets fixed properly in the next devel cycle, I don't have any objection.

Quoted 8 lines

Also, I'd like to defend 6634ae10
"ptrace_init_task: initialize child->jobctl explicitly" which can be blamed for the 2nd problem. Afaics, this change is really needed and it fixes the bug. The changelog says "Currently this is harmless" but this is not right, dup_task_struct() can happen between SIGSTOP and SIGCONT, in this case the child can have the wrong JOBCTL_STOP_PENDING.
So, what do you think?

Looks good to me, provided proper fix is coming soon. :p

Quoted 18 lines

-------------------------------------------------------------------------------
[PATCH for 3.1] ptrace_unlink: ensure JOBCTL_STOP_SIGMASK is not zero
This is the temporary simple fix for 3.1, we need more changes in this area.
1. do_signal_stop() assumes that the running untraced thread in the stopped thread group is not possible. This was our goal but it is not yet achieved: a stopped-but-resumed tracee can clone the running thread which can initiate another group-stop.
   Remove WARN_ON_ONCE(!current->ptrace).
2. A new thread always starts with ->jobctl = 0. If it is auto-attached and this group is stopped, __ptrace_unlink() sets JOBCTL_STOP_PENDING but JOBCTL_STOP_SIGMASK part is zero, this triggers WANR_ON(!signr) in do_jobctl_trap() if another debugger attaches.

Quoted 7 lines

Change __ptrace_unlink() to set the artificial SIGSTOP for report.
Alternatively we could change ptrace_init_task() to copy signr from current, but this means we can copy it for no reason and hide the possible similar problems.
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2012-01-04 16:05 UTC #12 lore raw

Re: ptrace fixes for 3.2

Hello,

Quoted 9 lines · Tejun Heo

Quoted 4 lines · Oleg Nesterov
Can't understand how did we miss this, but WARN_ON_ONCE(!ptrace) in do_signal_stop() is not right. Debugger can resume the stopped task, and it can clone the _untraced_ thread running in the stopped group.
Right, we should be setting JOBCTL_STOP_PENDING for newly cloned tasks if sigstop is in effect.

Yes, probably this is the natural choice.

Quoted 1 line

Looks good to me, provided proper fix is coming soon. :p

Good.

Yes, this is the temporary fix for 3.2.

Thanks!

Oleg.