Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused

From: Stephen Zhang <starzhangzsd@gmail.com>
To: Dave Chinner <david@fromorbit.com>
Cc: djwong@kernel.org, dchinner@redhat.com, chandan.babu@oracle.com,
	zhangshida@kylinos.cn, linux-kernel@vger.kernel.org,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused
Date: Mon, 12 Sep 2022 14:39:23 +0800	[thread overview]
Message-ID: <CANubcdWe9thzi0WXHBg+vccP7UaGv1c8FiGQkORV6PGw_4cOwQ@mail.gmail.com> (raw)
In-Reply-To: <20220911231251.GA3600936@dread.disaster.area>

Dave Chinner <david@fromorbit.com> 于2022年9月12日周一 07:12写道：
> Given that all the types and comparisons involved are 64 bit
> unsigned:
>
> typedef uint64_t        xfs_fileoff_t;  /* block number in a file */
>
> #define XFS_FILEOFF_MAX(a,b) max_t(xfs_fileoff_t, (a), (b))
>
>         xfs_fileoff_t br_startoff;
>
>         xfs_fileoff_t           lastaddr = 0;
>         xfs_fileoff_t           lowest, max;
>
> We end up with the following calculations (in FSBs, not bytes):
>
>         lowest + len    = 0x800000ULL + 1
>                         = 0x800001ULL
>
>         got.br_startoff - max   = 0ULL - 0x800000
>                                 = 0xffffffffff800000ULL
>
> and so the existing check is:
>
>         if (0 >= 0x800001ULL && 0xffffffffff800000 >= 1)
>
> which evaluates as false because the extent that was found is not
> beyond the initial offset (first_unused) that we need to start
> searching at.
>
> With your modification, this would now evaluate as:
>
>         if (0xffffffffff800000 >= 1)
>
> Because of the underflow, this would then evaluate as true  and we'd
> return 0 as the first unused offset. This is incorrect as we do not
> have a hole at offset 0, nor is it within the correct directory
> offset segment, nor is it within the search bounds we have
> specified.
>
> If these were all signed types, then your proposed code might be
> correct. But they are unsigned and hence we have to ensure that we
> handle overflow/underflow appropriately.
>
> Which leads me to ask: did you test this change before you send
> it to the list?
>

I am so sorry about the mistake, and thanks for your elaboration about
this problem. it indeed teaches me a lesson about the necessity of test
even for the simplest change.

By the way, theoretically, in order to solve this, I wonder if we could
change the code in the following way:
====
xfs_bmap_first_unused(
                /*
                 * See if the hole before this extent will work.
                 */
-               if (got.br_startoff >= lowest + len &&
-                   got.br_startoff - max >= len)
+               if (got.br_startoff >= max + len)
                        break;
====

Thanks,

Stephen.