[PATCH] mtd: blkdevs: protect tr->devs list by mtd_table_mutex

[Hou Tao <houtao1@xxxxxxxxxx>]

There may be list corruption if there are concurrent list traversal
and list deletion on tr->devs as showed in the following case:

CPU 0                               CPU 1

open /dev/mtdblock1

// remove mtd1
blktrans_notify_remove()
    del_mtd_blktrans_dev()

close /dev/mtdblock1
  blktrans_release
    blktrans_dev_put
      acquire blktrans_ref_mutex     // remove mtd0
      // the final release           acquire mtd_table_mutex
      blktrans_dev_release()         blktrans_notify_remove()
        // remove mtdblock1            // next is mtdblock1
        list_del(&dev->list)           list_for_each_entry_safe()

We could fix the problem by acquiring blktrans_ref_mutex during
the traversal of tr->devs, but blktrans_ref_mutex needs to be released
before invoking tr->remote_dev(), so we also need to increase the kref
of current device else the device may be freed and decrease the kref
after the removal.

Or we could move the list deletion to del_mtd_blktrans_dev(), and protect
the operations on tr->devs by mtd_table_mutex which has already be taken.

The latter fix is simpler. We also can remove the unnecessary acquisitions
of blktrans_ref_mutex in add_mtd_blktrans_dev() because operations on
tr->devs have already been protected by mtd_table_mutex.

Fixes: 048d87199566 ("mtd: blktrans: Hotplug fixes")
Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
---
I found the problem by code review, and could not find a way to
ensure the problem, because the removal of mtd devices almost
comes from the removal of modules, and the open of /dev/mtdblockX
will prevent the module from removing.
---
 drivers/mtd/mtd_blkdevs.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 0c05f77f9b21..e04260237a25 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -35,7 +35,6 @@ static void blktrans_dev_release(struct kref *kref)
 	blk_mq_free_tag_set(dev->tag_set);
 	kfree(dev->tag_set);
 	put_disk(dev->disk);
-	list_del(&dev->list);
 	kfree(dev);
 }
 
@@ -350,7 +349,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 		BUG();
 	}
 
-	mutex_lock(&blktrans_ref_mutex);
 	list_for_each_entry(d, &tr->devs, list) {
 		if (new->devnum == -1) {
 			/* Use first free number */
@@ -362,7 +360,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 			}
 		} else if (d->devnum == new->devnum) {
 			/* Required number taken */
-			mutex_unlock(&blktrans_ref_mutex);
 			return -EBUSY;
 		} else if (d->devnum > new->devnum) {
 			/* Required number was free */
@@ -381,14 +378,12 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	 * with this number. */
 	if (new->devnum > (MINORMASK >> tr->part_bits) ||
 	    (tr->part_bits && new->devnum >= 27 * 26)) {
-		mutex_unlock(&blktrans_ref_mutex);
 		goto error1;
 	}
 
 	list_add_tail(&new->list, &tr->devs);
- added:
-	mutex_unlock(&blktrans_ref_mutex);
 
+ added:
 	mutex_init(&new->lock);
 	kref_init(&new->ref);
 	if (!tr->writesect)
@@ -484,6 +479,8 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
 		BUG();
 	}
 
+	list_del(&old->list);
+
 	if (old->disk_attributes)
 		sysfs_remove_group(&disk_to_dev(old->disk)->kobj,
 						old->disk_attributes);
-- 
2.22.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/