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

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

[Matthias Goergens]

ext4_xattr_block_set() takes a reference on the ea_inode via
ext4_xattr_inode_lookup_create() before it builds the new xattr block.
The new-block path drops that reference once
ext4_xattr_inode_inc_ref_all() has counted every entry in the block.
When a separately built block is discarded in favour of an identical
cached block, the cached block already counts its entries and the
lookup reference is left unbalanced.

Each affected reuse leaks one on-disk reference, and the ea_inode and
its data blocks stay allocated after the last xattr referring to them
is gone.  Setting the same xattr on two files is enough when the value
needs an EA inode and the entry does not fit in the inode body: the
second set reuses the first file's block and leaves the ea_inode at
refcount 2, and removing the xattr from both files leaves it at 1.

Drop the reference when the entry was not written into the old block in
place.  new_bh != bs->bh is not the right test: the in-place path can
reach 'inserted' and match a different cached block, and there the
reference belongs to the old block and ext4_xattr_release_block() drops
it, so dropping it here too would under-count.  Conversely, new_bh ==
bs->bh with a cloned s->base (re-setting an unchanged external value on
a shared block) leaks the reference as well, so the check follows both
branches.

If the decrement fails the filesystem is already broken: report the
error and return it without rolling back, putting and clearing ea_inode
first so that cleanup does not retry the decrement.  The reused block
keeps its incremented refcount and its block quota charge; e2fsck
corrects the refcount.

Found by the Sashiko AI review bot while reviewing commit 54b6bd40898d
("ext4: stop retrying saturated xattr cache entries"):
https://sashiko.dev/#/patchset/20260802065941.1726052-1-matthias.goergens@gmail.com?part=1

Fixes: 0a46ef234756 ("ext4: do not create EA inode under buffer lock")
Cc: stable@vger.kernel.org
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
v3:
- Error path per Jan: report with ext4_error_inode() and return, no
  unwinding.  Put and clear ea_inode there so cleanup does not retry the
  decrement.  After a successful decrement the branch reaches cleanup
  with error == 0, which only puts the inode, so nothing is needed there.
  The new-block path is unchanged.
- Keep the in-place condition instead of new_bh != bs->bh; reasons and
  measurements in the reply to v2.
- Changelog: the normal-path leak does not retain quota charges; state
  the preconditions of the example.
- Shorter comment, EXT4_ERROR_INODE(); rebased onto ext4 dev; Cc stable.
v2: fail the operation on a failed decrement instead of masking it.
 fs/ext4/xattr.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 5c310747b9650..d9e60b5952d2d 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2113,6 +2113,24 @@ 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 (ea_inode && !(bs->bh && s->base == bs->bh->b_data)) {
+				/*
+				 * The cached block already owns its EA inode
+				 * references.  For an in-place update the lookup
+				 * reference belongs to the old block and is
+				 * dropped when that block is released.
+				 */
+				error = ext4_xattr_inode_dec_ref(handle,
+								 ea_inode);
+				if (error) {
+					EXT4_ERROR_INODE(inode,
+							 "dec ref error=%d",
+							 error);
+					ext4_put_ea_inode(ea_inode);
+					ea_inode = NULL;
+					goto cleanup;
+				}
+			}
 		} 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 v3] ext4: drop extra ea_inode ref in cached-block reuse path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] ext4: double decrement of EA inode refcount in cache hit path
- [Low] ext4: build failure due to undefined ext4_put_ea_inode()
--

commit 918f13e87ab50346ca215dbe65d8940b03723d64
Author: Matthias Goergens <matthias.goergens@gmail.com>

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

    ext4_xattr_block_set() takes a reference on the ea_inode via
    ext4_xattr_inode_lookup_create() before it builds the new xattr block.
    The new-block path drops that reference once
    ext4_xattr_inode_inc_ref_all() has counted every entry in the block.
    When a separately built block is discarded in favour of an identical
    cached block, the cached block already counts its entries and the
    lookup reference is left unbalanced.

> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 6127e94ea4f5d..0da96678051e4 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2032,6 +2032,24 @@ 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 (ea_inode && !(bs->bh && s->base == bs->bh->b_data)) {
> +				/*
> +				 * The cached block already owns its EA inode
> +				 * references.  For an in-place update the lookup
> +				 * reference belongs to the old block and is
> +				 * dropped when that block is released.
> +				 */
> +				error = ext4_xattr_inode_dec_ref(handle,
> +								 ea_inode);

[Severity: Critical]
Does this code leave ea_inode set after successfully decrementing the
reference count?

If ea_inode isn't cleared here (and its in-memory reference via iput
isn't dropped), won't execution eventually fall through to the cleanup
label below where ext4_xattr_inode_dec_ref is unconditionally called a
second time for the non-NULL pointer?

> +				if (error) {
> +					EXT4_ERROR_INODE(inode,
> +							 "dec ref error=%d",
> +							 error);
> +					ext4_put_ea_inode(ea_inode);

[Severity: Low]
Does this break the build? ext4_put_ea_inode appears to be undefined.
Should this be iput?

> +					ea_inode = NULL;
> +					goto cleanup;
> +				}
> +			}

[ ... ]

>  	error = 0;
>  
>  cleanup:
>  	if (ea_inode) {
>  		int error2;
>  
>  		error2 = ext4_xattr_inode_dec_ref(handle, ea_inode);
>  		if (error2)
>  			ext4_warning_inode(ea_inode, "dec ref error=%d",
>  					   error2);

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

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

[Matthias Goergens]

Ted, Jan: both findings come from the tree the review ran against, not
from the patch.  The baseline was ext4/HEAD, which is 96485e446260 from
October 2020: HEAD in tytso/ext4.git points at master, which stopped
there, while this patch is against dev (9091c97be340).

- [Critical] double decrement: at 96485e446260 the cleanup label dropped
  the lookup reference whenever ea_inode was set, error or not.  Since
  0a46ef234756 ("ext4: do not create EA inode under buffer lock") it
  does so only when error is set, and a successful cache hit reaches
  cleanup with error == 0, so the new ext4_xattr_inode_dec_ref() is the
  only drop of the lookup reference on that path.  If that call fails,
  ea_inode is cleared before the goto.

- [Low] ext4_put_ea_inode() undefined: it was added by 4f48af3dc755
  ("ext4: introduce ext4_put_ea_inode() for safe deferred iput") and is
  declared in fs/ext4/xattr.h.

Sashiko takes the tree from the ext4 T: entry in MAINTAINERS, which
names no branch, so it uses the repository's HEAD.  I've sent a patch
naming dev there, which is also what linux-next merges; pointing HEAD at
dev would also help other tools that follow the default branch.  I'll
report the stale baseline to the Sashiko maintainers as well.

Matthias

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

[Jan Kara]

On Wed 16-09-26 15:10:46, Matthias Goergens wrote:
> ext4_xattr_block_set() takes a reference on the ea_inode via
> ext4_xattr_inode_lookup_create() before it builds the new xattr block.
> The new-block path drops that reference once
> ext4_xattr_inode_inc_ref_all() has counted every entry in the block.
> When a separately built block is discarded in favour of an identical
> cached block, the cached block already counts its entries and the
> lookup reference is left unbalanced.
> 
> Each affected reuse leaks one on-disk reference, and the ea_inode and
> its data blocks stay allocated after the last xattr referring to them
> is gone.  Setting the same xattr on two files is enough when the value
> needs an EA inode and the entry does not fit in the inode body: the
> second set reuses the first file's block and leaves the ea_inode at
> refcount 2, and removing the xattr from both files leaves it at 1.
> 
> Drop the reference when the entry was not written into the old block in
> place.  new_bh != bs->bh is not the right test: the in-place path can
> reach 'inserted' and match a different cached block, and there the
> reference belongs to the old block and ext4_xattr_release_block() drops
> it, so dropping it here too would under-count.  Conversely, new_bh ==
> bs->bh with a cloned s->base (re-setting an unchanged external value on
> a shared block) leaks the reference as well, so the check follows both
> branches.
> 
> If the decrement fails the filesystem is already broken: report the
> error and return it without rolling back, putting and clearing ea_inode
> first so that cleanup does not retry the decrement.  The reused block
> keeps its incremented refcount and its block quota charge; e2fsck
> corrects the refcount.
> 
> Found by the Sashiko AI review bot while reviewing commit 54b6bd40898d
> ("ext4: stop retrying saturated xattr cache entries"):
> https://sashiko.dev/#/patchset/20260802065941.1726052-1-matthias.goergens@gmail.com?part=1
> 
> Fixes: 0a46ef234756 ("ext4: do not create EA inode under buffer lock")
> Cc: stable@vger.kernel.org
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>

Grumble. This code is really subtle and needs some simplification. But your
fix looks good to me now so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> v3:
> - Error path per Jan: report with ext4_error_inode() and return, no
>   unwinding.  Put and clear ea_inode there so cleanup does not retry the
>   decrement.  After a successful decrement the branch reaches cleanup
>   with error == 0, which only puts the inode, so nothing is needed there.
>   The new-block path is unchanged.
> - Keep the in-place condition instead of new_bh != bs->bh; reasons and
>   measurements in the reply to v2.
> - Changelog: the normal-path leak does not retain quota charges; state
>   the preconditions of the example.
> - Shorter comment, EXT4_ERROR_INODE(); rebased onto ext4 dev; Cc stable.
> v2: fail the operation on a failed decrement instead of masking it.
>  fs/ext4/xattr.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 5c310747b9650..d9e60b5952d2d 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2113,6 +2113,24 @@ 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 (ea_inode && !(bs->bh && s->base == bs->bh->b_data)) {
> +				/*
> +				 * The cached block already owns its EA inode
> +				 * references.  For an in-place update the lookup
> +				 * reference belongs to the old block and is
> +				 * dropped when that block is released.
> +				 */
> +				error = ext4_xattr_inode_dec_ref(handle,
> +								 ea_inode);
> +				if (error) {
> +					EXT4_ERROR_INODE(inode,
> +							 "dec ref error=%d",
> +							 error);
> +					ext4_put_ea_inode(ea_inode);
> +					ea_inode = NULL;
> +					goto cleanup;
> +				}
> +			}
>  		} 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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR