| Message ID | YJmx3Ru/5oyq6Y1a@makrotopia.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4] mtd: super: don't rely on mtdblock device minor | expand |
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.
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
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.
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.
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 --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))
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(-)