[PATCH] scsi disk: report size without overflow

[PATCH] scsi disk: report size without overflow

[Randy.Dunlap]

From: Randy Dunlap <rdunlap@xenotime.net>

With CONFIG_LBD=n, 'sz' (32 bits) can overflow when capacity is
multiplied (even * 2), as seen in a report from Dale Blount
on lkml.  Also make sure that 'mb' will not overflow.

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---

 drivers/scsi/sd.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff -Naurp linux-2614-rc5/drivers/scsi/sd.c~scsi_disk_capacity linux-2614-rc5/drivers/scsi/sd.c
--- linux-2614-rc5/drivers/scsi/sd.c~scsi_disk_capacity	2005-10-20 18:55:35.000000000 -0700
+++ linux-2614-rc5/drivers/scsi/sd.c	2005-10-20 20:44:44.000000000 -0700
@@ -49,6 +49,7 @@
 #include <linux/blkpg.h>
 #include <linux/kref.h>
 #include <linux/delay.h>
+#include <asm/div64.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -1253,16 +1254,16 @@ got_data:
 		 * Jacques Gelinas (Jacques@solucorp.qc.ca)
 		 */
 		int hard_sector = sector_size;
-		sector_t sz = sdkp->capacity * (hard_sector/256);
+		u64 sz = sdkp->capacity * (hard_sector/256);
 		request_queue_t *queue = sdp->request_queue;
-		sector_t mb;
+		u64 mb;
 
 		blk_queue_hardsect_size(queue, hard_sector);
 		/* avoid 64-bit division on 32-bit platforms */
 		mb = sz >> 1;
-		sector_div(sz, 1250);
+		do_div(sz, 1250);
 		mb -= sz - 974;
-		sector_div(mb, 1950);
+		do_div(mb, 1950);
 
 		printk(KERN_NOTICE "SCSI device %s: "
 		       "%llu %d-byte hdwr sectors (%llu MB)\n",


---

Re: [PATCH] scsi disk: report size without overflow

[James Bottomley]

On Thu, 2005-10-20 at 20:55 -0700, Randy.Dunlap wrote:
> With CONFIG_LBD=n, 'sz' (32 bits) can overflow when capacity is
> multiplied (even * 2), as seen in a report from Dale Blount
> on lkml.  Also make sure that 'mb' will not overflow.
> 
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>

Mathematically, it doesn't look like we need to use 64 bits at all;
what's wrong with the attached?  It looks like the code once allowed 256
byte sectors, but the if statement above now forbids them.

I'm really not terribly sympathetic to anyone trying Terrabyte discs on
a non-LBD 32 bit kernel.  The potential for overflow (and consequent
data loss) is pretty huge. 

James

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1253,14 +1253,13 @@ got_data:
 		 * Jacques Gelinas (Jacques@solucorp.qc.ca)
 		 */
 		int hard_sector = sector_size;
-		sector_t sz = sdkp->capacity * (hard_sector/256);
+		sector_t sz = sdkp->capacity * (hard_sector/512);
 		request_queue_t *queue = sdp->request_queue;
-		sector_t mb;
+		sector_t mb = sz;
 
 		blk_queue_hardsect_size(queue, hard_sector);
 		/* avoid 64-bit division on 32-bit platforms */
-		mb = sz >> 1;
-		sector_div(sz, 1250);
+		sector_div(sz, 625);
 		mb -= sz - 974;
 		sector_div(mb, 1950);
 

Re: [PATCH] scsi disk: report size without overflow

[Randy.Dunlap]

On Fri, 21 Oct 2005, James Bottomley wrote:

> On Thu, 2005-10-20 at 20:55 -0700, Randy.Dunlap wrote:
> > With CONFIG_LBD=n, 'sz' (32 bits) can overflow when capacity is
> > multiplied (even * 2), as seen in a report from Dale Blount
> > on lkml.  Also make sure that 'mb' will not overflow.
> >
> > Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
>
> Mathematically, it doesn't look like we need to use 64 bits at all;
> what's wrong with the attached?  It looks like the code once allowed 256
> byte sectors, but the if statement above now forbids them.

What if statement forbids 256-byte sectors?  I don't see it.

	if (sector_size != 512 &&
	    sector_size != 1024 &&
	    sector_size != 2048 &&
	    sector_size != 4096 &&
	    sector_size != 256) {
		printk(KERN_NOTICE "%s : unsupported sector size "
		       "%d.\n", diskname, sector_size);

allows sector_size of 256.

> I'm really not terribly sympathetic to anyone trying Terrabyte discs on
> a non-LBD 32 bit kernel.  The potential for overflow (and consequent
> data loss) is pretty huge.

Yes, I agree with that.

> James
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1253,14 +1253,13 @@ got_data:
>  		 * Jacques Gelinas (Jacques@solucorp.qc.ca)
>  		 */
>  		int hard_sector = sector_size;
> -		sector_t sz = sdkp->capacity * (hard_sector/256);
> +		sector_t sz = sdkp->capacity * (hard_sector/512);
>  		request_queue_t *queue = sdp->request_queue;
> -		sector_t mb;
> +		sector_t mb = sz;
>
>  		blk_queue_hardsect_size(queue, hard_sector);
>  		/* avoid 64-bit division on 32-bit platforms */
> -		mb = sz >> 1;
> -		sector_div(sz, 1250);
> +		sector_div(sz, 625);
>  		mb -= sz - 974;
>  		sector_div(mb, 1950);
>
>
>
>

-- 
~Randy

Re: [PATCH] scsi disk: report size without overflow

[Dale Blount]

> > I'm really not terribly sympathetic to anyone trying Terrabyte discs on
> > a non-LBD 32 bit kernel.  The potential for overflow (and consequent
> > data loss) is pretty huge.
> 
> Yes, I agree with that.
> 

I'll agree with that as well, but there should be a note left on
CONFIG_LBD's help menu that states 1TB (or whatever the actual limit is)
instead of 2TB then.

Also are there any performance implications for using CONFIG_LBD?

Dale

Re: [PATCH] scsi disk: report size without overflow

[James Bottomley]

On Fri, 2005-10-21 at 17:02 -0400, Dale Blount wrote:
> I'll agree with that as well, but there should be a note left on
> CONFIG_LBD's help menu that states 1TB (or whatever the actual limit is)
> instead of 2TB then.

No one's really bothered to find out, I suspect.  If the kernel is using
block offsets as all unsigned (which I'm  not sure anyone's fully
validated), and the block size is 512, then 2TB is the limit.  For
signed, which is a safer assumption, it's 1TB.  However, you also have
the constant source of overflow danger, like you just discovered.  Every
arithmetical calculation has to divide first then multiply (because the
reverse will overflow). Note also that fdisk overflows beyond a certain
point (which I suspect is also 1TB).

> Also are there any performance implications for using CONFIG_LBD?

Yes, on 32 bits sector_t becomes unsigned long long (a 64 bit quantity).
This takes double the storage (or registers).  I don't think it will
affect storage performance any, but it increases the sizes in the kernel
and makes the processor do a bit more work.

James

Re: [PATCH] scsi disk: report size without overflow

[James Bottomley]

On Fri, 2005-10-21 at 13:46 -0700, Randy.Dunlap wrote:
> On Fri, 21 Oct 2005, James Bottomley wrote:
> > what's wrong with the attached?  It looks like the code once allowed 256
> > byte sectors, but the if statement above now forbids them.
> 
> What if statement forbids 256-byte sectors?  I don't see it.
> 
> 	if (sector_size != 512 &&
> 	    sector_size != 1024 &&
> 	    sector_size != 2048 &&
> 	    sector_size != 4096 &&
> 	    sector_size != 256) {

Sorry, didn't read far enough down the if statement.

To support 256, we can still do it, but the assignment has to become

sector_t sz = (sdkp->capacity/2) * (hard_sector/256);

James

Re: [PATCH] scsi disk: report size without overflow

[Randy.Dunlap]

On Fri, 21 Oct 2005 16:17:17 -0500 James Bottomley wrote:

> On Fri, 2005-10-21 at 13:46 -0700, Randy.Dunlap wrote:
> > On Fri, 21 Oct 2005, James Bottomley wrote:
> > > what's wrong with the attached?  It looks like the code once allowed 256
> > > byte sectors, but the if statement above now forbids them.
> > 
> > What if statement forbids 256-byte sectors?  I don't see it.
> > 
> > 	if (sector_size != 512 &&
> > 	    sector_size != 1024 &&
> > 	    sector_size != 2048 &&
> > 	    sector_size != 4096 &&
> > 	    sector_size != 256) {
> 
> Sorry, didn't read far enough down the if statement.
> 
> To support 256, we can still do it, but the assignment has to become
> 
> sector_t sz = (sdkp->capacity/2) * (hard_sector/256);

OK by me.

Thanks,
---
~Randy