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;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <A9ECDF14-95A1-4B1E-A815-4B6ABF4916C6@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_7DC4E546-4390-4370-AF1A-053B5A4A010E";
 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:   Wed, 2 Aug 2023 00:07:04 -0600
In-Reply-To: <20230801151828.GB11332@frogsfrogsfrogs>
Cc:     zhangshida <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>
Precedence: bulk


--Apple-Mail=_7DC4E546-4390-4370-AF1A-053B5A4A010E
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Aug 1, 2023, at 9:18 AM, Darrick J. Wong <djwong@kernel.org> wrote:
>=20
> On Tue, Aug 01, 2023 at 07:23:37PM +0800, zhangshida wrote:
>> From: Shida Zhang <zhangshida@kylinos.cn>
>>=20
>> With the configuration PAGE_SIZE 64k and filesystem blocksize 64k,
>> a problem occurred when more than 13 million files were directly =
created
>> under a directory:
>>=20
>> EXT4-fs error (device xx): ext4_dx_csum_set:492: inode #xxxx: comm =
xxxxx: dir seems corrupt?  Run e2fsck -D.
>> EXT4-fs error (device xx): ext4_dx_csum_verify:463: inode #xxxx: comm =
xxxxx: dir seems corrupt?  Run e2fsck -D.
>> EXT4-fs error (device xx): dx_probe:856: inode #xxxx: block 8188: =
comm xxxxx: Directory index failed checksum
>>=20
>> When enough files are created, the fake_dirent->reclen will be =
0xffff.
>> it doesn't equal to the blocksize 65536, i.e. 0x10000.
>>=20
>> But it is not the same condition when blocksize equals to 4k.
>> when enough files are created, the fake_dirent->reclen will be =
0x1000.
>> it equals to the blocksize 4k, i.e. 0x1000.
>>=20
>> The problem seems to be related to the limitation of the 16-bit field
>> when the blocksize is set to 64k.
>> To address this, helpers like ext4_rec_len_{from,to}_disk has already
>> been introduce to complete the conversion between the encoded and the
>> plain form of rec_len.
>>=20
>> So fix this one by using the helper, and all the other
>> le16_to_cpu(ext4_dir_entry{,_2}.rec_len) accesses in this file too.
>>=20
>> Cc: stable@kernel.org
>> Fixes: dbe89444042a ("ext4: Calculate and verify checksums for htree =
nodes")
>> Suggested-by: Andreas Dilger <adilger@dilger.ca>
>> Suggested-by: Darrick J. Wong <djwong@kernel.org>
>> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
>> ---
>> v1->v2:
>> Use the existing helper to covert the rec_len, as suggested by =
Andreas.
>> v2->v3:
>> 1,Covert all the other rec_len if necessary, as suggested by Darrick.
>> 2,Rephrase the commit message.
>>=20
>> fs/ext4/namei.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>=20
>> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
>> index 0caf6c730ce3..8cb377b8ad86 100644
>> --- a/fs/ext4/namei.c
>> +++ b/fs/ext4/namei.c
>> @@ -346,14 +346,14 @@ static struct ext4_dir_entry_tail =
*get_dirent_tail(struct inode *inode,
>>=20
>> #ifdef PARANOID
>> 	struct ext4_dir_entry *d, *top;
>> +	int blocksize =3D EXT4_BLOCK_SIZE(inode->i_sb);
>>=20
>> 	d =3D (struct ext4_dir_entry *)bh->b_data;
>> 	top =3D (struct ext4_dir_entry *)(bh->b_data +
>> -		(EXT4_BLOCK_SIZE(inode->i_sb) -
>> -		 sizeof(struct ext4_dir_entry_tail)));
>> -	while (d < top && d->rec_len)
>> +		(blocksize - sizeof(struct ext4_dir_entry_tail)));
>> +	while (d < top && ext4_rec_len_from_disk(d->rec_len, blocksize))
>> 		d =3D (struct ext4_dir_entry *)(((void *)d) +
>> -		    le16_to_cpu(d->rec_len));
>> +		    ext4_rec_len_from_disk(d->rec_len, blocksize));
>>=20
>> 	if (d !=3D top)
>> 		return NULL;
>=20
> This is sitll missing some pieces; what about this clause at line 367:
>=20
> 	if (t->det_reserved_zero1 ||
> 	    le16_to_cpu(t->det_rec_len) !=3D sizeof(struct =
ext4_dir_entry_tail) ||
> 	    t->det_reserved_zero2 ||
> 	    t->det_reserved_ft !=3D EXT4_FT_DIR_CSUM)
> 		return NULL;
>=20
>> @@ -445,13 +445,13 @@ static struct dx_countlimit =
*get_dx_countlimit(struct inode *inode,
>> 	struct ext4_dir_entry *dp;
>> 	struct dx_root_info *root;
>> 	int count_offset;
>> +	int blocksize =3D EXT4_BLOCK_SIZE(inode->i_sb);
>>=20
>> -	if (le16_to_cpu(dirent->rec_len) =3D=3D =
EXT4_BLOCK_SIZE(inode->i_sb))
>> +	if (ext4_rec_len_from_disk(dirent->rec_len, blocksize) =3D=3D =
blocksize)
>> 		count_offset =3D 8;
>> -	else if (le16_to_cpu(dirent->rec_len) =3D=3D 12) {
>> +	else if (ext4_rec_len_from_disk(dirent->rec_len, blocksize) =3D=3D=
 12) {
>=20
> Why not lift this ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ to =
a
> local variable?  @dirent doesn't change, right?
>=20
>> 		dp =3D (struct ext4_dir_entry *)(((void *)dirent) + 12);
>> -		if (le16_to_cpu(dp->rec_len) !=3D
>> -		    EXT4_BLOCK_SIZE(inode->i_sb) - 12)
>> +		if (ext4_rec_len_from_disk(dp->rec_len, blocksize) !=3D =
blocksize - 12)
>> 			return NULL;
>> 		root =3D (struct dx_root_info *)(((void *)dp + 12));
>> 		if (root->reserved_zero ||
>=20
> What about dx_make_map?
>=20
> Here's all the opencoded access I could find:
>=20
> $ git grep le16.*rec_len fs/ext4/
> fs/ext4/namei.c:356:                le16_to_cpu(d->rec_len));
> fs/ext4/namei.c:367:        le16_to_cpu(t->det_rec_len) !=3D =
sizeof(struct ext4_dir_entry_tail) ||
> fs/ext4/namei.c:449:    if (le16_to_cpu(dirent->rec_len) =3D=3D =
EXT4_BLOCK_SIZE(inode->i_sb))
> fs/ext4/namei.c:451:    else if (le16_to_cpu(dirent->rec_len) =3D=3D =
12) {
> fs/ext4/namei.c:453:            if (le16_to_cpu(dp->rec_len) !=3D
> fs/ext4/namei.c:1338:                   map_tail->size =3D =
le16_to_cpu(de->rec_len);

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.

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_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).

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 "len"
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 at
runtime for when we eventually allow ext4 with blocksize > PAGE_SIZE.

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);

	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));
}


Cheers, Andreas


--Apple-Mail=_7DC4E546-4390-4370-AF1A-053B5A4A010E
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+AFAmTJ8ogACgkQcqXauRfM
H+CaKA//ajPC+6psxaK1xYxWe6I3WkSIJFKZqJ9wrvxvSZ3IhySoEBBgoQHK4AgB
71TDe4ijq8S8BQ33BHpxTStuIjfoUKz1qbKRvnE5+YHb1oi9w/YwZMnS3II6DxU1
UJCRu4rgl3ZEJmgNDXE9z4Lan8ynUMc/a1wowHSGEoBFrD2D1qzjnpPPfZdldq/r
EV+iBOFNrAu/omzqW7SywaNU67hh8RsJDUBAoL6+JabWnRgRp81NPjccsJnTgGod
J4b2JQHvISv5f7yeKN6KFBs7lg0k2N8YnAoxamTku3pOSQwvhFHYq7A105ydhDg2
KxyeF3daNeFcfCsXFzaDpsoFOHXL0qRxUBLV333g+pd87j0QPTFc3Xzg41LCkggh
rSB6m6J2KMOTHA3Dc+RmaWk/hFPPjqHeuGyXgNO0w9EfuREFDtYBZaNqjChiwgQR
rWsjQRcjfZT5UjKEDTJntShTkQRa1PHyVBz0EhqEsUz7TP7Fd/tNauOcAu4q+9hO
qp/8buY9USPIgx/sou7d/YdLBzvrONYQNSf5lFb/3k7gWG6Qb5UjAV5zwv3Qf/uP
MoX9R0DU4NHbN8uXGkrMWBdAbxfnP5a4bdQ+2H+ps0OSSBMTSt1Ag6UPWV7QGTjv
oyLKQmgId2qtAa0OiTITkTBv1TgppKhZYoadYe171vxJaQ40OWk=
=HuQO
-----END PGP SIGNATURE-----

--Apple-Mail=_7DC4E546-4390-4370-AF1A-053B5A4A010E--