| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150731122157.78bad539@bbrezillon>
Date: Fri, 31 Jul 2015 12:21:57 +0200
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Andrea Scian <rnd4@...e-tech.it>
Cc: linux-mtd@...ts.infradead.org,
David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] mtd: nand: add nand_check_erased helper
functions
Hi Andrea,
On Fri, 31 Jul 2015 12:06:32 +0200
Andrea Scian <rnd4@...e-tech.it> wrote:
>
> Dear Boris,
>
> thanks for pointing this out again.
>
> I'm on the same topic too, using iMX6 (I'll try to test you patch on the
> next days, if I found some spare time, unfortunately I got a 3.10
> kernel, so I think the patch will not apply cleanly :-( ).
>
> See my comment below (and on the next mail too)
>
> Il 31/07/2015 09:10, Boris Brezillon ha scritto:
> > On Thu, 30 Jul 2015 19:34:53 +0200
> > Boris Brezillon <boris.brezillon@...e-electrons.com> wrote:
> >
> >> Add two helper functions to help NAND controller drivers test whether a
> >> specific NAND region is erased or not.
> >>
> >> Signed-off-by: Boris Brezillon <boris.brezillon@...e-electrons.com>
> >> ---
> >> drivers/mtd/nand/nand_base.c | 104 +++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/mtd/nand.h | 8 ++++
> >> 2 files changed, 112 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index ceb68ca..1542ea7 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -1101,6 +1101,110 @@ out:
> >> EXPORT_SYMBOL(nand_lock);
> >>
> >> /**
> >> + * nand_check_erased_buf - check if a buffer contains (almost) only 0xff data
> >> + * @buf: buffer to test
> >> + * @len: buffer length
> >> + * @bitflips_threshold:maximum number of bitflips
> >> + *
> >> + * Check if a buffer contains only 0xff, which means the underlying region
> >> + * has been erased and is ready to be programmed.
> >> + * The bitflips_threshold specify the maximum number of bitflips before
> >> + * considering the region is not erased.
> >> + * Note: The logic of this function has been extracted from the memweight
> >> + * implementation, except that nand_check_erased_buf function exit before
> >> + * testing the whole buffer if the number of bitflips exceed the
> >> + * bitflips_threshold value.
> >> + *
> >> + * Returns a positive number of bitflips or -ERROR_CODE.
> >> + */
> >> +int nand_check_erased_buf(void *buf, int len, int bitflips_threshold)
> >> +{
> >> + const unsigned char *bitmap = buf;
> >> + int bitflips = 0;
> >> + int weight;
> >> + int longs;
> >> +
> >> + for (; len && ((unsigned long)bitmap) % sizeof(long);
> >> + len--, bitmap++) {
> >> + weight = hweight8(*bitmap);
> >> +
> >> + bitflips += sizeof(u8) - weight;
> >> + if (bitflips > bitflips_threshold)
> >> + return -EINVAL;
>
> I think it's better to do something like:
>
> if (UNLIKELY(bitflips > bitflips_threshold))
> return -EINVAL;
>
> isn't it? :-)
> (the same for the other if)
Maybe, or maybe not. It depends on whether you expect to have a lot of
corrupted pages or a lot of blank pages with bitflips ;-).
Anyway, I'm not opposed to this change.
>
>
> >> + }
> >> +
> >> +
> >> + for (longs = len / sizeof(long); longs;
> >> + longs--, bitmap += sizeof(long)) {
> >> + BUG_ON(longs >= INT_MAX / BITS_PER_LONG);
> >> + weight = hweight_long(*((unsigned long *)bitmap));
> >> +
> >> + bitflips += sizeof(long) - weight;
> >> + if (bitflips > bitflips_threshold)
> >> + return -EINVAL;
> >> + }
> >> +
> >> + len %= sizeof(long);
> >> +
> >> + for (; len > 0; len--, bitmap++) {
> >> + weight = hweight8(*bitmap);
> >> + bitflips += sizeof(u8) - weight;
> >> + }
> >> +
> >> + return bitflips;
> >> +}
> >> +EXPORT_SYMBOL(nand_check_erased_buf);
> >> +
> >> +/**
> >> + * nand_check_erased_ecc_chunk - check if an ECC chunk contains (almost) only
> >> + * 0xff data
> >> + * @data: data buffer to test
> >> + * @datalen: data length
> >> + * @ecc: ECC buffer
> >> + * @ecclen: ECC length
> >> + * @extraoob: extra OOB buffer
> >> + * @extraooblen: extra OOB length
> >> + * @bitflips_threshold: maximum number of bitflips
> >> + *
> >> + * Check if a data buffer and its associated ECC and OOB data contains only
> >> + * 0xff pattern, which means the underlying region has been erased and is
> >> + * ready to be programmed.
> >> + * The bitflips_threshold specify the maximum number of bitflips before
> >> + * considering the region as not erased.
> >> + *
> >> + * Returns a positive number of bitflips or -ERROR_CODE.
> >> + */
> >> +int nand_check_erased_ecc_chunk(void *data, int datalen,
> >> + void *ecc, int ecclen,
> >> + void *extraoob, int extraooblen,
> >> + int bitflips_threshold)
> >> +{
> >> + int bitflips = 0;
> >> + int ret;
> >> +
> >> + ret = nand_check_erased_buf(data, datalen, bitflips_threshold);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + bitflips += ret;
> >> + bitflips_threshold -= ret;
> >> +
> >> + ret = nand_check_erased_buf(ecc, ecclen, bitflips_threshold);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + bitflips += ret;
> >> + bitflips_threshold -= ret;
> >> +
> >> + ret = nand_check_erased_buf(extraoob, extraooblen, bitflips_threshold);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >
> > Forgot the memset operations here:
> >
> > memset(data, 0xff, datalen);
> > memset(ecc, 0xff, ecclen);
> > memset(extraoob, 0xff, extraooblen);
>
> Yes, you're right.. I did the same mistake on my first implementation
> too ;-)
Hehe.
>
> As additional optimization you may also check if the lower layer already
> did the check for you (e.g. if you have an iMXQP as we saw in latest
> days), but I think it's a minor one, because you'll face this situation
> very very unlikely.
If the hardware is capable of doing such test (I mean counting the
number of bits to one and considering the page as erased under a given
limit of bitflips), there's a lot of chance it will implement its own
ecc_read_page function, and will never use this helper.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists