Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

[Ted Ts'o]

On Sun, Nov 21, 2010 at 07:26:11PM +0800, Arun Bhanu wrote:
> I saw this in kernel log messages while testing 2.6.37-rc2. I think it
> appeared while mounting an external hard-disk. I can't seem to
> reproduce it.

I could be wrong but this looks like it's a bug in mm/migrate.c in
migrate_page_move_mapping(): it is calling radix_tree_lookup_slot()
without first taking an rcu_read_lock().

It was triggered by a memory allocation out of ext4_fill_super(),
which then triggered a memory compaction/migration, but I don't
believe it's otherwise related to the ext4 code.

Over to the linux-mm folks for confirmation...

					- Ted

> Please let me know if you need more info.
> 
> [47064.272151] ===================================================
> [47064.273474] [ INFO: suspicious rcu_dereference_check() usage. ]
> [47064.273956] ---------------------------------------------------
> [47064.274431] include/linux/radix-tree.h:145 invoked rcu_dereference_check() without protection!
> [47064.274905] 
> [47064.274906] other info that might help us debug this:
> [47064.274907] 
> [47064.276303] 
> [47064.276303] rcu_scheduler_active = 1, debug_locks = 0
> [47064.277202] 2 locks held by mount/21199:
> [47064.277635]  #0:  (&type->s_umount_key#20/1){+.+.+.}, at: [<c05007f0>] sget+0x21f/0x35d
> [47064.278078]  #1:  (&(&inode->i_data.tree_lock)->rlock){..-.-.}, at: [<c04f5e67>] migrate_page_move_mapping+0x3a/0x120
> [47064.278529] 
> [47064.278529] stack backtrace:
> [47064.279409] Pid: 21199, comm: mount Not tainted 2.6.37-rc2-ab2-589136bfa784a4558b397f017ca2f06f0ca9080e+ #1
> [47064.279864] Call Trace:
> [47064.280313]  [<c0822e61>] ? printk+0x2d/0x34
> [47064.280765]  [<c04709c8>] lockdep_rcu_dereference+0x97/0x9f
> [47064.281220]  [<c04f5e28>] radix_tree_deref_slot+0x4a/0x4f
> [47064.281680]  [<c04f5ea7>] migrate_page_move_mapping+0x7a/0x120
> [47064.282129]  [<c04f6323>] migrate_page+0x1f/0x35
> [47064.282573]  [<c04f6464>] move_to_new_page+0x12b/0x164
> [47064.283017]  [<c04f6773>] migrate_pages+0x1e1/0x2ee
> [47064.283463]  [<c04eed89>] ? compaction_alloc+0x0/0x1ef
> [47064.283918]  [<c04eebdc>] compact_zone+0x24f/0x3fc
> [47064.284363]  [<c04ef0f4>] try_to_compact_pages+0x17c/0x1e6
> [47064.284820]  [<c04cac33>] __alloc_pages_nodemask+0x397/0x6b7
> [47064.285276]  [<c04caf75>] __get_free_pages+0x22/0x33
> [47064.285729]  [<c04f4304>] __kmalloc+0x2f/0x112
> [47064.286193]  [<c0435adc>] ? should_resched+0xd/0x28
> [47064.286655]  [<c0578ca9>] kzalloc.clone.58+0x12/0x14
> [47064.287118]  [<c057c7f1>] ext4_fill_super+0x1090/0x2521
> [47064.287573]  [<c040ea02>] ? native_sched_clock+0x14/0x52
> [47064.288029]  [<c054774a>] ? disk_name+0x86/0x90
> [47064.288477]  [<c0523bfa>] ? set_blocksize+0x33/0x78
> [47064.288930]  [<c0500aed>] mount_bdev+0x123/0x16d
> [47064.289385]  [<c057b761>] ? ext4_fill_super+0x0/0x2521
> [47064.289842]  [<c05776fd>] ? ext4_mount+0x0/0x24
> [47064.290300]  [<c057771c>] ext4_mount+0x1f/0x24
> [47064.290760]  [<c057b761>] ? ext4_fill_super+0x0/0x2521
> [47064.291222]  [<c05003e1>] vfs_kern_mount+0xa1/0x1ad
> [47064.291687]  [<c0511c36>] ? get_fs_type+0x38/0x91
> [47064.292149]  [<c0500546>] do_kern_mount+0x3d/0xc8
> [47064.292615]  [<c05142b2>] do_mount+0x614/0x640
> [47064.293056]  [<c04d69f0>] ? strndup_user+0x2e/0x3f
> [47064.293489]  [<c05144a1>] sys_mount+0x6d/0x99
> [47064.293925]  [<c040971f>] sysenter_do_call+0x12/0x38
> [47064.368116] EXT4-fs (sdb1): mounted filesystem with ordered data mode. Opts: (null)
> 
> -Arun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

[Minchan Kim]

On Sun, Nov 21, 2010 at 08:30:24AM -0500, Ted Ts'o wrote:
> On Sun, Nov 21, 2010 at 07:26:11PM +0800, Arun Bhanu wrote:
> > I saw this in kernel log messages while testing 2.6.37-rc2. I think it
> > appeared while mounting an external hard-disk. I can't seem to
> > reproduce it.
> 
> I could be wrong but this looks like it's a bug in mm/migrate.c in
> migrate_page_move_mapping(): it is calling radix_tree_lookup_slot()
> without first taking an rcu_read_lock().
> 
> It was triggered by a memory allocation out of ext4_fill_super(),
> which then triggered a memory compaction/migration, but I don't
> believe it's otherwise related to the ext4 code.
> 
> Over to the linux-mm folks for confirmation...

I think it's no problem. 

That's because migration always holds lock_page on the file page.
So the page couldn't remove from radix. 


-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

[Ted Ts'o]

On Mon, Nov 22, 2010 at 12:39:49AM +0900, Minchan Kim wrote:
> 
> I think it's no problem. 
> 
> That's because migration always holds lock_page on the file page.
> So the page couldn't remove from radix. 

It may be "ok" in that it won't cause a race, but it still leaves an
unsightly warning if LOCKDEP is enabled, and LOCKDEP warnings will
cause /proc_lock_stat to be disabled.  So I think it still needs to be
fixed by adding rcu_read_lock()/rcu_read_unlock() to
migrate_page_move_mapping().

      	 					     - Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

[Minchan Kim]

On Mon, Nov 22, 2010 at 2:37 AM, Ted Ts'o <tytso@mit.edu> wrote:
> On Mon, Nov 22, 2010 at 12:39:49AM +0900, Minchan Kim wrote:
>>
>> I think it's no problem.
>>
>> That's because migration always holds lock_page on the file page.
>> So the page couldn't remove from radix.
>
> It may be "ok" in that it won't cause a race, but it still leaves an
> unsightly warning if LOCKDEP is enabled, and LOCKDEP warnings will
> cause /proc_lock_stat to be disabled.  So I think it still needs to be
> fixed by adding rcu_read_lock()/rcu_read_unlock() to
> migrate_page_move_mapping().
>
>                                                     - Ted
>

Yes. if it is really "ok" about race, we will add rcu_read_lock with
below comment to prevent false positive.
"suppress RCU lockdep false positives".
But I am not sure it's good although rcu_read_lock is little cost.
Whenever we find false positive, should we add rcu_read_lock to
suppress although it's no problem in real product?
Couldn't we provide following function? (or we might have already it
but I missed it. )

/*
 * Suppress RCU lockdep false positive.
 */
#ifdef CONFIG_PROVE_RCU
#define rcu_read_lock_suppress rcu_read_lock
#else
#define rcu_read_lock_suppress
#endif


-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG?] [Ext4] INFO: suspicious rcu_dereference_check() usage

[KOSAKI Motohiro]

> On Mon, Nov 22, 2010 at 12:39:49AM +0900, Minchan Kim wrote:
> > 
> > I think it's no problem. 
> > 
> > That's because migration always holds lock_page on the file page.
> > So the page couldn't remove from radix. 
> 
> It may be "ok" in that it won't cause a race, but it still leaves an
> unsightly warning if LOCKDEP is enabled, and LOCKDEP warnings will
> cause /proc_lock_stat to be disabled.  So I think it still needs to be
> fixed by adding rcu_read_lock()/rcu_read_unlock() to
> migrate_page_move_mapping().

Hi Ted,

Current mmotm has following patch and I think it should be fixed your
issue.

Thanks.




From: Zeng Zhaoming <zengzm.kernel@gmail.com>

find_task_by_vpid() should be protected by rcu_read_lock(), to prevent
free_pid() reclaiming pid.

Signed-off-by: Zeng Zhaoming <zengzm.kernel@gmail.com>
Cc: "Paul E. McKenney" <paulmck@us.ibm.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/mempolicy.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN mm/mempolicy.c~mm-mempolicyc-add-rcu-read-lock-to-protect-pid-structure mm/mempolicy.c
--- a/mm/mempolicy.c~mm-mempolicyc-add-rcu-read-lock-to-protect-pid-structure
+++ a/mm/mempolicy.c
@@ -1307,15 +1307,18 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
                goto out;

        /* 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();


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>