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@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx>
>>
>> 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
