Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1588760 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF "Steven J. Magnani" <steve.magnani@digidescorp.com> - 2017-02-27 16:40 +0100
    Re: [PATCH] sd: close hole in > 2T device rejection when  !CONFIG_LBDAF Steve Magnani <steve.magnani@digidescorp.com> - 2017-02-27 18:20 +0100
      Re: [PATCH] sd: close hole in > 2T device rejection when !CONFIG_LBDAF "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-02-28 04:30 +0100
      Re: [PATCH] sd: close hole in > 2T device rejection when  !CONFIG_LBDAF Steve Magnani <steve.magnani@digidescorp.com> - 2017-02-28 15:00 +0100

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

When the kernel is compiled _without_ support for large (>= 2TiB) block
devices, code in the sd driver's read_capacity() routines rejects devices
whose count of native-sized blocks does not fit in the 32 bit sector_t
type. A device reporting 4294967296 512-byte blocks will be rejected, but
a device of equal capacity reporting 2147483648 1024-byte blocks will not.

The latter case causes problems, for example misreporting of device
capacity by BLKGETSIZE64. This is because the kernel converts a device's
capacity in native-sized blocks to an equivalent number of 512-byte units
and stores the result in a sector_t - which is too small for devices such
as those noted above.

To prevent this, when the kernel is compiled without support for large
block devices, the read_capacity() routines must reject any device whose
capacity is 2 TiB or greater regardless of its count of native-sized
blocks.

Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
---
--- a/drivers/scsi/sd.c	2017-02-24 20:29:44.510036363 -0600
+++ b/drivers/scsi/sd.c	2017-02-27 08:19:37.864786958 -0600
@@ -2066,7 +2066,7 @@ static int read_capacity_16(struct scsi_
 	int the_result;
 	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
 	unsigned int alignment;
-	unsigned long long lba;
+	unsigned long long lba, lba_in_sectors;
 	unsigned sector_size;
 
 	if (sdp->no_read_capacity_16)
@@ -2122,7 +2122,10 @@ static int read_capacity_16(struct scsi_
 		return -ENODEV;
 	}
 
-	if ((sizeof(sdkp->capacity) == 4) && (lba >= 0xffffffffULL)) {
+	/* Make sure logical_to_sectors() won't overflow */
+	lba_in_sectors = lba << (ilog2(sector_size) - 9);
+	if ((sizeof(sdkp->capacity) == 4) &&
+	    ((lba >= 0xffffffffULL) || (lba_in_sectors >= 0xffffffffULL))) {
 		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 == 0xffffffff)) {
+	/* 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 >= 0xffffffffULL)) {
 		sd_printk(KERN_ERR, sdkp, "Too big for this kernel. Use a "
 			"kernel compiled with support for large block "
 			"devices.\n");

[toc] | [next] | [standalone]

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

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 >= 0xffffffffULL)) {
>> +	/* Make sure logical_to_sectors() won't overflow */
>> +	lba_in_sectors = lba << (ilog2(sector_size) - 9);
>> +	if ((sizeof(sdkp->capacity) == 4) &&
>> +	    ((lba >= 0xffffffffULL) || (lba_in_sectors >= 0xffffffffULL))) {
>>   		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 == 0xffffffff)) {
>> +	/* 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 >= 0xffffffffULL)) {
>>   		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 <standard.disclaimer>

[toc] | [prev] | [next] | [standalone]

#1589142

>>>>> "Bart" == Bart Van Assche <Bart.VanAssche@sandisk.com> 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

[toc] | [prev] | [next] | [standalone]

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

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 >= 0xffffffffULL)) {
> +	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 == 0xffffffff)) {
> +	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");

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web