mtd locking fix and cleanups

mtd locking fix and cleanups

[Christoph Hellwig]

Hi mtd maintainers,

the first series in this patch fixes a lock order reversal reported by
Guenter based on a recent commit, although from code inspection it should
have been around for much longer.

The rest is drive-by cleanups in the area that I noticed while trying to
understand the locking changes.  These are untested and need to be handled
with care!

Diffstat:
 ftl.c         |    2 -
 mtd_blkdevs.c |   60 ++++++----------------------------------------------------
 rfd_ftl.c     |    2 -
 3 files changed, 9 insertions(+), 55 deletions(-)

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

[PATCH 1/8] mtd_blkdevs: don't hold del_mtd_blktrans_dev in blktrans_{open, release}

[Christoph Hellwig]

There is nothing that this protects against except for slightly reducing
the window when new opens can appear just before calling del_gendisk.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/mtd/mtd_blkdevs.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 44bea3f65060..6b81a1c9ccbe 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -207,7 +207,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 	if (!dev)
 		return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
 
-	mutex_lock(&mtd_table_mutex);
 	mutex_lock(&dev->lock);
 
 	if (dev->open)
@@ -233,7 +232,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 unlock:
 	dev->open++;
 	mutex_unlock(&dev->lock);
-	mutex_unlock(&mtd_table_mutex);
 	blktrans_dev_put(dev);
 	return ret;
 
@@ -244,7 +242,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 	module_put(dev->tr->owner);
 	kref_put(&dev->ref, blktrans_dev_release);
 	mutex_unlock(&dev->lock);
-	mutex_unlock(&mtd_table_mutex);
 	blktrans_dev_put(dev);
 	return ret;
 }
@@ -256,7 +253,6 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
 	if (!dev)
 		return;
 
-	mutex_lock(&mtd_table_mutex);
 	mutex_lock(&dev->lock);
 
 	if (--dev->open)
@@ -272,7 +268,6 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
 	}
 unlock:
 	mutex_unlock(&dev->lock);
-	mutex_unlock(&mtd_table_mutex);
 	blktrans_dev_put(dev);
 }
 
-- 
2.30.2


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

[PATCH 2/8] mtd_blkdevs: use lockdep_assert_held

[Christoph Hellwig]

Use lockdep_assert_held to ensure mtd_table_mutex is held instead of
mutex_trylock games.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/mtd_blkdevs.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 6b81a1c9ccbe..81f0ce2a0b5a 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -310,10 +310,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	struct gendisk *gd;
 	int ret;
 
-	if (mutex_trylock(&mtd_table_mutex)) {
-		mutex_unlock(&mtd_table_mutex);
-		BUG();
-	}
+	lockdep_assert_held(&mtd_table_mutex);
 
 	mutex_lock(&blktrans_ref_mutex);
 	list_for_each_entry(d, &tr->devs, list) {
@@ -444,10 +441,7 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
 {
 	unsigned long flags;
 
-	if (mutex_trylock(&mtd_table_mutex)) {
-		mutex_unlock(&mtd_table_mutex);
-		BUG();
-	}
+	lockdep_assert_held(&mtd_table_mutex);
 
 	if (old->disk_attributes)
 		sysfs_remove_group(&disk_to_dev(old->disk)->kobj,
-- 
2.30.2


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

[PATCH 3/8] mtd/ftl: don't cast away the type when calling add_mtd_blktrans_dev

[Christoph Hellwig]

Pass the actual mtd_blktrans_dev instead of casting the containing
structure to void *.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/ftl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ftl.c b/drivers/mtd/ftl.c
index 9b33c082179d..f655d2905270 100644
--- a/drivers/mtd/ftl.c
+++ b/drivers/mtd/ftl.c
@@ -1029,7 +1029,7 @@ static void ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 
 		partition->mbd.tr = tr;
 		partition->mbd.devnum = -1;
-		if (!add_mtd_blktrans_dev((void *)partition))
+		if (!add_mtd_blktrans_dev(&partition->mbd))
 			return;
 	}
 
-- 
2.30.2


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

[PATCH 4/8] mtd/rfd_ftl: don't cast away the type when calling add_mtd_blktrans_dev

[Christoph Hellwig]

Pass the actual mtd_blktrans_dev instead of casting the containing
structure to void *.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/rfd_ftl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/rfd_ftl.c b/drivers/mtd/rfd_ftl.c
index 6e0d5ce9b010..6cf9356d3567 100644
--- a/drivers/mtd/rfd_ftl.c
+++ b/drivers/mtd/rfd_ftl.c
@@ -754,7 +754,7 @@ static void rfd_ftl_add_mtd(struct mtd_blktrans_ops *tr, struct mtd_info *mtd)
 		printk(KERN_INFO PREFIX "name: '%s' type: %d flags %x\n",
 				mtd->name, mtd->type, mtd->flags);
 
-		if (!add_mtd_blktrans_dev((void*)part))
+		if (!add_mtd_blktrans_dev(&part->mbd))
 			return;
 	}
 out:
-- 
2.30.2


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

[PATCH 5/8] mtd_blkdevs: simplify blktrans_dev_get

[Christoph Hellwig]

->private_data is set before the disk is added and never cleared, so don't
bother trying to handle a NULL pointer there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/mtd_blkdevs.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 81f0ce2a0b5a..f5e13b919614 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -39,16 +39,12 @@ static void blktrans_dev_release(struct kref *kref)
 
 static struct mtd_blktrans_dev *blktrans_dev_get(struct gendisk *disk)
 {
-	struct mtd_blktrans_dev *dev;
+	struct mtd_blktrans_dev *dev = disk->private_data;
 
 	mutex_lock(&blktrans_ref_mutex);
-	dev = disk->private_data;
-
-	if (!dev)
-		goto unlock;
 	kref_get(&dev->ref);
-unlock:
 	mutex_unlock(&blktrans_ref_mutex);
+
 	return dev;
 }
 
@@ -204,9 +200,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 	struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk);
 	int ret = 0;
 
-	if (!dev)
-		return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
-
 	mutex_lock(&dev->lock);
 
 	if (dev->open)
@@ -250,9 +243,6 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
 {
 	struct mtd_blktrans_dev *dev = blktrans_dev_get(disk);
 
-	if (!dev)
-		return;
-
 	mutex_lock(&dev->lock);
 
 	if (--dev->open)
@@ -276,9 +266,6 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk);
 	int ret = -ENXIO;
 
-	if (!dev)
-		return ret;
-
 	mutex_lock(&dev->lock);
 
 	if (!dev->mtd)
-- 
2.30.2


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

[PATCH 6/8] mtd_blkdevs: remove blktrans_ref_mutex

[Christoph Hellwig]

blktrans_ref_mutex is not actually needed.  The kref is serialized
internally, and devnum assignment in add_mtd_blktrans_dev happens before
the disk is added and thus any of the block_device_operations methods
otherwise using it are called.  It  is also already serialized by the
global mtd_table_mutex.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/mtd_blkdevs.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index f5e13b919614..6f226b7cbb5c 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -23,7 +23,6 @@
 #include "mtdcore.h"
 
 static LIST_HEAD(blktrans_majors);
-static DEFINE_MUTEX(blktrans_ref_mutex);
 
 static void blktrans_dev_release(struct kref *kref)
 {
@@ -41,18 +40,13 @@ static struct mtd_blktrans_dev *blktrans_dev_get(struct gendisk *disk)
 {
 	struct mtd_blktrans_dev *dev = disk->private_data;
 
-	mutex_lock(&blktrans_ref_mutex);
 	kref_get(&dev->ref);
-	mutex_unlock(&blktrans_ref_mutex);
-
 	return dev;
 }
 
 static void blktrans_dev_put(struct mtd_blktrans_dev *dev)
 {
-	mutex_lock(&blktrans_ref_mutex);
 	kref_put(&dev->ref, blktrans_dev_release);
-	mutex_unlock(&blktrans_ref_mutex);
 }
 
 
@@ -299,7 +293,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 
 	lockdep_assert_held(&mtd_table_mutex);
 
-	mutex_lock(&blktrans_ref_mutex);
 	list_for_each_entry(d, &tr->devs, list) {
 		if (new->devnum == -1) {
 			/* Use first free number */
@@ -311,7 +304,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 */
@@ -329,14 +321,11 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	 * minor numbers and that the disk naming code below can cope
 	 * with this number. */
 	if (new->devnum > (MINORMASK >> tr->part_bits) ||
-	    (tr->part_bits && new->devnum >= 27 * 26)) {
-		mutex_unlock(&blktrans_ref_mutex);
+	    (tr->part_bits && new->devnum >= 27 * 26))
 		return ret;
-	}
 
 	list_add_tail(&new->list, &tr->devs);
  added:
-	mutex_unlock(&blktrans_ref_mutex);
 
 	mutex_init(&new->lock);
 	kref_init(&new->ref);
-- 
2.30.2


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

[PATCH 7/8] mtd_blkdevs: simplify blktrans_getgeo

[Christoph Hellwig]

No need to grab a mtd_blktrans_dev given that ->open already holds one
and ->getgeo can only be called on an open disk.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/mtd_blkdevs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 6f226b7cbb5c..012c0966010a 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -257,7 +257,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
 
 static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 {
-	struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk);
+	struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data;
 	int ret = -ENXIO;
 
 	mutex_lock(&dev->lock);
@@ -268,7 +268,6 @@ static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	ret = dev->tr->getgeo ? dev->tr->getgeo(dev, geo) : -ENOTTY;
 unlock:
 	mutex_unlock(&dev->lock);
-	blktrans_dev_put(dev);
 	return ret;
 }
 
-- 
2.30.2


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

[PATCH 8/8] mtd_blkdevs: simplify the refcounting in blktrans_{open, release}

[Christoph Hellwig]

Always grab a reference to the mtd_blktrans_dev in ->open instead of
just on the first open, and do away with the additional temporary
references in ->open and ->release.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mtd/mtd_blkdevs.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 012c0966010a..b8ae1ec14e17 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -36,14 +36,6 @@ static void blktrans_dev_release(struct kref *kref)
 	kfree(dev);
 }
 
-static struct mtd_blktrans_dev *blktrans_dev_get(struct gendisk *disk)
-{
-	struct mtd_blktrans_dev *dev = disk->private_data;
-
-	kref_get(&dev->ref);
-	return dev;
-}
-
 static void blktrans_dev_put(struct mtd_blktrans_dev *dev)
 {
 	kref_put(&dev->ref, blktrans_dev_release);
@@ -191,15 +183,16 @@ static blk_status_t mtd_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 static int blktrans_open(struct block_device *bdev, fmode_t mode)
 {
-	struct mtd_blktrans_dev *dev = blktrans_dev_get(bdev->bd_disk);
+	struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data;
 	int ret = 0;
 
+	kref_get(&dev->ref);
+
 	mutex_lock(&dev->lock);
 
 	if (dev->open)
 		goto unlock;
 
-	kref_get(&dev->ref);
 	__module_get(dev->tr->owner);
 
 	if (!dev->mtd)
@@ -219,7 +212,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 unlock:
 	dev->open++;
 	mutex_unlock(&dev->lock);
-	blktrans_dev_put(dev);
 	return ret;
 
 error_release:
@@ -227,7 +219,6 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 		dev->tr->release(dev);
 error_put:
 	module_put(dev->tr->owner);
-	kref_put(&dev->ref, blktrans_dev_release);
 	mutex_unlock(&dev->lock);
 	blktrans_dev_put(dev);
 	return ret;
@@ -235,14 +226,13 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
 
 static void blktrans_release(struct gendisk *disk, fmode_t mode)
 {
-	struct mtd_blktrans_dev *dev = blktrans_dev_get(disk);
+	struct mtd_blktrans_dev *dev = disk->private_data;
 
 	mutex_lock(&dev->lock);
 
 	if (--dev->open)
 		goto unlock;
 
-	kref_put(&dev->ref, blktrans_dev_release);
 	module_put(dev->tr->owner);
 
 	if (dev->mtd) {
-- 
2.30.2


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

Re: mtd locking fix and cleanups

[Miquel Raynal]

Hi Christoph,

Christoph Hellwig <hch@lst.de> wrote on Mon, 23 Aug 2021 09:33:51 +0200:

> Hi mtd maintainers,
> 
> the first series in this patch fixes a lock order reversal reported by
> Guenter based on a recent commit, although from code inspection it should
> have been around for much longer.
> 
> The rest is drive-by cleanups in the area that I noticed while trying to
> understand the locking changes.  These are untested and need to be handled
> with care!

Thanks for the fix and the various cleanups, they look good to me. I
don't know how important these drivers are nor if they are still
actually used, it would be good to have some feedback from people still
following. In the mean time I'll apply the series right away, I don't
want to wait an additional month or so because of the merge window.

Thanks,
Miquèl

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

Re: [PATCH 8/8] mtd_blkdevs: simplify the refcounting in blktrans_{open, release}

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:59 UTC, Christoph Hellwig wrote:
> Always grab a reference to the mtd_blktrans_dev in ->open instead of
> just on the first open, and do away with the additional temporary
> references in ->open and ->release.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 7/8] mtd_blkdevs: simplify blktrans_getgeo

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:58 UTC, Christoph Hellwig wrote:
> No need to grab a mtd_blktrans_dev given that ->open already holds one
> and ->getgeo can only be called on an open disk.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 6/8] mtd_blkdevs: remove blktrans_ref_mutex

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:57 UTC, Christoph Hellwig wrote:
> blktrans_ref_mutex is not actually needed.  The kref is serialized
> internally, and devnum assignment in add_mtd_blktrans_dev happens before
> the disk is added and thus any of the block_device_operations methods
> otherwise using it are called.  It  is also already serialized by the
> global mtd_table_mutex.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 5/8] mtd_blkdevs: simplify blktrans_dev_get

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:56 UTC, Christoph Hellwig wrote:
> ->private_data is set before the disk is added and never cleared, so don't
> bother trying to handle a NULL pointer there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 4/8] mtd/rfd_ftl: don't cast away the type when calling add_mtd_blktrans_dev

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:55 UTC, Christoph Hellwig wrote:
> Pass the actual mtd_blktrans_dev instead of casting the containing
> structure to void *.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 3/8] mtd/ftl: don't cast away the type when calling add_mtd_blktrans_dev

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:54 UTC, Christoph Hellwig wrote:
> Pass the actual mtd_blktrans_dev instead of casting the containing
> structure to void *.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 2/8] mtd_blkdevs: use lockdep_assert_held

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:53 UTC, Christoph Hellwig wrote:
> Use lockdep_assert_held to ensure mtd_table_mutex is held instead of
> mutex_trylock games.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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

Re: [PATCH 1/8] mtd_blkdevs: don't hold del_mtd_blktrans_dev in blktrans_{open, release}

[Miquel Raynal]

On Mon, 2021-08-23 at 07:33:52 UTC, Christoph Hellwig wrote:
> There is nothing that this protects against except for slightly reducing
> the window when new opens can appear just before calling del_gendisk.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel

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