Bug in mtd_speedtest?

Bug in mtd_speedtest?

[David Lambert]

I think I may have found an overflow condition in the speed calculation 
of mtd_speedtest on some platforms with larger flash partitions:

Consider for example if goodebcnt = 15000, and mtd->erasesize = 
256*1024, then there is an intermediate product of  3932160000 which 
results in the sign bit being set on a 32 bit integer. Maybe k should be 
an unsigned long long?

.....
static long calc_speed(void)
{
     long ms, k, speed;

     ms = (finish.tv_sec - start.tv_sec) * 1000 +
          (finish.tv_usec - start.tv_usec) / 1000;
     k = goodebcnt * mtd->erasesize / 1024;
     speed = (k * 1000) / ms;
     return speed;
}
.....

Re: Bug in mtd_speedtest?

[Artem Bityutskiy]

On Thu, 2011-03-03 at 17:45 -0600, David Lambert wrote:
> I think I may have found an overflow condition in the speed calculation 
> of mtd_speedtest on some platforms with larger flash partitions:
> 
> Consider for example if goodebcnt = 15000, and mtd->erasesize = 
> 256*1024, then there is an intermediate product of  3932160000 which 
> results in the sign bit being set on a 32 bit integer. Maybe k should be 
> an unsigned long long?

Please, just send a patch :-)

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: Bug in mtd_speedtest?

[David Lambert]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

Artem,
     Please find attached patch.

Regards,

Dave.

On 03/04/2011 12:59 AM, Artem Bityutskiy wrote:
> On Thu, 2011-03-03 at 17:45 -0600, David Lambert wrote:
>> I think I may have found an overflow condition in the speed calculation
>> of mtd_speedtest on some platforms with larger flash partitions:
>>
>> Consider for example if goodebcnt = 15000, and mtd->erasesize =
>> 256*1024, then there is an intermediate product of  3932160000 which
>> results in the sign bit being set on a 32 bit integer. Maybe k should be
>> an unsigned long long?
> Please, just send a patch :-)
>


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1006 bytes --]

Index: mtd_speedtest.c
===================================================================
RCS file: /home/cvsroot/ECMB/src/snapgear/linux-2.6.30.1.x/drivers/mtd/tests/mtd_speedtest.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 mtd_speedtest.c
--- mtd_speedtest.c	10 Aug 2009 19:42:00 -0000	1.1.1.1
+++ mtd_speedtest.c	4 Mar 2011 17:51:23 -0000
@@ -281,13 +281,14 @@
 
 static long calc_speed(void)
 {
-	long ms, k, speed;
+	uint64_t k;
+	long ms;
 
-	ms = (finish.tv_sec - start.tv_sec) * 1000 +
-	     (finish.tv_usec - start.tv_usec) / 1000;
-	k = goodebcnt * mtd->erasesize / 1024;
-	speed = (k * 1000) / ms;
-	return speed;
+	ms = (finish.tv_sec - start.tv_sec) * 1000 + /* Time in milli-seconds from start to ...*/
+	     (finish.tv_usec - start.tv_usec) / 1000;	  /* finish */
+	k = (goodebcnt * (mtd->erasesize / 1024) * 1000); /* Number of kBytes transferred * 1000 */
+	do_div(k, ms);	  /* k now contains number of kBytes/second */
+	return k;
 }
 
 static int scan_for_bad_eraseblocks(void)

RE: Bug in mtd_speedtest?

[Jon Povey]

linux-mtd-bounces@lists.infradead.org wrote:
> Artem,
>      Please find attached patch.

The comments make lines over 80 chars and are not needed,
it is clear enough from the code what it does.

--
Jon Povey
jon.povey@racelogic.co.uk

Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .

The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network

Re: Bug in mtd_speedtest?

[Artem Bityutskiy]

On Fri, 2011-03-04 at 11:53 -0600, David Lambert wrote:
> Artem,
>      Please find attached patch.

Thank you, but you made pretty good job to prevent me from applying
it! :-)

> Index: mtd_speedtest.c
> ===================================================================
> RCS
> file: /home/cvsroot/ECMB/src/snapgear/linux-2.6.30.1.x/drivers/mtd/tests/mtd_speedtest.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 mtd_speedtest.c

This means git am won't work... neither patch -p1 ...
Also, not author name (From:), not commit message, not Signed-off-by...

> --- mtd_speedtest.c     10 Aug 2009 19:42:00 -0000      1.1.1.1
> +++ mtd_speedtest.c     4 Mar 2011 17:51:23 -0000
> @@ -281,13 +281,14 @@
>  
>  static long calc_speed(void)
>  {
> -       long ms, k, speed;
> +       uint64_t k;
> +       long ms;
>  
> -       ms = (finish.tv_sec - start.tv_sec) * 1000 +
> -            (finish.tv_usec - start.tv_usec) / 1000;
> -       k = goodebcnt * mtd->erasesize / 1024;
> -       speed = (k * 1000) / ms;
> -       return speed;
> +       ms = (finish.tv_sec - start.tv_sec) * 1000 + /* Time in milli-seconds from start to ...*/
> +            (finish.tv_usec - start.tv_usec) / 1000;     /* finish */

This would not pass scripts/checkpatch.pl. Please, kill the comments.
Also, I you use spaces instead of tabs...

> +       k = (goodebcnt * (mtd->erasesize / 1024) * 1000); /* Number of kBytes transferred * 1000 */
> +       do_div(k, ms);    /* k now contains number of kBytes/second */

The old code had a check against ms == 0, you removed it, why?

> +       return k;

Anyway, since I anyway spent 20 minutes with this patch, here is what I
ended up. I've pushed it to my l2 tree. If I made a mistake or you are
unhappy about it, please, send your *properly formatted* version
instead.

Thanks!

>From 008cfa97a772a83407a7e2657c0c5bfe36c7fc7f Mon Sep 17 00:00:00 2001
From: David Lambert <dave@lambsys.com>
Date: Mon, 7 Mar 2011 12:00:46 +0200
Subject: [PATCH] mtd: speedtest: fix integer overflow

32-bit integers used in 'calc_speed()' may overflow and lead to
incorrect results. Use 64-bit integers instead.

Signed-off-by: David Lambert <dave@lambsys.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 drivers/mtd/tests/mtd_speedtest.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/tests/mtd_speedtest.c b/drivers/mtd/tests/mtd_speedtest.c
index 3ce6fce..22b51c1 100644
--- a/drivers/mtd/tests/mtd_speedtest.c
+++ b/drivers/mtd/tests/mtd_speedtest.c
@@ -314,16 +314,16 @@ static inline void stop_timing(void)
 
 static long calc_speed(void)
 {
-	long ms, k, speed;
+       uint64_t k;
+       long ms;
 
 	ms = (finish.tv_sec - start.tv_sec) * 1000 +
 	     (finish.tv_usec - start.tv_usec) / 1000;
-	k = goodebcnt * mtd->erasesize / 1024;
-	if (ms)
-		speed = (k * 1000) / ms;
-	else
-		speed = 0;
-	return speed;
+	if (ms == 0)
+		return 0;
+	k = goodebcnt * (mtd->erasesize / 1024) * 1000;
+	do_div(k, ms);
+	return k;
 }
 
 static int scan_for_bad_eraseblocks(void)
-- 
1.7.2.3

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)