Re: [BUG] ext4: unvalidated h_refcount on a shared xattr block -> double free, detected too late

[Guikun Yang]

From: Yang Guikun <yangguikun19@xxxxxx>
To: linux-ext4@xxxxxxxxxxxxxxx, Theodore Ts'o <tytso@xxxxxxx>
Cc: Jan Kara <jack@xxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [BUG] ext4: unvalidated h_refcount on a shared xattr block -> double free, detected too late
In-Reply-To: <tencent_B025D3B328709FF3A8B5D4B3EFED913C2707@xxxxxx>
References: <tencent_B025D3B328709FF3A8B5D4B3EFED913C2707@xxxxxx>

Hi,

Friendly ping on this one - it has been quiet for a bit over two weeks, and I
wanted to add data rather than send a bare ping.

Update: the reproducer still fires on 7.3.0-rc7, 11/11 runs with a 64MB image
and a 5-thread hammer (the churn count raised from 8 to 64; 3 xattr-churn + 2
donor-churn threads). The earlier numbers stand too: 6/6 on 7.3.0-rc5 with the
8MB single-thread config. The release-path code in question is unchanged
between rc5, rc7 and current master, so this is not a regression and has not
been fixed in the meantime. The one race-dependent panic from the original
report has not recurred in the rc7 campaign; the deterministic double free is
the reproducible part.

The suggested direction from the original report stands: a
"freeing already freed block" error raised from the xattr release path means
a wrong free has already happened, so containing it (fs abort / remount-ro
rather than error + continue) looks like the sensible hardening point.

If it helps, a minimal containment would check the block bitmap bit right
before the ext4_free_blocks() call in ext4_xattr_release_block(): this call is
the one that clears the bit, so at that point it must still be set. If it is
already clear, the on-disk h_refcount was wrong and the block has had a
double lifetime since mount - the state that merits the ERRORS= policy:

--- cut ---
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1320,6 +1320,12 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
 		get_bh(bh);
 		unlock_buffer(bh);
 
+		if (!ext4_test_bit(EXT4_B2C(EXT4_SB(inode->i_sb),
+				   bh->b_blocknr -
+				   ext4_group_first_block_no(inode->i_sb,
+				   ext4_inode_to_group(inode))),
+				   bitmap_bh->b_data))
+			ext4_error(inode->i_sb,
+				   "releasing xattr block %lu which is already free; on-disk h_refcount is wrong",
+				   (unsigned long)bh->b_blocknr);
 		if (ext4_has_feature_ea_inode(inode->i_sb))
 			ext4_xattr_inode_dec_ref_all(handle, inode, bh,
--- cut ---

(That sketch glosses over fetching the group's bitmap_bh and the cluster
conversion; I will write it out properly and test it if you think the
approach is worth pursuing. Any containment shape that makes this state hit
ERRORS= instead of continuing silently works for me.)

Thanks,
Guikun Yang <yangguikun19@xxxxxx>