[PATCH] rework/fix is_single_threaded()

[PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

- Fix the comment, is_single_threaded(p) actually means that nobody shares
  ->mm with p.

  I think this helper should be renamed, and it should not have arguments.
  With or without this patch it must not be used unless p == current,
  otherwise we can't safely use p->signal or p->mm.

- "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
  zombie group leader, use signal->live instead.

- Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
  otherwise we can return the wrong "false".

- Use for_each_process() instead of do_each_thread(), all threads must use
  the same ->mm.

- Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
  to iterate over the process list. If there is another CLONE_VM process
  it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
  a freshly forked CLONE_VM task, but this doesn't matter because we must
  see its parent and return false.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.30/lib/is_single_threaded.c~ISS	2009-04-06 00:03:42.000000000 +0200
+++ 6.30/lib/is_single_threaded.c	2009-04-13 23:39:56.000000000 +0200
@@ -12,34 +12,44 @@
 
 #include <linux/sched.h>
 
-/**
- * is_single_threaded - Determine if a thread group is single-threaded or not
- * @p: A task in the thread group in question
- *
- * This returns true if the thread group to which a task belongs is single
- * threaded, false if it is not.
+/*
+ * Returns true if the task does not share ->mm with another thread/process.
  */
-bool is_single_threaded(struct task_struct *p)
+bool is_single_threaded(struct task_struct *task)
 {
-	struct task_struct *g, *t;
-	struct mm_struct *mm = p->mm;
+	struct mm_struct *mm = task->mm;
+	struct task_struct *p, *t;
+	bool ret;
 
-	if (atomic_read(&p->signal->count) != 1)
-		goto no;
+	might_sleep();
 
-	if (atomic_read(&p->mm->mm_users) != 1) {
-		read_lock(&tasklist_lock);
-		do_each_thread(g, t) {
-			if (t->mm == mm && t != p)
-				goto no_unlock;
-		} while_each_thread(g, t);
-		read_unlock(&tasklist_lock);
-	}
+	if (atomic_read(&task->signal->live) != 1)
+		return false;
 
-	return true;
+	if (atomic_read(&mm->mm_users) == 1)
+		return true;
 
-no_unlock:
-	read_unlock(&tasklist_lock);
-no:
-	return false;
+	ret = false;
+	down_write(&mm->mmap_sem);
+	rcu_read_lock();
+	for_each_process(p) {
+		if (unlikely(p->flags & PF_KTHREAD))
+			continue;
+		if (unlikely(p == task->group_leader))
+			continue;
+
+		t = p;
+		do {
+			if (unlikely(t->mm == mm))
+				goto found;
+			if (likely(t->mm))
+				break;
+		} while_each_thread(p, t);
+	}
+	ret = true;
+found:
+	rcu_read_unlock();
+	up_write(&mm->mmap_sem);
+
+	return ret;
 }

Re: [PATCH] rework/fix is_single_threaded()

[Andrew Morton]

On Mon, 13 Apr 2009 23:45:13 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> - Fix the comment, is_single_threaded(p) actually means that nobody shares
>   ->mm with p.
> 
>   I think this helper should be renamed, and it should not have arguments.
>   With or without this patch it must not be used unless p == current,
>   otherwise we can't safely use p->signal or p->mm.
> 
> - "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
>   zombie group leader, use signal->live instead.
> 
> - Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
>   otherwise we can return the wrong "false".
> 
> - Use for_each_process() instead of do_each_thread(), all threads must use
>   the same ->mm.
> 
> - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
>   to iterate over the process list. If there is another CLONE_VM process
>   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
>   a freshly forked CLONE_VM task, but this doesn't matter because we must
>   see its parent and return false.

hum.  I'm searching for a reason to merge this into 2.6.30 but I'm not
seeing one?

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 04/15, Andrew Morton wrote:
>
> On Mon, 13 Apr 2009 23:45:13 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > - Fix the comment, is_single_threaded(p) actually means that nobody shares
> >   ->mm with p.
> >
> >   I think this helper should be renamed, and it should not have arguments.
> >   With or without this patch it must not be used unless p == current,
> >   otherwise we can't safely use p->signal or p->mm.
> >
> > - "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
> >   zombie group leader, use signal->live instead.
> >
> > - Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
> >   otherwise we can return the wrong "false".
> >
> > - Use for_each_process() instead of do_each_thread(), all threads must use
> >   the same ->mm.
> >
> > - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
> >   to iterate over the process list. If there is another CLONE_VM process
> >   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
> >   a freshly forked CLONE_VM task, but this doesn't matter because we must
> >   see its parent and return false.
>
> hum.  I'm searching for a reason to merge this into 2.6.30 but I'm not
> seeing one?

No reasons to merge.

Also, David seem to have valid concerns about using ->mmap_sem. Most probably
this patch should be changed anyway. Let's see what David thinks.

Oleg.

Re: [PATCH] rework/fix is_single_threaded()

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> - Fix the comment, is_single_threaded(p) actually means that nobody shares
>   ->mm with p.
> 
>   I think this helper should be renamed,

What we want to know when we ask this function is whether or not a process is
single-threaded, hence the name.  The fact that because:

	CLONE_THREAD => CLONE_SIGHAND => CLONE_VM

we can work this out purely by checking that there aren't any processes that
share VM space with us is immaterial.

> and it should not have arguments.  With or without this patch it must not be
> used unless p == current, otherwise we can't safely use p->signal or p->mm.

Well, I can live with that, but you need to check with the SELinux people too.
Whilst they do currently limit the selinux_setprocattr() to current only, they
still hand the task pointer that function is given around.

> - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
>   to iterate over the process list. If there is another CLONE_VM process
>   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
>   a freshly forked CLONE_VM task, but this doesn't matter because we must
>   see its parent and return false.

Hmmm...  I'd quite like to avoid using down_write() if possible.  Why do we
need to do this?  Is it just to stop processes that might cease using mm from
doing so until we've finished?

David

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 04/16, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > - Fix the comment, is_single_threaded(p) actually means that nobody shares
> >   ->mm with p.
> >
> >   I think this helper should be renamed,
>
> What we want to know when we ask this function is whether or not a process is
> single-threaded, hence the name.  The fact that because:
>
> 	CLONE_THREAD => CLONE_SIGHAND => CLONE_VM
>
> we can work this out purely by checking that there aren't any processes that
> share VM space with us is immaterial.

Confused... I already asked this in http://marc.info/?t=123853355800001
"what is_single_threaded() does?" and perhaps I misunderstood you.

So, once again, what it should do? If we only care about CLONE_THREAD (implies
CLONE_VM), then we can just do

	bool is_single_threaded(struct task_struct *p)
	{
		return atomic_read(&p->signal->live) == 1;
	}

But, if it should check p doesn't share VM space (this is what it does
with or without the patch), then we have to scan all processes.

> > and it should not have arguments.  With or without this patch it must not be
> > used unless p == current, otherwise we can't safely use p->signal or p->mm.
>
> Well, I can live with that, but you need to check with the SELinux people too.
> Whilst they do currently limit the selinux_setprocattr() to current only, they
> still hand the task pointer that function is given around.

Yes, I see. But (apart from "not safe" above), from the security pov it doesn't
make sense to call is_single_threaded(p) unless p == current ? The task can
fork right after the check.

> > - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
> >   to iterate over the process list. If there is another CLONE_VM process
> >   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
> >   a freshly forked CLONE_VM task, but this doesn't matter because we must
> >   see its parent and return false.
>
> Hmmm...  I'd quite like to avoid using down_write() if possible.

Cough. And I'd like to avoid taking tasklist_lock as much as possible ;)
tasklist is the global and overused lock. Not good to take it to scan the
process list.

> Why do we
> need to do this?  Is it just to stop processes that might cease using mm from
> doing so until we've finished?

Suppose we have a process P which shares ->mm with "task" (the argument), so
we should return "false".

P does clone(CLONE_VM) and exits. rcu_read_lock() can't guarantee we will
see the new task with the same ->mm. And without ->mmap_sem P can call
exit_mm() and set P->mm = NULL.

Hmm. But we can just add a barrier?

	bool is_single_threaded(struct task_struct *task)
	{
		struct mm_struct *mm = task->mm;
		struct task_struct *p, *t;
		bool ret;

		if (atomic_read(&task->signal->live) != 1)
			return false;

		if (atomic_read(&mm->mm_users) == 1)
			return true;

		ret = false;
		rcu_read_lock();
		for_each_process(p) {
			if (unlikely(p->flags & PF_KTHREAD))
				continue;
			if (unlikely(p == task->group_leader))
				continue;

			t = p;
			do {
				if (unlikely(t->mm == mm))
					goto found;
				if (likely(t->mm))
					break;

				/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
				   t->mm == NULL. Perhaps it had the same ->mm ?
				   If t has forked CLONE_VM task and called exit_mm(),
				   make sure next_thread() or for_each_process()->next_task()
				   will see it.
				   !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
				*/
				smp_rmb();

			} while_each_thread(p, t);
		}
		ret = true;
	found:
		rcu_read_unlock();

		return ret;
	}

What do you think?

Oleg.

Re: [PATCH] rework/fix is_single_threaded()

[Stephen Smalley]

On Thu, 2009-04-16 at 15:36 +0200, Oleg Nesterov wrote:
> On 04/16, David Howells wrote:
> >
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > - Fix the comment, is_single_threaded(p) actually means that nobody shares
> > >   ->mm with p.
> > >
> > >   I think this helper should be renamed,
> >
> > What we want to know when we ask this function is whether or not a process is
> > single-threaded, hence the name.  The fact that because:
> >
> > 	CLONE_THREAD => CLONE_SIGHAND => CLONE_VM
> >
> > we can work this out purely by checking that there aren't any processes that
> > share VM space with us is immaterial.
> 
> Confused... I already asked this in http://marc.info/?t=123853355800001
> "what is_single_threaded() does?" and perhaps I misunderstood you.
> 
> So, once again, what it should do? If we only care about CLONE_THREAD (implies
> CLONE_VM), then we can just do
> 
> 	bool is_single_threaded(struct task_struct *p)
> 	{
> 		return atomic_read(&p->signal->live) == 1;
> 	}
> 
> But, if it should check p doesn't share VM space (this is what it does
> with or without the patch), then we have to scan all processes.

In the SELinux case, we care about shared VM space.   The purpose of the
check for SELinux is to prevent the security contexts of tasks that
share a VM from diverging from one another, as we cannot enforce any
separation among them.

> > > and it should not have arguments.  With or without this patch it must not be
> > > used unless p == current, otherwise we can't safely use p->signal or p->mm.
> >
> > Well, I can live with that, but you need to check with the SELinux people too.
> > Whilst they do currently limit the selinux_setprocattr() to current only, they
> > still hand the task pointer that function is given around.
> 
> Yes, I see. But (apart from "not safe" above), from the security pov it doesn't
> make sense to call is_single_threaded(p) unless p == current ? The task can
> fork right after the check.

Right.  In the SELinux case, we will only ever call it with p ==
current, and if you want to make that explicit by dropping the task
argument and directly acting on current, feel free.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 04/16, Oleg Nesterov wrote:
>
> Suppose we have a process P which shares ->mm with "task" (the argument), so
> we should return "false".
>
> P does clone(CLONE_VM) and exits. rcu_read_lock() can't guarantee we will
> see the new task with the same ->mm. And without ->mmap_sem P can call
> exit_mm() and set P->mm = NULL.
>
> Hmm. But we can just add a barrier?
>
> 	bool is_single_threaded(struct task_struct *task)
> 	{
> 		struct mm_struct *mm = task->mm;
> 		struct task_struct *p, *t;
> 		bool ret;
>
> 		if (atomic_read(&task->signal->live) != 1)
> 			return false;
>
> 		if (atomic_read(&mm->mm_users) == 1)
> 			return true;
>
> 		ret = false;
> 		rcu_read_lock();
> 		for_each_process(p) {
> 			if (unlikely(p->flags & PF_KTHREAD))
> 				continue;
> 			if (unlikely(p == task->group_leader))
> 				continue;
>
> 			t = p;
> 			do {
> 				if (unlikely(t->mm == mm))
> 					goto found;
> 				if (likely(t->mm))
> 					break;
>
> 				/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> 				   t->mm == NULL. Perhaps it had the same ->mm ?
> 				   If t has forked CLONE_VM task and called exit_mm(),
> 				   make sure next_thread() or for_each_process()->next_task()
> 				   will see it.
> 				   !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> 				*/
> 				smp_rmb();

Sorry, forgot to mention...

But what if P does clone(CLONE_VM), exits, and for_each_process/while_each_thread
doesn't see it? IOW, what if we already see the result of list_del_rcu() ?

I think, in that case we must also see the result of clone()->list_add_tail_rcu()
which has a barrier, so we are safe.

Hmm. I feel this all has a simpler explanation, or I missed something...

Oleg.

Re: [PATCH] rework/fix is_single_threaded()

[Andrew Morton]

On Thu, 16 Apr 2009 16:54:44 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/16, Oleg Nesterov wrote:
> >
> > Suppose we have a process P which shares ->mm with "task" (the argument), so
> > we should return "false".
> >
> > P does clone(CLONE_VM) and exits. rcu_read_lock() can't guarantee we will
> > see the new task with the same ->mm. And without ->mmap_sem P can call
> > exit_mm() and set P->mm = NULL.
> >
> > Hmm. But we can just add a barrier?
> >
> > 	bool is_single_threaded(struct task_struct *task)
> > 	{
> > 		struct mm_struct *mm = task->mm;
> > 		struct task_struct *p, *t;
> > 		bool ret;
> >
> > 		if (atomic_read(&task->signal->live) != 1)
> > 			return false;
> >
> > 		if (atomic_read(&mm->mm_users) == 1)
> > 			return true;
> >
> > 		ret = false;
> > 		rcu_read_lock();
> > 		for_each_process(p) {
> > 			if (unlikely(p->flags & PF_KTHREAD))
> > 				continue;
> > 			if (unlikely(p == task->group_leader))
> > 				continue;
> >
> > 			t = p;
> > 			do {
> > 				if (unlikely(t->mm == mm))
> > 					goto found;
> > 				if (likely(t->mm))
> > 					break;
> >
> > 				/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> > 				   t->mm == NULL. Perhaps it had the same ->mm ?
> > 				   If t has forked CLONE_VM task and called exit_mm(),
> > 				   make sure next_thread() or for_each_process()->next_task()
> > 				   will see it.
> > 				   !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> > 				*/
> > 				smp_rmb();
> 
> Sorry, forgot to mention...
> 
> But what if P does clone(CLONE_VM), exits, and for_each_process/while_each_thread
> doesn't see it? IOW, what if we already see the result of list_del_rcu() ?
> 
> I think, in that case we must also see the result of clone()->list_add_tail_rcu()
> which has a barrier, so we are safe.
> 
> Hmm. I feel this all has a simpler explanation, or I missed something...
> 

It appears that this patch is rather stuck.  Should I drop it?


From: Oleg Nesterov <oleg@redhat.com>

- Fix the comment, is_single_threaded(p) actually means that nobody shares
  ->mm with p.

  I think this helper should be renamed, and it should not have arguments.
  With or without this patch it must not be used unless p == current,
  otherwise we can't safely use p->signal or p->mm.

- "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
  zombie group leader, use signal->live instead.

- Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
  otherwise we can return the wrong "false".

- Use for_each_process() instead of do_each_thread(), all threads must use
  the same ->mm.

- Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
  to iterate over the process list. If there is another CLONE_VM process
  it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
  a freshly forked CLONE_VM task, but this doesn't matter because we must
  see its parent and return false.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <jmorris@namei.org>
Cc: Roland McGrath <roland@redhat.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 lib/is_single_threaded.c |   62 +++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff -puN lib/is_single_threaded.c~rework-fix-is_single_threaded lib/is_single_threaded.c
--- a/lib/is_single_threaded.c~rework-fix-is_single_threaded
+++ a/lib/is_single_threaded.c
@@ -12,34 +12,44 @@
 
 #include <linux/sched.h>
 
-/**
- * is_single_threaded - Determine if a thread group is single-threaded or not
- * @p: A task in the thread group in question
- *
- * This returns true if the thread group to which a task belongs is single
- * threaded, false if it is not.
+/*
+ * Returns true if the task does not share ->mm with another thread/process.
  */
-bool is_single_threaded(struct task_struct *p)
+bool is_single_threaded(struct task_struct *task)
 {
-	struct task_struct *g, *t;
-	struct mm_struct *mm = p->mm;
-
-	if (atomic_read(&p->signal->count) != 1)
-		goto no;
-
-	if (atomic_read(&p->mm->mm_users) != 1) {
-		read_lock(&tasklist_lock);
-		do_each_thread(g, t) {
-			if (t->mm == mm && t != p)
-				goto no_unlock;
-		} while_each_thread(g, t);
-		read_unlock(&tasklist_lock);
+	struct mm_struct *mm = task->mm;
+	struct task_struct *p, *t;
+	bool ret;
+
+	might_sleep();
+
+	if (atomic_read(&task->signal->live) != 1)
+		return false;
+
+	if (atomic_read(&mm->mm_users) == 1)
+		return true;
+
+	ret = false;
+	down_write(&mm->mmap_sem);
+	rcu_read_lock();
+	for_each_process(p) {
+		if (unlikely(p->flags & PF_KTHREAD))
+			continue;
+		if (unlikely(p == task->group_leader))
+			continue;
+
+		t = p;
+		do {
+			if (unlikely(t->mm == mm))
+				goto found;
+			if (likely(t->mm))
+				break;
+		} while_each_thread(p, t);
 	}
+	ret = true;
+found:
+	rcu_read_unlock();
+	up_write(&mm->mmap_sem);
 
-	return true;
-
-no_unlock:
-	read_unlock(&tasklist_lock);
-no:
-	return false;
+	return ret;
 }
_

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 06/18, Andrew Morton wrote:
>
> It appears that this patch is rather stuck.  Should I drop it?

Well, I am biased of course...

I think the patch is correct. David dislikes down_write(->mmap_sem),
but imho it is better than the global tasklist_lock.

Looks like we can avoid ->mmap_sem too, but imho this change needs
another patch, it is subtle.

OTOH, I can't say this patch is important, both the fixed bugs and
improvements are minor.

Oleg.

> From: Oleg Nesterov <oleg@redhat.com>
>
> - Fix the comment, is_single_threaded(p) actually means that nobody shares
>   ->mm with p.
>
>   I think this helper should be renamed, and it should not have arguments.
>   With or without this patch it must not be used unless p == current,
>   otherwise we can't safely use p->signal or p->mm.
>
> - "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
>   zombie group leader, use signal->live instead.
>
> - Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
>   otherwise we can return the wrong "false".
>
> - Use for_each_process() instead of do_each_thread(), all threads must use
>   the same ->mm.
>
> - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
>   to iterate over the process list. If there is another CLONE_VM process
>   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
>   a freshly forked CLONE_VM task, but this doesn't matter because we must
>   see its parent and return false.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Roland McGrath <roland@redhat.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  lib/is_single_threaded.c |   62 +++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 26 deletions(-)
>
> diff -puN lib/is_single_threaded.c~rework-fix-is_single_threaded lib/is_single_threaded.c
> --- a/lib/is_single_threaded.c~rework-fix-is_single_threaded
> +++ a/lib/is_single_threaded.c
> @@ -12,34 +12,44 @@
>
>  #include <linux/sched.h>
>
> -/**
> - * is_single_threaded - Determine if a thread group is single-threaded or not
> - * @p: A task in the thread group in question
> - *
> - * This returns true if the thread group to which a task belongs is single
> - * threaded, false if it is not.
> +/*
> + * Returns true if the task does not share ->mm with another thread/process.
>   */
> -bool is_single_threaded(struct task_struct *p)
> +bool is_single_threaded(struct task_struct *task)
>  {
> -	struct task_struct *g, *t;
> -	struct mm_struct *mm = p->mm;
> -
> -	if (atomic_read(&p->signal->count) != 1)
> -		goto no;
> -
> -	if (atomic_read(&p->mm->mm_users) != 1) {
> -		read_lock(&tasklist_lock);
> -		do_each_thread(g, t) {
> -			if (t->mm == mm && t != p)
> -				goto no_unlock;
> -		} while_each_thread(g, t);
> -		read_unlock(&tasklist_lock);
> +	struct mm_struct *mm = task->mm;
> +	struct task_struct *p, *t;
> +	bool ret;
> +
> +	might_sleep();
> +
> +	if (atomic_read(&task->signal->live) != 1)
> +		return false;
> +
> +	if (atomic_read(&mm->mm_users) == 1)
> +		return true;
> +
> +	ret = false;
> +	down_write(&mm->mmap_sem);
> +	rcu_read_lock();
> +	for_each_process(p) {
> +		if (unlikely(p->flags & PF_KTHREAD))
> +			continue;
> +		if (unlikely(p == task->group_leader))
> +			continue;
> +
> +		t = p;
> +		do {
> +			if (unlikely(t->mm == mm))
> +				goto found;
> +			if (likely(t->mm))
> +				break;
> +		} while_each_thread(p, t);
>  	}
> +	ret = true;
> +found:
> +	rcu_read_unlock();
> +	up_write(&mm->mmap_sem);
>
> -	return true;
> -
> -no_unlock:
> -	read_unlock(&tasklist_lock);
> -no:
> -	return false;
> +	return ret;
>  }
> _
>

Re: [PATCH] rework/fix is_single_threaded()

[Andrew Morton]

On Thu, 18 Jun 2009 21:42:38 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 06/18, Andrew Morton wrote:
> >
> > It appears that this patch is rather stuck.  Should I drop it?
> 
> Well, I am biased of course...
> 
> I think the patch is correct. David dislikes down_write(->mmap_sem),
> but imho it is better than the global tasklist_lock.
> 
> Looks like we can avoid ->mmap_sem too, but imho this change needs
> another patch, it is subtle.
> 
> OTOH, I can't say this patch is important, both the fixed bugs and
> improvements are minor.
> 

hm, that's still all a bit marginal/waffly.

> 
> > From: Oleg Nesterov <oleg@redhat.com>
> >
> > - Fix the comment, is_single_threaded(p) actually means that nobody shares
> >   ->mm with p.
> >
> >   I think this helper should be renamed, and it should not have arguments.
> >   With or without this patch it must not be used unless p == current,
> >   otherwise we can't safely use p->signal or p->mm.
> >
> > - "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
> >   zombie group leader, use signal->live instead.
> >
> > - Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
> >   otherwise we can return the wrong "false".
> >
> > - Use for_each_process() instead of do_each_thread(), all threads must use
> >   the same ->mm.
> >
> > - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
> >   to iterate over the process list. If there is another CLONE_VM process
> >   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
> >   a freshly forked CLONE_VM task, but this doesn't matter because we must
> >   see its parent and return false.

Maybe we should do the locking change in a separate and subsequent
patch?

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 06/22, Andrew Morton wrote:
>
> hm, that's still all a bit marginal/waffly.
>
> >
> > > From: Oleg Nesterov <oleg@redhat.com>
> > >
> > > - Fix the comment, is_single_threaded(p) actually means that nobody shares
> > >   ->mm with p.
> > >
> > >   I think this helper should be renamed, and it should not have arguments.
> > >   With or without this patch it must not be used unless p == current,
> > >   otherwise we can't safely use p->signal or p->mm.
> > >
> > > - "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
> > >   zombie group leader, use signal->live instead.
> > >
> > > - Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
> > >   otherwise we can return the wrong "false".
> > >
> > > - Use for_each_process() instead of do_each_thread(), all threads must use
> > >   the same ->mm.
> > >
> > > - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
> > >   to iterate over the process list. If there is another CLONE_VM process
> > >   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
> > >   a freshly forked CLONE_VM task, but this doesn't matter because we must
> > >   see its parent and return false.
>
> Maybe we should do the locking change in a separate and subsequent
> patch?

Sure, I can split these changes. Or we can just forget about this patch.


But what is the problem with this patch?

David, do you still dislike ->mmap_sem? I didn't see other objections,
and again, imho tasklist_lock is worse.

Oleg.

Re: [PATCH] rework/fix is_single_threaded()

[Andrew Morton]

On Mon, 22 Jun 2009 19:14:31 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 06/22, Andrew Morton wrote:
> >
> > hm, that's still all a bit marginal/waffly.
> >
> > >
> > > > From: Oleg Nesterov <oleg@redhat.com>
> > > >
> > > > - Fix the comment, is_single_threaded(p) actually means that nobody shares
> > > >   ->mm with p.
> > > >
> > > >   I think this helper should be renamed, and it should not have arguments.
> > > >   With or without this patch it must not be used unless p == current,
> > > >   otherwise we can't safely use p->signal or p->mm.
> > > >
> > > > - "if (atomic_read(&p->signal->count) != 1)" is not right when we have a
> > > >   zombie group leader, use signal->live instead.
> > > >
> > > > - Add PF_KTHREAD check to skip kernel threads which may borrow p->mm,
> > > >   otherwise we can return the wrong "false".
> > > >
> > > > - Use for_each_process() instead of do_each_thread(), all threads must use
> > > >   the same ->mm.
> > > >
> > > > - Use down_write(mm->mmap_sem) + rcu_read_lock() instead of tasklist_lock
> > > >   to iterate over the process list. If there is another CLONE_VM process
> > > >   it can't pass exit_mm() which takes the same mm->mmap_sem. We can miss
> > > >   a freshly forked CLONE_VM task, but this doesn't matter because we must
> > > >   see its parent and return false.
> >
> > Maybe we should do the locking change in a separate and subsequent
> > patch?
> 
> Sure, I can split these changes. Or we can just forget about this patch.

noooo the world needs oleg patches.

> 
> But what is the problem with this patch?

The discussion between yourself and David seems to be unresolved, so
I have it in wait-and-see mode.

> David, do you still dislike ->mmap_sem? I didn't see other objections,
> and again, imho tasklist_lock is worse.

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 06/22, Andrew Morton wrote:
>
> On Mon, 22 Jun 2009 19:14:31 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > Sure, I can split these changes. Or we can just forget about this patch.
>
> noooo the world needs oleg patches.

Yeeeees, I am trying to blackmail ;)

> > But what is the problem with this patch?
>
> The discussion between yourself and David seems to be unresolved, so
> I have it in wait-and-see mode.
>
> > David, do you still dislike ->mmap_sem? I didn't see other objections,
> > and again, imho tasklist_lock is worse.

OK, I'll split and re-send.

Unless David replies.

Oleg.

Re: [PATCH] rework/fix is_single_threaded()

[David Howells]

Oleg Nesterov <oleg@redhat.com> wrote:

> Unless David replies.

Sorry, I missed the last three messages.

> David, do you still dislike ->mmap_sem? I didn't see other objections,
> and again, imho tasklist_lock is worse.

My main objection to taking mmap_sem is that it restricts where the function
can be used.  It can't, for example, be called by anyone holding a spinlock.
Furthermore, the more locks, the more chance of someone accidentally
deadlocking something.

But apart from that, go for it.

David

Re: [PATCH] rework/fix is_single_threaded()

[Oleg Nesterov]

On 07/09, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > David, do you still dislike ->mmap_sem? I didn't see other objections,
> > and again, imho tasklist_lock is worse.
>
> My main objection to taking mmap_sem is that it restricts where the function
> can be used.  It can't, for example, be called by anyone holding a spinlock.

Yes, it is might_sleep().

> Furthermore, the more locks, the more chance of someone accidentally
> deadlocking something.

The current code is not lockless too, tasklist_lock is not free and
can lead too deadlocks as well.

Anyway. I agree it is better to avoid ->mmap_sem, I'll send the patch
in a minute. But I'd really like to do this in a separate patch, the
change is subtle and needs a changelog at least.

> But apart from that, go for it.

Great, thanks. Then I'll make the patch on top.

Oleg.