Fix races in ptrace

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.

index a232bb5..79b5f5d 100644
@@ -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);

index 5051ca6..a9cb295 100644
@@ -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();
 		}
 

Hi Alexander,

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

Sebastian

12.08.2013, 20:41, "Sebastian Andrzej Siewior" <[email protected]>:

Hi Sebastian,

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Alexander,

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?

@@ -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

21.08.2013, 21:24, "Sebastian Andrzej Siewior" <[email protected]>:

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

I'm tempted to add this:

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

index e0a05de..bd60b9d 100644
@@ -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

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

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

29.08.2013, 22:28, "Sebastian Andrzej Siewior" <[email protected]>:

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

As explained by Alexander Fyodorov <[email protected]>:

|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…

---
 include/linux/sched.h |   48 +++++++++++++++++++++++++++++++++++++++++++++---
 kernel/ptrace.c       |    7 ++++++-
 kernel/sched/core.c   |   24 +++++++++++++++++++++---
 3 files changed, 72 insertions(+), 7 deletions(-)

@@ -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

@@ -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);

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html