diff mbox series

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

Message ID 156685614992.2853532.4191470495720238021.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: fixes for 5.4 | expand

Commit Message

Darrick J. Wong Aug. 26, 2019, 9:49 p.m. UTC 
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(-)

Comments

Dave Chinner Aug. 26, 2019, 11:15 p.m. UTC | #1 
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>
Eric Sandeen Aug. 27, 2019, 1:01 p.m. UTC | #2 
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?

-Eric
Darrick J. Wong Aug. 27, 2019, 3:19 p.m. UTC | #3 
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
>
Eric Sandeen Aug. 27, 2019, 3:20 p.m. UTC | #4 
On 8/27/19 10:19 AM, Darrick J. Wong wrote:
> 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.

Oh sorry, patch/thread reading skills lacking, too early in the AM.

-Eric
diff mbox series

Patch

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;
+	return 0;
 }
 
 static xfs_failaddr_t