Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Boris Brezillon <bbrezillon@kernel.org>
Cc: Marek Vasut <marek.vasut@gmail.com>,
	Richard Weinberger <richard@nod.at>,
	Boris Brezillon <boris.brezillon@bootlin.com>,
	MTD Maling List <linux-mtd@lists.infradead.org>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH 2/2] mtd: Check add_mtd_device() ret code
Date: Wed, 30 Jan 2019 10:12:30 +0100	[thread overview]
Message-ID: <CAMuHMdXKzinbtDf=cLJCAMMxeSiDrG-Hq_t3r2-kSvZQzizWYg@mail.gmail.com> (raw)
In-Reply-To: <20190130095530.44e7fe8a@bbrezillon>

Hi Boris,

On Wed, Jan 30, 2019 at 9:55 AM Boris Brezillon <bbrezillon@kernel.org> wrote:
> On Tue, 29 Jan 2019 16:29:55 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Jan 29, 2019 at 12:03 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> > > On Tue, 22 Jan 2019 13:31:59 +0100
> > > Boris Brezillon <bbrezillon@kernel.org> wrote:
> > > > On Tue, 22 Jan 2019 12:21:11 +0100
> > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, Jan 2, 2019 at 3:37 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> > > > > > add_mtd_device() can fail. We should always check its return value
> > > > > > and gracefully handle the failure case. Fix the call sites where this
> > > > > > not done (in mtdpart.c) and add a __must_check attribute to the
> > > > > > prototype to avoid this kind of mistakes.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> > > > > > ---
> > > > > > No Fixes or Cc-stable tag here, as this seems to have worked just fine
> > > > > > without checking add_mtd_device() ret code until we started to expose
> > > > > > MTD devices as NVMEM providers (queued for 4.21).
> > > > >
> > > > > Oh yes ;-)
> > > > >
> > > > > https://lore.kernel.org/lkml/87feac02-e955-1897-d4a4-d6d6d1082e45@gmail.com/t/
> > > > >
> > > > > Your patch is very similar to mine, so the crash is gone.
> > > >
> > > > Oops, sorry about that. I completely forgot about this patch. It seems
> > > > the discussion led to a different conclusion though (patch
> > > > allocate_partitions() to reject wrong parts early) and the v2 was never
> > > > sent (or I missed it). Anyway, I guess we should have done both (check
> > > > add_mtd_device() ret code everywhere and patch allocate_partitions() to
> > > > reject bad parts early).
> > > >
> > > > > However, the warning is still there:
> > > > >
> > > > >     m25p80 spi0.0: s25sl032p (4096 Kbytes)
> > > > >     3 fixed-partitions partitions found on MTD device spi0.0
> > > > >     Creating 3 MTD partitions on "spi0.0":
> > > > >     0x000000000000-0x000000080000 : "loader"
> > > > >     0x000000080000-0x000000600000 : "user"
> > > > >     mtd: partition "user" extends beyond the end of device "spi0.0" --
> > > > > size truncated to 0x380000
> > > > >     0x000000600000-0x000004000000 : "flash"
> > > > >     mtd: partition "flash" is out of reach -- disabled
> > > > >     ------------[ cut here ]------------
> > > > >     WARNING: CPU: 0 PID: 1 at drivers/mtd/mtdcore.c:571
> > > > > add_mtd_device+0x90/0x3b0
> > > > >
> > > > > Interestingly, only one partition is created, covering the full size of the
> > > > > device:
> > > > >
> > > > >     # cat /proc/partitions
> > > > >     major minor  #blocks  name
> > > > >
> > > > >       31        0       4096 mtdblock0
> > > > >
> > > > > While I would expect two partitions, "loader" and truncated "user":
> > > > >
> > > > >       31        0        512 mtdblock0
> > > > >       31        1       3584 mtdblock1
> > > >
> > > > Yes, makes sense, I guess your patch was better than mine :-/. Can you
> > > > try with the following diff applied and let me know if it solves the
> > > > problem?
> > >
> > > Gentle ping: is this diff fixing your problem, and do you want me to
> > > send a proper patch for it or should I let you send one?
> >
> > Yes, the diff below fixes the partitions for me, so
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Note that the warning is still there, but that's probably OK.
>
> Just out of curiosity, why do you need to define parts that do not fit
> in the flash?

All of this started as an accident on my side ;-)
The Koelsch board has 2 QSPI FLASHes: the first is 64 MiB large, the
second is 4 MiB large.  Which FLASH is used is selected by a switch, but
the partitioning is specified in DT.
I wanted to use the smaller FLASH for some test, but forgot to update the
partitioning in DT, leading to:

    mtd: partition "user" extends beyond the end of device "spi0.0" --
size truncated to 0x380000

That kernel message indicated MTD tried to handle this gracefully, but
still, the kernel crashed later.

Anyway, it may be considered good practice to handle bad partition
tables, like is done for disk partitioning, so I think it's worthwhile to
have this fixed, which is what your patches achieve.
There's still a WARN_ON() drawing the user's attention, though.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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