Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Bart Van Assche]

On Tue, 2017-04-04 at 19:35 -0400, Martin K. Petersen wrote:
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fb9b4d29af0b..6084c415c070 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2102,6 +2102,16 @@ static void read_capacity_error(struct scsi_disk 
> *sdkp, struct scsi_device *sdp,
>  
>  #define READ_CAPACITY_RETRIES_ON_RESET   10
>  
> +static bool sd_addressable_capacity(u64 lba, unsigned int sector_size)
> +{
> + u64 last_sector = lba + 1ULL << ilog2(sector_size) - 9;
> +
> + if (sizeof(sector_t) == 4 && last_sector > 0xULL)
> + return false;
> +
> + return true;
> +}

Hello Martin,

How about replacing 0xULL with U32_MAX, adding parentheses in the
last_sector computation to make clear that + and - have precedence over <<
and adding a comment above sd_addressable_capacity() that explains its
purpose and also that the shift operation must not be replaced with a call
to logical_to_sectors()? Otherwise this patch looks fine to me.

Thanks,

Bart.

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Martin K. Petersen]

Bart Van Assche  writes:

Hi Bart,

> Sorry but I still don't understand why the two checks are
> different. How about the (untested) patch below? The approach below
> avoids that the check is duplicated and - at least in my opinion -
> results in code that is easier to read.

Just tripped over this issue in connection with something else. However,
I had to make a few passes to convince myself that your proposed fix was
correct. How about something like the following?

Martin

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fb9b4d29af0b..6084c415c070 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2102,6 +2102,16 @@ static void read_capacity_error(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
 
 #define READ_CAPACITY_RETRIES_ON_RESET 10
 
+static bool sd_addressable_capacity(u64 lba, unsigned int sector_size)
+{
+   u64 last_sector = lba + 1ULL << ilog2(sector_size) - 9;
+
+   if (sizeof(sector_t) == 4 && last_sector > 0xULL)
+   return false;
+
+   return true;
+}
+
 static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
unsigned char *buffer)
 {
@@ -2167,7 +2177,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return -ENODEV;
}
 
-   if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {
+   if (!sd_addressable_capacity(lba, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2256,7 +2266,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return sector_size;
}
 
-   if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {
+   if (!sd_addressable_capacity(lba, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Steve Magnani]

On 02/27/2017 12:57 PM, Bart Van Assche wrote:

...
How about the (untested) patch below? The approach below avoids that the check 
is
duplicated and - at least in my opinion - results in code that is easier to 
read.
I find lba_too_large() a little dense, but functionally OK. The "shift 
>= 0" clause could be dropped.

I tested this on my "problem" system (READ CAPACITY(10)) without incident.


diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb6e68dd6df0..3533d1e46bde 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2082,6 +2082,16 @@ static void read_capacity_error(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
sdkp->capacity = 0; /* unknown mapped to zero - as usual */
  }
  
+/*

+ * Check whether or not logical_to_sectors(sdp, lba) will overflow.
+ */
+static bool lba_too_large(u64 lba, u32 logical_block_size)
+{
+   int shift = sizeof(sector_t) * 8 + 9 - ilog2(logical_block_size);
+
+   return shift >= 0 && shift < 64 && lba >= (1ULL << shift);
+}
+
  #define RC16_LEN 32
  #if RC16_LEN > SD_BUF_SIZE
  #error RC16_LEN must not be more than SD_BUF_SIZE
@@ -2154,7 +2164,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   if (lba_too_large(lba + 1, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2243,7 +2253,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   if (lba_too_large(lba + 1ULL, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Martin K. Petersen]

> "Bart" == Bart Van Assche  writes:

Bart,

Bart> Sorry but I still don't understand why the two checks are
Bart> different. How about the (untested) patch below? The approach
Bart> below avoids that the check is duplicated and - at least in my
Bart> opinion - results in code that is easier to read.

I'll take a closer look at your patch tomorrow. I am sympathetic to
having a sanity check helper function. That would also give us a single
place to filter out crackpot values reported by USB doodads.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Bart Van Assche]

On Mon, 2017-02-27 at 11:13 -0600, Steve Magnani wrote:
> On 02/27/2017 10:13 AM, Bart Van Assche wrote:
> > Why are the two checks slightly different? Could the same code be used for
> > both checks?
> 
> The checks are different because with READ CAPACITY(16) a _really_ huge 
> device could report a max LBA so large that left-shifting it causes bits 
> to drop off the end. That's not an issue with READ CAPACITY(10) because 
> at most the 32-bit LBA reported by the device will become a 35-bit value 
> (since the max supported block size is 4096 == (512 << 3)).

Sorry but I still don't understand why the two checks are different. How about
the (untested) patch below? The approach below avoids that the check is
duplicated and - at least in my opinion - results in code that is easier to 
read.

Thanks,

Bart.


diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb6e68dd6df0..3533d1e46bde 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2082,6 +2082,16 @@ static void read_capacity_error(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
sdkp->capacity = 0; /* unknown mapped to zero - as usual */
 }
 
+/*
+ * Check whether or not logical_to_sectors(sdp, lba) will overflow.
+ */
+static bool lba_too_large(u64 lba, u32 logical_block_size)
+{
+   int shift = sizeof(sector_t) * 8 + 9 - ilog2(logical_block_size);
+
+   return shift >= 0 && shift < 64 && lba >= (1ULL << shift);
+}
+
 #define RC16_LEN 32
 #if RC16_LEN > SD_BUF_SIZE
 #error RC16_LEN must not be more than SD_BUF_SIZE
@@ -2154,7 +2164,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return -ENODEV;
}
 
-   if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {
+   if (lba_too_large(lba + 1, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2243,7 +2253,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, 
struct scsi_device *sdp,
return sector_size;
}
 
-   if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {
+   if (lba_too_large(lba + 1ULL, sector_size)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
-- 
2.11.0

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Steve Magnani]

Hi Bart -

Thanks for taking the time to look this over.

On 02/27/2017 10:13 AM, Bart Van Assche wrote:

On Mon, 2017-02-27 at 09:22 -0600, Steven J. Magnani wrote:

@@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
return -ENODEV;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = lba << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   ((lba >= 0xULL) || (lba_in_sectors >= 0xULL))) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");
@@ -2162,6 +2165,7 @@ static int read_capacity_10(struct scsi_
int the_result;
int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
sector_t lba;
+   unsigned long long lba_in_sectors;
unsigned sector_size;
  
  	do {

@@ -2208,7 +2212,10 @@ static int read_capacity_10(struct scsi_
return sector_size;
}
  
-	if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {

+   /* Make sure logical_to_sectors() won't overflow */
+   lba_in_sectors = ((unsigned long long) lba) << (ilog2(sector_size) - 9);
+   if ((sizeof(sdkp->capacity) == 4) &&
+   (lba_in_sectors >= 0xULL)) {
sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
"kernel compiled with support for large block "
"devices.\n");

Why are the two checks slightly different? Could the same code be used for
both checks?
The checks are different because with READ CAPACITY(16) a _really_ huge 
device could report a max LBA so large that left-shifting it causes bits 
to drop off the end. That's not an issue with READ CAPACITY(10) because 
at most the 32-bit LBA reported by the device will become a 35-bit value 
(since the max supported block size is 4096 == (512 << 3)).



BTW, using the macro below would make the above checks less
verbose and easier to read:

/*
  * Test whether the result of a shift-left operation would be larger than
  * what fits in a variable with the type of @a.
  */
#define shift_left_overflows(a, b)  \
({  \
typeof(a) _minus_one = -1LL;\
typeof(a) _plus_one = 1;\
bool _a_is_signed = _minus_one < 0;  \
int _shift = sizeof(a) * 8 - ((b) + _a_is_signed);  \
_shift < 0 || ((a) & ~((_plus_one << _shift) - 1)) != 0;\
})

Bart.
Perhaps but I am not a fan of putting braces in non-obvious places such 
as within array subscripts (which I've encountered recently) or 
conditional expressions, which is what this amounts to.


Regards,

 Steven J. Magnani   "I claim this network for MARS!
 www.digidescorp.com  Earthling, return my space modulator!"

 #include 

Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF

[Bart Van Assche]

On Mon, 2017-02-27 at 09:22 -0600, Steven J. Magnani wrote:
> @@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
>   return -ENODEV;
>   }
>  
> - if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xULL)) {
> + /* Make sure logical_to_sectors() won't overflow */
> + lba_in_sectors = lba << (ilog2(sector_size) - 9);
> + if ((sizeof(sdkp->capacity) == 4) &&
> + ((lba >= 0xULL) || (lba_in_sectors >= 0xULL))) {
>   sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
>   "kernel compiled with support for large block "
>   "devices.\n");
> @@ -2162,6 +2165,7 @@ static int read_capacity_10(struct scsi_
>   int the_result;
>   int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
>   sector_t lba;
> + unsigned long long lba_in_sectors;
>   unsigned sector_size;
>  
>   do {
> @@ -2208,7 +2212,10 @@ static int read_capacity_10(struct scsi_
>   return sector_size;
>   }
>  
> - if ((sizeof(sdkp->capacity) == 4) && (lba == 0x)) {
> + /* Make sure logical_to_sectors() won't overflow */
> + lba_in_sectors = ((unsigned long long) lba) << (ilog2(sector_size) - 9);
> + if ((sizeof(sdkp->capacity) == 4) &&
> + (lba_in_sectors >= 0xULL)) {
>   sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
>   "kernel compiled with support for large block "
>   "devices.\n");

Why are the two checks slightly different? Could the same code be used for
both checks? BTW, using the macro below would make the above checks less
verbose and easier to read:

/*
 * Test whether the result of a shift-left operation would be larger than
 * what fits in a variable with the type of @a.
 */
#define shift_left_overflows(a, b)  \
({  \
typeof(a) _minus_one = -1LL;\
typeof(a) _plus_one = 1;\
bool _a_is_signed = _minus_one < 0; \
int _shift = sizeof(a) * 8 - ((b) + _a_is_signed);  \
_shift < 0 || ((a) & ~((_plus_one << _shift) - 1)) != 0;\
})

Bart.