[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

[PATCH v2] 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.

v2:
- The v1 dropped the reference best-effort: a failure was logged but
  masked by the later "error = 0" on the success path, so the syscall
  could succeed with the on-disk refcount one too high.  Fail the
  operation instead, and undo the locally-owned part of the reuse
  through ext4_xattr_reuse_undo(), the same helper the success path
  uses for the old block.  Any reference that nonetheless persists
  (e.g. under errors=continue) is repaired by e2fsck.
- A failed decrement is never retried: it may already have mutated
  the refcount before failing (e.g. an EA inode inheriting S_SYNC from
  the root directory can fail ext4_mark_iloc_dirty() after the
  mutation in nojournal mode), so a retry could decrement twice.  The
  failure paths free the quota charge and drop the inode reference
  directly.
- Propagate the same masked failure at the new-block branch's
  identical drop: it has always been best-effort (warn and continue).

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")
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 fs/ext4/xattr.c | 62 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 982a1f831e228..0ce937d890042 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1362,6 +1362,17 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
 	return;
 }
 
+/* Undo the locally-owned part of a cached-block reuse. */
+static void ext4_xattr_reuse_undo(handle_t *handle, struct inode *inode,
+				  struct buffer_head *bh)
+{
+	struct ext4_xattr_inode_array *ea_inode_array = NULL;
+
+	ext4_xattr_release_block(handle, inode, bh, &ea_inode_array,
+				 0 /* extra_credits */);
+	ext4_xattr_inode_array_free(ea_inode_array);
+}
+
 /*
  * Find the available free space for EAs. This also returns the total number of
  * bytes used by EA entries.
@@ -2107,6 +2118,39 @@ 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 block already holds its own
+				 * reference; drop the extra one unless the
+				 * entry went into the old buffer directly.
+				 */
+				error = ext4_xattr_inode_dec_ref(handle,
+								 ea_inode);
+				if (error) {
+					/*
+					 * The decrement may already have run;
+					 * do not retry it: free the charge,
+					 * undo the reuse, and fail the op.
+					 */
+					ext4_error_inode(inode, __func__,
+							 __LINE__, 0,
+							"dec ref error=%d",
+							 error);
+					ext4_xattr_inode_free_quota(inode,
+								    ea_inode,
+								    i_size_read(ea_inode));
+					iput(ea_inode);
+					ea_inode = NULL;
+					if (new_bh != bs->bh)
+						ext4_xattr_reuse_undo(handle,
+								      inode,
+								      new_bh);
+					goto cleanup;
+				}
+				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");
@@ -2143,13 +2187,21 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 			if (error)
 				goto getblk_failed;
 			if (ea_inode) {
-				/* Drop the extra ref on ea_inode. */
+				/*
+				 * Drop the extra ref on ea_inode; on
+				 * failure the decrement may already have
+				 * run, so just fail and free the block.
+				 */
 				error = ext4_xattr_inode_dec_ref(handle,
 								 ea_inode);
-				if (error)
-					ext4_warning_inode(ea_inode,
-							   "dec ref error=%d",
-							   error);
+				if (error) {
+					ext4_xattr_inode_free_quota(inode,
+								    ea_inode,
+								    i_size_read(ea_inode));
+					iput(ea_inode);
+					ea_inode = NULL;
+					goto getblk_failed;
+				}
 				iput(ea_inode);
 				ea_inode = NULL;
 			}
-- 
2.55.0

Re: [PATCH v2] 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] Permanent reference count leak for EA inodes when jumping to `getblk_failed` after `ext4_xattr_inode_inc_ref_all()` succeeds.
--

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

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

This commit addresses an EA inode reference count leak when reusing an
identical cached block in ext4_xattr_block_set(). It also modifies the
new-block allocation path to fail the operation instead of just warning
if dropping the extra reference fails.

> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 6127e94ea4f5d..29b08b69da2be 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c

[ ... ]

> @@ -2075,13 +2119,21 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  			if (error)
>  				goto getblk_failed;
>  			if (ea_inode) {
> -				/* Drop the extra ref on ea_inode. */
> +				/*
> +				 * Drop the extra ref on ea_inode; on
> +				 * failure the decrement may already have
> +				 * run, so just fail and free the block.
> +				 */
>  				error = ext4_xattr_inode_dec_ref(handle,
>  								 ea_inode);
> -				if (error)
> -					ext4_warning_inode(ea_inode,
> -							   "dec ref error=%d",
> -							   error);
> +				if (error) {
> +					ext4_xattr_inode_free_quota(inode,
> +								    ea_inode,
> +								    i_size_read(ea_inode));
> +					iput(ea_inode);
> +					ea_inode = NULL;
> +					goto getblk_failed;

[Severity: High]
This isn't a bug introduced by this patch, but this new goto getblk_failed
path appears to exacerbate a pre-existing reference count leak for EA inodes.

Just above this block, ext4_xattr_inode_inc_ref_all() successfully iterates
over all EA entries and increments their inode reference counts:

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

If the decrement of the extra reference fails, the code now jumps to
getblk_failed. The getblk_failed label frees the newly allocated physical
block and jumps to cleanup:

getblk_failed:
	ext4_free_blocks(handle, inode, NULL, block, 1,
			 EXT4_FREE_BLOCKS_METADATA);
	goto cleanup;

However, it completely skips rolling back the newly incremented EA inode
references via ext4_xattr_inode_dec_ref_all(). Because inc_ref_all already
called ext4_mark_iloc_dirty() for those EA inodes, won't the artificially
elevated reference counts be committed to the journal?

Since the error here doesn't mark the filesystem as containing errors,
could this silently leak these references without triggering an automatic
e2fsck on the next reboot?

> +				}
>  				iput(ea_inode);
>  				ea_inode = NULL;
>  			}

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

[PATCH] ext4: compensate ea_inode refs and free block on new xattr block write error

[Matthias Goergens]

ext4_xattr_block_set() increments the on-disk reference counts of the
ea_inodes a new xattr block refers to before dirtying the block.  If
ext4_handle_dirty_metadata() then fails, the error path neither undoes
the increments nor frees the block.

With a journal the failure aborts the journal and the transaction is
discarded, so nothing leaks.  Without one, ext4_handle_dirty_metadata()
can return -EIO when sync_dirty_buffer() fails for an inode that needs
sync, the increments are already in the dirty ea_inode inode blocks and
reach disk: the reference counts stay one too high forever, the inodes
are never reclaimed, the new block stays allocated but unreferenced,
and the quota charged for the new value is never released.

Compensate on the failure: remove the block from the mbcache, drop the
references, free the block, and release the quota this operation
charged.  A concurrent setxattr may have taken the cached block
meanwhile, so re-check its reference count under the buffer lock and
skip the free if an owner appeared — degrading to the pre-existing
leak rather than corrupting a live owner.

Verified on a nojournal filesystem with ext4_handle_dirty_metadata()
instrumented to return -EIO for the new xattr block (the sync_dirty_
buffer() failure case): unpatched, e2fsck -fn reports the stale
ea_inode reference count, the lost block and the parent's i_blocks
residue; patched, the same injection leaves a clean filesystem and the
no-injection control behaves as before.

Reported-by: Sashiko AI review bot <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/linux-ext4/20260814070432.595B91F00A3A@smtp.kernel.org/
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 fs/ext4/xattr.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 982a1f831e228..e086c8395042a 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1362,6 +1362,63 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
 	return;
 }
 
+/*
+ * Undo the setup of a new xattr block that failed to be written out: drop
+ * the ext4_xattr_inode_inc_ref_all() references, free the block, and
+ * release the quota this operation charged for the new value (quota_len,
+ * zero when the value is not in an EA inode).
+ */
+static void ext4_xattr_new_block_fail(handle_t *handle, struct inode *inode,
+				      struct buffer_head *new_bh,
+				      size_t quota_len, int error)
+{
+	struct mb_cache *ea_block_cache = EA_BLOCK_CACHE(inode);
+	struct mb_cache_entry *oe;
+	struct ext4_xattr_inode_array *ea_inode_array = NULL;
+
+	ext4_error_inode(inode, __func__, __LINE__, 0,
+			 "xattr block dirty failed: %d", error);
+	lock_buffer(new_bh);
+retry_owner:
+	if (le32_to_cpu(BHDR(new_bh)->h_refcount) != 1) {
+		unlock_buffer(new_bh);
+		return;
+	}
+	if (ea_block_cache) {
+		oe = mb_cache_entry_delete_or_get(ea_block_cache,
+						  le32_to_cpu(BHDR(new_bh)->h_hash),
+						  new_bh->b_blocknr);
+		if (oe) {
+			unlock_buffer(new_bh);
+			mb_cache_entry_wait_unused(oe);
+			mb_cache_entry_put(ea_block_cache, oe);
+			lock_buffer(new_bh);
+			goto retry_owner;
+		}
+	}
+	get_bh(new_bh);
+	unlock_buffer(new_bh);
+
+	ext4_xattr_inode_dec_ref_all(handle, inode, new_bh,
+				     ENTRY(BHDR(new_bh) + 1),
+				     true /* block_csum */,
+				     &ea_inode_array,
+				     0 /* extra_credits */,
+				     true /* skip_quota */);
+	ext4_xattr_inode_array_free(ea_inode_array);
+	if (quota_len) {
+		/*
+		 * Reverses this operation's own ext4_xattr_inode_alloc_quota()
+		 * charge, so no EA inode pointer is needed here.
+		 */
+		ext4_xattr_inode_free_quota(inode, NULL, quota_len);
+		ext4_mark_inode_dirty(handle, inode);
+	}
+	ext4_free_blocks(handle, inode, new_bh, 0, 1,
+			 EXT4_FREE_BLOCKS_METADATA |
+			 EXT4_FREE_BLOCKS_FORGET);
+}
+
 /*
  * Find the available free space for EAs. This also returns the total number of
  * bytes used by EA entries.
@@ -2169,8 +2226,13 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
 			ext4_xattr_block_cache_insert(ea_block_cache, new_bh);
 			error = ext4_handle_dirty_metadata(handle, inode,
 							   new_bh);
-			if (error)
+			if (error) {
+				ext4_xattr_new_block_fail(handle, inode, new_bh,
+							  i->in_inode ?
+							  i->value_len : 0,
+							  error);
 				goto cleanup;
+			}
 		}
 	}
 
-- 
2.55.0

Re: [PATCH] ext4: compensate ea_inode refs and free block on new xattr block write error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces calls to `mb_cache_entry_delete_or_get()` and `mb_cache_entry_wait_unused()`, which are not defined anywhere in the Linux kernel codebase. This will cause compilation and linking failures due to implicit function declarations and undefined symbols.
--

--- Patch [1]: [PATCH] ext4: compensate ea_inode refs and free block on new xattr block write error ---
commit 867eb1668b3595297e1515877804d2ee15fc4038
Author: Matthias Goergens <matthias.goergens@gmail.com>

ext4: compensate ea_inode refs and free block on new xattr block write error

This commit adds error handling in ext4_xattr_block_set() to properly revert
the on-disk reference counts and free the newly allocated xattr block if
metadata dirtying fails. It introduces ext4_xattr_new_block_fail() to handle
mbcache cleanup, block freeing, and quota release.

Link: https://lore.kernel.org/linux-ext4/20260814070432.595B91F00A3A@smtp.kernel.org/

> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 6127e94ea4f5d..f49d1e74e3214 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c

[ ... ]

> @@ -1299,6 +1299,63 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
>  	return;
>  }
>  
> +/*
> + * Undo the setup of a new xattr block that failed to be written out: drop
> + * the ext4_xattr_inode_inc_ref_all() references, free the block, and
> + * release the quota this operation charged for the new value (quota_len,
> + * zero when the value is not in an EA inode).
> + */
> +static void ext4_xattr_new_block_fail(handle_t *handle, struct inode *inode,
> +				      struct buffer_head *new_bh,
> +				      size_t quota_len, int error)
> +{

[ ... ]

> +	lock_buffer(new_bh);
> +retry_owner:
> +	if (le32_to_cpu(BHDR(new_bh)->h_refcount) != 1) {
> +		unlock_buffer(new_bh);
> +		return;
> +	}
> +	if (ea_block_cache) {
> +		oe = mb_cache_entry_delete_or_get(ea_block_cache,
> +						  le32_to_cpu(BHDR(new_bh)->h_hash),
> +						  new_bh->b_blocknr);
> +		if (oe) {
> +			unlock_buffer(new_bh);
> +			mb_cache_entry_wait_unused(oe);

[Severity: Low]
Are mb_cache_entry_delete_or_get() and mb_cache_entry_wait_unused() defined
anywhere in the kernel codebase?

Building the kernel with EXT4 support enabled will likely fail to compile
or link because these required mbcache functions appear to be missing.

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

Re: [PATCH] ext4: compensate ea_inode refs and free block on new xattr block write error

[Matthias Goergens]

Sashiko's reviews on these threads have been genuinely useful — the
ea_inode refcount leak that this patch follows up on was its find.  This
one, though, is a false positive: mb_cache_entry_delete_or_get() and
mb_cache_entry_wait_unused() have been defined in fs/mbcache.c and
declared in include/linux/mbcache.h since commit 3dc96bba65f5 ("mbcache:
add functions to delete entry if unused", 2022), CONFIG_FS_MBCACHE is
selected by every EXT4 configuration, and fs/ext4/xattr.c already calls
mb_cache_entry_delete_or_get() at three sites before this patch — a tree
missing the symbols could not build ext4 at all.  The patch compiles,
and the fault-injection measurements in the commit message ran on a
built kernel.

Re: [PATCH] ext4: compensate ea_inode refs and free block on new xattr block write error\

[Theodore Tso]

On Mon, Aug 17, 2026 at 07:49:05PM -0500, Matthias Goergens wrote:
> Sashiko's reviews on these threads have been genuinely useful — the
> ea_inode refcount leak that this patch follows up on was its find.  This
> one, though, is a false positive: mb_cache_entry_delete_or_get() and
> mb_cache_entry_wait_unused() have been defined in fs/mbcache.c and
> declared in include/linux/mbcache.h since commit 3dc96bba65f5 ("mbcache:
> add functions to delete entry if unused", 2022)

Yeah, I had noticed this and filed a bug against Sashiko.

https://github.com/sashiko-dev/sashiko/issues/439

(Filed before github went down.)

					- Ted