Messages in this thread Patch in this message |  | | Date | Wed, 22 Oct 2014 23:30:41 +0200 | From | Oleg Nesterov <> | Subject | introduce task_rcu_dereference? |
| |
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;
|  |