| Message ID | 20210708131057.259327-2-naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix argument type of bio_trim() | expand |
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.
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. >
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.
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
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.
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);