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

[Steve Magnani <steve.magnani@digidescorp.com>]

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>