Received-SPF: pass (google.com: domain of linux-ext4+bounces-1809-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Andreas Dilger <adilger@dilger.ca>
Message-Id: <4CCBB8C1-30AD-411F-AED2-781677E89FB7@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_F190B5BB-5339-49A2-9CF9-1365BB9E66D9";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH 2/4] e2fsck: update quota when deallocating a bad inode
Date: Mon, 1 Apr 2024 14:52:23 -0600
In-Reply-To: <20240328172940.1609-3-luis.henriques@linux.dev>
Cc: Theodore Ts'o <tytso@mit.edu>,
 Ext4 Developers List <linux-ext4@vger.kernel.org>
To: "Luis Henriques (SUSE)" <luis.henriques@linux.dev>
References: <20240328172940.1609-1-luis.henriques@linux.dev>
 <20240328172940.1609-3-luis.henriques@linux.dev>


--Apple-Mail=_F190B5BB-5339-49A2-9CF9-1365BB9E66D9
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Mar 28, 2024, at 11:29 AM, Luis Henriques (SUSE) =
<luis.henriques@linux.dev> wrote:
>=20
> If a bad inode is found it will be deallocated.  However, if the
> filesystem has quota enabled, the quota information isn't being =
updated
> accordingly.  This issue was detected by running fstest ext4/019.
>=20
> This patch fixes the issue by decreasing the inode count from the =
quota
> and, if blocks are also being released, also subtract them as well.

I was wondering about the safety of this patch, since "bad inode"
usually means corrupted inode with random garbage, and continuing to
process it is as likely to introduce problems as it is to fix them.

The only caller of deallocate_inode() is e2fsck_process_bad_inode()
when the file type is bad or not matching the inode content.  This
appears it would mostly affect only the inode quota, though in some
rare cases it might affect the block quota (xattr or long symlink).

Looking at the history of deallocate_inode(), it appears risky that
it processes blocks from a corrupt inode at all.  The saving grace
is that it should only "undo" the blocks marked in-use by pass1 so
that a second e2fsck run is not needed to fix up the bitmaps and
counters.  It looks like the same is true with the quota accounting,
that deallocate_inode() is only updating the in-memory inode/block
counters for the UID/GID/PRJID but not messing with any on-disk data
until on-disk quotas are updated in pass5.

It wouldn't be terrible to update the comment before deallocate_inode()
to explain this more clearly, since "This function deallocates an inode"
doesn't really give the reader much more insight than the function name
"deallocate_inode()".  Maybe something more useful like:

/*
 * This function reverts various counters and bitmaps incremented in
 * pass1 for the inode, blocks, and quotas before it was decided the
 * inode was corrupt and needed to be cleared.  This avoids the need
 * to run e2fsck a second time (or have it restart itself) to repair
 * these counters.
 *
 * It does not modify any on-disk state, so even if the inode is bad
 * it _should_ reset in-memory state to before the inode was first
 * processed.
 */

... would be helpful to readers in the future.  In either case, you
can add my:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Cheers, Andreas

>=20
> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
> ---
> e2fsck/pass2.c | 33 +++++++++++++++++++++++----------
> 1 file changed, 23 insertions(+), 10 deletions(-)
>=20
> diff --git a/e2fsck/pass2.c b/e2fsck/pass2.c
> index b91628567a7f..e16b488af643 100644
> --- a/e2fsck/pass2.c
> +++ b/e2fsck/pass2.c
> @@ -1859,12 +1859,13 @@ static int deallocate_inode_block(ext2_filsys =
fs,
> static void deallocate_inode(e2fsck_t ctx, ext2_ino_t ino, char* =
block_buf)
> {
> 	ext2_filsys fs =3D ctx->fs;
> -	struct ext2_inode	inode;
> +	struct ext2_inode_large	inode;
> 	struct problem_context	pctx;
> 	__u32			count;
> 	struct del_block	del_block;
>=20
> -	e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
> +	e2fsck_read_inode_full(ctx, ino, EXT2_INODE(&inode),
> +			       sizeof(inode), "deallocate_inode");
> 	clear_problem_context(&pctx);
> 	pctx.ino =3D ino;
>=20
> @@ -1874,29 +1875,29 @@ static void deallocate_inode(e2fsck_t ctx, =
ext2_ino_t ino, char* block_buf)
> 	e2fsck_read_bitmaps(ctx);
> 	ext2fs_inode_alloc_stats2(fs, ino, -1, =
LINUX_S_ISDIR(inode.i_mode));
>=20
> -	if (ext2fs_file_acl_block(fs, &inode) &&
> +	if (ext2fs_file_acl_block(fs, EXT2_INODE(&inode)) &&
> 	    ext2fs_has_feature_xattr(fs->super)) {
> 		pctx.errcode =3D ext2fs_adjust_ea_refcount3(fs,
> -				ext2fs_file_acl_block(fs, &inode),
> +				ext2fs_file_acl_block(fs, =
EXT2_INODE(&inode)),
> 				block_buf, -1, &count, ino);
> 		if (pctx.errcode =3D=3D EXT2_ET_BAD_EA_BLOCK_NUM) {
> 			pctx.errcode =3D 0;
> 			count =3D 1;
> 		}
> 		if (pctx.errcode) {
> -			pctx.blk =3D ext2fs_file_acl_block(fs, &inode);
> +			pctx.blk =3D ext2fs_file_acl_block(fs, =
EXT2_INODE(&inode));
> 			fix_problem(ctx, PR_2_ADJ_EA_REFCOUNT, &pctx);
> 			ctx->flags |=3D E2F_FLAG_ABORT;
> 			return;
> 		}
> 		if (count =3D=3D 0) {
> 			ext2fs_block_alloc_stats2(fs,
> -				  ext2fs_file_acl_block(fs, &inode), =
-1);
> +				  ext2fs_file_acl_block(fs, =
EXT2_INODE(&inode)), -1);
> 		}
> -		ext2fs_file_acl_block_set(fs, &inode, 0);
> +		ext2fs_file_acl_block_set(fs, EXT2_INODE(&inode), 0);
> 	}
>=20
> -	if (!ext2fs_inode_has_valid_blocks2(fs, &inode))
> +	if (!ext2fs_inode_has_valid_blocks2(fs, EXT2_INODE(&inode)))
> 		goto clear_inode;
>=20
> 	/* Inline data inodes don't have blocks to iterate */
> @@ -1921,10 +1922,22 @@ static void deallocate_inode(e2fsck_t ctx, =
ext2_ino_t ino, char* block_buf)
> 		ctx->flags |=3D E2F_FLAG_ABORT;
> 		return;
> 	}
> +
> +	if ((ino !=3D quota_type2inum(PRJQUOTA, fs->super)) &&
> +	    (ino !=3D fs->super->s_orphan_file_inum) &&
> +	    (ino =3D=3D EXT2_ROOT_INO || ino >=3D =
EXT2_FIRST_INODE(ctx->fs->super)) &&
> +	    !(inode.i_flags & EXT4_EA_INODE_FL)) {
> +		if (del_block.num > 0)
> +			quota_data_sub(ctx->qctx, &inode, ino,
> +				       del_block.num * =
EXT2_CLUSTER_SIZE(fs->super));
> +		quota_data_inodes(ctx->qctx, (struct ext2_inode_large =
*)&inode,
> +				  ino, -1);
> +	}
> +
> clear_inode:
> 	/* Inode may have changed by block_iterate, so reread it */
> -	e2fsck_read_inode(ctx, ino, &inode, "deallocate_inode");
> -	e2fsck_clear_inode(ctx, ino, &inode, 0, "deallocate_inode");
> +	e2fsck_read_inode(ctx, ino, EXT2_INODE(&inode), =
"deallocate_inode");
> +	e2fsck_clear_inode(ctx, ino, EXT2_INODE(&inode), 0, =
"deallocate_inode");
> }
>=20
> /*


Cheers, Andreas


--Apple-Mail=_F190B5BB-5339-49A2-9CF9-1365BB9E66D9
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+AFAmYLHocACgkQcqXauRfM
H+Dm1g/+OfdtfqUUzpgDlAdc4OYdEXiXh1AhyNqb2favrIE6MY39XByin+PAyh7I
qLrGh1ZNffwT4lE9EVRaWjfDerFS71oHIBL2g0IkFEHoVqtpVOqtMqVPTx5yfUsc
Xk+Wu5dVR/f1UuPIvGWKywPMrYXkKWsZZ2MXmgpWKObqM6cnNlrAmLyRVYrLiCgE
W+WUtnnV1UM5z5qwNA6/uTTfWKUCRS1A9xvH2pKHzN5b3hNguO27nx+PIkFM2eBs
VTelNxd79nFJ6cp+wv+/QIQiaNrKrzk/AID39Znt9Fv91I1fPrFxMpXoDqEuxLpv
HJvDZ83nAVTr3TV52zodkMATedm/7+buPDgAhRlu4sa61jyX2y48c89EQPt63m7y
1YU41UOcDOCYNzUw30u9VpQg1VKAfSBJgDPMD/fXuDK6l8wd0c5NlxgLWJn9hfwD
qtQcVC3rCOFY++w8mmImGzhnSVV1USIWFC1EcDUS6LJVGLaphYpuwTSgqG/1B+Ux
hLAGoLwZfuuu7heCjkNwiRV9bXXtb3npEpQ70jHsYMtbf2rTnZrYUQNxoYckzrHS
20pNeIzJtBR9Dt98TVXN69TxSM7O6es5nT2umSHQUMkVzWTSNJv2OcqSGns1JqXH
pKsrQONQX4Nvk/FFt2VdUfaNewUhSrsxOX2fvibsEgimFnVzylE=
=KWun
-----END PGP SIGNATURE-----

--Apple-Mail=_F190B5BB-5339-49A2-9CF9-1365BB9E66D9--