sched: Revert delayed_put_task_struct() and fix use after free

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

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()                                  ...

include/linux/sched.h +2 -1

@@ -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

kernel/exit.c +4 -4

index 5d30019..326eae7 100644
@@ -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))

kernel/fork.c +0 -0

index 9b7d746..4d3ac3c 100644

Quoted 14 lines · Kirill Tkhai

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() ...

Quoted 20 lines · Oleg Nesterov

Quoted 14 lines · Kirill Tkhai

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.

Quoted 8 lines

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() ...

Yes, but perhaps in this particular case another simple fix makes more sense. The patch below needs a comment to explain that we check PF_EXITING because:

1. It doesn't make sense to migrate the exiting task. Although perhaps we could check ->mm == NULL instead.

But let me repeat that I do not understand this code, I am not sure we can equally treat is_idle_task() and PF_EXITING here...

2. If PF_EXITING is not set (or ->mm != NULL) then delayed_put_task_struct() won't be called until we drop rcu_read_lock(), and thus get_task_struct() is safe.

And. it seems that there is another problem? Can't task_h_load(cur) race with itself if 2 CPU's call task_numa_migrate() and inspect the same rq in parallel? Again, I don't understand this code, but update_cfs_rq_h_load() doesn't look "atomic". In fact I am not even sure about task_h_load(env->p), p == current but we do not disable preemption.

What do you think?

Yeah, you're sure about initial patch. Thanks for signal explanation.

Quoted 37 lines · Oleg Nesterov

Quoted 20 lines · Oleg Nesterov

Quoted 14 lines · Kirill Tkhai

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.

Quoted 8 lines

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() ...

Yes, but perhaps in this particular case another simple fix makes more sense. The patch below needs a comment to explain that we check PF_EXITING because:

1. It doesn't make sense to migrate the exiting task. Although perhaps we could check ->mm == NULL instead.

Quoted 7 lines

But let me repeat that I do not understand this code, I am not sure we can equally treat is_idle_task() and PF_EXITING here...

2. If PF_EXITING is not set (or ->mm != NULL) then delayed_put_task_struct() won't be called until we drop rcu_read_lock(), and thus get_task_struct() is safe.

Cool! Elegant fix. We set PF_EXITING in exit_signals(), which is earlier than release_task() is called.

Shouldn't we use smp_rmb/smp_wmb here?

Quoted 7 lines

And. it seems that there is another problem? Can't task_h_load(cur) race with itself if 2 CPU's call task_numa_migrate() and inspect the same rq in parallel? Again, I don't understand this code, but update_cfs_rq_h_load() doesn't look "atomic". In fact I am not even sure about task_h_load(env->p), p == current but we do not disable preemption.

What do you think?

We use it completely unlocked, so nothing good is here. Also we work with pointers.

As I understand in update_cfs_rq_h_load() we go from bottom to top, and then from top to bottom. We set cfs_rq::h_load_next to be able to do top-bottom passage (top is a root of "tree").

Yeah, this "way" may be overwritten by competitor. Also, task may change its cfs_rq.

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 13 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 
 	/*

Looks like, we have to use the same fix for task_numa_group().

grp = rcu_dereference(tsk->numa_group);

Below we dereference grp->nr_tasks.

Also, the same in rt.c and deadline.c, but we do no take second reference there. Wrong pointer dereference is not possible there, not so bad.

Kirill

Quoted 10 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 2 lines

kernel/sched/fair.c +0 -0

 	/*

Looks like, we have to use the same fix for task_numa_group().

Quoted 10 lines

Quoted 10 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 

Quoted 4 lines

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 2 lines

kernel/sched/fair.c +0 -0

 	/*

Looks like, we have to use the same fix for task_numa_group().

Don't think so, task_numa_group() is only called from task_numa_fault() which is on 'current' and neither idle and PF_EXITING should be faulting.

В Чт, 16/10/2014 в 09:59 +0200, Peter Zijlstra пишет:

Quoted 16 lines · Kirill Tkhai

Quoted 14 lines

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 12 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 
 	/*

Looks like, we have to use the same fix for task_numa_group().

Quoted 3 lines

Don't think so, task_numa_group() is only called from task_numa_fault() which is on 'current' and neither idle and PF_EXITING should be faulting.

Isn't task_numa_group() fully preemptible?

It seems cpu_rq(cpu)->curr is not always equal to p.

Quoted 17 lines

В Чт, 16/10/2014 в 09:59 +0200, Peter Zijlstra пишет:

Quoted 16 lines · Kirill Tkhai

Quoted 14 lines

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 12 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 
 	/*

Looks like, we have to use the same fix for task_numa_group().

Quoted 5 lines

Quoted 3 lines

Don't think so, task_numa_group() is only called from task_numa_fault() which is on 'current' and neither idle and PF_EXITING should be faulting.

Isn't task_numa_group() fully preemptible?

Not seeing how that is relevant.

Quoted 1 line

It seems cpu_rq(cpu)->curr is not always equal to p.

It should be afaict:

task_numa_fault()
p = current;

  task_numa_group(p, ..);

And like said, idle tasks and PF_EXITING task should never get (numa) faults for they should never be touching userspace.

В Чт, 16/10/2014 в 11:43 +0200, Peter Zijlstra пишет:

Quoted 35 lines · Kirill Tkhai

Quoted 17 lines

В Чт, 16/10/2014 в 09:59 +0200, Peter Zijlstra пишет:

Quoted 16 lines · Kirill Tkhai

Quoted 14 lines

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 12 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 
 	/*

Looks like, we have to use the same fix for task_numa_group().

Quoted 5 lines

Quoted 3 lines

Don't think so, task_numa_group() is only called from task_numa_fault() which is on 'current' and neither idle and PF_EXITING should be faulting.

Isn't task_numa_group() fully preemptible?

Not seeing how that is relevant.

Quoted 1 line

It seems cpu_rq(cpu)->curr is not always equal to p.

It should be afaict:

task_numa_fault()
p = current;

  task_numa_group(p, ..);

Quoted 2 lines

And like said, idle tasks and PF_EXITING task should never get (numa) faults for they should never be touching userspace.

I mean p can be moved to other cpu.

tsk = ACCESS_ONCE(cpu_rq(cpu)->curr);

tsk is not p, (i.e current) here.

В Чт, 16/10/2014 в 13:50 +0400, Kirill Tkhai пишет:

Quoted 39 lines

В Чт, 16/10/2014 в 11:43 +0200, Peter Zijlstra пишет:

Quoted 37 lines · Kirill Tkhai

Quoted 17 lines

В Чт, 16/10/2014 в 09:59 +0200, Peter Zijlstra пишет:

Quoted 16 lines · Kirill Tkhai

Quoted 14 lines

kernel/sched/fair.c static void task_numa_compare(struct tas · line 1171

Quoted 12 lines

kernel/sched/fair.c +1 -1

@@ -1165,7 +1165,7 @@ static void task_numa_compare(struct tas
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	if (is_idle_task(cur) || (curr->flags & PF_EXITING))
 		cur = NULL;
 
 	/*

Looks like, we have to use the same fix for task_numa_group().

Quoted 5 lines

Quoted 3 lines

Don't think so, task_numa_group() is only called from task_numa_fault() which is on 'current' and neither idle and PF_EXITING should be faulting.

Isn't task_numa_group() fully preemptible?

Not seeing how that is relevant.

Quoted 1 line

It seems cpu_rq(cpu)->curr is not always equal to p.

It should be afaict:

task_numa_fault()
p = current;

  task_numa_group(p, ..);

And like said, idle tasks and PF_EXITING task should never get (numa) faults for they should never be touching userspace.

Quoted 5 lines

I mean p can be moved to other cpu.

tsk = ACCESS_ONCE(cpu_rq(cpu)->curr);

tsk is not p, (i.e current) here.

Maybe I undestand wrong and preemption is disabled in memory fault?

Cool! Elegant fix. We set PF_EXITING in exit_signals(), which is earlier than release_task() is called.

Shouldn't we use smp_rmb/smp_wmb here?

What do you think?

Oleg.

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() ...

Quoted 3 lines · Oleg Nesterov

At least the code like

Quoted 6 lines

	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() ...

There is an rcu_read_lock() around it through task_numa_compare().

The lockless get_task_struct(tsk) is only safe if tsk == current and didn't pass exit_notify(), or if this tsk was found on a rcu protected list (say, for_each_process() or find_task_by_vpid()). IOW, it is only safe if release_task() was not called before we take rcu_read_lock(), in this case we can rely on the fact that delayed_put_pid() can not drop the (potentially) last reference until rcu_read_unlock().

And as Kirill pointed out task_numa_compare()->task_numa_assign() path does get_task_struct(dst_rq->curr) and this is not safe. The task_struct itself can't go away, but rcu_read_lock() can't save us from the final put_task_struct() in finish_task_switch(); this reference goes away without rcu gp.

Reported-by Kirill Tkhai <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/sched/fair.c |    8 +++++++-

 1 files changed, 7 insertions(+), 1 deletions(-)

kernel/sched/fair.c +0 -1

index 0090e8c..52049b9 100644
@@ -1158,7 +1158,13 @@ static void task_numa_compare(struct task_numa_env *env,
 
          rcu_read_lock();
          cur = ACCESS_ONCE(dst_rq->curr);
- if (cur->pid == 0) /* idle */

18.10.2014, 01:40, "Oleg Nesterov" <[email protected]>:

Quoted 38 lines

 The lockless get_task_struct(tsk) is only safe if tsk == current and didn't pass exit_notify(), or if this tsk was found on a rcu protected list (say, for_each_process() or find_task_by_vpid()). IOW, it is only safe if release_task() was not called before we take rcu_read_lock(), in this case we can rely on the fact that delayed_put_pid() can not drop the (potentially) last reference until rcu_read_unlock().

 And as Kirill pointed out task_numa_compare()->task_numa_assign() path does get_task_struct(dst_rq->curr) and this is not safe. The task_struct itself can't go away, but rcu_read_lock() can't save us from the final put_task_struct() in finish_task_switch(); this reference goes away without rcu gp.

Reported-by Kirill Tkhai <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

 ---
  kernel/sched/fair.c |    8 +++++++-

  1 files changed, 7 insertions(+), 1 deletions(-)

 diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
 index 0090e8c..52049b9 100644
 --- a/kernel/sched/fair.c
 +++ b/kernel/sched/fair.c
 @@ -1158,7 +1158,13 @@ static void task_numa_compare(struct task_numa_env *env,

          rcu_read_lock();
          cur = ACCESS_ONCE(dst_rq->curr);
 - if (cur->pid == 0) /* idle */
 + /*
 + * No need to move the exiting task, and this ensures that ->curr
 + * wasn't reaped and thus get_task_struct() in task_numa_assign()
 + * is safe; note that rcu_read_lock() can't protect from the final
 + * put_task_struct() after the last schedule().
 + */
 + if (is_idle_task(cur) || (cur->flags & PF_EXITING)) cur = NULL;

Quoted 1 line

          /*

Oleg, I've looked once again, and now it's not good for me. Where is the guarantee this memory hasn't been allocated again? If so, PF_EXITING is not of the task we are interesting, but it's not a task's even.

rcu_read_lock()                   ...                           ... cur = ACCESS_ONCE(dst_rq->curr);  ...                           ... <interrupt>                       rq->curr = next;              ... <interrupt>                           put_prev_task()           ... <interrupt>                               __put_prev_task       ... <interrupt>                                  kmem_cache_free()  ... <interrupt>                                  ...                <alocated again> <interrupt>                                  ...                memset(, 0, ) <interrupt>                                  ...                ... if (cur->flags & PF_EXITING)                 ...                ... <no>                                     ...                ... get_task_struct()                            ...                ...

How about this?

kernel/sched/fair.c +8 -1

index b78280c..d46427e 100644
@@ -1165,7 +1165,21 @@ static void task_numa_compare(struct task_numa_env *env,
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	/*
+	 * No need to move the exiting task, and this ensures that ->curr
+	 * wasn't reaped and thus get_task_struct() in task_numa_assign()
+	 * is safe; note that rcu_read_lock() can't protect from the final
+	 * put_task_struct() after the last schedule().
+	 */
+	if (is_idle_task(cur) || (cur->flags & PF_EXITING))
+		cur = NULL;

So you worry about the refcount doing 0->1 ? In which case the above is still wrong and we should be using atomic_inc_not_zero() in order to acquire the reference count.

Quoted 28 lines · Kirill Tkhai

How about this?

kernel/sched/fair.c +15 -1

index b78280c..d46427e 100644
@@ -1165,7 +1165,21 @@ static void task_numa_compare(struct task_numa_env *env,
 
 	rcu_read_lock();
 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	/*
+	 * No need to move the exiting task, and this ensures that ->curr
+	 * wasn't reaped and thus get_task_struct() in task_numa_assign()
+	 * is safe; note that rcu_read_lock() can't protect from the final
+	 * put_task_struct() after the last schedule().
+	 */
+	if (is_idle_task(cur) || (cur->flags & PF_EXITING))
+		cur = NULL;
+	/*
+	 * Check once again to be sure curr is still on dst_rq. Even if
+	 * it points on a new task, which is using the memory of freed
+	 * cur, it's OK, because we've locked RCU before
+	 * delayed_put_task_struct() callback is called to put its struct.
+	 */
+	if (cur != ACCESS_ONCE(dst_rq->curr))
 		cur = NULL;
 

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1185

Quoted 1 line

kernel/sched/fair.c +0 -0

 	/*

So you worry about the refcount doing 0->1 ? In which case the above is still wrong and we should be using atomic_inc_not_zero() in order to acquire the reference count.

18.10.2014, 01:40, "Oleg Nesterov" <[email protected]>:

Quoted 20 lines

...
The
task_struct itself can't go away,
...

kernel/sched/fair.c +7 -1

@@ -1158,7 +1158,13 @@ static void task_numa_compare(struct task_numa_env *env,
 
          rcu_read_lock();
          cur = ACCESS_ONCE(dst_rq->curr);
- if (cur->pid == 0) /* idle */
+ /*
+ * No need to move the exiting task, and this ensures that ->curr
+ * wasn't reaped and thus get_task_struct() in task_numa_assign()
+ * is safe; note that rcu_read_lock() can't protect from the final
+ * put_task_struct() after the last schedule().
+ */
+ if (is_idle_task(cur) || (cur->flags & PF_EXITING))
                  cur = NULL;
 

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1170

Quoted 1 line

kernel/sched/fair.c +0 -0

          /*

Oleg, I've looked once again, and now it's not good for me.

kernel/sched/fair.c +0 -0

@@ -1165,7 +1165,21 @@ static void task_numa_compare(struct task_numa_env *env,

^^

      rcu_read_lock();
      cur = ACCESS_ONCE(dst_rq->curr);

kernel/sched/fair.c +5 -1

-     if (cur->pid == 0) /* idle */
+     /*
+      * No need to move the exiting task, and this ensures that ->curr
+      * wasn't reaped and thus get_task_struct() in task_numa_assign()
+      * is safe; note that rcu_read_lock() can't protect from the final
+      * put_task_struct() after the last schedule().

Quoted 21 lines

  18.10.2014, 01:40, "Oleg Nesterov" <[email protected]>:

Quoted 20 lines

  ...
  The
  task_struct itself can't go away,
  ...
  --- a/kernel/sched/fair.c
  +++ b/kernel/sched/fair.c
  @@ -1158,7 +1158,13 @@ static void task_numa_compare(struct task_numa_env *env,

           rcu_read_lock();
           cur = ACCESS_ONCE(dst_rq->curr);
  - if (cur->pid == 0) /* idle */
  + /*
  + * No need to move the exiting task, and this ensures that ->curr
  + * wasn't reaped and thus get_task_struct() in task_numa_assign()
  + * is safe; note that rcu_read_lock() can't protect from the final
  + * put_task_struct() after the last schedule().
  + */
  + if (is_idle_task(cur) || (cur->flags & PF_EXITING)) cur = NULL;

Quoted 2 lines

Quoted 1 line

           /*

  Oleg, I've looked once again, and now it's not good for me.

 Ah. Thanks a lot Kirill for correcting me!

 I was looking at this rcu_read_lock() and I didn't even try to think what it can actually protect. Nothing.

 No, I don't think this can work. Let's look at the current code:

         rcu_read_lock();
         cur = ACCESS_ONCE(dst_rq->curr);
         if (cur->pid == 0) /* idle */

 And any dereference, even reading ->pid is not safe. This memory can be freed, unmapped, reused, etc.

 Looks like, task_numa_compare() needs to take dst_rq->lock and get the refernce first.

 Or, perhaps, we need to change the rules to ensure that any "task_struct *" pointer is rcu-safe. Perhaps we have more similar problems... I'd like to avoid this if possible.

 Hmm. I'll try to think more.

 Thanks!

19.10.2014, 00:59, "Oleg Nesterov" <[email protected]>:

Quoted 9 lines

 No, I don't think this can work. Let's look at the current code:

         rcu_read_lock();
         cur = ACCESS_ONCE(dst_rq->curr);
         if (cur->pid == 0) /* idle */

 And any dereference, even reading ->pid is not safe. This memory can be freed, unmapped, reused, etc.

Quoted 2 lines

 Looks like, task_numa_compare() needs to take dst_rq->lock and get the refernce first.

Yeah, detection of idle is not save. If we reorder the checks almost all problems will be gone. All except unmapping. JFI, is it possible with such kernel structures as task_struct?

kernel/sched/fair.c +0 -0

@@ -1165,7 +1165,30 @@ static void task_numa_compare(struct task_numa_env *env,
 
 	rcu_read_lock();

Quoted 3 lines

 Or, perhaps, we need to change the rules to ensure that any "task_struct *" pointer is rcu-safe. Perhaps we have more similar problems... I'd like to avoid this if possible.

RT tree has:

https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt-patches.git/
tree/patches/sched-delay-put-task.patch

Quoted 6 lines · Kirill Tkhai

kernel/sched/fair.c +0 -0

@@ -1165,7 +1165,30 @@ static void task_numa_compare(struct task_numa_env *env,
 
 	rcu_read_lock();

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1175

Quoted 10 lines

kernel/sched/fair.c +8 -1

 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	/*
+	 * No need to move the exiting task, and this ensures that ->curr
+	 * wasn't reaped and thus get_task_struct() in task_numa_assign()
+	 * is safe; note that rcu_read_lock() can't protect from the final
+	 * put_task_struct() after the last schedule().
+	 */
+	if (cur->flags & PF_EXITING)
+		cur = NULL;

so this needs probe_kernel_read(&cur->flags).

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1189

Quoted 2 lines

kernel/sched/fair.c +2 -0

+	if (cur != ACCESS_ONCE(dst_rq->curr))
+		cur = NULL;

Yes, if this task_struct was freed in between we do not care if this memory was reused (except PF_EXITING can be false positive). If it was freed and now the same memory is ->curr again we know that delayed_put_task_struct() can't be called until we drop rcu lock, even if PF_EXITING is already set again.

I won't argue, but you need to convince Peter to accept this hack ;)

Quoted 4 lines

Quoted 3 lines

 Or, perhaps, we need to change the rules to ensure that any "task_struct *" pointer is rcu-safe. Perhaps we have more similar problems... I'd like to avoid this if possible.

Quoted 4 lines

RT tree has:

https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt-patches.git/
tree/patches/sched-delay-put-task.patch

Yes, and this obviously implies more rcu callbacks in flight, and another gp before __put_task_struct(). but may be we will need to do this anyway...

Forgot to mention... Or we can make task_struct_cachep SLAB_DESTROY_BY_RCU, in this case ->curr (or any other "task_struct *" ponter) can not go away under rcu_read_lock(). task_numa_compare() still needs the PF_EXITING check, but we do not need to recheck ->curr or probe_kernel_read().

Quoted 5 lines

Forgot to mention... Or we can make task_struct_cachep SLAB_DESTROY_BY_RCU, in this case ->curr (or any other "task_struct *" ponter) can not go away under rcu_read_lock(). task_numa_compare() still needs the PF_EXITING check, but we do not need to recheck ->curr or probe_kernel_read().

Damn, please ignore ;) we still need to recheck ->curr.

Quoted 4 lines

Quoted 4 lines

RT tree has:

https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt-patches.git/
tree/patches/sched-delay-put-task.patch

Quoted 2 lines

Yes, and this obviously implies more rcu callbacks in flight, and another gp before __put_task_struct(). but may be we will need to do this anyway...

Forgot to mention... Or we can make task_struct_cachep SLAB_DESTROY_BY_RCU, in this case ->curr (or any other "task_struct *" ponter) can not go away under rcu_read_lock(). task_numa_compare() still needs the PF_EXITING check, but we do not need to recheck ->curr or probe_kernel_read().

OK, I think I'm finally awake enough to see what you're all talking about :-)

Quoted 2 lines · Oleg Nesterov

Quoted 2 lines

Quoted 2 lines

RT tree has:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt-patches.git/
tree/patches/sched-delay-put-task.patch

(answering the other email asking about this)

RT does this because we call put_task_struct() with preempt disabled and on RT the memory allocator has sleeping locks.

Quoted 7 lines

Quoted 2 lines

Yes, and this obviously implies more rcu callbacks in flight, and another gp before __put_task_struct(). but may be we will need to do this anyway...

Forgot to mention... Or we can make task_struct_cachep SLAB_DESTROY_BY_RCU, in this case ->curr (or any other "task_struct *" ponter) can not go away under rcu_read_lock(). task_numa_compare() still needs the PF_EXITING check, but we do not need to recheck ->curr or probe_kernel_read().

I think I would prefer SLAB_DESTROY_BY_RCU for this, because as you pointed out, I'm not sure mainline would like the extra callbacks.

Quoted 15 lines

19.10.2014, 00:59, "Oleg Nesterov" <[email protected]>:

Quoted 11 lines

No, I don't think this can work. Let's look at the current code:

         rcu_read_lock();
         cur = ACCESS_ONCE(dst_rq->curr);
         if (cur->pid == 0) /* idle */

And any dereference, even reading ->pid is not safe. This memory can be freed, unmapped, reused, etc.

Looks like, task_numa_compare() needs to take dst_rq->lock and get the refernce first.

Quoted 3 lines

Yeah, detection of idle is not save. If we reorder the checks almost all problems will be gone. All except unmapping. JFI, is it possible with such kernel structures as task_struct?

Yes, if DEBUG_PAGEALLOC. See kernel_map_pages() in arch/x86/mm/pageattr.c kernel_map_pages(enable => false) clears PAGE_PRESENT if slab returns the pages to system.

Quoted 6 lines

kernel/sched/fair.c +0 -0

@@ -1165,7 +1165,30 @@ static void task_numa_compare(struct task_numa_env *env,
 
 	rcu_read_lock();

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1175

Quoted 10 lines

kernel/sched/fair.c +8 -1

 	cur = ACCESS_ONCE(dst_rq->curr);
-	if (cur->pid == 0) /* idle */
+	/*
+	 * No need to move the exiting task, and this ensures that ->curr
+	 * wasn't reaped and thus get_task_struct() in task_numa_assign()
+	 * is safe; note that rcu_read_lock() can't protect from the final
+	 * put_task_struct() after the last schedule().
+	 */
+	if (cur->flags & PF_EXITING)
+		cur = NULL;

so this needs probe_kernel_read(&cur->flags).

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1189

Quoted 2 lines

kernel/sched/fair.c +2 -0

+	if (cur != ACCESS_ONCE(dst_rq->curr))
+		cur = NULL;

Yes, if this task_struct was freed in between we do not care if this memory was reused (except PF_EXITING can be false positive). If it was freed and now the same memory is ->curr again we know that delayed_put_task_struct() can't be called until we drop rcu lock, even if PF_EXITING is already set again.

I won't argue, but you need to convince Peter to accept this hack ;)

Quoted 5 lines

Quoted 4 lines

Or, perhaps, we need to change the rules to ensure that any "task_struct *" pointer is rcu-safe. Perhaps we have more similar problems... I'd like to avoid this if possible.

Quoted 4 lines

RT tree has:

https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt-patches.git/
tree/patches/sched-delay-put-task.patch

Yes, and this obviously implies more rcu callbacks in flight, and another gp before __put_task_struct(). but may be we will need to do this anyway...

I'm too tired for all this, but:

kernel/sched/fair.c static void task_numa_compare(struct task_numa_env *env, · line 1176

Quoted 1 line

kernel/sched/fair.c +1 -0

+	smp_rmb(); /* Pairs with dst_rq->lock unlocking which implies smp_wmb */

RELEASE does not imply a WMB.