Re: introduce task_rcu_dereference?

[Kirill Tkhai <ktkhai@parallels.com>]

В Ср, 22/10/2014 в 23:30 +0200, 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;
> 

I'm agree generic helper is better. But probe_slab_address() has a sence
if we know that SDBR is worse in our subject area. Less of code is
easier to support :) probe_slab_address() it's not a trivial logic.
Also, if we use mm primitives this increases kernel modularity.

Kirill