• Brian Norris's avatar
    mtd: blkdevs: fix potential deadlock + lockdep warnings · 9ea06026
    Brian Norris authored
    commit f3c63795 upstream.
    
    Commit 073db4a5 ("mtd: fix: avoid race condition when accessing
    mtd->usecount") fixed a race condition but due to poor ordering of the
    mutex acquisition, introduced a potential deadlock.
    
    The deadlock can occur, for example, when rmmod'ing the m25p80 module, which
    will delete one or more MTDs, along with any corresponding mtdblock
    devices. This could potentially race with an acquisition of the block
    device as follows.
    
     -> blktrans_open()
        ->  mutex_lock(&dev->lock);
        ->  mutex_lock(&mtd_table_mutex);
    
     -> del_mtd_device()
        ->  mutex_lock(&mtd_table_mutex);
        ->  blktrans_notify_remove() -> del_mtd_blktrans_dev()
           ->  mutex_lock(&dev->lock);
    
    This is a classic (potential) ABBA deadlock, which can be fixed by
    making the A->B ordering consistent everywhere. There was no real
    purpose to the ordering in the original patch, AFAIR, so this shouldn't
    be a problem. This ordering was actually already present in
    del_mtd_blktrans_dev(), for one, where the function tried to ensure that
    its caller already held mtd_table_mutex before it acquired &dev->lock:
    
            if (mutex_trylock(&mtd_table_mutex)) {
                    mutex_unlock(&mtd_table_mutex);
                    BUG();
            }
    
    So, reverse the ordering of acquisition of &dev->lock and &mtd_table_mutex so
    we always acquire mtd_table_mutex first.
    
    Snippets of the lockdep output follow:
    
      # modprobe -r m25p80
      [   53.419251]
      [   53.420838] ======================================================
      [   53.427300] [ INFO: possible circular locking dependency detected ]
      [   53.433865] 4.3.0-rc6 #96 Not tainted
      [   53.437686] -------------------------------------------------------
      [   53.444220] modprobe/372 is trying to acquire lock:
      [   53.449320]  (&new->lock){+.+...}, at: [<c043fe4c>] del_mtd_blktrans_dev+0x80/0xdc
      [   53.457271]
      [   53.457271] but task is already holding lock:
      [   53.463372]  (mtd_table_mutex){+.+.+.}, at: [<c0439994>] del_mtd_device+0x18/0x100
      [   53.471321]
      [   53.471321] which lock already depends on the new lock.
      [   53.471321]
      [   53.479856]
      [   53.479856] the existing dependency chain (in reverse order) is:
      [   53.487660]
      -> #1 (mtd_table_mutex){+.+.+.}:
      [   53.492331]        [<c043fc5c>] blktrans_open+0x34/0x1a4
      [   53.497879]        [<c01afce0>] __blkdev_get+0xc4/0x3b0
      [   53.503364]        [<c01b0bb8>] blkdev_get+0x108/0x320
      [   53.508743]        [<c01713c0>] do_dentry_open+0x218/0x314
      [   53.514496]        [<c0180454>] path_openat+0x4c0/0xf9c
      [   53.519959]        [<c0182044>] do_filp_open+0x5c/0xc0
      [   53.525336]        [<c0172758>] do_sys_open+0xfc/0x1cc
      [   53.530716]        [<c000f740>] ret_fast_syscall+0x0/0x1c
      [   53.536375]
      -> #0 (&new->lock){+.+...}:
      [   53.540587]        [<c063f124>] mutex_lock_nested+0x38/0x3cc
      [   53.546504]        [<c043fe4c>] del_mtd_blktrans_dev+0x80/0xdc
      [   53.552606]        [<c043f164>] blktrans_notify_remove+0x7c/0x84
      [   53.558891]        [<c04399f0>] del_mtd_device+0x74/0x100
      [   53.564544]        [<c043c670>] del_mtd_partitions+0x80/0xc8
      [   53.570451]        [<c0439aa0>] mtd_device_unregister+0x24/0x48
      [   53.576637]        [<c046ce6c>] spi_drv_remove+0x1c/0x34
      [   53.582207]        [<c03de0f0>] __device_release_driver+0x88/0x114
      [   53.588663]        [<c03de19c>] device_release_driver+0x20/0x2c
      [   53.594843]        [<c03dd9e8>] bus_remove_device+0xd8/0x108
      [   53.600748]        [<c03dacc0>] device_del+0x10c/0x210
      [   53.606127]        [<c03dadd0>] device_unregister+0xc/0x20
      [   53.611849]        [<c046d878>] __unregister+0x10/0x20
      [   53.617211]        [<c03da868>] device_for_each_child+0x50/0x7c
      [   53.623387]        [<c046eae8>] spi_unregister_master+0x58/0x8c
      [   53.629578]        [<c03e12f0>] release_nodes+0x15c/0x1c8
      [   53.635223]        [<c03de0f8>] __device_release_driver+0x90/0x114
      [   53.641689]        [<c03de900>] driver_detach+0xb4/0xb8
      [   53.647147]        [<c03ddc78>] bus_remove_driver+0x4c/0xa0
      [   53.652970]        [<c00cab50>] SyS_delete_module+0x11c/0x1e4
      [   53.658976]        [<c000f740>] ret_fast_syscall+0x0/0x1c
      [   53.664621]
      [   53.664621] other info that might help us debug this:
      [   53.664621]
      [   53.672979]  Possible unsafe locking scenario:
      [   53.672979]
      [   53.679169]        CPU0                    CPU1
      [   53.683900]        ----                    ----
      [   53.688633]   lock(mtd_table_mutex);
      [   53.692383]                                lock(&new->lock);
      [   53.698306]                                lock(mtd_table_mutex);
      [   53.704658]   lock(&new->lock);
      [   53.707946]
      [   53.707946]  *** DEADLOCK ***
    
    Fixes: 073db4a5 ("mtd: fix: avoid race condition when accessing mtd->usecount")
    Reported-by: default avatarFelipe Balbi <balbi@ti.com>
    Tested-by: default avatarFelipe Balbi <balbi@ti.com>
    Signed-off-by: default avatarBrian Norris <computersforpeace@gmail.com>
    Signed-off-by: default avatarLuis Henriques <luis.henriques@canonical.com>
    9ea06026
mtd_blkdevs.c 13.3 KB