===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
kernel/pid.c:419 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 1
1 lock held by scrashme/11079:
#0: (tasklist_lock){.?.?..}, at: [<ffffffff81112702>] sys_migrate_pages+0xd6/0x29d
stack backtrace:
Pid: 11079, comm: scrashme Not tainted 2.6.37-rc3+ #2
Call Trace:
[<ffffffff8107d095>] lockdep_rcu_dereference+0x9d/0xa5
[<ffffffff81069dc8>] find_task_by_pid_ns+0x44/0x5d
[<ffffffff81069e03>] find_task_by_vpid+0x22/0x24
[<ffffffff8111270f>] sys_migrate_pages+0xe3/0x29d
[<ffffffff8107c7c6>] ? trace_hardirqs_off_caller+0xa3/0x10b
[<ffffffff8147b635>] ? retint_swapgs+0x13/0x1b
[<ffffffff8107e2e7>] ? trace_hardirqs_on_caller+0x13f/0x172
[<ffffffff8147a95c>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff81009cb2>] system_call_fastpath+0x16/0x1b
Signed-off-by: Dave Jones <[email protected]>
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 4a57f13..2f0f55b 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1308,6 +1308,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
/* Find the mm_struct */
read_lock(&tasklist_lock);
+ rcu_read_lock();
task = pid ? find_task_by_vpid(pid) : current;
if (!task) {
read_unlock(&tasklist_lock);
@@ -1315,6 +1316,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
goto out;
}
mm = get_task_mm(task);
+ rcu_read_unlock();
read_unlock(&tasklist_lock);
err = -EINVAL;
Hi
> Signed-off-by: Dave Jones <[email protected]>
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 4a57f13..2f0f55b 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1308,6 +1308,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
>
> /* Find the mm_struct */
> read_lock(&tasklist_lock);
> + rcu_read_lock();
> task = pid ? find_task_by_vpid(pid) : current;
> if (!task) {
> read_unlock(&tasklist_lock);
> @@ -1315,6 +1316,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
> goto out;
> }
> mm = get_task_mm(task);
> + rcu_read_unlock();
> read_unlock(&tasklist_lock);
>
> err = -EINVAL;
Thanks reporting. but mmotm seems already has the same patch.
On Thu, Nov 25, 2010 at 09:50:37AM +0900, KOSAKI Motohiro wrote:
> > @@ -1315,6 +1316,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
> > goto out;
> > }
> > mm = get_task_mm(task);
> > + rcu_read_unlock();
> > read_unlock(&tasklist_lock);
> >
> > err = -EINVAL;
>
> Thanks reporting. but mmotm seems already has the same patch.
indeed. It's also better than mine, which missed the unlock on error path.
thanks,
Dave
On Thu, 2010-11-25 at 09:50 +0900, KOSAKI Motohiro wrote:
> Hi
>
> > Signed-off-by: Dave Jones <[email protected]>
> >
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index 4a57f13..2f0f55b 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -1308,6 +1308,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
> >
> > /* Find the mm_struct */
> > read_lock(&tasklist_lock);
> > + rcu_read_lock();
> > task = pid ? find_task_by_vpid(pid) : current;
> > if (!task) {
> > read_unlock(&tasklist_lock);
> > @@ -1315,6 +1316,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
> > goto out;
> > }
> > mm = get_task_mm(task);
> > + rcu_read_unlock();
> > read_unlock(&tasklist_lock);
> >
> > err = -EINVAL;
>
> Thanks reporting. but mmotm seems already has the same patch.
Another one,... again:
Do we still need the tasklist_lock in this case?
Also, why is that think complaining, surely the tasklist_lock pins any
and all PID objects?
> On Thu, 2010-11-25 at 09:50 +0900, KOSAKI Motohiro wrote:
> > Hi
> >
> > > Signed-off-by: Dave Jones <[email protected]>
> > >
> > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > > index 4a57f13..2f0f55b 100644
> > > --- a/mm/mempolicy.c
> > > +++ b/mm/mempolicy.c
> > > @@ -1308,6 +1308,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
> > >
> > > /* Find the mm_struct */
> > > read_lock(&tasklist_lock);
> > > + rcu_read_lock();
> > > task = pid ? find_task_by_vpid(pid) : current;
> > > if (!task) {
> > > read_unlock(&tasklist_lock);
> > > @@ -1315,6 +1316,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
> > > goto out;
> > > }
> > > mm = get_task_mm(task);
> > > + rcu_read_unlock();
> > > read_unlock(&tasklist_lock);
> > >
> > > err = -EINVAL;
> >
> > Thanks reporting. but mmotm seems already has the same patch.
>
> Another one,... again:
>
>
> Do we still need the tasklist_lock in this case?
>
> Also, why is that think complaining, surely the tasklist_lock pins any
> and all PID objects?
(cc to Christoph and Oleg)
Good spotting.
As far as I understand, find_task_by_pid() did required tasklist_lock in
old days. but It isn't now.
So I think we can remove tasklist_lock from here.
===========================================================================
>From 9de9f70f74e55d92b5e9057e22fc629405f63295 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <[email protected]>
Date: Thu, 16 Dec 2010 17:49:23 +0900
Subject: [PATCH] mempolicy: remove tasklist_lock from migrate_pages
Today, tasklist_lock in migrate_pages doesn't protect anything.
rcu_read_lock() provide enough protection from pid hash walk.
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/mempolicy.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 11ff260..9064945 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1308,16 +1308,13 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode,
/* Find the mm_struct */
rcu_read_lock();
- read_lock(&tasklist_lock);
task = pid ? find_task_by_vpid(pid) : current;
if (!task) {
- read_unlock(&tasklist_lock);
rcu_read_unlock();
err = -ESRCH;
goto out;
}
mm = get_task_mm(task);
- read_unlock(&tasklist_lock);
rcu_read_unlock();
err = -EINVAL;
--
1.6.5.2