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

[Boris Brezillon <bbrezillon@kernel.org>]

On Tue, 22 Jan 2019 12:21:11 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Boris,
> 
> 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?

--->8---
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 60104e1079c5..aefd3344991f 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -724,16 +724,14 @@ int add_mtd_partitions(struct mtd_info *master,
 {
        struct mtd_part *slave;
        uint64_t cur_offset = 0;
-       int i, ret;
+       int i, ret, actual_nbparts = 0;
 
        printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
 
        for (i = 0; i < nbparts; i++) {
                slave = allocate_partition(master, parts + i, i, cur_offset);
-               if (IS_ERR(slave)) {
-                       ret = PTR_ERR(slave);
-                       goto err_del_partitions;
-               }
+               if (IS_ERR(slave))
+                       continue;
 
                mutex_lock(&mtd_partitions_mutex);
                list_add(&slave->list, &mtd_partitions);
@@ -746,7 +744,7 @@ int add_mtd_partitions(struct mtd_info *master,
                        mutex_unlock(&mtd_partitions_mutex);
 
                        free_partition(slave);
-                       goto err_del_partitions;
+                       continue;
                }
 
                mtd_add_partition_attrs(slave);
@@ -754,14 +752,10 @@ int add_mtd_partitions(struct mtd_info *master,
                parse_mtd_partitions(&slave->mtd, parts[i].types, NULL);
 
                cur_offset = slave->offset + slave->mtd.size;
+               actual_nbparts++;
        }
 
-       return 0;
-
-err_del_partitions:
-       del_mtd_partitions(master);
-
-       return ret;
+       return actual_nbparts;
 }
 
 static DEFINE_SPINLOCK(part_parser_lock);
@@ -1003,10 +997,10 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
                }
                /* Found partitions! */
                if (ret > 0) {
-                       err = add_mtd_partitions(master, pparts.parts,
+                       ret = add_mtd_partitions(master, pparts.parts,
                                                 pparts.nr_parts);
                        mtd_part_parser_cleanup(&pparts);
-                       return err ? err : pparts.nr_parts;
+                       return ret;
                }
                /*
                 * Stash the first error we see; only report it if no parser

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