• Hans Verkuil's avatar
    [media] videobuf2: fix lockdep warning · f035eb4e
    Hans Verkuil authored
    The following lockdep warning has been there ever since commit a517cca6
    one year ago:
    
    [  403.117947] ======================================================
    [  403.117949] [ INFO: possible circular locking dependency detected ]
    [  403.117953] 3.16.0-rc6-test-media #961 Not tainted
    [  403.117954] -------------------------------------------------------
    [  403.117956] v4l2-ctl/15377 is trying to acquire lock:
    [  403.117959]  (&dev->mutex#3){+.+.+.}, at: [<ffffffffa005a6c3>] vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.117974]
    [  403.117974] but task is already holding lock:
    [  403.117976]  (&mm->mmap_sem){++++++}, at: [<ffffffff8118291f>] vm_mmap_pgoff+0x6f/0xc0
    [  403.117987]
    [  403.117987] which lock already depends on the new lock.
    [  403.117987]
    [  403.117990]
    [  403.117990] the existing dependency chain (in reverse order) is:
    [  403.117992]
    [  403.117992] -> #1 (&mm->mmap_sem){++++++}:
    [  403.117997]        [<ffffffff810d733c>] validate_chain.isra.39+0x5fc/0x9a0
    [  403.118006]        [<ffffffff810d8bc3>] __lock_acquire+0x4d3/0xd30
    [  403.118010]        [<ffffffff810d9da7>] lock_acquire+0xa7/0x160
    [  403.118014]        [<ffffffff8118c9ec>] might_fault+0x7c/0xb0
    [  403.118018]        [<ffffffffa0028a25>] video_usercopy+0x425/0x610 [videodev]
    [  403.118028]        [<ffffffffa0028c25>] video_ioctl2+0x15/0x20 [videodev]
    [  403.118034]        [<ffffffffa0022764>] v4l2_ioctl+0x184/0x1a0 [videodev]
    [  403.118040]        [<ffffffff811d77d0>] do_vfs_ioctl+0x2f0/0x4f0
    [  403.118307]        [<ffffffff811d7a51>] SyS_ioctl+0x81/0xa0
    [  403.118311]        [<ffffffff8199dc69>] system_call_fastpath+0x16/0x1b
    [  403.118319]
    [  403.118319] -> #0 (&dev->mutex#3){+.+.+.}:
    [  403.118324]        [<ffffffff810d6a96>] check_prevs_add+0x746/0x9f0
    [  403.118329]        [<ffffffff810d733c>] validate_chain.isra.39+0x5fc/0x9a0
    [  403.118333]        [<ffffffff810d8bc3>] __lock_acquire+0x4d3/0xd30
    [  403.118336]        [<ffffffff810d9da7>] lock_acquire+0xa7/0x160
    [  403.118340]        [<ffffffff81999664>] mutex_lock_interruptible_nested+0x64/0x640
    [  403.118344]        [<ffffffffa005a6c3>] vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.118349]        [<ffffffffa0022122>] v4l2_mmap+0x62/0xa0 [videodev]
    [  403.118354]        [<ffffffff81197270>] mmap_region+0x3d0/0x5d0
    [  403.118359]        [<ffffffff8119778d>] do_mmap_pgoff+0x31d/0x400
    [  403.118363]        [<ffffffff81182940>] vm_mmap_pgoff+0x90/0xc0
    [  403.118366]        [<ffffffff81195cef>] SyS_mmap_pgoff+0x1df/0x2a0
    [  403.118369]        [<ffffffff810085c2>] SyS_mmap+0x22/0x30
    [  403.118376]        [<ffffffff8199dc69>] system_call_fastpath+0x16/0x1b
    [  403.118381]
    [  403.118381] other info that might help us debug this:
    [  403.118381]
    [  403.118383]  Possible unsafe locking scenario:
    [  403.118383]
    [  403.118385]        CPU0                    CPU1
    [  403.118387]        ----                    ----
    [  403.118388]   lock(&mm->mmap_sem);
    [  403.118391]                                lock(&dev->mutex#3);
    [  403.118394]                                lock(&mm->mmap_sem);
    [  403.118397]   lock(&dev->mutex#3);
    [  403.118400]
    [  403.118400]  *** DEADLOCK ***
    [  403.118400]
    [  403.118403] 1 lock held by v4l2-ctl/15377:
    [  403.118405]  #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff8118291f>] vm_mmap_pgoff+0x6f/0xc0
    [  403.118411]
    [  403.118411] stack backtrace:
    [  403.118415] CPU: 0 PID: 15377 Comm: v4l2-ctl Not tainted 3.16.0-rc6-test-media #961
    [  403.118418] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
    [  403.118420]  ffffffff82a6c9d0 ffff8800af37fb00 ffffffff819916a2 ffffffff82a6c9d0
    [  403.118425]  ffff8800af37fb40 ffffffff810d5715 ffff8802308e4200 0000000000000000
    [  403.118429]  ffff8802308e4a48 ffff8802308e4a48 ffff8802308e4200 0000000000000001
    [  403.118433] Call Trace:
    [  403.118441]  [<ffffffff819916a2>] dump_stack+0x4e/0x7a
    [  403.118445]  [<ffffffff810d5715>] print_circular_bug+0x1d5/0x2a0
    [  403.118449]  [<ffffffff810d6a96>] check_prevs_add+0x746/0x9f0
    [  403.118455]  [<ffffffff8119c172>] ? find_vmap_area+0x42/0x70
    [  403.118459]  [<ffffffff810d733c>] validate_chain.isra.39+0x5fc/0x9a0
    [  403.118463]  [<ffffffff810d8bc3>] __lock_acquire+0x4d3/0xd30
    [  403.118468]  [<ffffffff810d9da7>] lock_acquire+0xa7/0x160
    [  403.118472]  [<ffffffffa005a6c3>] ? vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.118476]  [<ffffffffa005a6c3>] ? vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.118480]  [<ffffffff81999664>] mutex_lock_interruptible_nested+0x64/0x640
    [  403.118484]  [<ffffffffa005a6c3>] ? vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.118488]  [<ffffffffa005a6c3>] ? vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.118493]  [<ffffffff810d8055>] ? mark_held_locks+0x75/0xa0
    [  403.118497]  [<ffffffffa005a6c3>] vb2_fop_mmap+0x33/0x90 [videobuf2_core]
    [  403.118502]  [<ffffffffa0022122>] v4l2_mmap+0x62/0xa0 [videodev]
    [  403.118506]  [<ffffffff81197270>] mmap_region+0x3d0/0x5d0
    [  403.118510]  [<ffffffff8119778d>] do_mmap_pgoff+0x31d/0x400
    [  403.118513]  [<ffffffff81182940>] vm_mmap_pgoff+0x90/0xc0
    [  403.118517]  [<ffffffff81195cef>] SyS_mmap_pgoff+0x1df/0x2a0
    [  403.118521]  [<ffffffff810085c2>] SyS_mmap+0x22/0x30
    [  403.118525]  [<ffffffff8199dc69>] system_call_fastpath+0x16/0x1b
    
    The reason is that vb2_fop_mmap and vb2_fop_get_unmapped_area take the core lock
    while they are called with the mmap_sem semaphore held. But elsewhere in the code
    the core lock is taken first but calls to copy_to/from_user() can take the mmap_sem
    semaphore as well, potentially causing a classical A-B/B-A deadlock.
    
    However, the mmap/get_unmapped_area calls really shouldn't take the core lock
    at all. So what would happen if they don't take the core lock anymore?
    
    There are two situations that need to be taken into account: calling mmap while
    new buffers are being added and calling mmap while buffers are being deleted.
    
    The first case works almost fine without a lock: in all cases mmap relies on
    correctly filled-in q->num_buffers/q->num_planes values and those are only
    updated by reqbufs and create_buffers *after* any new buffers have been
    initialized completely. Except in one case: if an error occurred while allocating
    the buffers it will increase num_buffers and rely on __vb2_queue_free to
    decrease it again. So there is a short period where the buffer information
    may be wrong.
    
    The second case definitely does pose a problem: buffers may be in the process
    of being deleted, without the internal structure being updated.
    
    In order to fix this a new mutex is added to vb2_queue that is taken when
    buffers are allocated or deleted, and in vb2_mmap. That way vb2_mmap won't
    get stale buffer data. Note that this is a problem only for MEMORY_MMAP, so
    even though __qbuf_userptr and __qbuf_dmabuf also mess around with buffers
    (mem_priv in particular), this doesn't clash with vb2_mmap or
    vb2_get_unmapped_area since those are MMAP specific.
    
    As an additional bonus the hack in __buf_prepare, the USERPTR case, can be
    removed as well since mmap() no longer takes the core lock.
    
    All in all a much cleaner solution.
    Signed-off-by: 's avatarHans Verkuil <hans.verkuil@cisco.com>
    Acked-by: 's avatarLaurent Pinchart <laurent.pinchart@ideasonboard.com>
    Acked-by: 's avatarMarek Szyprowski <m.szyprowski@samsung.com>
    Signed-off-by: 's avatarMauro Carvalho Chehab <m.chehab@samsung.com>
    f035eb4e
videobuf2-core.h 23.9 KB