Re: [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: Dave Chinner <david@fromorbit.com>,
	linux-xfs@vger.kernel.org, bfoster@redhat.com
Subject: Re: [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys
Date: Tue, 27 Aug 2019 08:19:01 -0700	[thread overview]
Message-ID: <20190827151901.GB1037350@magnolia> (raw)
In-Reply-To: <4f959cf2-4381-ffa0-ad58-9e12a40534f1@sandeen.net>

On Tue, Aug 27, 2019 at 08:01:16AM -0500, Eric Sandeen wrote:
> On 8/26/19 6:15 PM, Dave Chinner wrote:
> > On Mon, Aug 26, 2019 at 02:49:09PM -0700, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> In xfs_bmbt_diff_two_keys, we perform a signed int64_t subtraction with
> >> two unsigned 64-bit quantities.  If the second quantity is actually the
> >> "maximum" key (all ones) as used in _query_all, the subtraction
> >> effectively becomes addition of two positive numbers and the function
> >> returns incorrect results.  Fix this with explicit comparisons of the
> >> unsigned values.  Nobody needs this now, but the online repair patches
> >> will need this to work properly.
> >>
> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> ---
> >>  fs/xfs/libxfs/xfs_bmap_btree.c |   16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> >> index fbb18ba5d905..3c1a805b3775 100644
> >> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> >> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> >> @@ -400,8 +400,20 @@ xfs_bmbt_diff_two_keys(
> >>  	union xfs_btree_key	*k1,
> >>  	union xfs_btree_key	*k2)
> >>  {
> >> -	return (int64_t)be64_to_cpu(k1->bmbt.br_startoff) -
> >> -			  be64_to_cpu(k2->bmbt.br_startoff);
> >> +	uint64_t		a = be64_to_cpu(k1->bmbt.br_startoff);
> >> +	uint64_t		b = be64_to_cpu(k2->bmbt.br_startoff);
> >> +
> >> +	/*
> >> +	 * Note: This routine previously casted a and b to int64 and subtracted
> >> +	 * them to generate a result.  This lead to problems if b was the
> >> +	 * "maximum" key value (all ones) being signed incorrectly, hence this
> >> +	 * somewhat less efficient version.
> >> +	 */
> >> +	if (a > b)
> >> +		return 1;
> >> +	else if (b > a)
> >> +		return -1;
> > 
> > No need for an else here, but otherwise OK.
> > 
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> In fact having the else means the a == b case isn't handled, even if it
> should never happen, so might a static checker eventually complain about
> reaching the end of a non-void function?

Hmm?  There's a return 0 after that which Dave's reply clipped.

--D

> -Eric
> 