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

[sashiko-bot@kernel.org]

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