Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220909030756.3916297-1-zhangshida@kylinos.cn> <20220911231251.GA3600936@dread.disaster.area>
In-Reply-To: <20220911231251.GA3600936@dread.disaster.area>
From:   Stephen Zhang <starzhangzsd@gmail.com>
Date:   Mon, 12 Sep 2022 14:39:23 +0800
Message-ID: <CANubcdWe9thzi0WXHBg+vccP7UaGv1c8FiGQkORV6PGw_4cOwQ@mail.gmail.com>
Subject: Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Dave Chinner <david@fromorbit.com> =E4=BA=8E2022=E5=B9=B49=E6=9C=8812=E6=97=
=A5=E5=91=A8=E4=B8=80 07:12=E5=86=99=E9=81=93=EF=BC=9A
> 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 =3D 0;
>         xfs_fileoff_t           lowest, max;
>
> We end up with the following calculations (in FSBs, not bytes):
>
>         lowest + len    =3D 0x800000ULL + 1
>                         =3D 0x800001ULL
>
>         got.br_startoff - max   =3D 0ULL - 0x800000
>                                 =3D 0xffffffffff800000ULL
>
> and so the existing check is:
>
>         if (0 >=3D 0x800001ULL && 0xffffffffff800000 >=3D 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 >=3D 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:
=3D=3D=3D=3D
xfs_bmap_first_unused(
                /*
                 * See if the hole before this extent will work.
                 */
-               if (got.br_startoff >=3D lowest + len &&
-                   got.br_startoff - max >=3D len)
+               if (got.br_startoff >=3D max + len)
                        break;
=3D=3D=3D=3D

Thanks,

Stephen.