[v3,2/3] block: Fix target variable of BLKSECTGET ioctl

Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
introduced a routine to call the kernel BLKSECTGET ioctl, which stores the
result back to user space.  However, the size of the data returned depends
on the routine handling the ioctl.  The (compat_)blkdev_ioctl returns a
short, while sg_ioctl returns an int.  Thus, on big-endian systems, we can
find ourselves accidentally shifting the result to a much larger value.
(On s390x, a short is 16 bits while an int is 32 bits.)

Also, the two ioctl handlers return values in different scales (block
returns sectors, while sg returns bytes), so some tweaking of the outputs
is required such that hdev_get_max_transfer_length returns a value in a
consistent set of units.

Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
---
 block/file-posix.c    | 17 ++++++++++-------
 include/block/block.h |  1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

On 20.01.2017 17:25, Eric Farman wrote:
> Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
> introduced a routine to call the kernel BLKSECTGET ioctl, which stores the
> result back to user space.  However, the size of the data returned depends
> on the routine handling the ioctl.  The (compat_)blkdev_ioctl returns a
> short, while sg_ioctl returns an int.  Thus, on big-endian systems, we can
> find ourselves accidentally shifting the result to a much larger value.
> (On s390x, a short is 16 bits while an int is 32 bits.)
> 
> Also, the two ioctl handlers return values in different scales (block
> returns sectors, while sg returns bytes), so some tweaking of the outputs
> is required such that hdev_get_max_transfer_length returns a value in a
> consistent set of units.
> 
> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  block/file-posix.c    | 17 ++++++++++-------
>  include/block/block.h |  1 +
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 28b47d9..9f83725 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
>      state->opaque = NULL;
>  }
>  
> -static int hdev_get_max_transfer_length(int fd)
> +static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
>  {
>  #ifdef BLKSECTGET
> -    int max_sectors = 0;
> -    if (ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> -        return max_sectors;
> +    int max_bytes = 0;
> +    short max_sectors = 0;
> +    if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
> +        return max_bytes;
> +    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
> +        return max_sectors << BDRV_SECTOR_BITS;
>      } else {
>          return -errno;
>      }
> @@ -672,9 +675,9 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>  
>      if (!fstat(s->fd, &st)) {
>          if (S_ISBLK(st.st_mode)) {
> -            int ret = hdev_get_max_transfer_length(s->fd);
> -            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
> -                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
> +            int ret = hdev_get_max_transfer_length(bs, s->fd);
> +            if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
> +                bs->bl.max_transfer = pow2floor(ret);
>              }
>          }
>      }
> diff --git a/include/block/block.h b/include/block/block.h
> index 8b0dcda..4e81f20 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -116,6 +116,7 @@ typedef struct HDGeometry {
>  
>  #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>                                       INT_MAX >> BDRV_SECTOR_BITS)
> +#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)

I'd rather like it the other way round (i.e.

#define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX)
#define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >>
BDRV_SECTOR_BITS)

), but I don't suppose I can interest you in a respin, can I? Would it
be OK with you if I changed you commit when I apply it?

Max

>  
>  /*
>   * Allocation status flags
>

On 01/31/2017 07:25 PM, Max Reitz wrote:
> On 20.01.2017 17:25, Eric Farman wrote:
>> Commit 6f6071745bd0 ("raw-posix: Fetch max sectors for host block device")
>> introduced a routine to call the kernel BLKSECTGET ioctl, which stores the
>> result back to user space.  However, the size of the data returned depends
>> on the routine handling the ioctl.  The (compat_)blkdev_ioctl returns a
>> short, while sg_ioctl returns an int.  Thus, on big-endian systems, we can
>> find ourselves accidentally shifting the result to a much larger value.
>> (On s390x, a short is 16 bits while an int is 32 bits.)
>>
>> Also, the two ioctl handlers return values in different scales (block
>> returns sectors, while sg returns bytes), so some tweaking of the outputs
>> is required such that hdev_get_max_transfer_length returns a value in a
>> consistent set of units.
>>
>> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
>> ---
>>  block/file-posix.c    | 17 ++++++++++-------
>>  include/block/block.h |  1 +
>>  2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 28b47d9..9f83725 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -651,12 +651,15 @@ static void raw_reopen_abort(BDRVReopenState *state)
>>      state->opaque = NULL;
>>  }
>>
>> -static int hdev_get_max_transfer_length(int fd)
>> +static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
>>  {
>>  #ifdef BLKSECTGET
>> -    int max_sectors = 0;
>> -    if (ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
>> -        return max_sectors;
>> +    int max_bytes = 0;
>> +    short max_sectors = 0;
>> +    if (bs->sg && ioctl(fd, BLKSECTGET, &max_bytes) == 0) {
>> +        return max_bytes;
>> +    } else if (!bs->sg && ioctl(fd, BLKSECTGET, &max_sectors) == 0) {
>> +        return max_sectors << BDRV_SECTOR_BITS;
>>      } else {
>>          return -errno;
>>      }
>> @@ -672,9 +675,9 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
>>
>>      if (!fstat(s->fd, &st)) {
>>          if (S_ISBLK(st.st_mode)) {
>> -            int ret = hdev_get_max_transfer_length(s->fd);
>> -            if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
>> -                bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
>> +            int ret = hdev_get_max_transfer_length(bs, s->fd);
>> +            if (ret > 0 && ret <= BDRV_REQUEST_MAX_BYTES) {
>> +                bs->bl.max_transfer = pow2floor(ret);
>>              }
>>          }
>>      }
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 8b0dcda..4e81f20 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -116,6 +116,7 @@ typedef struct HDGeometry {
>>
>>  #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>                                       INT_MAX >> BDRV_SECTOR_BITS)
>> +#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>
> I'd rather like it the other way round (i.e.
>
> #define BDRV_REQUEST_MAX_BYTES MIN(SIZE_MAX, INT_MAX)
> #define BDRV_REQUEST_MAX_SECTORS (BDRV_REQUEST_MAX_BYTES >>
> BDRV_SECTOR_BITS)
>
> ), but I don't suppose I can interest you in a respin, can I? Would it
> be OK with you if I changed you commit when I apply it?
>
> Max


That's far cleaner looking, so absolutely go ahead.  Thank you!

  - Eric

>
>>
>>  /*
>>   * Allocation status flags
>>
>
>