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: <20230801112337.1856215-1-zhangshida@kylinos.cn>
 <20230801151828.GB11332@frogsfrogsfrogs> <A9ECDF14-95A1-4B1E-A815-4B6ABF4916C6@dilger.ca>
 <CANubcdUsDfiuGimNXjzoAF5ki8waCoFW31mg4vjpm073rS6+dw@mail.gmail.com>
 <20230803030903.GK11377@frogsfrogsfrogs> <12CFFD6D-ED0E-4D36-A7B7-ACCFB698A177@dilger.ca>
In-Reply-To: <12CFFD6D-ED0E-4D36-A7B7-ACCFB698A177@dilger.ca>
From:   Stephen Zhang <starzhangzsd@gmail.com>
Date:   Fri, 4 Aug 2023 10:11:10 +0800
Message-ID: <CANubcdXYm9dxx6WSECWmjrF9C_8bhsiUSC-xDe-Pz3Mvy0nFmw@mail.gmail.com>
Subject: Re: [PATCH v3] ext4: Fix rec_len verify error
To:     Andreas Dilger <adilger@dilger.ca>
Cc:     "Darrick J. Wong" <djwong@kernel.org>,
        "Theodore Ts'o" <tytso@mit.edu>, Zhang Yi <yi.zhang@huawei.com>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        zhangshida@kylinos.cn, stable@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Andreas Dilger <adilger@dilger.ca> =E4=BA=8E2023=E5=B9=B48=E6=9C=884=E6=97=
=A5=E5=91=A8=E4=BA=94 06:34=E5=86=99=E9=81=93=EF=BC=9A
>
> On Aug 2, 2023, at 9:09 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Thu, Aug 03, 2023 at 09:52:53AM +0800, Stephen Zhang wrote:
> >> Andreas Dilger <adilger@dilger.ca> =E4=BA=8E2023=E5=B9=B48=E6=9C=882=
=E6=97=A5=E5=91=A8=E4=B8=89 14:07=E5=86=99=E9=81=93=EF=BC=9A
> >>>
> >>> Not all of these cases are actual bugs.  The ext4_rec_len_from_disk()
> >>> function is only different for rec_len >=3D 2^16, so if it is compari=
ng
> >>> rec_len against "12" or "sizeof(struct ...)" then the inequality will
> >>> be correct regardless of how it is decoded.
> >>>
> >>> That said, it makes sense to use ext4_rec_len_from_disk() to access
> >>> rec_len consistently throughout the code, since that avoids potential
> >>> bugs in the future.  We know the code will eventually will be copied
> >>> some place where rec_len >=3D 2^16 is actually important, and we may =
as
> >>> well avoid that bug before it happens.
> >>>
> >>>
> >>> One thing this discussion *does* expose is that ext4_rec_len_from_dis=
k()
> >>> is hard-coded at compile time to differentiate between PAGE_SIZE > 64=
k
> >>> and PAGE_SIZE =3D 4K, because it was never possible to have blocksize=
 >
> >>> PAGE_SIZE, so only ARM/PPC ever had filesystems with blocksize=3D64Ki=
B
> >>> (and the Fujitsu Fugaku SPARC system with blocksize=3D256KiB).
> >>>
> >>> However, with the recent advent of the VM and IO layers allowing
> >>> blocksize > PAGE_SIZE this function will need to be changed to allow
> >>> the same on x86 PAGE_SIZE=3D4KiB systems.  Instead of checking
> >>>
> >>>  #if PAGE_SIZE >=3D 65536
> >>>
> >>> it should handle this based on the filesystem blocksize at runtime:
> >>>
> >>> static inline
> >>> unsigned int ext4_rec_len_from_disk(__le16 dlen, unsigned blocksize)
> >>> {
> >>>        unsigned len =3D le16_to_cpu(dlen);
> >>>
> >>>        if (blocksize < 65536)
> >>>                return len;
> >>>
> >>>        if (len =3D=3D EXT4_MAX_REC_LEN || len =3D=3D 0)
> >>>                return blocksize;
> >>>
> >>>        return (len & 65532) | ((len & 3) << 16);
> >>> }
> >>>
> >>> Strictly speaking, ((len & 65532) | ((len & 3) << 16) should equal "l=
en"
> >>> for any filesystem with blocksize < 65536, but IMHO it is more clear =
if
> >>> the code is written this way.
> >>>
> >>> Similarly, the encoding needs to be changed to handle large records a=
t
> >>> runtime for when we eventually allow ext4 with blocksize > PAGE_SIZE.
> >>>
> >>> static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned bloc=
ksize)
> >>> {
> >>>        BUG_ON(len > blocksize);
> >>>        BUG_ON(blocksize > (1 << 18));
> >>>        BUG_ON(len & 3);
> >>>
> >>>        if (len < 65536) /* always true for blocksize < 65536 */
> >>>                return cpu_to_le16(len);
> >>>
> >>>        if (len =3D=3D blocksize) {
> >>>                if (blocksize =3D=3D 65536)
> >>>                        return cpu_to_le16(EXT4_MAX_REC_LEN);
> >>>
> >>>                return cpu_to_le16(0);
> >>>        }
> >>>
> >>>        return cpu_to_le16((len & 65532) | ((len >> 16) & 3));
> >>> }
> >>>
> >>
> >> Hmm, at least it sounds reasonable to me based on my limited
> >> knowledge. However, I am not sure whether you want me to incorporate
> >> these changes into this particular commit or another patch within this
> >> submission.
> >>
> >> By default, I will simply leave it for further discussion. Please let
> >> me know if you have any ideas.
> >
> > ext4 doesn't support blocksize > PAGE_SIZE yet.  Don't worry about this
> > for now.
>
> I agree it doesn't need to be merged into the current patch.
>
> It's something that could be fixed in a follow-on patch, to have one less
> bug to fix in the future when ext4 *does* support blocksize > PAGE_SIZE,
> which isn't so far away anymore.
>

Okay, I will attempt to submit another follow-on patch based on this discus=
sion
after this one.

Cheers,
Shida

> Cheers, Andreas
>
>
>
>
>