On 2021/12/18 3:09, Tetsuo Handa wrote:
> On 2021/12/18 1:25, Jens Axboe wrote:
>> On 12/17/21 4:38 AM, Tetsuo Handa wrote:
>>> Use of "unsigned short" for loop_validate_block_size() is wrong [1], and
>>> commit af3c570fb0df422b ("loop: Use blk_validate_block_size() to validate
>>> block size") changed to use "unsigned int".
>>>
>>> However, since lo_simple_ioctl(LOOP_SET_BLOCK_SIZE) passes "unsigned long
>>> arg" to loop_set_block_size(), blk_validate_block_size() can't validate
>>> the upper 32bits on 64bits environment. A block size like 0x100000200
>>> should be rejected.
>>
>> Wouldn't it make more sense to validate that part on the loop side? A
>> block size > 32-bit doesn't make any sense.
>>
>
> I think doing below is embarrassing, for there is blk_validate_block_size() which is
> meant for validating the arg. Although use of "unsigned long" for blk_validate_block_size()
> might cause small bloating on 64 bits kernels, I think 64 bits kernels would not care.
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c3a36cfaa855..98871d7b601d 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1474,6 +1474,10 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
> err = blk_validate_block_size(arg);
> if (err)
> return err;
> +#if BITS_PER_LONG == 64
> + if (arg > UINT_MAX)
> + return -EINVAL;
> +#endif
>
> if (lo->lo_queue->limits.logical_block_size == arg)
> return 0;
>
> And reviving loop_validate_block_size() in order to use "unsigned long" does not make sense
> for 32bits kernels.
Well, this problem is not specific to the loop module.
nbd_set_size(struct nbd_device *nbd, loff_t bytesize, loff_t blksize) calls
blk_validate_block_size(blksize), and blksize can be > UINT_MAX, for
ioctl(NBD_SET_BLKSIZE) passes "unsigned long arg" to nbd_set_size().
But size bloating cannot be a problem because there are only 4 callers.
drivers/block/loop.c
line 996
line 1474
drivers/block/nbd.c, line 325
drivers/block/virtio_blk.c, line 878
I think we can (and should) use "unsigned long" for blk_validate_block_size().
