Re: [PATCH RFC] sched: Revert delayed_put_task_struct() and fix use after free

[Oleg Nesterov <oleg@redhat.com>]

On 10/15, Kirill Tkhai wrote:
>
> This WARN_ON_ONCE() placed into __schedule() triggers warning:
>
> @@ -2852,6 +2852,7 @@ static void __sched __schedule(void)
>
>  	if (likely(prev != next)) {
>  		rq->nr_switches++;
> +		WARN_ON_ONCE(atomic_read(&prev->usage) == 1);

I think you know this, but let me clarify just in case that this WARN()
is wrong, prev->usage == 1 is fine if the task does its last schedule()
and it was already (auto)reaped.

> This means the final put_task_struct() happens against RCU rules.

Well, yes, it doesn't use delayed_put_pid(). But this should be fine,
this drops the extra reference created by dup_task_struct().

However,

> Regarding to scheduler this may be a reason of use-after-free.
>
>     task_numa_compare()                    schedule()
>         rcu_read_lock()                        ...
>         cur = ACCESS_ONCE(dst_rq->curr)        ...
>             ...                                rq->curr = next;
>             ...                                    context_switch()
>             ...                                        finish_task_switch()
>             ...                                            put_task_struct()
>             ...                                                __put_task_struct()
>             ...                                                    free_task_struct()
>             task_numa_assign()                                     ...
>                 get_task_struct()                                  ...

Agreed. I don't understand this code (will try to take another look later),
but at first glance this looks wrong.

At least the code like

	rcu_read_lock();
	get_task_struct(foreign_rq->curr);
	rcu_read_unlock();

is certainly wrong. And _probably_ the problem should be fixed here. Perhaps
we can add try_to_get_task_struct() which does atomic_inc_not_zero() ...

> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1854,11 +1854,12 @@ extern void free_task(struct task_struct *tsk);
>  #define get_task_struct(tsk) do { atomic_inc(&(tsk)->usage); } while(0)
>
>  extern void __put_task_struct(struct task_struct *t);
> +extern void __put_task_struct_cb(struct rcu_head *rhp);
>
>  static inline void put_task_struct(struct task_struct *t)
>  {
>  	if (atomic_dec_and_test(&t->usage))
> -		__put_task_struct(t);
> +		call_rcu(&t->rcu, __put_task_struct_cb);
>  }
>
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 5d30019..326eae7 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -159,15 +159,15 @@ static void __exit_signal(struct task_struct *tsk)
>  	}
>  }
>
> -static void delayed_put_task_struct(struct rcu_head *rhp)
> +void __put_task_struct_cb(struct rcu_head *rhp)
>  {
>  	struct task_struct *tsk = container_of(rhp, struct task_struct, rcu);
>
>  	perf_event_delayed_put(tsk);
>  	trace_sched_process_free(tsk);
> -	put_task_struct(tsk);
> +	__put_task_struct(tsk);
>  }
> -
> +EXPORT_SYMBOL_GPL(__put_task_struct_cb);
>
>  void release_task(struct task_struct *p)
>  {
> @@ -207,7 +207,7 @@ void release_task(struct task_struct *p)
>
>  	write_unlock_irq(&tasklist_lock);
>  	release_thread(p);
> -	call_rcu(&p->rcu, delayed_put_task_struct);
> +	put_task_struct(p);
>
>  	p = leader;
>  	if (unlikely(zap_leader))
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 9b7d746..4d3ac3c 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -249,7 +249,6 @@ void __put_task_struct(struct task_struct *tsk)
>  	if (!profile_handoff_task(tsk))
>  		free_task(tsk);
>  }
> -EXPORT_SYMBOL_GPL(__put_task_struct);
>
>  void __init __weak arch_task_cache_init(void) { }

Hmm. I am not sure I understand how this patch can actually fix this problem.
It seems that it is still possible that get_task_struct() can be called after
call_rcu(__put_task_struct_cb) ? But perhaps I misread this patch.

And I think it adds another problem. Suppose we have a zombie which already
called schedule() in TASK_DEAD state. IOW, its ->usage == 1, its parent will
free this task when it calls sys_wait().

With this patch the code like

	rcu_read_lock();
	for_each_process(p) {
		if (pred(p) {
			get_task_struct(p);
			return p;
		}
	}
	rcu_read_unlock();

becomes unsafe: we can race with release_task(p) and get_task_struct() can
can be called when prev->usage is already 0 and this task_struct can be freed
omce you drop rcu_read_lock().

Oleg.