[PATCH 1/1] ext4: drop extra ea_inode ref in cached-block reuse path

[PATCH 1/1] ext4: drop extra ea_inode ref in cached-block reuse path

[Matthias Goergens]

ext4_xattr_block_set() acquires a reference to the ea_inode via
ext4_xattr_inode_lookup_create() before constructing the new xattr
block.  In the new-block allocation path this extra reference is dropped
after ext4_xattr_inode_inc_ref_all() accounts for every entry in the
block.  But when mbcache supplies an identical existing block and that
block is reused, the cached block already carries the correct refcount
for its entries, and the extra reference is never dropped.

The cleanup block at the end of the function only calls
ext4_xattr_inode_dec_ref() when error is non-zero, so the success path
through cache reuse leaks one refcount.  Each reuse event adds another;
eventually the saturated refcount prevents the ea_inode from being freed
when its last real reference is dropped.

Add the matching drop when the entry was not written directly to the old
physical buffer (bs->bh && s->base == bs->bh->b_data).  In that case the
reference belongs to the old buffer, which the later release_block()
handles.  When s->base is a clone or a fresh allocation and is discarded
in favour of the cached block, the extra reference must be dropped.

Found by the Sashiko AI review bot while reviewing the xattr
saturation-livelock fix.

Fixes: 0a46ef234756 ("ext4: do not create EA inode under buffer lock")
---
 fs/ext4/xattr.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 982a1f831e228..cf26932a528b9 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2107,6 +2107,26 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 			mb_cache_entry_touch(ea_block_cache, ce);
 			mb_cache_entry_put(ea_block_cache, ce);
 			ce = NULL;
+			if (!(bs->bh && s->base == bs->bh->b_data) &&
+			    ea_inode) {
+				/*
+				 * The reused cached block already holds a
+				 * reference to the ea_inode.  Drop the extra
+				 * reference from
+				 * ext4_xattr_inode_lookup_create() unless the
+				 * entry was written directly to the old
+				 * physical buffer, in which case the reference
+				 * belongs to that buffer.
+				 */
+				error = ext4_xattr_inode_dec_ref(handle,
+								 ea_inode);
+				if (error)
+					ext4_warning_inode(ea_inode,
+							   "dec ref error=%d",
+							   error);
+				iput(ea_inode);
+				ea_inode = NULL;
+			}
 		} else if (bs->bh && s->base == bs->bh->b_data) {
 			/* We were modifying this block in-place. */
 			ea_bdebug(bs->bh, "keeping this block");
-- 
2.55.0

Re: [PATCH 1/1] ext4: drop extra ea_inode ref in cached-block reuse path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] On-disk reference counts for ea_inodes and physical blocks are leaked if block creation fails after calling ext4_xattr_inode_inc_ref_all().
--

--- Patch [1]: [PATCH 1/1] ext4: drop extra ea_inode ref in cached-block reuse path ---
commit 57ed7d8076824778cc2e749011823fc10096cd99
Author: Matthias Goergens <matthias.goergens@gmail.com>

ext4: drop extra ea_inode ref in cached-block reuse path

ext4_xattr_block_set() acquires a reference to the ea_inode via
ext4_xattr_inode_lookup_create() before constructing the new xattr
block.  In the new-block allocation path this extra reference is dropped
after ext4_xattr_inode_inc_ref_all() accounts for every entry in the
block.  But when mbcache supplies an identical existing block and that
block is reused, the cached block already carries the correct refcount
for its entries, and the extra reference is never dropped.

> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 6127e94ea4f5d..cec71c993286d 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2032,6 +2032,26 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  			mb_cache_entry_touch(ea_block_cache, ce);
>  			mb_cache_entry_put(ea_block_cache, ce);
>  			ce = NULL;
> +			if (!(bs->bh && s->base == bs->bh->b_data) &&
> +			    ea_inode) {
[ ... ]
> +				error = ext4_xattr_inode_dec_ref(handle,
> +								 ea_inode);
> +				if (error)
> +					ext4_warning_inode(ea_inode,
> +							   "dec ref error=%d",
> +							   error);
> +				iput(ea_inode);
> +				ea_inode = NULL;
> +			}
>  		} else if (bs->bh && s->base == bs->bh->b_data) {

[Severity: High]
This is a pre-existing issue, but does this error path leak on-disk EA inode
reference counts, block quota, and physical blocks?

Further down in the new block allocation path of ext4_xattr_block_set(), the
code calls ext4_xattr_inode_inc_ref_all() to increment the on-disk reference
counts for all entries in the block:

			error = ext4_xattr_inode_inc_ref_all(handle, inode,
						      ENTRY(header(s->base)+1));
			if (error)
				goto getblk_failed;

If a subsequent operation like ext4_handle_dirty_metadata() fails, such as
returning -EIO due to a synchronous write failure in nojournal mode:

			error = ext4_handle_dirty_metadata(handle, inode,
							   new_bh);
			if (error)
				goto cleanup;

The code jumps directly to the cleanup label.

Will jumping to cleanup bypass block deallocation (getblk_failed) and
fail to revert the previously incremented EA inode references?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812044556.2209787-1-matthias.goergens@gmail.com?part=1