[PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[Lothar Waßmann]

When using prandom_bytes_state() it is critical to use the same block
size in all invocations that are to produce the same random sequence.
Otherwise the state of the PRNG will be out of sync if the blocksize
is not divisible by 4.
This leads to bogus verification errors in several tests which use
different block sizes to initialize the buffer for writing and
comparison.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/mtd/tests/oobtest.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
index 2e9e2d1..72c7359 100644
--- a/drivers/mtd/tests/oobtest.c
+++ b/drivers/mtd/tests/oobtest.c
@@ -213,8 +213,15 @@ static int verify_eraseblock_in_one_go(int ebnum)
 	int err = 0;
 	loff_t addr = ebnum * mtd->erasesize;
 	size_t len = mtd->ecclayout->oobavail * pgcnt;
+	int i;
+
+	for (i = 0; i < pgcnt; i++)
+		prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
+				use_len);
+	if (len % use_len)
+		prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
+				len % use_len);
 
-	prandom_bytes_state(&rnd_state, writebuf, len);
 	ops.mode      = MTD_OPS_AUTO_OOB;
 	ops.len       = 0;
 	ops.retlen    = 0;
@@ -594,7 +601,10 @@ static int __init mtd_oobtest_init(void)
 		if (bbt[i] || bbt[i + 1])
 			continue;
 		prandom_bytes_state(&rnd_state, writebuf,
-					mtd->ecclayout->oobavail * 2);
+					mtd->ecclayout->oobavail);
+		prandom_bytes_state(&rnd_state,
+					writebuf + mtd->ecclayout->oobavail,
+					mtd->ecclayout->oobavail);
 		addr = (i + 1) * mtd->erasesize - mtd->writesize;
 		ops.mode      = MTD_OPS_AUTO_OOB;
 		ops.len       = 0;
-- 
1.7.2.5

Re: [PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[Lothar Waßmann]

Hi,

Is anyone taking care of this?

Lothar Waßmann wrote:
> When using prandom_bytes_state() it is critical to use the same block
> size in all invocations that are to produce the same random sequence.
> Otherwise the state of the PRNG will be out of sync if the blocksize
> is not divisible by 4.
> This leads to bogus verification errors in several tests which use
> different block sizes to initialize the buffer for writing and
> comparison.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/mtd/tests/oobtest.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
> index 2e9e2d1..72c7359 100644
> --- a/drivers/mtd/tests/oobtest.c
> +++ b/drivers/mtd/tests/oobtest.c
> @@ -213,8 +213,15 @@ static int verify_eraseblock_in_one_go(int ebnum)
>  	int err = 0;
>  	loff_t addr = ebnum * mtd->erasesize;
>  	size_t len = mtd->ecclayout->oobavail * pgcnt;
> +	int i;
> +
> +	for (i = 0; i < pgcnt; i++)
> +		prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
> +				use_len);
> +	if (len % use_len)
> +		prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
> +				len % use_len);
>  
> -	prandom_bytes_state(&rnd_state, writebuf, len);
>  	ops.mode      = MTD_OPS_AUTO_OOB;
>  	ops.len       = 0;
>  	ops.retlen    = 0;
> @@ -594,7 +601,10 @@ static int __init mtd_oobtest_init(void)
>  		if (bbt[i] || bbt[i + 1])
>  			continue;
>  		prandom_bytes_state(&rnd_state, writebuf,
> -					mtd->ecclayout->oobavail * 2);
> +					mtd->ecclayout->oobavail);
> +		prandom_bytes_state(&rnd_state,
> +					writebuf + mtd->ecclayout->oobavail,
> +					mtd->ecclayout->oobavail);
>  		addr = (i + 1) * mtd->erasesize - mtd->writesize;
>  		ops.mode      = MTD_OPS_AUTO_OOB;
>  		ops.len       = 0;


-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[Akinobu Mita]

2014/1/22 Lothar Waßmann <LW@karo-electronics.de>:
> Hi,
>
> Is anyone taking care of this?
>
> Lothar Waßmann wrote:
>> When using prandom_bytes_state() it is critical to use the same block
>> size in all invocations that are to produce the same random sequence.
>> Otherwise the state of the PRNG will be out of sync if the blocksize
>> is not divisible by 4.
>> This leads to bogus verification errors in several tests which use
>> different block sizes to initialize the buffer for writing and
>> comparison.
>>
>> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>> ---
>>  drivers/mtd/tests/oobtest.c |   14 ++++++++++++--
>>  1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
>> index 2e9e2d1..72c7359 100644
>> --- a/drivers/mtd/tests/oobtest.c
>> +++ b/drivers/mtd/tests/oobtest.c
>> @@ -213,8 +213,15 @@ static int verify_eraseblock_in_one_go(int ebnum)
>>       int err = 0;
>>       loff_t addr = ebnum * mtd->erasesize;
>>       size_t len = mtd->ecclayout->oobavail * pgcnt;
>> +     int i;
>> +
>> +     for (i = 0; i < pgcnt; i++)
>> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
>> +                             use_len);
>> +     if (len % use_len)
>> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
>> +                             len % use_len);
>>
>> -     prandom_bytes_state(&rnd_state, writebuf, len);
>>       ops.mode      = MTD_OPS_AUTO_OOB;
>>       ops.len       = 0;
>>       ops.retlen    = 0;

I would rather fix the use of prandom_bytes_state() in write_eraseblock()
than fix in verify_eraseblock_in_one_go().

>> @@ -594,7 +601,10 @@ static int __init mtd_oobtest_init(void)
>>               if (bbt[i] || bbt[i + 1])
>>                       continue;
>>               prandom_bytes_state(&rnd_state, writebuf,
>> -                                     mtd->ecclayout->oobavail * 2);
>> +                                     mtd->ecclayout->oobavail);
>> +             prandom_bytes_state(&rnd_state,
>> +                                     writebuf + mtd->ecclayout->oobavail,
>> +                                     mtd->ecclayout->oobavail);
>>               addr = (i + 1) * mtd->erasesize - mtd->writesize;
>>               ops.mode      = MTD_OPS_AUTO_OOB;
>>               ops.len       = 0;

The same goes for this.  I would rather fix prandom_bytes_state() when
writing OOBs.

Re: [PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[Lothar Waßmann]

Hi,

Akinobu Mita wrote:
> 2014/1/22 Lothar Waßmann <LW@karo-electronics.de>:
> > Hi,
> >
> > Is anyone taking care of this?
> >
> > Lothar Waßmann wrote:
> >> When using prandom_bytes_state() it is critical to use the same block
> >> size in all invocations that are to produce the same random sequence.
> >> Otherwise the state of the PRNG will be out of sync if the blocksize
> >> is not divisible by 4.
> >> This leads to bogus verification errors in several tests which use
> >> different block sizes to initialize the buffer for writing and
> >> comparison.
> >>
> >> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> >> ---
> >>  drivers/mtd/tests/oobtest.c |   14 ++++++++++++--
> >>  1 files changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
> >> index 2e9e2d1..72c7359 100644
> >> --- a/drivers/mtd/tests/oobtest.c
> >> +++ b/drivers/mtd/tests/oobtest.c
> >> @@ -213,8 +213,15 @@ static int verify_eraseblock_in_one_go(int ebnum)
> >>       int err = 0;
> >>       loff_t addr = ebnum * mtd->erasesize;
> >>       size_t len = mtd->ecclayout->oobavail * pgcnt;
> >> +     int i;
> >> +
> >> +     for (i = 0; i < pgcnt; i++)
> >> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
> >> +                             use_len);
> >> +     if (len % use_len)
> >> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
> >> +                             len % use_len);
> >>
> >> -     prandom_bytes_state(&rnd_state, writebuf, len);
> >>       ops.mode      = MTD_OPS_AUTO_OOB;
> >>       ops.len       = 0;
> >>       ops.retlen    = 0;
> 
> I would rather fix the use of prandom_bytes_state() in write_eraseblock()
> than fix in verify_eraseblock_in_one_go().
> 
Why and how?
write_whole_device() (which calls write_eraseblock()) is used multiple
times with different verification methods (all blocks in one go or each
block individually).
If prandom_state_bytes() in write_eraseblock() would be changed, that
function would have to know, how the block are going to be checked
lateron to know how to set up the writebuffer.

Dto. for mtd_write_oob()


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[Akinobu Mita]

2014/1/23 Lothar Waßmann <LW@karo-electronics.de>:
> Hi,
>
> Akinobu Mita wrote:
>> 2014/1/22 Lothar Waßmann <LW@karo-electronics.de>:
>> > Hi,
>> >
>> > Is anyone taking care of this?
>> >
>> > Lothar Waßmann wrote:
>> >> When using prandom_bytes_state() it is critical to use the same block
>> >> size in all invocations that are to produce the same random sequence.
>> >> Otherwise the state of the PRNG will be out of sync if the blocksize
>> >> is not divisible by 4.
>> >> This leads to bogus verification errors in several tests which use
>> >> different block sizes to initialize the buffer for writing and
>> >> comparison.
>> >>
>> >> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
>> >> ---
>> >>  drivers/mtd/tests/oobtest.c |   14 ++++++++++++--
>> >>  1 files changed, 12 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
>> >> index 2e9e2d1..72c7359 100644
>> >> --- a/drivers/mtd/tests/oobtest.c
>> >> +++ b/drivers/mtd/tests/oobtest.c
>> >> @@ -213,8 +213,15 @@ static int verify_eraseblock_in_one_go(int ebnum)
>> >>       int err = 0;
>> >>       loff_t addr = ebnum * mtd->erasesize;
>> >>       size_t len = mtd->ecclayout->oobavail * pgcnt;
>> >> +     int i;
>> >> +
>> >> +     for (i = 0; i < pgcnt; i++)
>> >> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
>> >> +                             use_len);
>> >> +     if (len % use_len)
>> >> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
>> >> +                             len % use_len);
>> >>
>> >> -     prandom_bytes_state(&rnd_state, writebuf, len);
>> >>       ops.mode      = MTD_OPS_AUTO_OOB;
>> >>       ops.len       = 0;
>> >>       ops.retlen    = 0;
>>
>> I would rather fix the use of prandom_bytes_state() in write_eraseblock()
>> than fix in verify_eraseblock_in_one_go().
>>
> Why and how?

I thought that it could reduce calls of prandom_bytes_state() and
it makes code simpler than increasing calls.

> write_whole_device() (which calls write_eraseblock()) is used multiple
> times with different verification methods (all blocks in one go or each
> block individually).
> If prandom_state_bytes() in write_eraseblock() would be changed, that
> function would have to know, how the block are going to be checked
> lateron to know how to set up the writebuffer.

Instead of calling prandom_bytes_state() in the for loop in
write_eraseblock(), call prandom_bytes_state() at once before going
into the loop and use correct offset in writebuf in the loop.
Although, we also need to fix verify_eraseblock() in the same way.

Doesn't that fix this problem?

Re: [PATCH] mtd: mtd_oobtest: fix verify errors due to incorrect use of prandom_bytes_state()

[Lothar Waßmann]

Hi,

Akinobu Mita wrote:
> 2014/1/23 Lothar Waßmann <LW@karo-electronics.de>:
> > Hi,
> >
> > Akinobu Mita wrote:
> >> 2014/1/22 Lothar Waßmann <LW@karo-electronics.de>:
> >> > Hi,
> >> >
> >> > Is anyone taking care of this?
> >> >
> >> > Lothar Waßmann wrote:
> >> >> When using prandom_bytes_state() it is critical to use the same block
> >> >> size in all invocations that are to produce the same random sequence.
> >> >> Otherwise the state of the PRNG will be out of sync if the blocksize
> >> >> is not divisible by 4.
> >> >> This leads to bogus verification errors in several tests which use
> >> >> different block sizes to initialize the buffer for writing and
> >> >> comparison.
> >> >>
> >> >> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> >> >> ---
> >> >>  drivers/mtd/tests/oobtest.c |   14 ++++++++++++--
> >> >>  1 files changed, 12 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/drivers/mtd/tests/oobtest.c b/drivers/mtd/tests/oobtest.c
> >> >> index 2e9e2d1..72c7359 100644
> >> >> --- a/drivers/mtd/tests/oobtest.c
> >> >> +++ b/drivers/mtd/tests/oobtest.c
> >> >> @@ -213,8 +213,15 @@ static int verify_eraseblock_in_one_go(int ebnum)
> >> >>       int err = 0;
> >> >>       loff_t addr = ebnum * mtd->erasesize;
> >> >>       size_t len = mtd->ecclayout->oobavail * pgcnt;
> >> >> +     int i;
> >> >> +
> >> >> +     for (i = 0; i < pgcnt; i++)
> >> >> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
> >> >> +                             use_len);
> >> >> +     if (len % use_len)
> >> >> +             prandom_bytes_state(&rnd_state, &writebuf[i * use_len],
> >> >> +                             len % use_len);
> >> >>
> >> >> -     prandom_bytes_state(&rnd_state, writebuf, len);
> >> >>       ops.mode      = MTD_OPS_AUTO_OOB;
> >> >>       ops.len       = 0;
> >> >>       ops.retlen    = 0;
> >>
> >> I would rather fix the use of prandom_bytes_state() in write_eraseblock()
> >> than fix in verify_eraseblock_in_one_go().
> >>
> > Why and how?
> 
> I thought that it could reduce calls of prandom_bytes_state() and
> it makes code simpler than increasing calls.
> 
> > write_whole_device() (which calls write_eraseblock()) is used multiple
> > times with different verification methods (all blocks in one go or each
> > block individually).
> > If prandom_state_bytes() in write_eraseblock() would be changed, that
> > function would have to know, how the block are going to be checked
> > lateron to know how to set up the writebuffer.
> 
> Instead of calling prandom_bytes_state() in the for loop in
> write_eraseblock(), call prandom_bytes_state() at once before going
> into the loop and use correct offset in writebuf in the loop.
> Although, we also need to fix verify_eraseblock() in the same way.
> 
> Doesn't that fix this problem?
>
Of course one could fix it that way, but that would be a much more
invasive change that also needs more testing.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________