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

[Stefan Agner <stefan@agner.ch>]

On 09.02.2018 10:54, Boris Brezillon wrote:
> 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").
> 

Ok, will send a patch for that.

--
Stefan

>> 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;