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

From: Daniel Golle <daniel@makrotopia.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	linux-mtd@lists.infradead.org,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Richard Weinberger <richard@nod.at>,
	David Woodhouse <dwmw2@infradead.org>, Jan Kara <jack@suse.cz>,
	Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH v4] mtd: super: don't rely on mtdblock device minor
Date: Tue, 11 May 2021 08:57:15 +0100	[thread overview]
Message-ID: <YJo42y+lP9peFlkd@makrotopia.org> (raw)
In-Reply-To: <20210511054955.GA6542@lst.de>

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/