[PATCH] mtd: super: don't reply on mtdblock device minor

[PATCH] mtd: super: don't reply on mtdblock device minor

[Daniel Golle]

For mtdblock 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>
---
 drivers/mtd/mtdsuper.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index c3e2098372f2e..dc52f9929c8fb 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>
@@ -121,7 +122,8 @@ int get_tree_mtd(struct fs_context *fc,
 {
 #ifdef CONFIG_BLOCK
 	struct block_device *bdev;
-	int ret, major;
+	struct mtd_blktrans_dev *blktrans_dev;
+	int ret, major, part_bits;
 #endif
 	int mtdnr;
 
@@ -169,21 +171,38 @@ 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
 	 */
-	bdev = lookup_bdev(fc->source);
+	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");
 
 	major = MAJOR(bdev->bd_dev);
-	mtdnr = MINOR(bdev->bd_dev);
-	bdput(bdev);
 
-	if (major == MTD_BLOCK_MAJOR)
-		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	if (major == MTD_BLOCK_MAJOR) {
+		if (!bdev->bd_disk) {
+			blkdev_put(bdev, FMODE_READ);
+			BUG();
+			return -EINVAL;
+		}
+
+		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
+		if (!blktrans_dev || !blktrans_dev->tr) {
+			blkdev_put(bdev, FMODE_READ);
+			BUG();
+			return -EINVAL;
+		}
+		mtdnr = blktrans_dev->devnum;
+		part_bits = blktrans_dev->tr->part_bits;
+		blkdev_put(bdev, FMODE_READ);
+		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
+			return -EINVAL;
 
+		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	}
+	blkdev_put(bdev, FMODE_READ);
 #endif /* CONFIG_BLOCK */
 
 	if (!(fc->sb_flags & SB_SILENT))
-- 
2.31.0


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

Re: [PATCH] mtd: super: don't reply on mtdblock device minor

[Miquel Raynal]

Hi Daniel,

Daniel Golle <daniel@makrotopia.org> wrote on Wed, 7 Apr 2021 22:55:32
+0100:

> For mtdblock 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>
> ---
>  drivers/mtd/mtdsuper.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> index c3e2098372f2e..dc52f9929c8fb 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>
> @@ -121,7 +122,8 @@ int get_tree_mtd(struct fs_context *fc,
>  {
>  #ifdef CONFIG_BLOCK
>  	struct block_device *bdev;
> -	int ret, major;
> +	struct mtd_blktrans_dev *blktrans_dev;
> +	int ret, major, part_bits;
>  #endif
>  	int mtdnr;
>  
> @@ -169,21 +171,38 @@ 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
>  	 */
> -	bdev = lookup_bdev(fc->source);
> +	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");
>  
>  	major = MAJOR(bdev->bd_dev);
> -	mtdnr = MINOR(bdev->bd_dev);
> -	bdput(bdev);
>  
> -	if (major == MTD_BLOCK_MAJOR)
> -		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
> +	if (major == MTD_BLOCK_MAJOR) {
> +		if (!bdev->bd_disk) {
> +			blkdev_put(bdev, FMODE_READ);
> +			BUG();
> +			return -EINVAL;
> +		}
> +
> +		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
> +		if (!blktrans_dev || !blktrans_dev->tr) {
> +			blkdev_put(bdev, FMODE_READ);
> +			BUG();

Could you avoid these calls? I doubt they bring anything useful and
returning -EINVAL should be enough anyway.

> +			return -EINVAL;
> +		}
> +		mtdnr = blktrans_dev->devnum;
> +		part_bits = blktrans_dev->tr->part_bits;
> +		blkdev_put(bdev, FMODE_READ);
> +		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
> +			return -EINVAL;
>  
> +		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
> +	}
> +	blkdev_put(bdev, FMODE_READ);
>  #endif /* CONFIG_BLOCK */
>  
>  	if (!(fc->sb_flags & SB_SILENT))

Otherwise lgtm.

Thanks,
Miquèl

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

[PATCH v2] mtd: super: don't reply on mtdblock device minor

[Daniel Golle]

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>
---
v2: remove BUG() calls as requested by Miquel Raynal

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

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index c3e2098372f2e..d3af80a7ce86e 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>
@@ -121,7 +122,8 @@ int get_tree_mtd(struct fs_context *fc,
 {
 #ifdef CONFIG_BLOCK
 	struct block_device *bdev;
-	int ret, major;
+	struct mtd_blktrans_dev *blktrans_dev;
+	int ret, major, part_bits;
 #endif
 	int mtdnr;
 
@@ -169,21 +171,36 @@ 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
 	 */
-	bdev = lookup_bdev(fc->source);
+	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");
 
 	major = MAJOR(bdev->bd_dev);
-	mtdnr = MINOR(bdev->bd_dev);
-	bdput(bdev);
 
-	if (major == MTD_BLOCK_MAJOR)
-		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	if (major == MTD_BLOCK_MAJOR) {
+		if (!bdev->bd_disk) {
+			blkdev_put(bdev, FMODE_READ);
+			return -EINVAL;
+		}
+
+		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
+		if (!blktrans_dev || !blktrans_dev->tr) {
+			blkdev_put(bdev, FMODE_READ);
+			return -EINVAL;
+		}
+		mtdnr = blktrans_dev->devnum;
+		part_bits = blktrans_dev->tr->part_bits;
+		blkdev_put(bdev, FMODE_READ);
+		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
+			return -EINVAL;
 
+		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	}
+	blkdev_put(bdev, FMODE_READ);
 #endif /* CONFIG_BLOCK */
 
 	if (!(fc->sb_flags & SB_SILENT))
-- 
2.31.0


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

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

[Daniel Golle]

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>
---
v3: fix typo in patch description
v2: remove BUG() calls as requested by Miquel Raynal

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

diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index c3e2098372f2e..d3af80a7ce86e 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>
@@ -121,7 +122,8 @@ int get_tree_mtd(struct fs_context *fc,
 {
 #ifdef CONFIG_BLOCK
 	struct block_device *bdev;
-	int ret, major;
+	struct mtd_blktrans_dev *blktrans_dev;
+	int ret, major, part_bits;
 #endif
 	int mtdnr;
 
@@ -169,21 +171,36 @@ 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
 	 */
-	bdev = lookup_bdev(fc->source);
+	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");
 
 	major = MAJOR(bdev->bd_dev);
-	mtdnr = MINOR(bdev->bd_dev);
-	bdput(bdev);
 
-	if (major == MTD_BLOCK_MAJOR)
-		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	if (major == MTD_BLOCK_MAJOR) {
+		if (!bdev->bd_disk) {
+			blkdev_put(bdev, FMODE_READ);
+			return -EINVAL;
+		}
+
+		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
+		if (!blktrans_dev || !blktrans_dev->tr) {
+			blkdev_put(bdev, FMODE_READ);
+			return -EINVAL;
+		}
+		mtdnr = blktrans_dev->devnum;
+		part_bits = blktrans_dev->tr->part_bits;
+		blkdev_put(bdev, FMODE_READ);
+		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
+			return -EINVAL;
 
+		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
+	}
+	blkdev_put(bdev, FMODE_READ);
 #endif /* CONFIG_BLOCK */
 
 	if (!(fc->sb_flags & SB_SILENT))
-- 
2.31.0


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

Re: [PATCH v3] mtd: super: don't rely on mtdblock device minor

[Miquel Raynal]

Hi Daniel,

Daniel Golle <daniel@makrotopia.org> wrote on Mon, 12 Apr 2021 16:14:44
+0100:

> 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.

Unfortunately I cannot apply your patch anymore, would you mind
rebasing it on top of v5.13-rc1?

Thanks,
Miquèl

> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> v3: fix typo in patch description
> v2: remove BUG() calls as requested by Miquel Raynal
> 
>  drivers/mtd/mtdsuper.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> index c3e2098372f2e..d3af80a7ce86e 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>
> @@ -121,7 +122,8 @@ int get_tree_mtd(struct fs_context *fc,
>  {
>  #ifdef CONFIG_BLOCK
>  	struct block_device *bdev;
> -	int ret, major;
> +	struct mtd_blktrans_dev *blktrans_dev;
> +	int ret, major, part_bits;
>  #endif
>  	int mtdnr;
>  
> @@ -169,21 +171,36 @@ 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
>  	 */
> -	bdev = lookup_bdev(fc->source);
> +	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");
>  
>  	major = MAJOR(bdev->bd_dev);
> -	mtdnr = MINOR(bdev->bd_dev);
> -	bdput(bdev);
>  
> -	if (major == MTD_BLOCK_MAJOR)
> -		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
> +	if (major == MTD_BLOCK_MAJOR) {
> +		if (!bdev->bd_disk) {
> +			blkdev_put(bdev, FMODE_READ);
> +			return -EINVAL;
> +		}
> +
> +		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
> +		if (!blktrans_dev || !blktrans_dev->tr) {
> +			blkdev_put(bdev, FMODE_READ);
> +			return -EINVAL;
> +		}
> +		mtdnr = blktrans_dev->devnum;
> +		part_bits = blktrans_dev->tr->part_bits;
> +		blkdev_put(bdev, FMODE_READ);
> +		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
> +			return -EINVAL;
>  
> +		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
> +	}
> +	blkdev_put(bdev, FMODE_READ);
>  #endif /* CONFIG_BLOCK */
>  
>  	if (!(fc->sb_flags & SB_SILENT))


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

Re: [PATCH v3] mtd: super: don't rely on mtdblock device minor

[Daniel Golle]

On Mon, May 10, 2021 at 12:18:37PM +0200, Miquel Raynal wrote:
> Hi Daniel,
> 
> Daniel Golle <daniel@makrotopia.org> wrote on Mon, 12 Apr 2021 16:14:44
> +0100:
> 
> > 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.
> 
> Unfortunately I cannot apply your patch anymore, would you mind
> rebasing it on top of v5.13-rc1?

I will send a rebased and improved version.

Thank you!


> 
> Thanks,
> Miquèl
> 
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> > v3: fix typo in patch description
> > v2: remove BUG() calls as requested by Miquel Raynal
> > 
> >  drivers/mtd/mtdsuper.c | 31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
> > index c3e2098372f2e..d3af80a7ce86e 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>
> > @@ -121,7 +122,8 @@ int get_tree_mtd(struct fs_context *fc,
> >  {
> >  #ifdef CONFIG_BLOCK
> >  	struct block_device *bdev;
> > -	int ret, major;
> > +	struct mtd_blktrans_dev *blktrans_dev;
> > +	int ret, major, part_bits;
> >  #endif
> >  	int mtdnr;
> >  
> > @@ -169,21 +171,36 @@ 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
> >  	 */
> > -	bdev = lookup_bdev(fc->source);
> > +	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");
> >  
> >  	major = MAJOR(bdev->bd_dev);
> > -	mtdnr = MINOR(bdev->bd_dev);
> > -	bdput(bdev);
> >  
> > -	if (major == MTD_BLOCK_MAJOR)
> > -		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
> > +	if (major == MTD_BLOCK_MAJOR) {
> > +		if (!bdev->bd_disk) {
> > +			blkdev_put(bdev, FMODE_READ);
> > +			return -EINVAL;
> > +		}
> > +
> > +		blktrans_dev = (struct mtd_blktrans_dev *)(bdev->bd_disk->private_data);
> > +		if (!blktrans_dev || !blktrans_dev->tr) {
> > +			blkdev_put(bdev, FMODE_READ);
> > +			return -EINVAL;
> > +		}
> > +		mtdnr = blktrans_dev->devnum;
> > +		part_bits = blktrans_dev->tr->part_bits;
> > +		blkdev_put(bdev, FMODE_READ);
> > +		if (MINOR(bdev->bd_dev) != (mtdnr << part_bits))
> > +			return -EINVAL;
> >  
> > +		return mtd_get_sb_by_nr(fc, mtdnr, fill_super);
> > +	}
> > +	blkdev_put(bdev, FMODE_READ);
> >  #endif /* CONFIG_BLOCK */
> >  
> >  	if (!(fc->sb_flags & SB_SILENT))
> 

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

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

[Daniel Golle]

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(-)

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))
-- 
2.31.1


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

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

[Christoph Hellwig]

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.

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

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

[Daniel Golle]

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

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

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

[Christoph Hellwig]

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.

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

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

[Daniel Golle]

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.

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

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

[Christoph Hellwig]

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?

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