Circular locking (and possible deadlock), when exiting from mplayer -vo fbdev2

Hello,

When watching movie using mplayer -quiet -vo fbdev2 somefile.flv when
being on console, with 6 virtual terminals, and no other program using
fbdev, after pressing key Q (quit), kernel throws this in my face:


[ 3288.708198] 
[ 3288.711153] ===========================
[ 3288.712015] [ INFO: possible circular locking dependency detected ]
[ 3288.712015] 3.4.0-t43-devel-smp-10118-g233e562-dirty #12 Tainted: G           O
[ 3288.712015] -------------------------------------------------------
[ 3288.712015] mplayer/11769 is trying to acquire lock:
[ 3288.712015]  (&fb_info->lock){+.+.+.}, at: [<c137789c>] fb_release+0x1c/0x60
[ 3288.712015] 
[ 3288.712015] but task is already holding lock:
[ 3288.712015]  (&mm->mmap_sem){++++++}, at: [<c1160a8f>] vm_munmap+0x2f/0x60
[ 3288.712015] 
[ 3288.712015] which lock already depends on the new lock.
[ 3288.712015] 
[ 3288.712015] 
[ 3288.712015] the existing dependency chain (in reverse order) is:
[ 3288.712015] 
[ 3288.712015] -> #2 (&mm->mmap_sem){++++++}:
[ 3288.712015]        [<c10b9719>] lock_acquire+0x89/0x1b0
[ 3288.712015]        [<c11580c7>] might_fault+0x87/0xb0
[ 3288.712015]        [<f8e2c498>] drm_mode_getresources+0x188/0x5b0 [drm]
[ 3288.712015]        [<f8e1e582>] drm_ioctl+0x482/0x590 [drm]
[ 3288.712015]        [<c119ea2b>] do_vfs_ioctl+0x7b/0x5c0
[ 3288.712015]        [<c119efa2>] sys_ioctl+0x32/0x60
[ 3288.712015]        [<c17d5158>] sysenter_do_call+0x12/0x38
[ 3288.712015] 
[ 3288.712015] -> #1 (&dev->mode_config.mutex){+.+.+.}:
[ 3288.712015]        [<c10b9719>] lock_acquire+0x89/0x1b0
[ 3288.712015]        [<c17c9a50>] mutex_lock_nested+0x70/0x300
[ 3288.712015]        [<f8f2fd9c>] drm_fb_helper_set_par+0x3c/0xd0 [drm_kms_helper]
[ 3288.712015]        [<c137757f>] fb_set_var+0x1ef/0x4c0
[ 3288.712015]        [<c137866a>] do_fb_ioctl+0x34a/0x510
[ 3288.712015]        [<c137887b>] fb_ioctl+0x4b/0x60
[ 3288.712015]        [<c119ea2b>] do_vfs_ioctl+0x7b/0x5c0
[ 3288.712015]        [<c119efa2>] sys_ioctl+0x32/0x60
[ 3288.712015]        [<c17d5158>] sysenter_do_call+0x12/0x38
[ 3288.712015] 
[ 3288.712015] -> #0 (&fb_info->lock){+.+.+.}:
[ 3288.712015]        [<c10b8be5>] __lock_acquire+0x1265/0x1760
[ 3288.712015]        [<c10b9719>] lock_acquire+0x89/0x1b0
[ 3288.712015]        [<c17c9a50>] mutex_lock_nested+0x70/0x300
[ 3288.712015]        [<c137789c>] fb_release+0x1c/0x60
[ 3288.712015]        [<c118e5ec>] fput+0xfc/0x230
[ 3288.712015]        [<c115efa3>] remove_vma+0x43/0x70
[ 3288.712015]        [<c116097c>] do_munmap+0x23c/0x320
[ 3288.712015]        [<c1160a9d>] vm_munmap+0x3d/0x60
[ 3288.712015]        [<c116174d>] sys_munmap+0x1d/0x20
[ 3288.712015]        [<c17d5158>] sysenter_do_call+0x12/0x38
[ 3288.712015] 
[ 3288.712015] other info that might help us debug this:
[ 3288.712015] 
[ 3288.712015] Chain exists of:
[ 3288.712015]   &fb_info->lock --> &dev->mode_config.mutex --> &mm->mmap_sem
[ 3288.712015] 
[ 3288.712015]  Possible unsafe locking scenario:
[ 3288.712015] 
[ 3288.712015]        CPU0                    CPU1
[ 3288.712015]        ----                    ----
[ 3288.712015]   lock(&mm->mmap_sem);
[ 3288.712015]                                lock(&dev->mode_config.mutex);
[ 3288.712015]                                lock(&mm->mmap_sem);
[ 3288.712015]   lock(&fb_info->lock);
[ 3288.712015] 
[ 3288.712015]  *** DEADLOCK ***
[ 3288.712015] 
[ 3288.712015] 1 lock held by mplayer/11769:
[ 3288.712015]  #0:  (&mm->mmap_sem){++++++}, at: [<c1160a8f>] vm_munmap+0x2f/0x60
[ 3288.712015] 
[ 3288.712015] stack backtrace:
[ 3288.712015] Pid: 11769, comm: mplayer Tainted: G           O 3.4.0-t43-devel-smp-10118-g233e562-dirty #12
[ 3288.712015] Call Trace:
[ 3288.712015]  [<c17c0c53>] print_circular_bug+0x1af/0x1b9
[ 3288.712015]  [<c10b8be5>] __lock_acquire+0x1265/0x1760
[ 3288.712015]  [<c10b604d>] ? mark_held_locks+0x8d/0xf0
[ 3288.712015]  [<c113c895>] ? free_hot_cold_page+0xe5/0x180
[ 3288.712015]  [<c116b512>] ? free_page_and_swap_cache+0x22/0x50
[ 3288.712015]  [<c10b9719>] lock_acquire+0x89/0x1b0
[ 3288.712015]  [<c137789c>] ? fb_release+0x1c/0x60
[ 3288.712015]  [<c17c9a50>] mutex_lock_nested+0x70/0x300
[ 3288.712015]  [<c137789c>] ? fb_release+0x1c/0x60
[ 3288.712015]  [<c137789c>] ? fb_release+0x1c/0x60
[ 3288.712015]  [<c137789c>] fb_release+0x1c/0x60
[ 3288.712015]  [<c118e5ec>] fput+0xfc/0x230
[ 3288.712015]  [<c115efa3>] remove_vma+0x43/0x70
[ 3288.712015]  [<c116097c>] do_munmap+0x23c/0x320
[ 3288.712015]  [<c1160a9d>] vm_munmap+0x3d/0x60
[ 3288.712015]  [<c116174d>] sys_munmap+0x1d/0x20
[ 3288.712015]  [<c17d5158>] sysenter_do_call+0x12/0x38

There was Xorg server running on tty7 (but nobody logged in), and it is
using open source radeon driver.

I sometimes use fbterm on console (to have full UTF-8, better speed, or
just larger fonts changed dynamically), and it works without any similar problems.

This possible deadlock message shows up only on first mplayer start/stop
cycle. Next ones doesn't trigger displaying this warning, but I belive
this is only due lockdep disabling checking, not solving any problem.

I reproduced it on two kernel builds already.

Machine is single CPU, but kernel is SMP. (Compiling and running kernel without SMP
support gives exactly same dmesg INFO about deadlock when quiting
mplayer).


$ egrep 'SMP|LOCK|RCU' /boot/config-`uname -r`
CONFIG_LOCKDEP_SUPPORT=y
# CONFIG_RWSEM_GENERIC_SPINLOCK is not set
CONFIG_X86_32_SMP=y
CONFIG_CLOCKSOURCE_WATCHDOG=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC_CLOCKEVENTS_BUILD=y
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y
CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=y
# RCU Subsystem
CONFIG_TREE_RCU=y
# CONFIG_PREEMPT_RCU is not set
CONFIG_RCU_FANOUT2
CONFIG_RCU_FANOUT_LEAF
# CONFIG_RCU_FANOUT_EXACT is not set
# CONFIG_RCU_FAST_NO_HZ is not set
CONFIG_TREE_RCU_TRACE=y
CONFIG_HAVE_UNSTABLE_SCHED_CLOCK=y
CONFIG_USE_GENERIC_SMP_HELPERS=y
CONFIG_GENERIC_SMP_IDLE_THREAD=y
# CONFIG_INLINE_SPIN_TRYLOCK is not set
# CONFIG_INLINE_SPIN_TRYLOCK_BH is not set
# CONFIG_INLINE_SPIN_LOCK is not set
# CONFIG_INLINE_SPIN_LOCK_BH is not set
# CONFIG_INLINE_SPIN_LOCK_IRQ is not set
# CONFIG_INLINE_SPIN_LOCK_IRQSAVE is not set
CONFIG_UNINLINE_SPIN_UNLOCK=y
# CONFIG_INLINE_SPIN_UNLOCK_BH is not set
# CONFIG_INLINE_SPIN_UNLOCK_IRQ is not set
# CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE is not set
# CONFIG_INLINE_READ_TRYLOCK is not set
# CONFIG_INLINE_READ_LOCK is not set
# CONFIG_INLINE_READ_LOCK_BH is not set
# CONFIG_INLINE_READ_LOCK_IRQ is not set
# CONFIG_INLINE_READ_LOCK_IRQSAVE is not set
# CONFIG_INLINE_READ_UNLOCK is not set
# CONFIG_INLINE_READ_UNLOCK_BH is not set
# CONFIG_INLINE_READ_UNLOCK_IRQ is not set
# CONFIG_INLINE_READ_UNLOCK_IRQRESTORE is not set
# CONFIG_INLINE_WRITE_TRYLOCK is not set
# CONFIG_INLINE_WRITE_LOCK is not set
# CONFIG_INLINE_WRITE_LOCK_BH is not set
# CONFIG_INLINE_WRITE_LOCK_IRQ is not set
# CONFIG_INLINE_WRITE_LOCK_IRQSAVE is not set
# CONFIG_INLINE_WRITE_UNLOCK is not set
# CONFIG_INLINE_WRITE_UNLOCK_BH is not set
# CONFIG_INLINE_WRITE_UNLOCK_IRQ is not set
# CONFIG_INLINE_WRITE_UNLOCK_IRQRESTORE is not set
CONFIG_SMP=y
# CONFIG_X86_BIGSMP is not set
# CONFIG_KVM_CLOCK is not set
CONFIG_PARAVIRT_SPINLOCKS=y
CONFIG_PARAVIRT_CLOCK=y
CONFIG_HAVE_MEMBLOCK=y
CONFIG_HAVE_MEMBLOCK_NODE_MAP=y
CONFIG_ARCH_DISCARD_MEMBLOCK=y
CONFIG_SPLIT_PTLOCK_CPUS™9999
CONFIG_PM_SLEEP_SMP=y
CONFIG_PM_WAKELOCKS=y
CONFIG_PM_WAKELOCKS_LIMIT0
CONFIG_PM_WAKELOCKS_GC=y
# CONFIG_X86_P4_CLOCKMOD is not set
CONFIG_HAVE_TEXT_POKE_SMP=y
CONFIG_DM_DEBUG_BLOCK_STACK_TRACING=y
CONFIG_TCM_IBLOCK=m
# CONFIG_TELCLOCK is not set
CONFIG_I8253_LOCK=y
CONFIG_FILE_LOCKING=y
CONFIG_LOCKD=y
CONFIG_LOCKD_V4=y
CONFIG_LOCKUP_DETECTOR=y
CONFIG_HARDLOCKUP_DETECTOR=y
CONFIG_BOOTPARAM_HARDLOCKUP_PANIC=y
CONFIG_BOOTPARAM_HARDLOCKUP_PANIC_VALUE=1
CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC=y
CONFIG_BOOTPARAM_SOFTLOCKUP_PANIC_VALUE=1
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_PROVE_RCU=y
CONFIG_PROVE_RCU_REPEATEDLY=y
CONFIG_SPARSE_RCU_POINTER=y
CONFIG_LOCKDEP=y
CONFIG_LOCK_STAT=y
CONFIG_DEBUG_LOCKDEP=y
CONFIG_DEBUG_LOCKING_API_SELFTESTS=y
CONFIG_RCU_TORTURE_TEST=m
CONFIG_RCU_CPU_STALL_TIMEOUT`
CONFIG_RCU_CPU_STALL_INFO=y
CONFIG_RCU_TRACE=y
CONFIG_DEBUG_BLOCK_EXT_DEVT=y
CONFIG_XOR_BLOCKS=m





Commit g233e562 (a kernel which I was running - Linus' main tree) is
after

commit 804ce9866d56130032c9c8afc90a1297b7deed56
Merge: f5e7e84 c895305
Author: Linus Torvalds [off-list ref]
Date:   Fri Jun 1 16:57:51 2012 -0700

    Merge tag 'fbdev-updates-for-3.5' of git://github.com/schandinat/linu
    
    Pull fbdev updates from Florian Tobias Schandinat:
     - driver for AUO-K1900 and AUO-K1901 epaper controller
     - large updates for OMAP (e.g. decouple HDMI audio and video)
     - some updates for Exynos and SH Mobile
     - various other small fixes and cleanups
    
    * tag 'fbdev-updates-for-3.5' of git://github.com/schandinat/linux-2.
      ...
      fb: handle NULL pointers in framebuffer release
      ...

....

commit cc4401142c1cbc63b01d6024cbc7a9f804cb3143
Author: Dan Carpenter [off-list ref]
Date:   Mon May 14 23:58:37 2012 +0300

    fb: handle NULL pointers in framebuffer release
    
    This function is called with a potential NULL pointer in
    picolcd_init_framebuffer() and it causes a static checker warning.  T
    used to handle NULL pointers when the picolcd code was written, but a
    couple months later we added the "info->apertures" dereference.
    
    Signed-off-by: Dan Carpenter [off-list ref]
    Reviewed-by: Marcin Slusarz [off-list ref]
    Signed-off-by: Florian Tobias Schandinat [off-list ref]



so this may be a regression, should I try bissecting, or at least
reverting commit cc4401142c1c ?


Regards,
Witek

-- 
Witold Baryluk

My patch can't cause this.  I'd be surprised if my patch has any
effect beyond silencing a static checker warning, honestly.

I'm not sure this is a new bug.

The problem is we hold ->mmap_sem when we call fb_release() which
takes the info->lock.

We take the info->lock in do_fb_ioctl() before we call fb_set_var()
which calls drm_fb_helper_set_par() which takes the
mode_config.mutex.

In drm_mode_getresources() we take the mode_config.mutex() and call
put_user() which takes the ->mmap_sem.

So on one CPU we are holding the ->mmap_sem and want the info->lock.
On another CPU we are holding the info->lock and want the
config.mutex.  On the other CPU we hold the config.mutex and want
the ->mmap_sem.

Deadlock.

I'm not sure how to make this work with just two CPUs...  Or how to
fix it.  But I'm going to disclaim all responsibility and hope the
fbdev people can take a look.  ;)

regards,
dan carpenter

On 06-05 17:10, Dan Carpenter wrote:

My patch can't cause this.  I'd be surprised if my patch has any
effect beyond silencing a static checker warning, honestly.

Yes, you are right. Your patch cannot be cause of it. Sorry for
confusion.

I'm not sure this is a new bug.

Indeed, last time I used mplayer -vo fbdev2 was years ago, so bug could
appear anywhere in this period, or it is a more a matter of my
configuration.

The problem is we hold ->mmap_sem when we call fb_release() which
takes the info->lock.

We take the info->lock in do_fb_ioctl() before we call fb_set_var()
which calls drm_fb_helper_set_par() which takes the
mode_config.mutex.

In drm_mode_getresources() we take the mode_config.mutex() and call
put_user() which takes the ->mmap_sem.

So on one CPU we are holding the ->mmap_sem and want the info->lock.
On another CPU we are holding the info->lock and want the
config.mutex.  On the other CPU we hold the config.mutex and want
the ->mmap_sem.

Deadlock.

Temporary solution would be to break a cycle, and for example take
info->lock first on first processor before taking ->mmap_sem and calling
fb_release - it is just fb release, so performance doesn't matter so
much. Other possibility is to protect this code paths by separate common
lock, but it will increase memory and make it slower probably in this
function and other ones. I do not know which subsystems use
drm_mode_getresources, and if this is important performance wise.

There is probably other possibilities, but only fbdev people can tell
which is best.

I'm not sure how to make this work with just two CPUs...  Or how to
fix it.  But I'm going to disclaim all responsibility and hope the
fbdev people can take a look.  ;)

regards,
dan carpenter

thanks for analysis,

Regards,
Witek

-- 
Witold Baryluk

Hi,

On 06/05/2012 05:18 PM, Witold Baryluk wrote:

On 06-05 17:10, Dan Carpenter wrote:

quoted

My patch can't cause this.  I'd be surprised if my patch has any
effect beyond silencing a static checker warning, honestly.

Yes, you are right. Your patch cannot be cause of it. Sorry for
confusion.

quoted

I'm not sure this is a new bug.

Indeed, last time I used mplayer -vo fbdev2 was years ago, so bug could
appear anywhere in this period, or it is a more a matter of my
configuration.

quoted

The problem is we hold ->mmap_sem when we call fb_release() which
takes the info->lock.

We take the info->lock in do_fb_ioctl() before we call fb_set_var()
which calls drm_fb_helper_set_par() which takes the
mode_config.mutex.

In drm_mode_getresources() we take the mode_config.mutex() and call
put_user() which takes the ->mmap_sem.

So on one CPU we are holding the ->mmap_sem and want the info->lock.
On another CPU we are holding the info->lock and want the
config.mutex.  On the other CPU we hold the config.mutex and want
the ->mmap_sem.

Deadlock.

Temporary solution would be to break a cycle, and for example take
info->lock first on first processor before taking ->mmap_sem and calling
fb_release - it is just fb release, so performance doesn't matter so
much. Other possibility is to protect this code paths by separate common
lock, but it will increase memory and make it slower probably in this
function and other ones. I do not know which subsystems use
drm_mode_getresources, and if this is important performance wise.

There is probably other possibilities, but only fbdev people can tell
which is best.

That's not true. This is the wrong mailing list as this happens only
with DRM-based framebuffers (in drivers/gpu/drm/ not drivers/video/) and
therefore you should resend your initial mail to
dri-devel@lists.freedesktop.org.


Best regards,

Florian Tobias Schandinat

quoted

I'm not sure how to make this work with just two CPUs...  Or how to
fix it.  But I'm going to disclaim all responsibility and hope the
fbdev people can take a look.  ;)

regards,
dan carpenter

thanks for analysis,

Regards,
Witek