introduce task_rcu_dereference?

[Oleg Nesterov]

On 10/22, Kirill Tkhai wrote:
>
> Unlocked access to dst_rq->curr in task_numa_compare() is racy.
> If curr task is exiting this may be a reason of use-after-free:

Thanks.

And as you pointed out, there are other examples of unlocked
foreign_rq->curr usage.

So, Kirill, Peter, do you think that the patch below can help? Can
we change task_numa_group() and ->select_task_rq() to do nothing if
rq_curr_rcu_safe() returns NULL? It seems we can...

task_numa_compare() can use it too, we can make another patch on
top of this one.

	- Obviously just for the early review. Lacks the changelog
	  and the comments (at least).

	- Once again, I won't insist on probe_slab_address(). We can
	  add SDBR and change task_rcu_dereference() to simply read
	  ->sighand.

	- Also, I won't argue if you think that we do not need a
	  generic helper. In this case we can move this logic into
	  rq_curr_rcu_safe() and it will be a bit simpler.

	- OTOH, I am not sure we need rq_curr_rcu_safe(). The callers
	  can just use task_rcu_dereference() and check IS_ERR_OR_NULL,
	  I guess retry doesn't buy too much in this case.

Or do you think we need something else?

Oleg.

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 857ba40..0ba420e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2300,6 +2300,7 @@ extern void block_all_signals(int (*notifier)(void *priv), void *priv,
 			      sigset_t *mask);
 extern void unblock_all_signals(void);
 extern void release_task(struct task_struct * p);
+extern struct task_struct *task_rcu_dereference(struct task_struct **ptask);
 extern int send_sig_info(int, struct siginfo *, struct task_struct *);
 extern int force_sigsegv(int, struct task_struct *);
 extern int force_sig_info(int, struct siginfo *, struct task_struct *);
diff --git a/kernel/exit.c b/kernel/exit.c
index 32c58f7..4aa00c7 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -213,6 +213,37 @@ repeat:
 		goto repeat;
 }
 
+struct task_struct *task_rcu_dereference(struct task_struct **ptask)
+{
+	struct task_struct *task;
+	struct sighand_struct *sighand;
+
+	task = rcu_dereference(*ptask);
+	if (!task)
+		return NULL;
+
+	/* If it fails the check below must fail too */
+	probe_slab_address(&task->sighand, sighand);
+	/*
+	 * Pairs with atomic_dec_and_test() in put_task_struct(task).
+	 * If we have read the freed/reused memory, we must see that
+	 * the pointer was updated. The caller might want to retry in
+	 * this case.
+	 */
+	smp_rmb();
+	if (unlikely(task != ACCESS_ONCE(*ptask)))
+		return ERR_PTR(-EAGAIN);
+
+	/*
+	 * release_task(task) was already called; potentially before
+	 * the caller took rcu_read_lock() and in this case it can be
+	 * freed before rcu_read_unlock().
+	 */
+	if (!sighand)
+		return ERR_PTR(-EINVAL);
+	return task;
+}
+
 /*
  * This checks not only the pgrp, but falls back on the pid if no
  * satisfactory pgrp is found. I dunno - gdb doesn't work correctly
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 579712f..249c0c1 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -655,6 +655,18 @@ DECLARE_PER_CPU(struct rq, runqueues);
 #define cpu_curr(cpu)		(cpu_rq(cpu)->curr)
 #define raw_rq()		(&__raw_get_cpu_var(runqueues))
 
+static inline struct task_struct *rq_curr_rcu_safe(struct rq *rq)
+{
+	for (;;) {
+		struct task_struct *curr = task_rcu_dereference(&rq->curr);
+		/* NULL is not possible */
+		if (likely(!IS_ERR(curr)))
+			return curr;
+		if (PTR_ERR(curr) != -EAGAIN)
+			return NULL;
+	}
+}
+
 static inline u64 rq_clock(struct rq *rq)
 {
 	return rq->clock;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/