When run mtd_oobtest case, there will be one error for step(4),
which turned out it need add one check for out of page read in
nand_do_read_oob just like mtd_do_write_oob did it already.
This commit also fix one typo error for comments in mtd_do_write_oob
Signed-off-by: Jason Liu <[email protected]>
---
drivers/mtd/nand/nand_base.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 1f75a1b..75d199e 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
else
len = mtd->oobsize;
+ /* Do not allow read past end of page */
+ if ((ops->ooboffs + readlen) > len) {
+ DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read "
+ "past end of page\n", __func__);
+ return -EINVAL;
+ }
+
if (unlikely(ops->ooboffs >= len)) {
DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
"outside oob\n", __func__);
@@ -2377,7 +2384,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
return -EINVAL;
}
- /* Do not allow reads past end of device */
+ /* Do not allow write past end of device */
if (unlikely(to >= mtd->size ||
ops->ooboffs + ops->ooblen >
((mtd->size >> chip->page_shift) -
--
1.7.0.4
On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
>
> Signed-off-by: Jason Liu <[email protected]>
> ---
> drivers/mtd/nand/nand_base.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
Pushed to l2-mtd-2.6.git, thanks!
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
>
> Signed-off-by: Jason Liu <[email protected]>
> ---
> drivers/mtd/nand/nand_base.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 1f75a1b..75d199e 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1782,6 +1782,13 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
> else
> len = mtd->oobsize;
>
> + /* Do not allow read past end of page */
> + if ((ops->ooboffs + readlen) > len) {
> + DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to read "
> + "past end of page\n", __func__);
> + return -EINVAL;
> + }
As you reported to me in a private e-mail (although I prefer to always
have a public ML in CC when dealing with open source stuff) this patch
is wrong. Indeed, it limits the maximum amount of bytes which can be
read at one go to the OOB size, which is incorrect, because
mtd->read_oob() allows reading multiple pages at a time, see comment
near "struct mtd_oob_ops" at include/linux/mtd/mtd.h. So this patch
breaks ABI and hence, has to be reverted.
> if (unlikely(ops->ooboffs >= len)) {
> DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt to start read "
> "outside oob\n", __func__);
Side note: nand_base.c has a bunch of senseless unlikely() hints, would
be nice to clean that up some day.
> - /* Do not allow reads past end of device */
> + /* Do not allow write past end of device */
Care to make this a separate clean-up patch meanwhile?
Thank!
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
> When run mtd_oobtest case, there will be one error for step(4),
> which turned out it need add one check for out of page read in
> nand_do_read_oob just like mtd_do_write_oob did it already.
> This commit also fix one typo error for comments in mtd_do_write_oob
>
> Signed-off-by: Jason Liu <[email protected]>
I cannot reproduce this with nandsim, can you?
I tried:
1. sudo modprobe nandsim
sudo modprobe mtd_oobtest dev=0
2. sudo ./load_nandsim.sh 128 128 2048
sudo modprobe mtd_oobtest dev=0
In both cases the test passes: "mtd_oobtest: finished with 0 errors"
And in 'nand_do_read_oob()' I see the following piece of code:
/* Do not allow reads past end of device */
if (unlikely(from >= mtd->size ||
ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
(from >> chip->page_shift)) * len)) {
DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
"of device\n", __func__);
return -EINVAL;
}
which should handle the case AFAICS.
So, although I did think hard about this, it looks like there is no
problem. What is the kernel version you are looking at? I checked it
with my l2-mtd-2.6.git, which is the latest mtd-2.6.git plus a revert
commit of your patch:
http://git.infradead.org/users/dedekind/l2-mtd-2.6.git
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
Hi, Artem,
2010/12/14 Artem Bityutskiy <[email protected]>:
> On Fri, 2010-11-19 at 16:40 +0800, Jason Liu wrote:
>> When run mtd_oobtest case, there will be one error for step(4),
>> which turned out it need add one check for out of page read in
>> nand_do_read_oob just like mtd_do_write_oob did it already.
>> This commit also fix one typo error for comments in mtd_do_write_oob
>>
>> Signed-off-by: Jason Liu <[email protected]>
>
> I cannot reproduce this with nandsim, can you?
>
> I tried:
>
> 1. sudo modprobe nandsim
> ? sudo modprobe mtd_oobtest dev=0
>
> 2. sudo ./load_nandsim.sh 128 128 2048
> ? sudo modprobe mtd_oobtest dev=0
>
> In both cases the test passes: "mtd_oobtest: finished with 0 errors"
>
> And in 'nand_do_read_oob()' I see the following piece of code:
>
> ? ? ? ?/* Do not allow reads past end of device */
> ? ? ? ?if (unlikely(from >= mtd->size ||
> ? ? ? ? ? ? ? ? ? ? ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(from >> chip->page_shift)) * len)) {
> ? ? ? ? ? ? ? ?DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"of device\n", __func__);
> ? ? ? ? ? ? ? ?return -EINVAL;
> ? ? ? ?}
Here the mtd->size in nand_base.c should be the NAND flash chip size,
while in nand_oobtest,
/* Attempt to read off end of device */
ops.mode = MTD_OOB_AUTO;
ops.len = 0;
ops.retlen = 0;
ops.ooblen = mtd->ecclayout->oobavail;
ops.oobretlen = 0;
ops.ooboffs = 1;
ops.datbuf = NULL;
ops.oobbuf = readbuf;
printk(PRINT_PREF "attempting to read past end of device\n");
printk(PRINT_PREF "an error is expected...\n");
err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops);
if (err) {
printk(PRINT_PREF "error occurred as expected\n");
err = 0;
} else {
printk(PRINT_PREF "error: read past end of device\n");
errcnt += 1;
}
}
here, mtd->size is mtd partition size right? when the mtd partition is
not the last MTD partition, the error will happen.
I have tested on real NAND, while not on nandsim.
>
> which should handle the case AFAICS.
>
> So, although I did think hard about this, it looks like there is no
> problem. What is the kernel version you are looking at? I checked it
> with my l2-mtd-2.6.git, which is the latest mtd-2.6.git plus a revert
> commit of your patch:
>
> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git
Yes, as I have written the email to you before in a private email to
tell that this patch has issue and please drop it.
Thanks,
>
> --
> Best Regards,
> Artem Bityutskiy (??ԣ? ????????)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote:
> > /* Do not allow reads past end of device */
> > if (unlikely(from >= mtd->size ||
> > ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
> > (from >> chip->page_shift)) * len)) {
> > DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
> > "of device\n", __func__);
> > return -EINVAL;
> > }
>
> Here the mtd->size in nand_base.c should be the NAND flash chip size,
I think this is partition size as well.
> while in nand_oobtest,
>
> /* Attempt to read off end of device */
> ops.mode = MTD_OOB_AUTO;
> ops.len = 0;
> ops.retlen = 0;
> ops.ooblen = mtd->ecclayout->oobavail;
> ops.oobretlen = 0;
> ops.ooboffs = 1;
> ops.datbuf = NULL;
> ops.oobbuf = readbuf;
> printk(PRINT_PREF "attempting to read past end of device\n");
> printk(PRINT_PREF "an error is expected...\n");
> err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops);
> if (err) {
> printk(PRINT_PREF "error occurred as expected\n");
> err = 0;
> } else {
> printk(PRINT_PREF "error: read past end of device\n");
> errcnt += 1;
> }
> }
>
> here, mtd->size is mtd partition size right? when the mtd partition is
> not the last MTD partition, the error will happen.
I think this is the same in both. Try to debug this with printks().
>
> I have tested on real NAND, while not on nandsim.
Please, try to reproduce this with nandsim as well, and if it works, try
to find out why (using printk debugging)
--
Best Regards,
Artem Bityutskiy (Битюцкий Артём)
Hi, Artem,
2010/12/20 Artem Bityutskiy <[email protected]>:
> On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote:
>> > ? ? ? ?/* Do not allow reads past end of device */
>> > ? ? ? ?if (unlikely(from >= mtd->size ||
>> > ? ? ? ? ? ? ? ? ? ? ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) -
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(from >> chip->page_shift)) * len)) {
>> > ? ? ? ? ? ? ? ?DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end "
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"of device\n", __func__);
>> > ? ? ? ? ? ? ? ?return -EINVAL;
>> > ? ? ? ?}
>>
>> Here the mtd->size in nand_base.c should be the NAND flash chip size,
>
> I think this is partition size as well.
I think you are wrong. This should be the NAND flash chip size not the
partition size.
>
>> while in nand_oobtest,
>>
>> ? ? ? ? ? ? ? /* Attempt to read off end of device */
>> ? ? ? ? ? ? ? ? ops.mode ? ? ?= MTD_OOB_AUTO;
>> ? ? ? ? ? ? ? ? ops.len ? ? ? = 0;
>> ? ? ? ? ? ? ? ? ops.retlen ? ?= 0;
>> ? ? ? ? ? ? ? ? ops.ooblen ? ?= mtd->ecclayout->oobavail;
>> ? ? ? ? ? ? ? ? ops.oobretlen = 0;
>> ? ? ? ? ? ? ? ? ops.ooboffs ? = 1;
>> ? ? ? ? ? ? ? ? ops.datbuf ? ?= NULL;
>> ? ? ? ? ? ? ? ? ops.oobbuf ? ?= readbuf;
>> ? ? ? ? ? ? ? ? printk(PRINT_PREF "attempting to read past end of device\n");
>> ? ? ? ? ? ? ? ? printk(PRINT_PREF "an error is expected...\n");
>> ? ? ? ? ? ? ? ? err = mtd->read_oob(mtd, mtd->size - mtd->writesize, &ops);
>> ? ? ? ? ? ? ? ? if (err) {
>> ? ? ? ? ? ? ? ? ? ? ? ? printk(PRINT_PREF "error occurred as expected\n");
>> ? ? ? ? ? ? ? ? ? ? ? ? err = 0;
>> ? ? ? ? ? ? ? ? } else {
>> ? ? ? ? ? ? ? ? ? ? ? ? printk(PRINT_PREF "error: read past end of device\n");
>> ? ? ? ? ? ? ? ? ? ? ? ? errcnt += 1;
>> ? ? ? ? ? ? ? ? }
>> ? ? ? ? }
>>
>> here, mtd->size is mtd partition size right? when the mtd partition is
>> not the last MTD partition, the error will happen.
>
> I think this is the same in both. Try to debug this with printks().
>
>>
>> I have tested on real NAND, while not on nandsim.
>
> Please, try to reproduce this with nandsim as well, and if it works, try
> to find out why (using printk debugging)
Yes, I have reproduce this issue with nandsim, please check the following log,
root@freescale ~$ insmod nandsim.ko first_id_byte=0x20
second_id_byte=0xaa third_id_byte=0x00 fourth_id_byte=0x15
parts=0x400,0x400
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
[nandsim] warning: read_byte: unexpected data output cycle, state is
STATE_READY return 0x0
NAND device: Manufacturer ID: 0x20, Chip ID: 0xaa (ST Micro NAND
256MiB 1,8V 8-bit)
flash size: 256 MiB
page size: 2048 bytes
OOB area size: 64 bytes
sector size: 128 KiB
pages number: 131072
pages per sector: 64
bus width: 8
bits in sector size: 17
bits in page size: 11
bits in OOB size: 6
flash size with OOB: 270336 KiB
page address bytes: 5
sector address bytes: 3
options: 0x8
Scanning device for bad blocks
Creating 2 MTD partitions on "NAND 256MiB 1,8V 8-bit":
0x000000000000-0x000008000000 : "NAND simulator partition 0"
0x000008000000-0x000010000000 : "NAND simulator partition 1"
root@freescale ~$ cat /proc/mtd
dev: size erasesize name
mtd0: 00300000 00080000 "bootloader"
mtd1: 00500000 00080000 "nand.kernel"
mtd2: 10000000 00080000 "nand.rootfs"
mtd3: 10000000 00080000 "nand.userfs1"
mtd4: df800000 00080000 "nand.userfs2"
mtd5: 08000000 00020000 "NAND simulator partition 0"
mtd6: 08000000 00020000 "NAND simulator partition 1"
root@freescale ~$ insmod mtd_oobtest.ko dev=5
=================================================
mtd_oobtest: MTD device: 5
mtd_oobtest: MTD device size 134217728, eraseblock size 131072, page
size 2048, count of eraseblocks 1024, pages per eraseblock
64, OOB size 64
mtd_oobtest: scanning for bad eraseblocks
mtd_oobtest: scanned 1024 eraseblocks, 0 are bad
mtd_oobtest: test 1 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1024 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1024 eraseblocks
mtd_oobtest: test 2 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1024 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1024 eraseblocks
mtd_oobtest: test 3 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1024 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1024 eraseblocks
mtd_oobtest: test 4 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: attempting to start write past end of OOB
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to start read past end of OOB
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to write past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to read past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error: read past end of device
mtd_oobtest: attempting to write past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error occurred as expected
mtd_oobtest: attempting to read past end of device
mtd_oobtest: an error is expected...
mtd_oobtest: error: read past end of device
mtd_oobtest: test 5 of 5
mtd_oobtest: erasing whole device
mtd_oobtest: erased 1024 eraseblocks
mtd_oobtest: writing OOBs of whole device
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 0
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 256
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 512
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written up to eraseblock 768
mtd_oobtest: written 1023 eraseblocks
mtd_oobtest: verifying all eraseblocks
mtd_oobtest: verified up to eraseblock 0
mtd_oobtest: verified up to eraseblock 256
mtd_oobtest: verified up to eraseblock 512
mtd_oobtest: verified up to eraseblock 768
mtd_oobtest: verified 1023 eraseblocks
mtd_oobtest: finished with 2 errors
=================================================
>
> --
> Best Regards,
> Artem Bityutskiy (???????? ??ԣ?)
>
>