Locking Question

[Eric Sesterhenn <snakebyte@gmx.de>]

hi,

I have seen a lockdep warning about freeing a held lock in XFS when mounting a corrupted xfs image:

[ 3107.196539] XFS mounting filesystem loop0
[ 3107.213927] 
[ 3107.213936] =========================
[ 3107.214027] [ BUG: held lock freed! ]
[ 3107.214027] -------------------------
[ 3107.214027] mount/18967 is freeing memory cf724000-cf7241e7, with a lock still held there!
[ 3107.214027]  (&(&ip->i_lock)->mr_lock){----}, at: [<c039b4c5>] xfs_ilock+0x4f/0x67
[ 3107.214027] 2 locks held by mount/18967:
[ 3107.214027]  #0:  (&type->s_umount_key#21){----}, at: [<c0183201>] sget+0x1fe/0x336
[ 3107.214027]  #1:  (&(&ip->i_lock)->mr_lock){----}, at: [<c039b4c5>] xfs_ilock+0x4f/0x67
[ 3107.214027] 
[ 3107.214027] stack backtrace:
[ 3107.214027] Pid: 18967, comm: mount Not tainted 2.6.27-rc5-00361-g82a28c7 #86
[ 3107.214027]  [<c013dfbb>] debug_check_no_locks_freed+0xd6/0x115
[ 3107.214027]  [<c017ec0b>] kmem_cache_free+0x4a/0xbd
[ 3107.214027]  [<c039d42d>] ? xfs_idestroy+0x92/0x98
[ 3107.214027]  [<c039d42d>] xfs_idestroy+0x92/0x98
[ 3107.214027]  [<c039ba9c>] xfs_iget_core+0x2eb/0x48e
[ 3107.214027]  [<c039bca7>] xfs_iget+0x68/0xe1
[ 3107.214027]  [<c03ac73f>] xfs_mountfs+0x3e0/0x5d1
[ 3107.214027]  [<c013dee3>] ? trace_hardirqs_on+0xb/0xd
[ 3107.214027]  [<c013bae2>] ? lockdep_init_map+0x74/0x34a
[ 3107.214027]  [<c013bae2>] ? lockdep_init_map+0x74/0x34a
[ 3107.214027]  [<c01296dd>] ? init_timer+0x17/0x1a
[ 3107.214027]  [<c03c03c4>] xfs_fs_fill_super+0x1f3/0x393
[ 3107.214027]  [<c0183bc4>] get_sb_bdev+0xcd/0x10b
[ 3107.214027]  [<c016afe6>] ? kstrdup+0x2a/0x4c
[ 3107.214027]  [<c03be4ef>] xfs_fs_get_sb+0x13/0x15
[ 3107.214027]  [<c03c01d1>] ? xfs_fs_fill_super+0x0/0x393
[ 3107.214027]  [<c01837e7>] vfs_kern_mount+0x3b/0x76
[ 3107.214027]  [<c0183866>] do_kern_mount+0x32/0xba
[ 3107.214027]  [<c0196350>] do_new_mount+0x46/0x74
[ 3107.214027]  [<c01964f3>] do_mount+0x175/0x193
[ 3107.214027]  [<c013de9d>] ? trace_hardirqs_on_caller+0xf4/0x12f
[ 3107.214027]  [<c0166606>] ? __get_free_pages+0x1e/0x24
[ 3107.214027]  [<c07295d3>] ? lock_kernel+0x19/0x8c
[ 3107.214027]  [<c0196562>] ? sys_mount+0x51/0x9b
[ 3107.214027]  [<c0196575>] sys_mount+0x64/0x9b
[ 3107.214027]  [<c01038bd>] sysenter_do_call+0x12/0x31
[ 3107.214027]  =======================
[ 3107.220438] XFS: failed to read root inode

As far as I can see xfs_iget.c:xfs_iget_core() has several places
that seem problematic.

After line 228:

    228         if (lock_flags)
    229                 xfs_ilock(ip, lock_flags);
    230 
    231         if ((ip->i_d.di_mode == 0) && !(flags & XFS_IGET_CREATE)) {
    232                 xfs_idestroy(ip);
    233                 xfs_put_perag(mp, pag);
    234                 return ENOENT;
    235         }

we might take the lock for ip and then call xfs_idestroy() which frees the lock,
as far as I can tell there should be an additional 

if (lock_flags)
	xfs_iunlock(ip, lock_flags)

inside the if to release the lock before we free the memory.

Some lines later we have a similar issue:

    243         if (radix_tree_preload(GFP_KERNEL)) {
    244                 xfs_idestroy(ip);
    245                 delay(1);
    246                 goto again;
    247         }

The lock is still taken and we call xfs_idestroy().

Some lines later, usually the BUG_ON triggers first, but there is a config option somewhere to remove
them to save some space, so we should also unlock ip as far as I can tell.

    255         if (unlikely(error)) {
    256                 BUG_ON(error != -EEXIST);
    257                 write_unlock(&pag->pag_ici_lock);
    258                 radix_tree_preload_end();
    259                 xfs_idestroy(ip);
    260                 XFS_STATS_INC(xs_ig_dup);
    261                 goto again;
    262         }


Since I am not familar with the XFS code at all it would be nice if someone can
confirm that this is correct or tell me to take another look at this :-)

Thanks, Eric