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

[sashiko-bot@kernel.org]

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