On Tue, Apr 21, 2015 at 12:20:22PM +0200, Giuseppe Cantavenera wrote: > 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: [<ffffffff80401818>] __put_mtd_device+0x20/0x50 > kernel: [<ffffffff804086f4>] blktrans_release+0x8c/0xd8 > kernel: [<ffffffff802577e0>] __blkdev_put+0x1a8/0x200 > kernel: [<ffffffff802579a4>] blkdev_close+0x1c/0x30 > kernel: [<ffffffff8022006c>] __fput+0xac/0x250 > kernel: [<ffffffff80171208>] task_work_run+0xd8/0x120 > kernel: [<ffffffff8012c23c>] 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 by using the protected > version of put_mtd_device in blktrans_release and del_mtd_blktrans_dev FYI, I think there's a similar report here: http://patchwork.ozlabs.org/patch/415125/ But the patch had other flaws. I think you're more along the right track here. > Cc: stable@xxxxxxxxxxxxxxx > Cc: alexander.sverdlin@xxxxxxxxx > Signed-off-by: Giuseppe Cantavenera <giuseppe.cantavenera.ext@xxxxxxxxx> > Acked-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx> > --- > drivers/mtd/mtd_blkdevs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > index 2b0c5287..3a9f138 100644 > --- a/drivers/mtd/mtd_blkdevs.c > +++ b/drivers/mtd/mtd_blkdevs.c > @@ -213,7 +213,9 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) > goto error_put; > } > > + mutex_lock(&mtd_table_mutex); > ret = __get_mtd_device(dev->mtd); > + mutex_unlock(&mtd_table_mutex); I think this is a start, but it's not enough. See below [*]. > if (ret) > goto error_release; > dev->file_mode = mode; > @@ -253,7 +255,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode) > if (dev->mtd) { > if (dev->tr->release) > dev->tr->release(dev); > - __put_mtd_device(dev->mtd); > + put_mtd_device(dev->mtd); Same, see [*]. > } > unlock: > mutex_unlock(&dev->lock); > @@ -484,7 +486,7 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > if (old->open) { > if (old->tr->release) > old->tr->release(old); > - __put_mtd_device(old->mtd); > + put_mtd_device(old->mtd); This looks wrong. See: int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr) { struct mtd_blktrans_dev *dev, *next; mutex_lock(&mtd_table_mutex); /* Remove it from the list of active majors */ list_del(&tr->list); list_for_each_entry_safe(dev, next, &tr->devs, list) tr->remove_dev(dev); unregister_blkdev(tr->major, tr->name); mutex_unlock(&mtd_table_mutex); BUG_ON(!list_empty(&tr->devs)); return 0; } Now, many mtd_blkdev implementations call del_mtd_blktrans_dev() in their tr->remove_dev() functions (e.g., see inftl_remove_dev()). This means you will have: deregister_mtd_blktrans() |_ mutex_lock(&mtd_table_mutex) |_ tr->remove_dev() -> inftl_remove_dev() |_ del_mtd_blktrans_dev() |_ put_mtd_device() |_ mutex_lock(&mtd_table_mutex) <--- AA deadlock > } > > old->mtd = NULL; Note that I haven't actually tested this, so you may have introduced other bad locking issues too. I'd expect a little more rigor out of a patch marked for -stable, especially. To that end, it looks like maybe we could use some better comments to describe the expected locking. Patches are welcome, if you learn some things in trying to verify your patch! Speaking of that... [*] I poked through the locking a bit more, and I see that such a comment exists in 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); ... }; However, that is currently *not* the case. See blktrans_open() and blktrans_release(), which you are modifying. They are calling tr->open() and tr->release() *without* holding mtd_table_mutex. So the correct fix might be to hold the table mutex for those entire functions. Brian -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html