* [PATCH 0/3] fix argument type of bio_trim()
@ 2021-07-08 13:10 Naohiro Aota
2021-07-08 13:10 ` [PATCH 1/3] block: fix arg " Naohiro Aota
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Naohiro Aota @ 2021-07-08 13:10 UTC (permalink / raw)
To: linux-btrfs, linux-block
Cc: Jens Axboe, David Sterba, Chaitanya Kulkarni, 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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] block: fix arg type of bio_trim() 2021-07-08 13:10 [PATCH 0/3] fix argument type of bio_trim() Naohiro Aota @ 2021-07-08 13:10 ` Naohiro Aota 2021-07-08 14:57 ` David Sterba 2021-07-08 13:10 ` [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() Naohiro Aota 2021-07-08 13:10 ` [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() Naohiro Aota 2 siblings, 1 reply; 13+ messages in thread From: Naohiro Aota @ 2021-07-08 13:10 UTC (permalink / raw) To: linux-btrfs, linux-block Cc: Jens Axboe, David Sterba, Chaitanya Kulkarni, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] block: fix arg type of bio_trim() 2021-07-08 13:10 ` [PATCH 1/3] block: fix arg " Naohiro Aota @ 2021-07-08 14:57 ` David Sterba 2021-07-09 0:42 ` Damien Le Moal 2021-07-09 4:39 ` Naohiro Aota 0 siblings, 2 replies; 13+ messages in thread From: David Sterba @ 2021-07-08 14:57 UTC (permalink / raw) To: Naohiro Aota Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] block: fix arg type of bio_trim() 2021-07-08 14:57 ` David Sterba @ 2021-07-09 0:42 ` Damien Le Moal 2021-07-09 4:53 ` Naohiro Aota 2021-07-09 4:39 ` Naohiro Aota 1 sibling, 1 reply; 13+ messages in thread From: Damien Le Moal @ 2021-07-09 0:42 UTC (permalink / raw) To: dsterba, Naohiro Aota Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] block: fix arg type of bio_trim() 2021-07-09 0:42 ` Damien Le Moal @ 2021-07-09 4:53 ` Naohiro Aota 0 siblings, 0 replies; 13+ messages in thread From: Naohiro Aota @ 2021-07-09 4:53 UTC (permalink / raw) To: Damien Le Moal Cc: dsterba, linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] block: fix arg type of bio_trim() 2021-07-08 14:57 ` David Sterba 2021-07-09 0:42 ` Damien Le Moal @ 2021-07-09 4:39 ` Naohiro Aota 2021-07-09 4:55 ` Naohiro Aota 1 sibling, 1 reply; 13+ messages in thread From: Naohiro Aota @ 2021-07-09 4:39 UTC (permalink / raw) To: dsterba Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] block: fix arg type of bio_trim() 2021-07-09 4:39 ` Naohiro Aota @ 2021-07-09 4:55 ` Naohiro Aota 0 siblings, 0 replies; 13+ messages in thread From: Naohiro Aota @ 2021-07-09 4:55 UTC (permalink / raw) To: dsterba Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() 2021-07-08 13:10 [PATCH 0/3] fix argument type of bio_trim() Naohiro Aota 2021-07-08 13:10 ` [PATCH 1/3] block: fix arg " Naohiro Aota @ 2021-07-08 13:10 ` Naohiro Aota 2021-07-08 15:00 ` David Sterba 2021-07-08 13:10 ` [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() Naohiro Aota 2 siblings, 1 reply; 13+ messages in thread From: Naohiro Aota @ 2021-07-08 13:10 UTC (permalink / raw) To: linux-btrfs, linux-block Cc: Jens Axboe, David Sterba, Chaitanya Kulkarni, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() 2021-07-08 13:10 ` [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() Naohiro Aota @ 2021-07-08 15:00 ` David Sterba 2021-07-09 5:01 ` Naohiro Aota 0 siblings, 1 reply; 13+ messages in thread From: David Sterba @ 2021-07-08 15:00 UTC (permalink / raw) To: Naohiro Aota Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() 2021-07-08 15:00 ` David Sterba @ 2021-07-09 5:01 ` Naohiro Aota 0 siblings, 0 replies; 13+ messages in thread From: Naohiro Aota @ 2021-07-09 5:01 UTC (permalink / raw) To: dsterba Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() 2021-07-08 13:10 [PATCH 0/3] fix argument type of bio_trim() Naohiro Aota 2021-07-08 13:10 ` [PATCH 1/3] block: fix arg " Naohiro Aota 2021-07-08 13:10 ` [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() Naohiro Aota @ 2021-07-08 13:10 ` Naohiro Aota 2021-07-08 13:54 ` David Sterba 2021-07-08 15:03 ` David Sterba 2 siblings, 2 replies; 13+ messages in thread From: Naohiro Aota @ 2021-07-08 13:10 UTC (permalink / raw) To: linux-btrfs, linux-block Cc: Jens Axboe, David Sterba, Chaitanya Kulkarni, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() 2021-07-08 13:10 ` [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() Naohiro Aota @ 2021-07-08 13:54 ` David Sterba 2021-07-08 15:03 ` David Sterba 1 sibling, 0 replies; 13+ messages in thread From: David Sterba @ 2021-07-08 13:54 UTC (permalink / raw) To: Naohiro Aota Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() 2021-07-08 13:10 ` [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() Naohiro Aota 2021-07-08 13:54 ` David Sterba @ 2021-07-08 15:03 ` David Sterba 1 sibling, 0 replies; 13+ messages in thread From: David Sterba @ 2021-07-08 15:03 UTC (permalink / raw) To: Naohiro Aota Cc: linux-btrfs, linux-block, Jens Axboe, David Sterba, Chaitanya Kulkarni 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-07-09 5:01 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-08 13:10 [PATCH 0/3] fix argument type of bio_trim() Naohiro Aota 2021-07-08 13:10 ` [PATCH 1/3] block: fix arg " Naohiro Aota 2021-07-08 14:57 ` David Sterba 2021-07-09 0:42 ` Damien Le Moal 2021-07-09 4:53 ` Naohiro Aota 2021-07-09 4:39 ` Naohiro Aota 2021-07-09 4:55 ` Naohiro Aota 2021-07-08 13:10 ` [PATCH 2/3] btrfs: fix argument type of btrfs_bio_clone_partial() Naohiro Aota 2021-07-08 15:00 ` David Sterba 2021-07-09 5:01 ` Naohiro Aota 2021-07-08 13:10 ` [PATCH 3/3] btrfs: drop unnecessary ASSERT from btrfs_submit_direct() Naohiro Aota 2021-07-08 13:54 ` David Sterba 2021-07-08 15:03 ` David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.