[PATCH 0/3] fix argument type of bio_trim()

[PATCH 0/3] fix argument type of bio_trim()

[Naohiro Aota]

The function bio_trim has offset and size arguments that are declared as
int. But it actually expects sector value and the range of int is not
enough for bio->bi_iter.bi_size and bio_advance() which is/takes unsigned
int. So, let's fix the argument type of bio_trim() and its callers.

Patches 1 and 2 fix bio_trim() argument and its caller's argument.

Patch 3 is depending on patch 1 and 2 and do some cleanup for btrfs.

Chaitanya Kulkarni (2):
  block: fix arg type of bio_trim()
  btrfs: fix argument type of btrfs_bio_clone_partial()

Naohiro Aota (1):
  btrfs: drop unnecessary ASSERT from btrfs_submit_direct()

 block/bio.c          |  2 +-
 fs/btrfs/extent_io.c |  3 ++-
 fs/btrfs/extent_io.h |  3 ++-
 fs/btrfs/inode.c     | 12 ++++++++----
 include/linux/bio.h  |  2 +-
 5 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.32.0

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

[Naohiro Aota]

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)
 {
 	/* '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);
 extern struct bio *bio_split(struct bio *bio, int sectors,
 			     gfp_t gfp, struct bio_set *bs);
 
-- 
2.32.0

[PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial()

[Naohiro Aota]

From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

The offset and can never be negative use unsigned int instead of int type
for them.

Tested-by: Naohiro Aota <naohiro.aota@wdc.com>
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 fs/btrfs/extent_io.c | 3 ++-
 fs/btrfs/extent_io.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1f947e24091a..082f135bb3de 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3153,7 +3153,8 @@ struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs)
 	return bio;
 }
 
-struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size)
+struct bio *btrfs_bio_clone_partial(struct bio *orig, unsigned int offset,
+				    unsigned int size)
 {
 	struct bio *bio;
 	struct btrfs_io_bio *btrfs_bio;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 62027f551b44..f78b365b56cf 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -280,7 +280,8 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
 struct bio *btrfs_bio_alloc(u64 first_byte);
 struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs);
 struct bio *btrfs_bio_clone(struct bio *bio);
-struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size);
+struct bio *btrfs_bio_clone_partial(struct bio *orig, unsigned int offset,
+				    unsigned int size);
 
 int repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
 		      u64 length, u64 logical, struct page *page,
-- 
2.32.0

[PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct()

[Naohiro Aota]

When on SINGLE block group, btrfs_get_io_geometry() will return "the
size of the block group - the offset of the logical address within the
block group" as geom.len. Since we allow up to 8 GB zone size on zoned
btrfs, we can have up to 8 GB block group, so can have up to 8 GB
geom.len. With this setup, we easily hit the "ASSERT(geom.len <=
INT_MAX);".

The ASSERT looks like to guard btrfs_bio_clone_partial() and bio_trim()
which both take "int" (now "unsigned int" with the previous patch). So to
be precise the ASSERT should check if clone_len <= UINT_MAX. But
actually, clone_len is already capped by bio.bi_iter.bi_size which is
unsigned int. So the ASSERT is not necessary.

Drop the ASSERT and properly compare submit_len and geom.len in u64. Then,
let the implicit casting to convert it to unsigned int.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/inode.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8f60314c36c5..b6cc26dd7919 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8206,8 +8206,8 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
 	u64 start_sector;
 	int async_submit = 0;
 	u64 submit_len;
-	int clone_offset = 0;
-	int clone_len;
+	unsigned int clone_offset = 0;
+	unsigned int clone_len;
 	u64 logical;
 	int ret;
 	blk_status_t status;
@@ -8255,9 +8255,13 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
 			status = errno_to_blk_status(ret);
 			goto out_err_em;
 		}
-		ASSERT(geom.len <= INT_MAX);
 
-		clone_len = min_t(int, submit_len, geom.len);
+		/*
+		 * min()'s result is always capped by bio.bi_iter.bi_size
+		 * which is unsigned int. So the implicit casting it to
+		 * unsigned int is safe.
+		 */
+		clone_len = min(submit_len, geom.len);
 
 		/*
 		 * This will never fail as it's passing GPF_NOFS and
-- 
2.32.0

Re: [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct()

[David Sterba]

On Thu, Jul 08, 2021 at 10:10:57PM +0900, Naohiro Aota wrote:
> When on SINGLE block group, btrfs_get_io_geometry() will return "the
> size of the block group - the offset of the logical address within the
> block group" as geom.len. Since we allow up to 8 GB zone size on zoned
> btrfs, we can have up to 8 GB block group, so can have up to 8 GB
> geom.len. With this setup, we easily hit the "ASSERT(geom.len <=
> INT_MAX);".
> 
> The ASSERT looks like to guard btrfs_bio_clone_partial() and bio_trim()
> which both take "int" (now "unsigned int" with the previous patch). So to
> be precise the ASSERT should check if clone_len <= UINT_MAX. But
> actually, clone_len is already capped by bio.bi_iter.bi_size which is
> unsigned int. So the ASSERT is not necessary.
> 
> Drop the ASSERT and properly compare submit_len and geom.len in u64. Then,
> let the implicit casting to convert it to unsigned int.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/inode.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8f60314c36c5..b6cc26dd7919 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8206,8 +8206,8 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  	u64 start_sector;
>  	int async_submit = 0;
>  	u64 submit_len;
> -	int clone_offset = 0;
> -	int clone_len;
> +	unsigned int clone_offset = 0;
> +	unsigned int clone_len;
>  	u64 logical;
>  	int ret;
>  	blk_status_t status;
> @@ -8255,9 +8255,13 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  			status = errno_to_blk_status(ret);
>  			goto out_err_em;
>  		}
> -		ASSERT(geom.len <= INT_MAX);
>  
> -		clone_len = min_t(int, submit_len, geom.len);
> +		/*
> +		 * min()'s result is always capped by bio.bi_iter.bi_size
> +		 * which is unsigned int. So the implicit casting it to
> +		 * unsigned int is safe.
> +		 */
> +		clone_len = min(submit_len, geom.len);

I'd rather rely on explicit casts and asserts than a comment stating
what should happen. submit_len ang geom.len are both u64 and cast to u32
clone_len.

I think submit_len should be u32 too (to copy the bio.bi_size) to match
the types used for the purpose and also clone_len.

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

[David Sterba]

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.

>  {
>  	/* '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.

Re: [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial()

[David Sterba]

On Thu, Jul 08, 2021 at 10:10:56PM +0900, Naohiro Aota wrote:
> From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> 
> The offset and can never be negative use unsigned int instead of int type
> for them.
> 
> Tested-by: Naohiro Aota <naohiro.aota@wdc.com>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  fs/btrfs/extent_io.c | 3 ++-
>  fs/btrfs/extent_io.h | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1f947e24091a..082f135bb3de 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3153,7 +3153,8 @@ struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs)
>  	return bio;
>  }
>  
> -struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size)
> +struct bio *btrfs_bio_clone_partial(struct bio *orig, unsigned int offset,
> +				    unsigned int size)
>  {
>  	struct bio *bio;
>  	struct btrfs_io_bio *btrfs_bio;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 62027f551b44..f78b365b56cf 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -280,7 +280,8 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
>  struct bio *btrfs_bio_alloc(u64 first_byte);
>  struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs);
>  struct bio *btrfs_bio_clone(struct bio *bio);
> -struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size);
> +struct bio *btrfs_bio_clone_partial(struct bio *orig, unsigned int offset,
> +				    unsigned int size);

This is passed to bio_trim that you change to take sector_t, should this
be the same?

>  
>  int repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
>  		      u64 length, u64 logical, struct page *page,
> -- 
> 2.32.0

Re: [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct()

[David Sterba]

On Thu, Jul 08, 2021 at 10:10:57PM +0900, Naohiro Aota wrote:
> When on SINGLE block group, btrfs_get_io_geometry() will return "the
> size of the block group - the offset of the logical address within the
> block group" as geom.len. Since we allow up to 8 GB zone size on zoned
> btrfs, we can have up to 8 GB block group, so can have up to 8 GB
> geom.len. With this setup, we easily hit the "ASSERT(geom.len <=
> INT_MAX);".
> 
> The ASSERT looks like to guard btrfs_bio_clone_partial() and bio_trim()
> which both take "int" (now "unsigned int" with the previous patch). So to
> be precise the ASSERT should check if clone_len <= UINT_MAX. But
> actually, clone_len is already capped by bio.bi_iter.bi_size which is
> unsigned int. So the ASSERT is not necessary.
> 
> Drop the ASSERT and properly compare submit_len and geom.len in u64. Then,
> let the implicit casting to convert it to unsigned int.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>  fs/btrfs/inode.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 8f60314c36c5..b6cc26dd7919 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -8206,8 +8206,8 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap,
>  	u64 start_sector;
>  	int async_submit = 0;
>  	u64 submit_len;
> -	int clone_offset = 0;
> -	int clone_len;
> +	unsigned int clone_offset = 0;
> +	unsigned int clone_len;

After reading the other patches, clone_offset should be sector_t or u64.
clone_len is fine as u32 as it only gets update from the bio.bi_size,
but later in the code there's

	clone_offset += clone_len;

and clone_offset is passed to btrfs_bio_clone_partial -> bio_trim, that
you've changed to sector_t.

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

[Damien Le Moal]

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

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

[Naohiro Aota]

On Thu, Jul 08, 2021 at 04:57:22PM +0200, 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.

Sure. I'll add the following ASSERT to catch the case.

diff --git a/block/bio.c b/block/bio.c
index d342ce84f6cf..54b573414126 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1467,10 +1467,14 @@ EXPORT_SYMBOL(bio_split);
  */
 void bio_trim(struct bio *bio, sector_t offset, sector_t size)
 {
+	const uint_max_sectors = UINT_MAX << SECTOR_SHIFT;
+
 	/* 'bio' is a cloned bio which we need to trim to match
 	 * the given offset and size.
 	 */
 
+	ASSERT(offset <= uint_max_sectors && size < uint_max_sectors);
+
 	size <<= 9;
 	if (offset == 0 && size == bio->bi_iter.bi_size)
 		return;


> >  {
> >  	/* '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.

True. Chaitanya, what is the intention of droping it? maybe just a mistake?

> The patch is simple I can take it through the btrfs tree with the other
> fixes unless there are objections.

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

[Naohiro Aota]

On Fri, Jul 09, 2021 at 12:42:04AM +0000, Damien Le Moal wrote:
> 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.

I'll add the following (fixed) WARN_ON to check it.

# I thought I could use ASSERT everywhere but actually it's from
# btrfs...

This function is not about TRIM command, but to trim a bio. So the
size overflow is invalid.

> 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...

Ah, yeah. So the sanity check (with comment style fix) should be like this.

diff --git a/block/bio.c b/block/bio.c
index d342ce84f6cf..3fb2f1d7bb69 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1467,10 +1467,18 @@ EXPORT_SYMBOL(bio_split);
  */
 void bio_trim(struct bio *bio, sector_t offset, sector_t size)
 {
-	/* 'bio' is a cloned bio which we need to trim to match
-	 * the given offset and size.
+	const sector_t uint_max_sectors = UINT_MAX << SECTOR_SHIFT;
+
+	/*
+	 * 'bio' is a cloned bio which we need to trim to match the given
+	 * offset and size.
 	 */
 
+	/* sanity check */
+	if (WARN_ON(offset > uint_max_sectors && size > uint_max_sectors) ||
+	    WARN_ON(offset + size > bio->bi_iter.bi_size))
+		return;
+
 	size <<= 9;
 	if (offset == 0 && size == bio->bi_iter.bi_size)
 		return;

> > 
> >>  {
> >>  	/* '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

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

[Naohiro Aota]

On Fri, Jul 09, 2021 at 04:39:47AM +0000, Naohiro Aota wrote:
> On Thu, Jul 08, 2021 at 04:57:22PM +0200, 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.
> 
> Sure. I'll add the following ASSERT to catch the case.
> 
> diff --git a/block/bio.c b/block/bio.c
> index d342ce84f6cf..54b573414126 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1467,10 +1467,14 @@ EXPORT_SYMBOL(bio_split);
>   */
>  void bio_trim(struct bio *bio, sector_t offset, sector_t size)
>  {
> +	const uint_max_sectors = UINT_MAX << SECTOR_SHIFT;
> +
>  	/* 'bio' is a cloned bio which we need to trim to match
>  	 * the given offset and size.
>  	 */
>  
> +	ASSERT(offset <= uint_max_sectors && size < uint_max_sectors);
> +
>  	size <<= 9;
>  	if (offset == 0 && size == bio->bi_iter.bi_size)
>  		return;
> 

Please ignore this one. I failed to add the type and cannot use ASSERT
here. Updated diff available in the reply to Damien.

Re: [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial()

[Naohiro Aota]

On Thu, Jul 08, 2021 at 05:00:25PM +0200, David Sterba wrote:
> On Thu, Jul 08, 2021 at 10:10:56PM +0900, Naohiro Aota wrote:
> > From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> > 
> > The offset and can never be negative use unsigned int instead of int type
> > for them.
> > 
> > Tested-by: Naohiro Aota <naohiro.aota@wdc.com>
> > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> > ---
> >  fs/btrfs/extent_io.c | 3 ++-
> >  fs/btrfs/extent_io.h | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 1f947e24091a..082f135bb3de 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -3153,7 +3153,8 @@ struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs)
> >  	return bio;
> >  }
> >  
> > -struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size)
> > +struct bio *btrfs_bio_clone_partial(struct bio *orig, unsigned int offset,
> > +				    unsigned int size)
> >  {
> >  	struct bio *bio;
> >  	struct btrfs_io_bio *btrfs_bio;
> > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> > index 62027f551b44..f78b365b56cf 100644
> > --- a/fs/btrfs/extent_io.h
> > +++ b/fs/btrfs/extent_io.h
> > @@ -280,7 +280,8 @@ void extent_clear_unlock_delalloc(struct btrfs_inode *inode, u64 start, u64 end,
> >  struct bio *btrfs_bio_alloc(u64 first_byte);
> >  struct bio *btrfs_io_bio_alloc(unsigned int nr_iovecs);
> >  struct bio *btrfs_bio_clone(struct bio *bio);
> > -struct bio *btrfs_bio_clone_partial(struct bio *orig, int offset, int size);
> > +struct bio *btrfs_bio_clone_partial(struct bio *orig, unsigned int offset,
> > +				    unsigned int size);
> 
> This is passed to bio_trim that you change to take sector_t, should this
> be the same?

btrfs_bio_clone_partial() expects byte-based value, so using sector_t
is misleading. Should we use u64 here? But the values must be <=
UINT_MAX.

> >  
> >  int repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
> >  		      u64 length, u64 logical, struct page *page,
> > -- 
> > 2.32.0