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

Steve Magnani
2017-02-27T17:14:54Z

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>