Re: [PATCH] mtd: nand: warn if hamming layout is used with too large ECC

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Fri, 09 Feb 2018 10:20:37 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 09.02.2018 09:50, Boris Brezillon wrote:
> > On Fri,  9 Feb 2018 00:33:05 +0100
> > Stefan Agner <stefan@agner.ch> wrote:
> >   
> >> Warn in case a driver uses too large ECC with hamming layout.
> >> This is especially helpful since hamming layout is the default
> >> layout when using hardware ECC and no specific OOB layout is
> >> specified.
> >>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  drivers/mtd/nand/nand_base.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index 96c97588e1ba..2f3f43d0e288 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -197,6 +197,8 @@ static int nand_ooblayout_free_lp_hamming(struct mtd_info *mtd, int section,
> >>  		return -EINVAL;
> >>  	}
> >>
> >> +	WARN_ON(mtd->oobsize - ecc_offset < ecc->total);
> >> +  
> > 
> > Did you hit this problem? Anyway, if there's a case where the number of
> > ECC bytes does not fit in the space reserved for ECC, there's a bug
> > before this point, and this should be checked at init/probe time.
> >   
> 
> Yes, I realized that vf610_nfc.c, which is currently is using the
> hamming ooblayout. This probably crept in with commit 3cf32d180227
> ("mtd: nand: vf610: switch to mtd_ooblayout_ops").

Actually, it's bogus since 6a623e076944 ("mtd: nand: add ooblayout for
old hamming layout") which was fixing a bug I had introduced with my
mtd_ooblayout_ops series :-).

> 
> When using 32-bit ECC mode the driver uses 60 bytes out of 64 bytes OOB,
> so it actually fits into the OOB.
> 
> The layout is just bogus for that case.

Yep, you should use nand_ooblayout_lp_ops, which was the one used by
default before 6a623e076944 ("mtd: nand: add ooblayout for old hamming
layout").

> Surprisingly the oobavail
> calculation ends up being correct, but only because the calculation
> overflows twice:
> 
> mtd_ooblayout_count_bytes calls first with section 0, which results in
> 38. the second call leads to an overflow ("-36").
> mtd_ooblayout_count_bytes then adds 38 to that overflow, which then
> overflows again to the correct value of overall free bytes of 2... I did
> not try actually using the free OOB area, I guess this would fail....

That should be fixed to be more robust, indeed.

> 
> Of course the underlying issue that ooblayout for vf610_nfc.c is bogus
> needs to be fixed, I will send a patch for that.
> 
> But some kind of sanity check somewhere might be worthwhile, I was a bit
> surprised that this overflowing happens on a driver in operational use
> and goes unnoticed. I realize that this patch is not ideal. Maybe making
> length signed, then we could sanity check in
> mtd_ooblayout_count_bytes...
> 
> --
> Stefan
> 
> >>  	if (section == 0) {
> >>  		oobregion->offset = 2;
> >>  		oobregion->length = ecc_offset - 2;  



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com