Hi,

On Sat, 26. Apr 08:03, Darrick J. Wong wrote:

On Sat, Apr 26, 2025 at 04:42:31PM +0300, Fedor Pchelkin wrote:

...

Currently the difference is computed on 32-bit unsigned values although eventually it is stored in a variable of int64_t type. This gives awkward results, e.g. when the diff _should_ be negative, it is represented as some large positive int64_t value.

Perform the calculations directly in int64_t as all other diff_two_keys routines actually do.

Found by Linux Verification Center (linuxtesting.org) with Svace static analysis tool.

Fixes: 08438b1e386b ("xfs: plumb in needed functions for range querying of the freespace btrees") Cc: stable@vger.kernel.org Signed-off-by: Fedor Pchelkin pchelkin@ispras.ru

---

fs/xfs/libxfs/xfs_alloc_btree.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c index a4ac37ba5d51..b3c54ae90e25 100644 --- a/fs/xfs/libxfs/xfs_alloc_btree.c +++ b/fs/xfs/libxfs/xfs_alloc_btree.c @@ -238,13 +238,13 @@ xfs_cntbt_diff_two_keys( ASSERT(!mask || (mask->alloc.ar_blockcount && mask->alloc.ar_startblock));

• diff = be32_to_cpu(k1->alloc.ar_blockcount) -

• be32_to_cpu(k2->alloc.ar_blockcount);
  

• diff = (int64_t)be32_to_cpu(k1->alloc.ar_blockcount) -

• 	be32_to_cpu(k2->alloc.ar_blockcount);
  

Perhaps it's time to hoist cmp_int to include/ and refactor all these things to use it?

#define cmp_int(l, r) ((l > r) - (l < r))

--D

Thanks, that would be worth it, I think. Though the current xfs ***diff_two_keys() implementations try to compute and return the actual difference between two values, not the result of their comparison. Now looking at diff_two_keys() use cases, I see only the latter one is needed anyway so a good bit to refactor.

The thing I'm pondering over now is whether the macro in its current form is okay to move up to include/. There is no argument restrictions and typechecking intended to catch up obviously misleading usage patterns though we'd need some if this is hoisted to a generic header and exported for potential use by others?

There are four places where cmp_int is defined at the moment: - bcachefs - md/bcache - xfs_zone_gc - pipe.c

bcachefs is the largest user having all kinds of different arguments providing to the macro, bitfields included. It also has several rather generic wrappers, like u64_cmp, unsigned_cmp, u8_cmp, cmp_le32 and others..

AF_UNIX code even has

#define cmp_ptr(l, r) (((l) > (r)) - ((l) < (r)))

for pointer comparisons.

So in my opinion we'd probably need to come up with something like a new include/linux/cmp.h header where all this stuff will be gathered in a generic way.

Any objections/suggestions on that? Or just moving

#define cmp_int(l, r) ((l > r) - (l < r))

to a generic header and dropping the corresponding defines from the separate in-kernel users would be enough in this case?

-- Thanks, Fedor

...

if (diff) return diff;

• return be32_to_cpu(k1->alloc.ar_startblock) -

• be32_to_cpu(k2->alloc.ar_startblock);
  

• return (int64_t)be32_to_cpu(k1->alloc.ar_startblock) -

• 	be32_to_cpu(k2->alloc.ar_startblock);
  

} static xfs_failaddr_t -- 2.49.0