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

[Darrick J. Wong]

On Mon, Sep 12, 2022 at 02:39:23PM +0800, Stephen Zhang wrote:
> Dave Chinner <david@xxxxxxxxxxxxx> 于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)

Er... what problem does this solve?  Is there a defect in this range
checking code?  Why not leave it alone, since that's less retesting for
all of us.

--D

>                         break;
> ====
> 
> Thanks,
> 
> Stephen.