linux-stable.git/drivers/mtd/mtd_blkdevs.c, branch linux-3.12.y

mtd: blkdevs: fix potential deadlock + lockdep warnings

2016-11-28T21:22:56+00:00

commit f3c63795e90f0c6238306883b6c72f14d5355721 upstream.

Commit 073db4a51ee4 ("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: [] del_mtd_blktrans_dev+0x80/0xdc
  [   53.457271]
  [   53.457271] but task is already holding lock:
  [   53.463372]  (mtd_table_mutex){+.+.+.}, at: [] 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]        [] blktrans_open+0x34/0x1a4
  [   53.497879]        [] __blkdev_get+0xc4/0x3b0
  [   53.503364]        [] blkdev_get+0x108/0x320
  [   53.508743]        [] do_dentry_open+0x218/0x314
  [   53.514496]        [] path_openat+0x4c0/0xf9c
  [   53.519959]        [] do_filp_open+0x5c/0xc0
  [   53.525336]        [] do_sys_open+0xfc/0x1cc
  [   53.530716]        [] ret_fast_syscall+0x0/0x1c
  [   53.536375]
  -> #0 (&new->lock){+.+...}:
  [   53.540587]        [] mutex_lock_nested+0x38/0x3cc
  [   53.546504]        [] del_mtd_blktrans_dev+0x80/0xdc
  [   53.552606]        [] blktrans_notify_remove+0x7c/0x84
  [   53.558891]        [] del_mtd_device+0x74/0x100
  [   53.564544]        [] del_mtd_partitions+0x80/0xc8
  [   53.570451]        [] mtd_device_unregister+0x24/0x48
  [   53.576637]        [] spi_drv_remove+0x1c/0x34
  [   53.582207]        [] __device_release_driver+0x88/0x114
  [   53.588663]        [] device_release_driver+0x20/0x2c
  [   53.594843]        [] bus_remove_device+0xd8/0x108
  [   53.600748]        [] device_del+0x10c/0x210
  [   53.606127]        [] device_unregister+0xc/0x20
  [   53.611849]        [] __unregister+0x10/0x20
  [   53.617211]        [] device_for_each_child+0x50/0x7c
  [   53.623387]        [] spi_unregister_master+0x58/0x8c
  [   53.629578]        [] release_nodes+0x15c/0x1c8
  [   53.635223]        [] __device_release_driver+0x90/0x114
  [   53.641689]        [] driver_detach+0xb4/0xb8
  [   53.647147]        [] bus_remove_driver+0x4c/0xa0
  [   53.652970]        [] SyS_delete_module+0x11c/0x1e4
  [   53.658976]        [] 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: 073db4a51ee4 ("mtd: fix: avoid race condition when accessing mtd->usecount")
Reported-by: Felipe Balbi 
Tested-by: Felipe Balbi 
Signed-off-by: Brian Norris 
Signed-off-by: Jiri Slaby

mtd: fix: avoid race condition when accessing mtd->usecount

2015-07-30T12:10:52+00:00

commit 073db4a51ee43ccb827f54a4261c0583b028d5ab upstream.

On A MIPS 32-cores machine a BUG_ON was triggered because some acesses to
mtd->usecount were done without taking mtd_table_mutex.
kernel: Call Trace:
kernel: [] __put_mtd_device+0x20/0x50
kernel: [] blktrans_release+0x8c/0xd8
kernel: [] __blkdev_put+0x1a8/0x200
kernel: [] blkdev_close+0x1c/0x30
kernel: [] __fput+0xac/0x250
kernel: [] task_work_run+0xd8/0x120
kernel: [] work_notifysig+0x10/0x18
kernel:
kernel:
        Code: 2442ffff  ac8202d8  000217fe <00020336> dc820128  10400003
               00000000  0040f809  00000000
kernel: ---[ end trace 080fbb4579b47a73 ]---

Fixed by taking the mutex in blktrans_open and blktrans_release.

Note that this locking is already suggested in
include/linux/mtd/blktrans.h:

struct mtd_blktrans_ops {
...
	/* Called with mtd_table_mutex held; no race with add/remove */
	int (*open)(struct mtd_blktrans_dev *dev);
	void (*release)(struct mtd_blktrans_dev *dev);
...
};

But we weren't following it.

Originally reported by (and patched by) Zhang and Giuseppe,
independently. Improved and rewritten.

Reported-by: Zhang Xingcai 
Reported-by: Giuseppe Cantavenera 
Tested-by: Giuseppe Cantavenera 
Acked-by: Alexander Sverdlin 
Signed-off-by: Brian Norris 
Signed-off-by: Jiri Slaby

block_device_operations->release() should return void

2013-05-07T06:16:21+00:00

The value passed is 0 in all but "it can never happen" cases (and those
only in a couple of drivers) *and* it would've been lost on the way
out anyway, even if something tried to pass something meaningful.
Just don't bother.

Signed-off-by: Al Viro

mtd_blktrans_ops->release() should return void

2013-05-06T01:31:22+00:00

Both existing instances always return 0 and even if they didn't,
the value would be lost on the way out.  Just don't bother...

Signed-off-by: Al Viro

mtd: mtd_blkdevs: Replace request handler kthread with a workqueue

2012-11-21T15:02:23+00:00

By replacing a kthread with a workqueue, the code is now a bit clearer.
There's also a slight reduction of code size (numbers apply for x86):
Before:
   text	   data	    bss	    dec	    hex	filename
   3248	     36	      0	   3284	    cd4	drivers/mtd/mtd_blkdevs.o

After:
   text	   data	    bss	    dec	    hex	filename
   3150	     36	      0	   3186	    c72	drivers/mtd/mtd_blkdevs.o

Due to lack of real hardware, tests have been performed on an emulated
environment with mtdswap and mtdblock over nandsim devices.
Some real testing should be done, before merging this patch.

Signed-off-by: Ezequiel Garcia 
Signed-off-by: Artem Bityutskiy

mtd: mtd_blkdev: Use a different name for block_device_operations variable

2012-11-21T15:02:05+00:00

struct mtd_blktrans_ops is a type, and mtd_blktrans_ops is a variable.
To improve code clarity it's better to not use the same names,
so we just change the latter.

Signed-off-by: Ezequiel Garcia 
Signed-off-by: Artem Bityutskiy

mtd: mtdblock: call mtd_sync() only if opened for write

2012-03-26T23:11:11+00:00

Because it is useless to call it if the device is opened in R/O mode, and also
harmful: on CFI NOR flash it may block for long time waiting for erase
operations to complete is another partition with a R/W file-system on this
chip.

Artem Bityutskiy: write commit message, amend the patch to match the latest
tree (we use mtd_sync(), not mtd->sync() nowadays).

Signed-off-by: Alexander Stein 
Signed-off-by: Artem Bityutskiy 
Signed-off-by: David Woodhouse

mtd: mtd_blkdevs: don't increase 'open' count on error path

2012-01-09T18:04:01+00:00

Some error paths in mtd_blkdevs were fixed in the following commit:

    commit 94735ec4044a6d318b83ad3c5794e931ed168d10
    mtd: mtd_blkdevs: fix error path in blktrans_open

But on these error paths, the block device's `dev->open' count is
already incremented before we check for errors. This meant that, while
the error path was handled correctly on the first time through
blktrans_open(), the device is erroneously considered already open on
the second time through.

This problem can be seen, for instance, when a UBI volume is
simultaneously mounted as a UBIFS partition and read through its
corresponding gluebi mtdblockX device. This results in blktrans_open()
passing its error checks (with `dev->open > 0') without actually having
a handle on the device. Here's a summarized log of the actions and
results with nandsim:

    # modprobe nandsim
    # modprobe mtdblock
    # modprobe gluebi
    # modprobe ubifs
    # ubiattach /dev/ubi_ctrl -m 0
    ...
    # ubimkvol /dev/ubi0 -N test -s 16MiB
    ...
    # mount -t ubifs ubi0:test /mnt
    # ls /dev/mtdblock*
    /dev/mtdblock0  /dev/mtdblock1
    # cat /dev/mtdblock1 > /dev/null
    cat: can't open '/dev/mtdblock4': Device or resource busy
    # cat /dev/mtdblock1 > /dev/null

    CPU 0 Unable to handle kernel paging request at virtual address
    fffffff0, epc == 8031536c, ra == 8031f280
    Oops[#1]:
    ...
    Call Trace:
    [<8031536c>] ubi_leb_read+0x14/0x164
    [<8031f280>] gluebi_read+0xf0/0x148
    [<802edba8>] mtdblock_readsect+0x64/0x198
    [<802ecfe4>] mtd_blktrans_thread+0x330/0x3f4
    [<8005be98>] kthread+0x88/0x90
    [<8000bc04>] kernel_thread_helper+0x10/0x18

Cc: stable@kernel.org [3.0+]
Signed-off-by: Brian Norris 
Signed-off-by: Artem Bityutskiy 
Signed-off-by: David Woodhouse

mtd: mark block device queue as non-rotational

2011-10-14T08:22:48+00:00

This is similar to what the nbd driver does, among others.

Signed-off-by: Dan McGee 
Signed-off-by: Artem Bityutskiy

mtd: mtd_blkdevs: fix error path in blktrans_open

2011-05-25T00:53:45+00:00

The 'blktrans_open()' does not handle possible '__get_mtd_device()' failures
because it does not check the error code. Moreover, the 'dev->tr->open()'
failures are not handled correctly because in this case the function just
goes ahead and gets the mtd device, then returns an error. But Instead, it
should _not_ try to get the mtd device, then it should put back the module
and the kref.

This patch fixes the issue. Note, I only compile-tested it. This patch was
inspired by a bug report about a similar issue in 2.6.34 kernels
sent by Mike Turner  to the MTD mailing list:

http://lists.infradead.org/pipermail/linux-mtd/2011-April/034980.html

Signed-off-by: Artem Bityutskiy 
Signed-off-by: David Woodhouse