lkml.org 
[lkml]   [2011]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Subject[RFC] seqlock,lockdep: Add lock primitives to read_seqbegin().
    From
    Date
    I got a freeze between "seqlock_t rename_lock" and "DEFINE_BRLOCK(vfsmount_lock)"
    in 2.6.38. The bug was already fixed in linux-2.6 git tree.
    http://www.spinics.net/lists/linux-fsdevel/msg43078.html

    This bug is timing dependent and lockdep shows nothing before the freeze.
    I think that lockdep cannot detect this bug because no lock primitives are
    called within read_seqbegin().

    /* Lock out other writers and update the count.
    * Acts like a normal spin_lock/unlock.
    * Don't need preempt_disable() because that is in the spin_lock already.
    */
    static inline void write_seqlock(seqlock_t *sl)
    {
    spin_lock(&sl->lock);
    ++sl->sequence;
    smp_wmb();
    }

    static inline void write_sequnlock(seqlock_t *sl)
    {
    smp_wmb();
    sl->sequence++;
    spin_unlock(&sl->lock);
    }

    static __always_inline unsigned read_seqbegin(const seqlock_t *sl)
    {
    unsigned ret;

    repeat:
    ret = sl->sequence;
    smp_rmb();
    if (unlikely(ret & 1)) {
    cpu_relax();
    goto repeat;
    }

    return ret;
    }

    read_seqbegin() waits until the writer (if any) calls write_sequnlock().
    read_seqbegin() acts like a sort of normal spin_lock()+spin_unlock().

    --- linux-2.6.38.1.orig/include/linux/seqlock.h
    +++ linux-2.6.38.1/include/linux/seqlock.h
    @@ -86,6 +86,11 @@ static inline int write_tryseqlock(seqlo
    static __always_inline unsigned read_seqbegin(const seqlock_t *sl)
    {
    unsigned ret;
    +#ifdef CONFIG_PROVE_LOCKING
    + unsigned long flags;
    + spin_lock_irqsave(&((seqlock_t *) sl)->lock, flags);
    + spin_unlock_irqrestore(&((seqlock_t *) sl)->lock, flags);
    +#endif

    repeat:
    ret = sl->sequence;
    Therefore, if we apply the patch above, lockdep can detect this bug.

    [ 179.457160] =======================================================
    [ 179.458004] [ INFO: possible circular locking dependency detected ]
    [ 179.458004] 2.6.38.1 #2
    [ 179.458004] -------------------------------------------------------
    [ 179.458004] ccs_execute_han/3717 is trying to acquire lock:
    [ 179.458004] (vfsmount_lock){++++..}, at: [<c04d9910>] vfsmount_lock_local_lock+0x0/0x60
    [ 179.458004]
    [ 179.458004] but task is already holding lock:
    [ 179.458004] (rename_lock){+.+...}, at: [<c04d37dc>] __d_path+0x3c/0x80
    [ 179.458004]
    [ 179.458004] which lock already depends on the new lock.
    [ 179.458004]
    [ 179.458004]
    [ 179.458004] the existing dependency chain (in reverse order) is:
    [ 179.458004]
    [ 179.458004] -> #1 (rename_lock){+.+...}:
    [ 179.458004] [<c046d5f4>] __lock_acquire+0x244/0x6d0
    [ 179.458004] [<c046dafb>] lock_acquire+0x7b/0xa0
    [ 179.458004] [<c06c50a5>] _raw_spin_lock_irqsave+0x45/0x80
    [ 179.458004] [<c04d308b>] is_subdir+0x2b/0xd0
    [ 179.458004] [<c04dac29>] sys_pivot_root+0x219/0x290
    [ 179.458004] [<c0402c50>] sysenter_do_call+0x12/0x36
    [ 179.458004]
    [ 179.458004] -> #0 (vfsmount_lock){++++..}:
    [ 179.458004] [<c046d39e>] validate_chain+0x10ee/0x1100
    [ 179.458004] [<c046d5f4>] __lock_acquire+0x244/0x6d0
    [ 179.458004] [<c046dafb>] lock_acquire+0x7b/0xa0
    [ 179.458004] [<c04d9943>] vfsmount_lock_local_lock+0x33/0x60
    [ 179.458004] [<c04d284c>] prepend_path+0x1c/0x150
    [ 179.458004] [<c04d37f4>] __d_path+0x54/0x80
    [ 179.458004] [<e090e1f2>] ccs_realpath_from_path+0x122/0x3e0 [ccsecurity]
    [ 179.458004] [<e090eca4>] ccs_get_exe+0x54/0x60 [ccsecurity]
    [ 179.458004] [<e090ccdb>] ccs_manager+0x6b/0x1e0 [ccsecurity]
    [ 179.458004] [<e090d547>] ccs_write_control+0xc7/0x250 [ccsecurity]
    [ 179.458004] [<e090dec8>] ccs_write+0x8/0x10 [ccsecurity]
    [ 179.458004] [<c050145d>] proc_reg_write+0x5d/0x90
    [ 179.458004] [<c04c0256>] vfs_write+0x96/0x140
    [ 179.458004] [<c04c080d>] sys_write+0x3d/0x70
    [ 179.458004] [<c0402c50>] sysenter_do_call+0x12/0x36
    [ 179.458004]
    [ 179.458004] other info that might help us debug this:
    [ 179.458004]
    [ 179.458004] 4 locks held by ccs_execute_han/3717:
    [ 179.458004] #0: (&head->io_sem){+.+.+.}, at: [<e090d4ea>] ccs_write_control+0x6a/0x250 [ccsecurity]
    [ 179.458004] #1: (&ccs_ss){.+.+.+}, at: [<e090d4f7>] ccs_write_control+0x77/0x250 [ccsecurity]
    [ 179.458004] #2: (&mm->mmap_sem){++++++}, at: [<e090ec72>] ccs_get_exe+0x22/0x60 [ccsecurity]
    [ 179.458004] #3: (rename_lock){+.+...}, at: [<c04d37dc>] __d_path+0x3c/0x80
    [ 179.458004]
    [ 179.458004] stack backtrace:
    [ 179.458004] Pid: 3717, comm: ccs_execute_han Not tainted 2.6.38.1 #2
    [ 179.458004] Call Trace:
    [ 179.458004] [<c046bceb>] ? print_circular_bug+0xbb/0xc0
    [ 179.458004] [<c046d39e>] ? validate_chain+0x10ee/0x1100
    [ 179.458004] [<c0431216>] ? scheduler_tick+0x146/0x1f0
    [ 179.458004] [<c046d5f4>] ? __lock_acquire+0x244/0x6d0
    [ 179.458004] [<c046dafb>] ? lock_acquire+0x7b/0xa0
    [ 179.458004] [<c04d9910>] ? vfsmount_lock_local_lock+0x0/0x60
    [ 179.458004] [<c04d9943>] ? vfsmount_lock_local_lock+0x33/0x60
    [ 179.458004] [<c04d9910>] ? vfsmount_lock_local_lock+0x0/0x60
    [ 179.458004] [<c04d284c>] ? prepend_path+0x1c/0x150
    [ 179.458004] [<c04d37f4>] ? __d_path+0x54/0x80
    [ 179.458004] [<e090e1f2>] ? ccs_realpath_from_path+0x122/0x3e0 [ccsecurity]
    [ 179.458004] [<e090ec72>] ? ccs_get_exe+0x22/0x60 [ccsecurity]
    [ 179.458004] [<c06c42af>] ? down_read+0x7f/0x90
    [ 179.458004] [<e090eca4>] ? ccs_get_exe+0x54/0x60 [ccsecurity]
    [ 179.458004] [<e090ccdb>] ? ccs_manager+0x6b/0x1e0 [ccsecurity]
    [ 179.458004] [<e090d4f7>] ? ccs_write_control+0x77/0x250 [ccsecurity]
    [ 179.458004] [<e090d547>] ? ccs_write_control+0xc7/0x250 [ccsecurity]
    [ 179.458004] [<e090d4f7>] ? ccs_write_control+0x77/0x250 [ccsecurity]
    [ 179.458004] [<e090dec0>] ? ccs_write+0x0/0x10 [ccsecurity]
    [ 179.458004] [<e090dec8>] ? ccs_write+0x8/0x10 [ccsecurity]
    [ 179.458004] [<c050145d>] ? proc_reg_write+0x5d/0x90
    [ 179.458004] [<c04c0256>] ? vfs_write+0x96/0x140
    [ 179.458004] [<c04af2e7>] ? sys_mmap_pgoff+0x77/0x100
    [ 179.458004] [<c0501400>] ? proc_reg_write+0x0/0x90
    [ 179.458004] [<c04c080d>] ? sys_write+0x3d/0x70
    [ 179.458004] [<c0402c50>] ? sysenter_do_call+0x12/0x36
    But there is still a problem. It seems to me that lockdep checks for this bug
    only when a new locking pattern (a locking pattern which was not already added
    to lockdep database) is added. This freeze can be triggered by running

    while :; do newns /sbin/pivot_root /proc/ /proc/sys/; done

    on one terminal and running

    while :; do /bin/ls -l /proc/*/exe; done
    on another terminal. (The "newns" is a program that unshares the mnt namespace
    before execve() using CLONE_NEWNS.) But even after applying the patch above,
    lockdep does not show the trace above.

    lockdep shows the trace above only after I run test programs for TOMOYO (which
    causes a locking pattern that was generated by neither
    "/sbin/pivot_root /proc/ /proc/sys/" nor "/bin/ls -l /proc/*/exe" to be added
    to lockdep database).
    I think that we want some method for rechecking already added locking pattern.
    Maybe it is run by (e.g.) every 60 seconds. Maybe it is run when stall checking
    mechanisms report the possibility of stall. (The sysrq key didn't work after
    the freeze occurred.)

    Also, what to do with __read_seqcount_begin() case? Since seqcount_t does not
    have a spinlock embedded into the struct, we can't use lock primitives...?

    Regards.


    \
     
     \ /
      Last update: 2011-03-26 05:15    [W:0.035 / U:1.432 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site