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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
