linux-rt-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH rt] Fix races in ptrace
@ 2013-07-24 16:23 Alexander Fyodorov
  2013-08-12 16:41 ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Fyodorov @ 2013-07-24 16:23 UTC (permalink / raw)
  To: linux-rt-users

Hi all,

I ran into a race on 2.6.33-rt kernel which I think still exists in the latest RT patches. The following patch fixes it but it may be incomplete: probably spinlock should be taken in all other places that check saved_state too.  I only compile-tested this patch for 3.6-rt.


read_lock(&tasklist_lock) in ptrace_stop() is converted to mutex on RT kernel, and it can remove __TASK_TRACED from task->state (by moving it to task->saved_state). If parent does wait() on child followed by a sys_ptrace call, the following race can happen:

 - child sets __TASK_TRACED in ptrace_stop()
 - parent does wait() which eventually calls wait_task_stopped() and returns child's pid
 - child blocks on read_lock(&tasklist_lock) in ptrace_stop() and moves __TASK_TRACED flag to saved_state
 - parent calls sys_ptrace, which calls ptrace_check_attach() and wait_task_inactive()

ptrace_check_attach() checks only child->state and will fail, and wait_task_inactive() checks both child->state and child->saved_state but it doesn't hold child->pi_lock so it can fail because of memory ordering too. Locking child->pi_lock and checking saved_state solves both races.

Signed-off-by: Alexander Fyodorov <halcy <at> yandex.ru>

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index a232bb5..79b5f5d 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -159,8 +159,20 @@ int ptrace_check_attach(struct task_struct *child, bool ignore_state)
 		spin_lock_irq(&child->sighand->siglock);
 		WARN_ON_ONCE(task_is_stopped(child));
 		if (ignore_state || (task_is_traced(child) &&
-				     !(child->jobctl & JOBCTL_LISTENING)))
+				     !(child->jobctl & JOBCTL_LISTENING))) {
 			ret = 0;
+		} else {
+			/*
+			 * Test again before failing, but this time take
+			 * the spinlock and look at child->saved_state
+			 */
+			raw_spin_lock(&child->pi_lock);
+			if ((task_is_traced(child) ||
+					(child->saved_state & __TASK_TRACED)) &&
+					!(child->jobctl & JOBCTL_LISTENING))
+				ret = 0;
+			raw_spin_unlock(&child->pi_lock);
+		}
 		spin_unlock_irq(&child->sighand->siglock);
 	}
 	read_unlock(&tasklist_lock);
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5051ca6..a9cb295 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1215,8 +1215,21 @@ unsigned long wait_task_inactive(struct task_struct *p, long match_state)
 		 */
 		while (task_running(rq, p)) {
 			if (match_state && unlikely(p->state != match_state)
-			    && unlikely(p->saved_state != match_state))
-				return 0;
+			    && unlikely(p->saved_state != match_state)) {
+				/*
+				 * Check again under spinlock
+				 */
+				raw_spin_lock_irqsave(&p->pi_lock, flags);
+
+				if (unlikely(p->state != match_state &&
+					     p->saved_state != match_state)) {
+					raw_spin_unlock_irqrestore(&p->pi_lock,
+							flags);
+					return 0;
+				}
+
+				raw_spin_unlock_irqrestore(&p->pi_lock, flags);
+			}
 			cpu_relax();
 		}
 

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-07-24 16:23 [PATCH rt] Fix races in ptrace Alexander Fyodorov
@ 2013-08-12 16:41 ` Sebastian Andrzej Siewior
  2013-08-12 21:13   ` Alexander Fyodorov
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-08-12 16:41 UTC (permalink / raw)
  To: Alexander Fyodorov; +Cc: linux-rt-users

* Alexander Fyodorov | 2013-07-24 20:23:27 [+0400]:

>Hi all,
Hi Alexander,

>I ran into a race on 2.6.33-rt kernel which I think still exists in the latest RT patches. The following patch fixes it but it may be incomplete: probably spinlock should be taken in all other places that check saved_state too.  I only compile-tested this patch for 3.6-rt.

It looks interresting, thanks for the report. Do you have maybe a
test-case which can provoke the bug?

Sebastian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-12 16:41 ` Sebastian Andrzej Siewior
@ 2013-08-12 21:13   ` Alexander Fyodorov
  2013-08-21 17:24     ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Fyodorov @ 2013-08-12 21:13 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linux-rt-users@vger.kernel.org

12.08.2013, 20:41, "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>:
> * Alexander Fyodorov | 2013-07-24 20:23:27 [+0400]:
> Hi Alexander,

Hi Sebastian,

>> I ran into a race on 2.6.33-rt kernel which I think still exists in the latest RT patches. The following patch fixes it but it may be incomplete: probably spinlock should be taken in all other places that check saved_state too.  I only compile-tested this patch for 3.6-rt.
>
> It looks interresting, thanks for the report. Do you have maybe a
> test-case which can provoke the bug?

I can't publish the test but essentially it was doing the following: one process (application) has 20 threads sending signals to each other (creating contention for tasklist_lock and profiling events), and another one (profiler) attaches to the application and then does:
	for (;;) {
		pid = wait();
		ptrace(PTRACE_CONT, pid);
	}
Is there a need to write a test?
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-12 21:13   ` Alexander Fyodorov
@ 2013-08-21 17:24     ` Sebastian Andrzej Siewior
  2013-08-22 14:23       ` Alexander Fyodorov
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-08-21 17:24 UTC (permalink / raw)
  To: Alexander Fyodorov; +Cc: linux-rt-users@vger.kernel.org

* Alexander Fyodorov | 2013-08-13 01:13:56 [+0400]:

>Hi Sebastian,
Hi Alexander,

>Is there a need to write a test?

It is nice to have so it can be checked if this bug is fixed "some place
else".

Now that I looked at it for a while, would this fix your trouble?

diff --git a/kernel/rtmutex.c b/kernel/rtmutex.c
--- a/kernel/rtmutex.c
+++ b/kernel/rtmutex.c
@@ -724,6 +724,7 @@ static void  noinline __sched rt_spin_lock_slowlock(struct rt_mutex *lock)
 	struct task_struct *lock_owner, *self = current;
 	struct rt_mutex_waiter waiter, *top_waiter;
 	int ret;
+	int new_state;
 
 	rt_mutex_init_waiter(&waiter, true);
 
@@ -744,8 +745,11 @@ static void  noinline __sched rt_spin_lock_slowlock(struct rt_mutex *lock)
 	 * try_to_wake_up().
 	 */
 	pi_lock(&self->pi_lock);
+	new_state = TASK_UNINTERRUPTIBLE;
+	if (task_is_traced(self))
+		new_state |= __TASK_TRACED;
 	self->saved_state = self->state;
-	__set_current_state(TASK_UNINTERRUPTIBLE);
+	__set_current_state(new_state);
 	pi_unlock(&self->pi_lock);
 
 	ret = task_blocks_on_rt_mutex(lock, &waiter, self, 0);

This should avoid that the trace state is lost while waiting on that
mutex and the checks for "is traced" may remain the same.

Sebastian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-21 17:24     ` Sebastian Andrzej Siewior
@ 2013-08-22 14:23       ` Alexander Fyodorov
  2013-08-29 16:33         ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Fyodorov @ 2013-08-22 14:23 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linux-rt-users@vger.kernel.org

21.08.2013, 21:24, "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>:
> Now that I looked at it for a while, would this fix your trouble?
>
> diff --git a/kernel/rtmutex.c b/kernel/rtmutex.c
> --- a/kernel/rtmutex.c
> +++ b/kernel/rtmutex.c
> @@ -724,6 +724,7 @@ static void  noinline __sched rt_spin_lock_slowlock(struct rt_mutex *lock)
>          struct task_struct *lock_owner, *self = current;
>          struct rt_mutex_waiter waiter, *top_waiter;
>          int ret;
> + int new_state;
>
>          rt_mutex_init_waiter(&waiter, true);
>
> @@ -744,8 +745,11 @@ static void  noinline __sched rt_spin_lock_slowlock(struct rt_mutex *lock)
>           * try_to_wake_up().
>           */
>          pi_lock(&self->pi_lock);
> + new_state = TASK_UNINTERRUPTIBLE;
> + if (task_is_traced(self))
> + new_state |= __TASK_TRACED;
>          self->saved_state = self->state;
> - __set_current_state(TASK_UNINTERRUPTIBLE);
> + __set_current_state(new_state);
>          pi_unlock(&self->pi_lock);
>
>          ret = task_blocks_on_rt_mutex(lock, &waiter, self, 0);
>
> This should avoid that the trace state is lost while waiting on that
> mutex and the checks for "is traced" may remain the same.

This was the first thing I tried but it did not work. ptrace_check_attach() passes TASK_TRACED to wait_task_inactive() which tests for equality:

unsigned long wait_task_inactive(struct task_struct *p, long match_state)
< ... >
                        if (match_state && unlikely(p->state != match_state)
                            && unlikely(p->saved_state != match_state)) {

But this test will fail because we've added TASK_UNINTERRUPTIBLE and possibly removed some other bits from p->state. So I decided to add pi_lock locking instead - it is slower but it works with 100% guarantee.

Also adding __TASK_TRACED does not help with all the other places where saved_state is checked and more races might still be hidden.
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-22 14:23       ` Alexander Fyodorov
@ 2013-08-29 16:33         ` Sebastian Andrzej Siewior
  2013-08-29 17:26           ` Alexander Fyodorov
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-08-29 16:33 UTC (permalink / raw)
  To: Alexander Fyodorov; +Cc: linux-rt-users@vger.kernel.org

* Alexander Fyodorov | 2013-08-22 18:23:18 [+0400]:

>This was the first thing I tried but it did not work. ptrace_check_attach() passes TASK_TRACED to wait_task_inactive() which tests for equality:
>
>unsigned long wait_task_inactive(struct task_struct *p, long match_state)
>< ... >
>                        if (match_state && unlikely(p->state != match_state)
>                            && unlikely(p->saved_state != match_state)) {
>
>But this test will fail because we've added TASK_UNINTERRUPTIBLE and possibly removed some other bits from p->state. So I decided to add pi_lock locking instead - it is slower but it works with 100% guarantee.
>
>Also adding __TASK_TRACED does not help with all the other places where saved_state is checked and more races might still be hidden.

I'm tempted to add this:

---
 include/linux/sched.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index e0a05de..bd60b9d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -175,7 +175,6 @@ extern char ___assert_task_state[1 - 2*!!(
 				 TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
 				 __TASK_TRACED)
 
-#define task_is_traced(task)	((task->state & __TASK_TRACED) != 0)
 #define task_is_stopped(task)	((task->state & __TASK_STOPPED) != 0)
 #define task_is_dead(task)	((task)->exit_state != 0)
 #define task_is_stopped_or_traced(task)	\
@@ -2532,6 +2531,24 @@ static inline int signal_pending_state(long state, struct task_struct *p)
 	return (state & TASK_INTERRUPTIBLE) || __fatal_signal_pending(p);
 }
 
+static inline bool task_is_traced(struct task_struct *task)
+{
+	bool traced = false;
+
+	if (task->state & __TASK_TRACED)
+		return true;
+#ifdef CONFIG_PREEMPT_RT_FULL
+	/* in case the task is sleeping on tasklist_lock */
+	raw_spin_lock_irq(&task->pi_lock);
+	if (task->state & __TASK_TRACED)
+		traced = true;
+	else if (task->saved_state & __TASK_TRACED)
+		traced = true;
+	raw_spin_unlock_irq(&task->pi_lock);
+#endif
+	return traced;
+}
+
 /*
  * cond_resched() and cond_resched_lock(): latency reduction via
  * explicit rescheduling in places that are safe. The return

to the next v3.10-rt release. I tested this with:

--- cat ptrace-test.c ---

#include <unistd.h>
#include <sys/types.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/ptrace.h>
#include <sys/types.h>
#include <sys/wait.h>

#include <sys/time.h>
#include <sys/resource.h>

#define NUM_CHILD       5
#define LOCK_CHILD      3

static pid_t pids[NUM_CHILD];

static void play(void)
{
	pid_t pid;
	int ret;

	pid = getpid();
	fprintf(stderr, "%s(%d) ready\n", __func__, pid);
	ret = ptrace(PTRACE_TRACEME, 0, NULL, 0);
	if (ret) {
		fprintf(stderr, "ERR %d: %m\n", pid);
		return;
	}
	do {
		kill(pid, SIGUSR1);
	} while (1);
}

static void kill_kids(void)
{
	int i;

	for (i = 0; i < NUM_CHILD; i++)  {
		if (pids[i])
			kill(pids[i], SIGKILL);
	}
}

static void get_prio_fork(void)
{
	pid_t pid;
	int i;

	pid = fork();
	if (pid < 0) {
		fprintf(stderr, "fork(): %m\n");
		exit(1);
	}
	if (pid)
		return;
	do  {
		for (i = 0; i < NUM_CHILD; i++) {
			int ret;

			ret = getpriority(PRIO_PROCESS, pids[i]);
			if (ret < 0) {
				printf("=> %m %d\n", pids[i]);
				exit(1);
			}
		}
	} while(1);
	exit(0);
}

int main(void)
{
	int i;


	for (i = 0; i < NUM_CHILD; i++) {
		pid_t pid;

		pid = fork();
		if (pid < 0) {
			fprintf(stderr, "Fork error: %m\n");
			kill_kids();
			exit(1);
		}
		if (pid == 0) {
			play();
			exit(0);
		}
		pids[i] = pid;
	}

	for (i = 0; i < LOCK_CHILD; i++)
		get_prio_fork();

	do {
		for (i = 0; i < NUM_CHILD; i++) {
			int ret;

			ret = waitpid(pids[i], NULL, 0);
			if (ret < 0) {
				fprintf(stderr, "%s(2) %d / %d %m\n", __func__,
						pids[i], ret);
				exit(1);
			}
			ret = ptrace(PTRACE_CONT, pids[i], NULL, 0);
			if (ret) {
				fprintf(stderr, "%s(3) %d / %d %m\n", __func__,
						pids[i], ret);
				ret = kill(pids[i], 0);
				if (!ret)
					fprintf(stderr, "process is available\n");
				exit(1);
			}
		}
	} while(1);
	return 0;
}

--- cat end ---

and it seems to work. Any comments?

Sebastian

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-29 16:33         ` Sebastian Andrzej Siewior
@ 2013-08-29 17:26           ` Alexander Fyodorov
  2013-08-29 18:28             ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Fyodorov @ 2013-08-29 17:26 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linux-rt-users@vger.kernel.org

> +static inline bool task_is_traced(struct task_struct *task)
> +{
> + bool traced = false;
> +
> + if (task->state & __TASK_TRACED)
> + return true;
> +#ifdef CONFIG_PREEMPT_RT_FULL
> + /* in case the task is sleeping on tasklist_lock */
> + raw_spin_lock_irq(&task->pi_lock);
> + if (task->state & __TASK_TRACED)
> + traced = true;
> + else if (task->saved_state & __TASK_TRACED)
> + traced = true;
> + raw_spin_unlock_irq(&task->pi_lock);
> +#endif
> + return traced;
> +}

Since this is a low-level function, maybe its better to use raw_spin_lock_irqsave()? In case someone in the future will call task_is_traced() with disabled interrupts. Otherwise looks good.

Still this is only half of the solution because the patch doesn't solve the race in wait_task_inactive() (and all other places which test both state and saved_state without holding pi_lock).

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-29 17:26           ` Alexander Fyodorov
@ 2013-08-29 18:28             ` Sebastian Andrzej Siewior
  2013-08-29 18:47               ` Alexander Fyodorov
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-08-29 18:28 UTC (permalink / raw)
  To: Alexander Fyodorov; +Cc: linux-rt-users@vger.kernel.org

On 08/29/2013 07:26 PM, Alexander Fyodorov wrote:
>> +static inline bool task_is_traced(struct task_struct *task)
>> +{
>> + bool traced = false;
>> +
>> + if (task->state & __TASK_TRACED)
>> + return true;
>> +#ifdef CONFIG_PREEMPT_RT_FULL
>> + /* in case the task is sleeping on tasklist_lock */
>> + raw_spin_lock_irq(&task->pi_lock);
>> + if (task->state & __TASK_TRACED)
>> + traced = true;
>> + else if (task->saved_state & __TASK_TRACED)
>> + traced = true;
>> + raw_spin_unlock_irq(&task->pi_lock);
>> +#endif
>> + return traced;
>> +}
> 
> Since this is a low-level function, maybe its better to use raw_spin_lock_irqsave()? In case someone in the future will call task_is_traced() with disabled interrupts. Otherwise looks good.

The other function around don't do this and excpect it process context.
Thanks so far.

> 
> Still this is only half of the solution because the patch doesn't solve the race in wait_task_inactive() (and all other places which test both state and saved_state without holding pi_lock).

So you are concerned that missing pi_lock in wait_task_inactive(). This
is a problem if the task wakes up from sleeping on the lock while its
state is beeing checked. Hmm it indeed looks legal.
I keep that patch in queue but disabled and take another look once I
get back.
Does this missing pi_lock() affects you or is just a precaution?

> 

Sebastian

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-29 18:28             ` Sebastian Andrzej Siewior
@ 2013-08-29 18:47               ` Alexander Fyodorov
  2013-08-29 18:49                 ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Alexander Fyodorov @ 2013-08-29 18:47 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linux-rt-users@vger.kernel.org

29.08.2013, 22:28, "Sebastian Andrzej Siewior" <bigeasy@linutronix.de>:
> On 08/29/2013 07:26 PM, Alexander Fyodorov wrote:
>>  Still this is only half of the solution because the patch doesn't solve the race in wait_task_inactive() (and all other places which test both state and saved_state without holding pi_lock).
>
> So you are concerned that missing pi_lock in wait_task_inactive(). This
> is a problem if the task wakes up from sleeping on the lock while its
> state is beeing checked. Hmm it indeed looks legal.
> I keep that patch in queue but disabled and take another look once I
> get back.
> Does this missing pi_lock() affects you or is just a precaution?

Yes, my test application failed because of it, a detailed description is in the first message in the thread. I've also looked at other places that test saved_state but I don't understand the code enough to decide whether there are similar bugs there or not.
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH rt] Fix races in ptrace
  2013-08-29 18:47               ` Alexander Fyodorov
@ 2013-08-29 18:49                 ` Sebastian Andrzej Siewior
  2013-11-30 20:07                   ` [PATCH v2] ptrace: fix ptrace vs tasklist_lock race Sebastian Andrzej Siewior
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-08-29 18:49 UTC (permalink / raw)
  To: Alexander Fyodorov; +Cc: linux-rt-users@vger.kernel.org

On 08/29/2013 08:47 PM, Alexander Fyodorov wrote:
> 
> Yes, my test application failed because of it, a detailed description is in the first message in the thread. I've also looked at other places that test saved_state but I don't understand the code enough to decide whether there are similar bugs there or not.

Yes, you did I just wanted to make sure since it looks like a very
small race window. Will take a look on the remaining part once I get
back…

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] ptrace: fix ptrace vs tasklist_lock race
  2013-08-29 18:49                 ` Sebastian Andrzej Siewior
@ 2013-11-30 20:07                   ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 11+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-11-30 20:07 UTC (permalink / raw)
  To: Alexander Fyodorov, Steven Rostedt
  Cc: linux-rt-users@vger.kernel.org, Thomas Gleixner

As explained by Alexander Fyodorov <halcy@yandex.ru>:

|read_lock(&tasklist_lock) in ptrace_stop() is converted to mutex on RT kernel,
|and it can remove __TASK_TRACED from task->state (by moving  it to
|task->saved_state). If parent does wait() on child followed by a sys_ptrace
|call, the following race can happen:
|
|- child sets __TASK_TRACED in ptrace_stop()
|- parent does wait() which eventually calls wait_task_stopped() and returns
|  child's pid
|- child blocks on read_lock(&tasklist_lock) in ptrace_stop() and moves
|  __TASK_TRACED flag to saved_state
|- parent calls sys_ptrace, which calls ptrace_check_attach() and wait_task_inactive()

The patch is based on his initial patch where an additional check is
added in case the __TASK_TRACED moved to ->saved_state. The pi_lock is
taken in case the caller is interrupted between looking into ->state and
->saved_state.

Additionally I also extended the check in task_is_stopped_or_traced()
and consider the two possible states in ptrace_freeze_traced().
In wait_task_inactive() the state is now checked under the pi_lock as
well. I merged Steven's patch in this one
(rfc-sched-rt-fix-wait_task_interactive-to-test-rt_spin_lock-state.patch)
which already checked both places but without the lock.
This seems to pass my tiny testcase…

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/sched.h |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 kernel/ptrace.c       |    7 ++++++-
 kernel/sched/core.c   |   24 +++++++++++++++++++++---
 3 files changed, 72 insertions(+), 7 deletions(-)

--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -165,11 +165,8 @@ extern char ___assert_task_state[1 - 2*!
 				 TASK_UNINTERRUPTIBLE | __TASK_STOPPED | \
 				 __TASK_TRACED)
 
-#define task_is_traced(task)	((task->state & __TASK_TRACED) != 0)
 #define task_is_stopped(task)	((task->state & __TASK_STOPPED) != 0)
 #define task_is_dead(task)	((task)->exit_state != 0)
-#define task_is_stopped_or_traced(task)	\
-			((task->state & (__TASK_STOPPED | __TASK_TRACED)) != 0)
 #define task_contributes_to_load(task)	\
 				((task->state & TASK_UNINTERRUPTIBLE) != 0 && \
 				 (task->flags & PF_FROZEN) == 0)
@@ -2410,6 +2407,51 @@ static inline int need_resched(void)
 	return unlikely(test_thread_flag(TIF_NEED_RESCHED));
 }
 
+static inline bool __task_is_stopped_or_traced(struct task_struct *task)
+{
+	if (task->state & (__TASK_STOPPED | __TASK_TRACED))
+		return true;
+#ifdef CONFIG_PREEMPT_RT_FULL
+	if (task->saved_state & (__TASK_STOPPED | __TASK_TRACED))
+		return true;
+#endif
+	return false;
+}
+
+static inline bool task_is_stopped_or_traced(struct task_struct *task)
+{
+	bool traced_stopped;
+
+#ifdef CONFIG_PREEMPT_RT_FULL
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	traced_stopped = __task_is_stopped_or_traced(task);
+	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+#else
+	traced_stopped = __task_is_stopped_or_traced(task);
+#endif
+	return traced_stopped;
+}
+
+static inline bool task_is_traced(struct task_struct *task)
+{
+	bool traced = false;
+
+	if (task->state & __TASK_TRACED)
+		return true;
+#ifdef CONFIG_PREEMPT_RT_FULL
+	/* in case the task is sleeping on tasklist_lock */
+	raw_spin_lock_irq(&task->pi_lock);
+	if (task->state & __TASK_TRACED)
+		traced = true;
+	else if (task->saved_state & __TASK_TRACED)
+		traced = true;
+	raw_spin_unlock_irq(&task->pi_lock);
+#endif
+	return traced;
+}
+
 /*
  * cond_resched() and cond_resched_lock(): latency reduction via
  * explicit rescheduling in places that are safe. The return
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -135,7 +135,12 @@ static bool ptrace_freeze_traced(struct
 
 	spin_lock_irq(&task->sighand->siglock);
 	if (task_is_traced(task) && !__fatal_signal_pending(task)) {
-		task->state = __TASK_TRACED;
+		raw_spin_lock_irq(&task->pi_lock);
+		if (task->state & __TASK_TRACED)
+			task->state = __TASK_TRACED;
+		else
+			task->saved_state = __TASK_TRACED;
+		raw_spin_unlock_irq(&task->pi_lock);
 		ret = true;
 	}
 	spin_unlock_irq(&task->sighand->siglock);
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1024,6 +1024,20 @@ struct migration_arg {
 
 static int migration_cpu_stop(void *data);
 
+static bool check_task_state(struct task_struct *p, long match_state)
+{
+	bool match = false;
+
+	raw_spin_lock_irq(&p->pi_lock);
+	if (p->state == match_state)
+		match = true;
+	else if (p->saved_state == match_state)
+		match = true;
+	raw_spin_unlock_irq(&p->pi_lock);
+
+	return match;
+}
+
 /*
  * wait_task_inactive - wait for a thread to unschedule.
  *
@@ -1068,8 +1082,11 @@ unsigned long wait_task_inactive(struct
 		 * is actually now running somewhere else!
 		 */
 		while (task_running(rq, p)) {
-			if (match_state && unlikely(p->state != match_state))
+			if (match_state) {
+				if (!unlikely(check_task_state(p, match_state)))
+					return 0;
 				return 0;
+			}
 			cpu_relax();
 		}
 
@@ -1083,7 +1100,8 @@ unsigned long wait_task_inactive(struct
 		running = task_running(rq, p);
 		on_rq = p->on_rq;
 		ncsw = 0;
-		if (!match_state || p->state == match_state)
+		if (!match_state || p->state == match_state
+		    || p->saved_state == match_state)
 			ncsw = p->nvcsw | LONG_MIN; /* sets MSB */
 		task_rq_unlock(rq, p, &flags);
 
@@ -1579,7 +1597,7 @@ static void try_to_wake_up_local(struct
  */
 int wake_up_process(struct task_struct *p)
 {
-	WARN_ON(task_is_stopped_or_traced(p));
+	WARN_ON(__task_is_stopped_or_traced(p));
 	return try_to_wake_up(p, TASK_NORMAL, 0);
 }
 EXPORT_SYMBOL(wake_up_process);
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-11-30 20:07 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-24 16:23 [PATCH rt] Fix races in ptrace Alexander Fyodorov
2013-08-12 16:41 ` Sebastian Andrzej Siewior
2013-08-12 21:13   ` Alexander Fyodorov
2013-08-21 17:24     ` Sebastian Andrzej Siewior
2013-08-22 14:23       ` Alexander Fyodorov
2013-08-29 16:33         ` Sebastian Andrzej Siewior
2013-08-29 17:26           ` Alexander Fyodorov
2013-08-29 18:28             ` Sebastian Andrzej Siewior
2013-08-29 18:47               ` Alexander Fyodorov
2013-08-29 18:49                 ` Sebastian Andrzej Siewior
2013-11-30 20:07                   ` [PATCH v2] ptrace: fix ptrace vs tasklist_lock race Sebastian Andrzej Siewior

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).