sys_getppid oopses on debug kernel (v2)

PATCH New Oleg Nesterov 2 participants 5 messages Aug 9, 2006

Share & tools

Oleg Nesterov

author

2006-08-09 10:14 UTC #0 lore raw

Quoted 14 lines · Andrew Morton

Although I'm not sure it's needed for this problem. A getppid() which does
asmlinkage long sys_getppid(void)
{
	int pid;

	read_lock(&tasklist_lock);
	pid = current->group_leader->real_parent->tgid;
	read_unlock(&tasklist_lock);

	return pid;
}

Quoted 1 line

seems like a fine implementation to me ;)

Why do we need to use ->group_leader? All threads should have the same ->real_parent.

Why do we need tasklist_lock? I think rcu_read_lock() is enough.

In other words, do you see any problems with this code

	smlinkage long sys_getppid(void)
	{
		int pid;

		rcu_read_lock();
		pid = rcu_dereference(current->real_parent)->tgid;
		rcu_read_unlock();

		return pid;
	}

? Yes, we may read a stale value for ->real_parent, but the memory can't be freed while we are under rcu_read_lock(). And in this case the returned value is ok because the task could be reparented just after return anyway.

Oleg.

Kirill Korotaev

2006-08-09 12:06 UTC #1 lore raw

Quoted 10 lines

Quoted 10 lines
Although I'm not sure it's needed for this problem. A getppid() which does
asmlinkage long sys_getppid(void)
{
	int pid;

	read_lock(&tasklist_lock);
	pid = current->group_leader->real_parent->tgid;
	read_unlock(&tasklist_lock);

Quoted 8 lines

Quoted 4 lines
	return pid;
}
seems like a fine implementation to me ;)
Why do we need to use ->group_leader? All threads should have the same ->real_parent.

I'm not sure this is true for old LinuxThreads...

Quoted 15 lines

Why do we need tasklist_lock? I think rcu_read_lock() is enough.
In other words, do you see any problems with this code
	smlinkage long sys_getppid(void)
	{
		int pid;

		rcu_read_lock();
		pid = rcu_dereference(current->real_parent)->tgid;
		rcu_read_unlock();

		return pid;
	}

Quoted 4 lines

? Yes, we may read a stale value for ->real_parent, but the memory can't be freed while we are under rcu_read_lock(). And in this case the returned value is ok because the task could be reparented just after return anyway.

Your patch doesn't cure the problem.
rcu_read_lock just disables preemtion and rcu_dereference introduces memory barrier. _None_ of this _prevents_
another CPU from freeing old real_parent in parallel with your dereference.

You can minimize the probability very much by making local_irq_disable()/enable() around the code in question, but still it won't be a real fix (at least due to NMIs).

Kirill

Oleg Nesterov

author

2006-08-09 12:31 UTC #2 lore raw

Quoted 4 lines · Kirill Korotaev

Quoted 3 lines
Why do we need to use ->group_leader? All threads should have the same ->real_parent.
I'm not sure this is true for old LinuxThreads...

Yes, from the user-space pov it may be not true, but ->group_leader->real_parent should be equal ->real_parent anyway.

Quoted 15 lines

Quoted 15 lines
Why do we need tasklist_lock? I think rcu_read_lock() is enough.
In other words, do you see any problems with this code
	smlinkage long sys_getppid(void)
	{
		int pid;

		rcu_read_lock();
		pid = rcu_dereference(current->real_parent)->tgid;
		rcu_read_unlock();

		return pid;
	}

Quoted 8 lines

Quoted 4 lines
? Yes, we may read a stale value for ->real_parent, but the memory can't be freed while we are under rcu_read_lock(). And in this case the returned value is ok because the task could be reparented just after return anyway.
Your patch doesn't cure the problem.
rcu_read_lock just disables preemtion and rcu_dereference introduces memory barrier. _None_ of this _prevents_
another CPU from freeing old real_parent in parallel with your dereference.

How so? Note that release_task() doesn't call put_task_struct(), it does call_rcu(&p->rcu, delayed_put_task_struct) instead. When delayed_put_task_struct() is called, all CPUs must see the new value of ->real_parent (otherwise RCU is just broken). If CPU sees the old value of ->real_parent, rcu_read_lock() protects us from delayed_put_task_struct() on another CPU.

Ok, I think this is the same "classic" pattern as:

	old = global_ptr;
	global_ptr = new;
	call_rcu(..free_old...);
vs
	rcu_read_lock();
	use(global_ptr);
	rcu_read_unlock();

Do you agree?

Oleg.

Kirill Korotaev

2006-08-09 13:01 UTC #3 lore raw

Quoted 22 lines

Quoted 4 lines
Your patch doesn't cure the problem.
rcu_read_lock just disables preemtion and rcu_dereference introduces memory barrier. _None_ of this _prevents_
another CPU from freeing old real_parent in parallel with your dereference.
How so? Note that release_task() doesn't call put_task_struct(), it does call_rcu(&p->rcu, delayed_put_task_struct) instead. When delayed_put_task_struct() is called, all CPUs must see the new value of ->real_parent (otherwise RCU is just broken). If CPU sees the old value of ->real_parent, rcu_read_lock() protects us from delayed_put_task_struct() on another CPU.
Ok, I think this is the same "classic" pattern as:
	old = global_ptr;
	global_ptr = new;
	call_rcu(..free_old...);
vs
	rcu_read_lock();
	use(global_ptr);
	rcu_read_unlock();

Quoted 1 line

Do you agree?

Agree :)
Sorry, I didn't notice that task structs are freed now with RCU :))) In this case your patch really helps the problem. Care to prepare it?

Thanks,
Kirill

Oleg Nesterov

author

2006-08-09 14:00 UTC #4 lore raw

PATCH 1/1 sys_getppid-oopses-on-debug-kernel-v2-simplify

On top of Kirill's sys_getppid-oopses-on-debug-kernel-v2.patch

- We don't need ->group_leader->real_parent, all threads should have the same ->real_parent.

- We don't need tasklist_lock, task_struct is freed by RCU, so rcu_read_lock() should be enough.

(Compile tested)

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

kernel/timer.c renamed from kernel/timer.c~ +7 ~~-17~~

@@ -1324,28 +1324,18 @@ asmlinkage long sys_getpid(void)
 }
 
 /*
- * Accessing ->group_leader->real_parent is not SMP-safe, it could
- * change from under us. However, rather than getting any lock
- * we can use an optimistic algorithm: get the parent
- * pid, and go back and check that the parent is still
- * the same. If it has changed (which is extremely unlikely
- * indeed), we just try again..
- *
- * NOTE! This depends on the fact that even if we _do_
- * get an old value of "parent", we can happily dereference
- * the pointer (it was and remains a dereferencable kernel pointer
- * no matter what): we just can't necessarily trust the result
- * until we know that the parent pointer is valid.
- *
- * NOTE2: ->group_leader never changes from under us.
+ * Accessing ->real_parent is not SMP-safe, it could
+ * change from under us. However, we can use a stale
+ * value of ->real_parent under rcu_read_lock(), see
+ * release_task()->call_rcu(delayed_put_task_struct).
  */
 asmlinkage long sys_getppid(void)
 {
 	int pid;
 
-	read_lock(&tasklist_lock);
-	pid = current->group_leader->real_parent->tgid;
-	read_unlock(&tasklist_lock);
+	rcu_read_lock();
+	pid = rcu_dereference(current->real_parent)->tgid;
+	rcu_read_unlock();
 
 	return pid;
 }