linux-kernel - Re: [PATCH 6/6] xfs: refactor xfs_btree_diff_two_ptrs() to take advantage of cmp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250701145319.GB10009@frogsfrogsfrogs>
Date: Tue, 1 Jul 2025 07:53:19 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Fedor Pchelkin <pchelkin@...ras.ru>
Cc: Carlos Maiolino <cem@...nel.org>, Christoph Hellwig <hch@....de>,
	linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	lvc-project@...uxtesting.org
Subject: Re: [PATCH 6/6] xfs: refactor xfs_btree_diff_two_ptrs() to take
 advantage of cmp_int()

On Thu, Jun 12, 2025 at 01:24:50PM +0300, Fedor Pchelkin wrote:
> Use cmp_int() to yield the result of a three-way-comparison instead of
> performing subtractions with extra casts.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Signed-off-by: Fedor Pchelkin <pchelkin@...ras.ru>
> ---
>  fs/xfs/libxfs/xfs_btree.c | 6 +++---
>  fs/xfs/libxfs/xfs_btree.h | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index d3591728998e..9a227b6b3296 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -5353,15 +5353,15 @@ xfs_btree_count_blocks(
>  }
>  
>  /* Compare two btree pointers. */
> -int64_t
> +int
>  xfs_btree_diff_two_ptrs(

I'm surprised you didn't rename the diff.*ptr functions too -- there's
no inherent meaning in the difference between two pointers, but it is
useful to compare them.  Renaming would make the sole callsite much
clearer in purpose:

	if (xfs_btree_cmp_two_ptrs(cur, pp, sibling) != 0)
		xchk_btree_set_corrupt(bs->sc, cur, level);

Other than that, the logic changes look correct to me.

--D

>  	struct xfs_btree_cur		*cur,
>  	const union xfs_btree_ptr	*a,
>  	const union xfs_btree_ptr	*b)
>  {
>  	if (cur->bc_ops->ptr_len == XFS_BTREE_LONG_PTR_LEN)
> -		return (int64_t)be64_to_cpu(a->l) - be64_to_cpu(b->l);
> -	return (int64_t)be32_to_cpu(a->s) - be32_to_cpu(b->s);
> +		return cmp_int(be64_to_cpu(a->l), be64_to_cpu(b->l));
> +	return cmp_int(be32_to_cpu(a->s), be32_to_cpu(b->s));
>  }
>  
>  struct xfs_btree_has_records {
> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> index 1bf20d509ac9..23598f287af5 100644
> --- a/fs/xfs/libxfs/xfs_btree.h
> +++ b/fs/xfs/libxfs/xfs_btree.h
> @@ -519,9 +519,9 @@ struct xfs_btree_block *xfs_btree_get_block(struct xfs_btree_cur *cur,
>  		int level, struct xfs_buf **bpp);
>  bool xfs_btree_ptr_is_null(struct xfs_btree_cur *cur,
>  		const union xfs_btree_ptr *ptr);
> -int64_t xfs_btree_diff_two_ptrs(struct xfs_btree_cur *cur,
> -				const union xfs_btree_ptr *a,
> -				const union xfs_btree_ptr *b);
> +int xfs_btree_diff_two_ptrs(struct xfs_btree_cur *cur,
> +			    const union xfs_btree_ptr *a,
> +			    const union xfs_btree_ptr *b);
>  void xfs_btree_get_sibling(struct xfs_btree_cur *cur,
>  			   struct xfs_btree_block *block,
>  			   union xfs_btree_ptr *ptr, int lr);
> -- 
> 2.49.0
> 
>