| Message ID | 20250314155447.124842-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: fix signedness issue in min() | expand |
On Fri, Mar 14, 2025 at 04:54:41PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Comparing a u64 to an loff_t causes a warning in min() > > fs/btrfs/extent_io.c: In function 'extent_write_locked_range': > include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_588' declared with attribute error: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error > fs/btrfs/extent_io.c:2472:27: note: in expansion of macro 'min' > 2472 | cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); > | ^~~ > > Use min_t() instead. > > Fixes: f286b1c72175 ("btrfs: prepare extent_io.c for future larger folio support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Thanks, there was another report and the upcoming for-next will have it fixed.
On Fri, 14 Mar 2025 16:54:41 +0100 Arnd Bergmann <arnd@kernel.org> wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Comparing a u64 to an loff_t causes a warning in min() > > fs/btrfs/extent_io.c: In function 'extent_write_locked_range': > include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_588' declared with attribute error: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error > fs/btrfs/extent_io.c:2472:27: note: in expansion of macro 'min' > 2472 | cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); > | ^~~ > > Use min_t() instead. It would be slightly better to use min_unsigned() since, regardless of the types involved, it can't discard significant bits. OTOH the real problem here is that both folio_pos() and folio_size() return signed types. David > > Fixes: f286b1c72175 ("btrfs: prepare extent_io.c for future larger folio support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/btrfs/extent_io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index c2451194be66..88bced0bfa51 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -2468,7 +2468,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f > continue; > } > > - cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); > + cur_end = min_t(u64, folio_pos(folio) + folio_size(folio) - 1, end); > cur_len = cur_end + 1 - cur; > > ASSERT(folio_test_locked(folio));
On Mon, Mar 17, 2025 at 02:16:37PM +0000, David Laight wrote: > On Fri, 14 Mar 2025 16:54:41 +0100 > Arnd Bergmann <arnd@kernel.org> wrote: > > > From: Arnd Bergmann <arnd@arndb.de> > > > > Comparing a u64 to an loff_t causes a warning in min() > > > > fs/btrfs/extent_io.c: In function 'extent_write_locked_range': > > include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_588' declared with attribute error: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error > > fs/btrfs/extent_io.c:2472:27: note: in expansion of macro 'min' > > 2472 | cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); > > | ^~~ > > > > Use min_t() instead. > > It would be slightly better to use min_unsigned() since, regardless of the types > involved, it can't discard significant bits. > > OTOH the real problem here is that both folio_pos() and folio_size() return signed types. folio_size() returns size_t: static inline size_t folio_size(const struct folio *folio) { return PAGE_SIZE << folio_order(folio); } Otherwise the min_t with force u64 is ok and lots of min() use (in btrfs) was converted to the typed variant in case the types don't match.
On Mon, 17 Mar 2025 20:26:39 +0100 David Sterba <dsterba@suse.cz> wrote: > On Mon, Mar 17, 2025 at 02:16:37PM +0000, David Laight wrote: > > On Fri, 14 Mar 2025 16:54:41 +0100 > > Arnd Bergmann <arnd@kernel.org> wrote: > > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > > > Comparing a u64 to an loff_t causes a warning in min() > > > > > > fs/btrfs/extent_io.c: In function 'extent_write_locked_range': > > > include/linux/compiler_types.h:557:45: error: call to '__compiletime_assert_588' declared with attribute error: min(folio_pos(folio) + folio_size(folio) - 1, end) signedness error > > > fs/btrfs/extent_io.c:2472:27: note: in expansion of macro 'min' > > > 2472 | cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); > > > | ^~~ > > > > > > Use min_t() instead. > > > > It would be slightly better to use min_unsigned() since, regardless of the types > > involved, it can't discard significant bits. > > > > OTOH the real problem here is that both folio_pos() and folio_size() return signed types. > > folio_size() returns size_t: > > static inline size_t folio_size(const struct folio *folio) > { > return PAGE_SIZE << folio_order(folio); > } > > Otherwise the min_t with force u64 is ok and lots of min() use (in > btrfs) was converted to the typed variant in case the types don't match. That is just broken. min_t(u64, x, y) is just min((u64)x, (u64)y) and you wouldn't do the same casts anywhere else unless you really had to. So you really shouldn't use min_t() unless there is no other way around the problem. Ok (u64) are unlikely to be a problem, but there are plenty of places where (u8) get used and can (and actually has) discard significant bits and cause bugs. David
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index c2451194be66..88bced0bfa51 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2468,7 +2468,7 @@ void extent_write_locked_range(struct inode *inode, const struct folio *locked_f continue; } - cur_end = min(folio_pos(folio) + folio_size(folio) - 1, end); + cur_end = min_t(u64, folio_pos(folio) + folio_size(folio) - 1, end); cur_len = cur_end + 1 - cur; ASSERT(folio_test_locked(folio));