linux-ext4 - Re: [PATCH v3] ext4: fix dirtyclusters double decrement on fs shutdown

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9c7d91dc-0d19-463a-860f-fb62c04a77ee@huawei.com>
Date: Wed, 14 Jan 2026 09:27:53 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Brian Foster <bfoster@...hat.com>
CC: <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v3] ext4: fix dirtyclusters double decrement on fs
 shutdown

On 2026-01-14 01:19, Brian Foster wrote:
> fstests test generic/388 occasionally reproduces a warning in
> ext4_put_super() associated with the dirty clusters count:
>
>   WARNING: CPU: 7 PID: 76064 at fs/ext4/super.c:1324 ext4_put_super+0x48c/0x590 [ext4]
>
> Tracing the failure shows that the warning fires due to an
> s_dirtyclusters_counter value of -1. IOW, this appears to be a
> spurious decrement as opposed to some sort of leak. Further tracing
> of the dirty cluster count deltas and an LLM scan of the resulting
> output identified the cause as a double decrement in the error path
> between ext4_mb_mark_diskspace_used() and the caller
> ext4_mb_new_blocks().
>
> First, note that generic/388 is a shutdown vs. fsstress test and so
> produces a random set of operations and shutdown injections. In the
> problematic case, the shutdown triggers an error return from the
> ext4_handle_dirty_metadata() call(s) made from
> ext4_mb_mark_context(). The changed value is non-zero at this point,
> so ext4_mb_mark_diskspace_used() does not exit after the error
> bubbles up from ext4_mb_mark_context(). Instead, the former
> decrements both cluster counters and returns the error up to
> ext4_mb_new_blocks(). The latter falls into the !ar->len out path
> which decrements the dirty clusters counter a second time, creating
> the inconsistency.
>
> To avoid this problem and simplify ownership of the cluster
> reservation in this codepath, lift the counter reduction to a single
> place in the caller. This makes it more clear that
> ext4_mb_new_blocks() is responsible for acquiring cluster
> reservation (via ext4_claim_free_clusters()) in the !delalloc case
> as well as releasing it, regardless of whether it ends up consumed
> or returned due to failure.
>
> Fixes: 0087d9fb3f29 ("ext4: Fix s_dirty_blocks_counter if block allocation failed with nodelalloc")
> Signed-off-by: Brian Foster <bfoster@...hat.com>

Looks good! Feel free to add:

Reviewed-by: Baokun Li <libaokun1@...wei.com>

> ---
>
> v3:
> - Fix up ext4_mb_mark_diskspace_used() call in mballoc-test.c.
> - Tweak reserved clusters release logic.
> v2: https://lore.kernel.org/linux-ext4/20260112143652.8085-1-bfoster@redhat.com/
> - Condense counter update logic instead of modifying return flow.
> - Added Fixes: tag.
> v1: https://lore.kernel.org/linux-ext4/20251212154735.512651-1-bfoster@redhat.com/
>
>
>  fs/ext4/mballoc-test.c |  2 +-
>  fs/ext4/mballoc.c      | 21 +++++----------------
>  2 files changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ext4/mballoc-test.c b/fs/ext4/mballoc-test.c
> index a9416b20ff64..4abb40d4561c 100644
> --- a/fs/ext4/mballoc-test.c
> +++ b/fs/ext4/mballoc-test.c
> @@ -567,7 +567,7 @@ test_mark_diskspace_used_range(struct kunit *test,
>  
>  	bitmap = mbt_ctx_bitmap(sb, TEST_GOAL_GROUP);
>  	memset(bitmap, 0, sb->s_blocksize);
> -	ret = ext4_mb_mark_diskspace_used(ac, NULL, 0);
> +	ret = ext4_mb_mark_diskspace_used(ac, NULL);
>  	KUNIT_ASSERT_EQ(test, ret, 0);
>  
>  	max = EXT4_CLUSTERS_PER_GROUP(sb);
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 56d50fd3310b..b3272266220d 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4185,8 +4185,7 @@ ext4_mb_mark_context(handle_t *handle, struct super_block *sb, bool state,
>   * Returns 0 if success or error code
>   */
>  static noinline_for_stack int
> -ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
> -				handle_t *handle, unsigned int reserv_clstrs)
> +ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac, handle_t *handle)
>  {
>  	struct ext4_group_desc *gdp;
>  	struct ext4_sb_info *sbi;
> @@ -4241,13 +4240,6 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
>  	BUG_ON(changed != ac->ac_b_ex.fe_len);
>  #endif
>  	percpu_counter_sub(&sbi->s_freeclusters_counter, ac->ac_b_ex.fe_len);
> -	/*
> -	 * Now reduce the dirty block count also. Should not go negative
> -	 */
> -	if (!(ac->ac_flags & EXT4_MB_DELALLOC_RESERVED))
> -		/* release all the reserved blocks if non delalloc */
> -		percpu_counter_sub(&sbi->s_dirtyclusters_counter,
> -				   reserv_clstrs);
>  
>  	return err;
>  }
> @@ -6332,7 +6324,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>  			ext4_mb_pa_put_free(ac);
>  	}
>  	if (likely(ac->ac_status == AC_STATUS_FOUND)) {
> -		*errp = ext4_mb_mark_diskspace_used(ac, handle, reserv_clstrs);
> +		*errp = ext4_mb_mark_diskspace_used(ac, handle);
>  		if (*errp) {
>  			ext4_discard_allocated_blocks(ac);
>  			goto errout;
> @@ -6363,12 +6355,9 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>  out:
>  	if (inquota && ar->len < inquota)
>  		dquot_free_block(ar->inode, EXT4_C2B(sbi, inquota - ar->len));
> -	if (!ar->len) {
> -		if ((ar->flags & EXT4_MB_DELALLOC_RESERVED) == 0)
> -			/* release all the reserved blocks if non delalloc */
> -			percpu_counter_sub(&sbi->s_dirtyclusters_counter,
> -						reserv_clstrs);
> -	}
> +	/* release any reserved blocks */
> +	if (reserv_clstrs)
> +		percpu_counter_sub(&sbi->s_dirtyclusters_counter, reserv_clstrs);
>  
>  	trace_ext4_allocate_blocks(ar, (unsigned long long)block);
>