linux-kernel - Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANubcdWe9thzi0WXHBg+vccP7UaGv1c8FiGQkORV6PGw_4cOwQ@mail.gmail.com>
Date:   Mon, 12 Sep 2022 14:39:23 +0800
From:   Stephen Zhang <starzhangzsd@...il.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     djwong@...nel.org, dchinner@...hat.com, chandan.babu@...cle.com,
        zhangshida@...inos.cn, linux-kernel@...r.kernel.org,
        linux-xfs@...r.kernel.org
Subject: Re: [PATCH] xfs: remove the redundant check in xfs_bmap_first_unused

Dave Chinner <david@...morbit.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.