Re: [PATCH 1/3] block: fix arg type of bio_trim()

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/07/09 0:00, David Sterba wrote:
> On Thu, Jul 08, 2021 at 10:10:55PM +0900, Naohiro Aota wrote:
>> From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>>
>> The function bio_trim has offset and size arguments that are declared
>> as int.
>>
>> The callers of this function uses sector_t type when passing the offset
>> and size e,g. drivers/md/raid1.c:narrow_write_error() and
>> drivers/md/raid1.c:narrow_write_error().
>>
>> Change offset & size arguments to sector_t type for bio_trim().
>>
>> Tested-by: Naohiro Aota <naohiro.aota@wdc.com>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>  block/bio.c         | 2 +-
>>  include/linux/bio.h | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 44205dfb6b60..d342ce84f6cf 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -1465,7 +1465,7 @@ EXPORT_SYMBOL(bio_split);
>>   * @offset:	number of sectors to trim from the front of @bio
>>   * @size:	size we want to trim @bio to, in sectors
>>   */
>> -void bio_trim(struct bio *bio, int offset, int size)
>> +void bio_trim(struct bio *bio, sector_t offset, sector_t size)
> 
> sectort_t seems to be the right one, there are << 9 in the function so
> that could lead to some bugs if the offset and size are at the boundary.

Need to add an overflow check:

size <<= 9;
...
bio->bi_iter.bi_size = size;

bi_size is "unsigned int" so if "size << 9" is larger than UINT_MAX, things will
break in ugly ways. And since trim is a hint to the device, in case of overflow,
the BIO size should probably simply be set to 0, with a WARN_ON signaling it.

Note that the potential overflow already exists with the current code as the BIO
size can be less than requested or 0 if size <<9 overflows the int type...

> 
>>  {
>>  	/* 'bio' is a cloned bio which we need to trim to match
>>  	 * the given offset and size.
>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>> index a0b4cfdf62a4..fb663152521e 100644
>> --- a/include/linux/bio.h
>> +++ b/include/linux/bio.h
>> @@ -379,7 +379,7 @@ static inline void bip_set_seed(struct bio_integrity_payload *bip,
>>  
>>  #endif /* CONFIG_BLK_DEV_INTEGRITY */
>>  
>> -extern void bio_trim(struct bio *bio, int offset, int size);
>> +void bio_trim(struct bio *bio, sector_t offset, sector_t size);
> 
> You may want to keep the extern for consistency in that file, though
> it's not necessary for the prototype.
> 
> The patch is simple I can take it through the btrfs tree with the other
> fixes unless there are objections.
> 


-- 
Damien Le Moal
Western Digital Research