locking question

locking question

[Rick Lindsley]

As part of better understanding some of the issues in SMP,
I've been working at documenting all the global kernel locks in use,
including what's left of the BKL, and have run into a use of the BKL
that seems pretty consistent and also pretty obscure.

The code base I'm inspecting is 2.4.0-test8. (Yes I know that's out of
date, but I had to take a snapshot *somewhere*!)

For those drivers which define a struct file_operations->release
function, I'm seeing that that function often calls lock_kernel()
at the beginning and unlock_kernel() at the end. (A simple example
can be found in arch/m68k/mvme16x/rtc.c.)  Various functions perform
various feats, but I can't for the life of me figure out what it is
guarding. In the example above, if it is guarding rtc_status, why
in the rtc_open function (directly above it) are there no locks held?
It often seems the case that things modified in the release() function
are left unguarded elsewhere. And in most cases, it seems that the BKL
is probably unnecessary (a local lock would accomplish the same.)

I'm slowly coming to the conclusion that either
    
    a) there is a global assumption (or need) in place that I can't get
       my hands around, or

    b) people have copied drivers as templates for so long that the only
       reason they grab the lock there is because the person before them
       did, or
    
    c) some other driver-specific reason.

Can somebody tell me what we're guarding against here? Why is it safe
to use the "guarded" data elsewhere (apparently) without the lock?

Rick
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Locking Question

[Eric Sesterhenn]

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