- 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 <[email protected]>
--- 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;
}
On Mon, 13 Apr 2009 23:45:13 +0200
Oleg Nesterov <[email protected]> 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?
Oleg Nesterov <[email protected]> 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
On 04/16, David Howells wrote:
>
> Oleg Nesterov <[email protected]> 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.
On 04/15, Andrew Morton wrote:
>
> On Mon, 13 Apr 2009 23:45:13 +0200
> Oleg Nesterov <[email protected]> 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.
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.
On Thu, 2009-04-16 at 15:36 +0200, Oleg Nesterov wrote:
> On 04/16, David Howells wrote:
> >
> > Oleg Nesterov <[email protected]> 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
On Thu, 16 Apr 2009 16:54:44 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
- 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 <[email protected]>
Cc: David Howells <[email protected]>
Cc: James Morris <[email protected]>
Cc: Roland McGrath <[email protected]>
Cc: Stephen Smalley <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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;
}
_
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 <[email protected]>
>
> - 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 <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: Roland McGrath <[email protected]>
> Cc: Stephen Smalley <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> 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;
> }
> _
>
On Thu, 18 Jun 2009 21:42:38 +0200
Oleg Nesterov <[email protected]> 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 <[email protected]>
> >
> > - 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?
On 06/22, Andrew Morton wrote:
>
> hm, that's still all a bit marginal/waffly.
>
> >
> > > From: Oleg Nesterov <[email protected]>
> > >
> > > - 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.
On Mon, 22 Jun 2009 19:14:31 +0200
Oleg Nesterov <[email protected]> wrote:
> On 06/22, Andrew Morton wrote:
> >
> > hm, that's still all a bit marginal/waffly.
> >
> > >
> > > > From: Oleg Nesterov <[email protected]>
> > > >
> > > > - 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.
On 06/22, Andrew Morton wrote:
>
> On Mon, 22 Jun 2009 19:14:31 +0200
> Oleg Nesterov <[email protected]> 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.
Oleg Nesterov <[email protected]> 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
On 07/09, David Howells wrote:
>
> Oleg Nesterov <[email protected]> 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.