Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <12CFFD6D-ED0E-4D36-A7B7-ACCFB698A177@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_C8462E92-ED49-4BD8-8977-EF791E645AF1";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v3] ext4: Fix rec_len verify error
Date:   Thu, 3 Aug 2023 16:34:02 -0600
In-Reply-To: <20230803030903.GK11377@frogsfrogsfrogs>
Cc:     Stephen Zhang <starzhangzsd@gmail.com>,
        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
To:     "Darrick J. Wong" <djwong@kernel.org>
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>
Precedence: bulk


--Apple-Mail=_C8462E92-ED49-4BD8-8977-EF791E645AF1
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

On Aug 2, 2023, at 9:09 PM, Darrick J. Wong <djwong@kernel.org> wrote:
>=20
> 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
>>>=20
>>> 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 =
comparing
>>> rec_len against "12" or "sizeof(struct ...)" then the inequality =
will
>>> be correct regardless of how it is decoded.
>>>=20
>>> 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.
>>>=20
>>>=20
>>> One thing this discussion *does* expose is that =
ext4_rec_len_from_disk()
>>> is hard-coded at compile time to differentiate between PAGE_SIZE > =
64k
>>> and PAGE_SIZE =3D 4K, because it was never possible to have =
blocksize >
>>> PAGE_SIZE, so only ARM/PPC ever had filesystems with blocksize=3D64KiB=

>>> (and the Fujitsu Fugaku SPARC system with blocksize=3D256KiB).
>>>=20
>>> 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
>>>=20
>>>  #if PAGE_SIZE >=3D 65536
>>>=20
>>> it should handle this based on the filesystem blocksize at runtime:
>>>=20
>>> static inline
>>> unsigned int ext4_rec_len_from_disk(__le16 dlen, unsigned blocksize)
>>> {
>>>        unsigned len =3D le16_to_cpu(dlen);
>>>=20
>>>        if (blocksize < 65536)
>>>                return len;
>>>=20
>>>        if (len =3D=3D EXT4_MAX_REC_LEN || len =3D=3D 0)
>>>                return blocksize;
>>>=20
>>>        return (len & 65532) | ((len & 3) << 16);
>>> }
>>>=20
>>> Strictly speaking, ((len & 65532) | ((len & 3) << 16) should equal =
"len"
>>> for any filesystem with blocksize < 65536, but IMHO it is more clear =
if
>>> the code is written this way.
>>>=20
>>> Similarly, the encoding needs to be changed to handle large records =
at
>>> runtime for when we eventually allow ext4 with blocksize > =
PAGE_SIZE.
>>>=20
>>> static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned =
blocksize)
>>> {
>>>        BUG_ON(len > blocksize);
>>>        BUG_ON(blocksize > (1 << 18));
>>>        BUG_ON(len & 3);
>>>=20
>>>        if (len < 65536) /* always true for blocksize < 65536 */
>>>                return cpu_to_le16(len);
>>>=20
>>>        if (len =3D=3D blocksize) {
>>>                if (blocksize =3D=3D 65536)
>>>                        return cpu_to_le16(EXT4_MAX_REC_LEN);
>>>=20
>>>                return cpu_to_le16(0);
>>>        }
>>>=20
>>>        return cpu_to_le16((len & 65532) | ((len >> 16) & 3));
>>> }
>>>=20
>>=20
>> 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.
>>=20
>> By default, I will simply leave it for further discussion. Please let
>> me know if you have any ideas.
>=20
> 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.

Cheers, Andreas


--Apple-Mail=_C8462E92-ED49-4BD8-8977-EF791E645AF1
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmTMK1oACgkQcqXauRfM
H+A7sBAAhXmLZ/oDD73vpfMHTdJmdWe0rB/SirU1fQ/f7WZsmyMqMN9gqMI7Fhpq
sxjn2ML1P9cLUgyjTCf5usJ743ewZX77Jv8y0+J3T6yd5/aFB0e4rqHR9coxN4L9
sO/3j9r0f+ZPJ4Wl7QHlPr2COjAAEDz0O4KtSKMk/+doGvsitB2oiB9x8Vj/BPBD
xU6a8OjmLotHzpR99pgVYb1whGTUeG5tFQTPBxAj9NBPQ3mr5wYZ5TfdkOtgpDAh
DeuKWGc6X1ByrLajuwybSTlxpNcKZr5aOmNuVPPkydBT89U6GdKXtCNYO6KS6ri0
Zv6LCPeL0A/cVApQ9KXLwJv6ckqMXnjJPoQZ7gceiSNR9kd0kTMGODPyITlo2pM5
CAcT778lWGWKiC6QyS/iewFXgrcaAyURclIwnckIrgzcEBvP6BLDkQXtQXeCx5D3
UIwM+MDHec0dTQXt25hVNr5sT6VUWVXGBJVCNt+cPhM5TIqecEYGYHuLS5p+AQhY
9VF2lisZTuUz/MThYGyzWY7j/kJfFYP1C3aN18VeA5xKLG8I7tHTC1ToWMIgrRk2
ci5kRUhbpL0ofioFSLSKN/kMNPJqlGoYps3KnC6nLnmlojwRUh9xp/VnxXBO44jd
gcTPcxOHAR09OiyQqkGMgq78+7zv05VMAIRKmySgpkbWxN1bWdo=
=1rAp
-----END PGP SIGNATURE-----

--Apple-Mail=_C8462E92-ED49-4BD8-8977-EF791E645AF1--