diff mbox series

[v4] mtd: super: don't rely on mtdblock device minor

Message ID YJmx3Ru/5oyq6Y1a@makrotopia.org
State New
Headers show
Series [v4] mtd: super: don't rely on mtdblock device minor | expand

Commit Message

Daniel Golle May 10, 2021, 10:21 p.m. UTC
For blktrans devices with partitions (ie. part_bits != 0) the
assumption that the minor number of the mtdblock device matches
the mtdnum doesn't hold true.
Properly resolve mtd device from blktrans layer instead.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v4: rebase on v5.13-rc1, improve error handling
v3: fix typo in patch description
v2: remove BUG() calls as requested by Miquel Raynal

 drivers/mtd/mtdsuper.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig May 11, 2021, 5:49 a.m. UTC | #1
On Mon, May 10, 2021 at 11:21:17PM +0100, Daniel Golle wrote:
> For blktrans devices with partitions (ie. part_bits != 0) the
> assumption that the minor number of the mtdblock device matches
> the mtdnum doesn't hold true.
> Properly resolve mtd device from blktrans layer instead.

Why are you changing the legacy lookup method that is clearly deprecated
in favor of the mdt* syntax?

> +
> +	if (MAJOR(bdev->bd_dev) == MTD_BLOCK_MAJOR) {
> +		if (!bdev->bd_disk)
> +			goto error_mtdblock;

bdev->bd_disk can't be NULL.

>  
> +		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);

Overly long line due to the not actually required cast.

But more importantly you can't just look at the private data of a random
block device that you just opened.  There is absolutely no guarantee that
it actually points to a specific private data.
Daniel Golle May 11, 2021, 7:57 a.m. UTC | #2
Hi Christoph,

thank you for the review!

On Tue, May 11, 2021 at 07:49:55AM +0200, Christoph Hellwig wrote:
> On Mon, May 10, 2021 at 11:21:17PM +0100, Daniel Golle wrote:
> > For blktrans devices with partitions (ie. part_bits != 0) the
> > assumption that the minor number of the mtdblock device matches
> > the mtdnum doesn't hold true.
> > Properly resolve mtd device from blktrans layer instead.
> 
> Why are you changing the legacy lookup method that is clearly deprecated
> in favor of the mdt* syntax?

Because it breaks if part_bits != 1 and despite being legacy, it should
not break (it would open the wrong MTD device).

> 
> > +
> > +	if (MAJOR(bdev->bd_dev) == MTD_BLOCK_MAJOR) {
> > +		if (!bdev->bd_disk)
> > +			goto error_mtdblock;
> 
> bdev->bd_disk can't be NULL.

Better safe than sorry, I thought, especially as this is not a hot
path. But good, I'll remove the extra check.

> 
> >  
> > +		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
> 
> Overly long line due to the not actually required cast.
> 
> But more importantly you can't just look at the private data of a random
> block device that you just opened.  There is absolutely no guarantee that
> it actually points to a specific private data.

I do check the major number above to be MTD_BLOCK_MAJOR, and in that
case I assume that my expectations towards private structure will hold
true. If not, please enlighten me.
The previous assumption of the device MINOR number being equal to the
MTD number obviously also only holds true for mtdblock devices (if at
all).


Cheers


Daniel
Christoph Hellwig May 11, 2021, 4:42 p.m. UTC | #3
On Tue, May 11, 2021 at 08:57:15AM +0100, Daniel Golle wrote:
> > Why are you changing the legacy lookup method that is clearly deprecated
> > in favor of the mdt* syntax?
> 
> Because it breaks if part_bits != 1 and despite being legacy, it should
> not break (it would open the wrong MTD device).

Or we should just drop it if it doesn't even work.

> I do check the major number above to be MTD_BLOCK_MAJOR, and in that
> case I assume that my expectations towards private structure will hold
> true. If not, please enlighten me.

No, you can't.  There's always the chance some other driver grabs
the major.  Take a look at what e.g. ubd does to grab the IDE major.
Daniel Golle May 11, 2021, 8:17 p.m. UTC | #4
On Tue, May 11, 2021 at 06:42:48PM +0200, Christoph Hellwig wrote:
> On Tue, May 11, 2021 at 08:57:15AM +0100, Daniel Golle wrote:
> > > Why are you changing the legacy lookup method that is clearly deprecated
> > > in favor of the mdt* syntax?
> > 
> > Because it breaks if part_bits != 1 and despite being legacy, it should
> > not break (it would open the wrong MTD device).
> 
> Or we should just drop it if it doesn't even work.

It does work for the (most common) case part_bits == 1.
Dropping it is a good option imho, we would need to make changes in
OpenWrt to no longer depend on it, but well, that's life ;)

> 
> > I do check the major number above to be MTD_BLOCK_MAJOR, and in that
> > case I assume that my expectations towards private structure will hold
> > true. If not, please enlighten me.
> 
> No, you can't.  There's always the chance some other driver grabs
> the major.  Take a look at what e.g. ubd does to grab the IDE major.

IDE with it's 10 majors and a huge load of legacy code involved doesn't
seem to be the prime example... I generally find *_MAJOR being
hard-coded via pre-compiler macros and actually quite a lot of code
seems to make assumptions about the backing driver based on the device
major number used.

Anyway, as the same shortcomming (assuming MTD_BLOCK_MAJOR is only used
by mtdblock driver) also affects the existing code and I'm not aware
of any other way to check the backing driver, I guess dropping that
legacy hack is indeed the only good option left.

MTD folks: please advise if you agree that this would be the way
forward to clean up things for part_bits != 1 to work.
Christoph Hellwig May 12, 2021, 6:42 a.m. UTC | #5
On Tue, May 11, 2021 at 09:17:23PM +0100, Daniel Golle wrote:
> IDE with it's 10 majors and a huge load of legacy code involved doesn't
> seem to be the prime example... I generally find *_MAJOR being
> hard-coded via pre-compiler macros and actually quite a lot of code
> seems to make assumptions about the backing driver based on the device
> major number used.

Nothing with many majors.  There are valid (but obscure) use cases
where a (new or niche) drivers wants to impersonate an old one.  And
the infrastructure allows for that.  So guessing from a major number
to a format of private data is absolutely dangerous in the long run
even if it works for trivial setups.

> Anyway, as the same shortcomming (assuming MTD_BLOCK_MAJOR is only used
> by mtdblock driver) also affects the existing code and I'm not aware
> of any other way to check the backing driver, I guess dropping that
> legacy hack is indeed the only good option left.
> 
> MTD folks: please advise if you agree that this would be the way
> forward to clean up things for part_bits != 1 to work.

So why do we even depend on the block device code for the legacy
name lookup?  Can't we just remove lookup_bdev entirely and just parse
the /dev/mtdblock name here directly without any sort of detour?
diff mbox series

Patch

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 38b6aa849c638..40bb63e7bc7f6 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -9,6 +9,7 @@ 
  */
 
 #include <linux/mtd/super.h>
+#include <linux/mtd/blktrans.h>
 #include <linux/namei.h>
 #include <linux/export.h>
 #include <linux/ctype.h>
@@ -120,8 +121,9 @@  int get_tree_mtd(struct fs_context *fc,
 				struct fs_context *fc))
 {
 #ifdef CONFIG_BLOCK
-	dev_t dev;
-	int ret;
+	struct block_device *bdev;
+	struct mtd_blktrans_dev *blktrans_dev;
+	int ret, part_bits;
 #endif
 	int mtdnr;
 
@@ -169,16 +171,32 @@  int get_tree_mtd(struct fs_context *fc,
 	/* try the old way - the hack where we allowed users to mount
 	 * /dev/mtdblock$(n) but didn't actually _use_ the blockdev
 	 */
-	ret = lookup_bdev(fc->source, &dev);
-	if (ret) {
+	bdev = blkdev_get_by_path(fc->source, FMODE_READ, NULL);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
 		errorf(fc, "MTD: Couldn't look up '%s': %d", fc->source, ret);
 		return ret;
 	}
-	pr_debug("MTDSB: lookup_bdev() returned 0\n");
+	pr_debug("MTDSB: blkdev_get_by_path() returned 0\n");
+
+	if (MAJOR(bdev->bd_dev) == MTD_BLOCK_MAJOR) {
+		if (!bdev->bd_disk)
+			goto error_mtdblock;
 
-	if (MAJOR(dev) == MTD_BLOCK_MAJOR)
-		return mtd_get_sb_by_nr(fc, MINOR(dev), fill_super);
+		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
+		if (!blktrans_dev || !blktrans_dev->tr)
+			goto error_mtdblock;
 
+		mtdnr = blktrans_dev->devnum;
+		part_bits = blktrans_dev->tr->part_bits;
+		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
+			goto error_mtdblock;
+
+		blkdev_put(bdev, FMODE_READ);
+		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	}
+error_mtdblock:
+	blkdev_put(bdev, FMODE_READ);
 #endif /* CONFIG_BLOCK */
 
 	if (!(fc->sb_flags & SB_SILENT))