[PATCH v2] ext4: fix race in ext4_mb_check_group_pa

[PATCH v2] ext4: fix race in ext4_mb_check_group_pa

[rafad900]

ext4_mb_check_group_pa drops the reference count on the previous
best PA using atomic_dec(&cpa->pa_count) without holding the
cpa->pa_lock.

This causes race with ext4_discard_preallocations() which checks
pa_count to decide whether a PA is still in use. If the pa_count
is dec between the check and the discard, the PA can be freed
while ext4_mb_check_group_pa() still holds a reference to
it.

Fix this by taking the cpa->pa_lock around the atomic_dec.
Similar to pa->pa_lock which is taken outside of the
ext4_mb_check_group_pa() function.

The race was found while testing a change related to a
Coccinelle warning from atomic_as_refcounter.cocci. The refcount
conversion was found to be incorrect but the change had revealed
the pre-exiting race condition.

Signed-off-by: rafad900 <rafad900@gmail.com>
---
Changes in v2:
- Identified correct race location: ext4_mb_check_group_pa rather
  than ext4_mb_use_preallocated (the inode PA path already holds
  pa_lock correctly)
- Dropped refcount_t conversion — incompatible with PA lifecycle
  where count=0 represents an idle but reusable PA
- Added spin_lock(&cpa->pa_lock) around atomic_dec in
  ext4_mb_check_group_pa

 fs/ext4/mballoc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ed1bd00e11cd..c9a118ae4658 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4833,7 +4833,9 @@ ext4_mb_check_group_pa(ext4_fsblk_t goal_block,
 		return cpa;
 
 	/* drop the previous reference */
+	spin_lock(&cpa->pa_lock);
 	atomic_dec(&cpa->pa_count);
+	spin_unlock(&cpa->pa_lock);
 	atomic_inc(&pa->pa_count);
 	return pa;
 }
-- 
2.43.0

Re: [PATCH v2] ext4: fix race in ext4_mb_check_group_pa

[Andreas Dilger]

On Jul 20, 2026, at 00:54, rafad900 <rafad900@gmail.com> wrote:
> ext4_mb_check_group_pa drops the reference count on the previous
> best PA using atomic_dec(&cpa->pa_count) without holding the
> cpa->pa_lock.
> 
> This causes race with ext4_discard_preallocations() which checks
> pa_count to decide whether a PA is still in use. If the pa_count
> is dec between the check and the discard, the PA can be freed
> while ext4_mb_check_group_pa() still holds a reference to it.

Can you please explain this race condition further?  I don't see where
ext4_mb_check_group_pa() is using cpa after the reference is dropped.

> Fix this by taking the cpa->pa_lock around the atomic_dec.
> Similar to pa->pa_lock which is taken outside of the
> ext4_mb_check_group_pa() function.

At this point, it wouldn't be clear why `pa_count` needs to be an
atomic at all, if `pa_lock` is always held during inc/dec/check?

> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index ed1bd00e11cd..c9a118ae4658 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4833,7 +4833,9 @@ ext4_mb_check_group_pa(ext4_fsblk_t goal_block,
> return cpa;
> 
> 	/* drop the previous reference */
> +	spin_lock(&cpa->pa_lock);
>  	atomic_dec(&cpa->pa_count);
> +	spin_unlock(&cpa->pa_lock);
>  	atomic_inc(&pa->pa_count);
>  	return pa;
>  }

In ext4_mb_check_group_pa() there is no reference to `cpa` after the
refcount is dropped.  In its one caller ext4_mb_use_preallocated():

		list_for_each_entry_rcu(tmp_pa, &lg->lg_prealloc_list[i],
					pa_node.lg_list) {
			spin_lock(&tmp_pa->pa_lock);
			if (tmp_pa->pa_deleted == 0 &&
			    tmp_pa->pa_free >= ac->ac_o_ex.fe_len) {
				cpa = ext4_mb_check_group_pa(goal_block,
							     tmp_pa, cpa);
			}
			spin_unlock(&tmp_pa->pa_lock);
		}
		rcu_read_unlock();
	}
	if (cpa) {
		ext4_mb_use_group_pa(ac, cpa);
		return true;
	}
	return false;
}

It *looks* like 'cpa' is used after ext4_mb_check_group_pa(), but it is
replaced on the return by 'tmp_pa' in that case, so there is no further
use after the refcount is dropped AFAICS.  Even the list iteration is
using 'tmp_pa', so that couldn't be it either.

There may be a race condition somewhere, but the commit message doesn't
provide clear details of what it is.

Cheers, Andreas

Re: [PATCH v2] ext4: fix race in ext4_mb_check_group_pa

[Rafael Alejandro Díaz Cruz]

I think I misinterpreted what was going on after the change in the first patch.
When making the change from atomic_t to refcount_t, I get this kernel
error while
testing on my x86 QEMU:
 ------------[ cut here ]------------
[   12.954674] refcount_t: addition on 0; use-after-free.
[   12.956476] WARNING: lib/refcount.c:25 at
refcount_warn_saturate+0x6a/0x90, CPU#0: dhcpcd-run-hook/229
[   12.959648] Modules linked in:
[   12.960735] CPU: 0 UID: 0 PID: 229 Comm: dhcpcd-run-hook Not
tainted 7.2.0-rc4-00001-g453fc8a20cd2-dirty #6 PREEMPT(lazy)
[   12.964446] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX,
arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   12.968371] RIP: 0010:refcount_warn_saturate+0x6a/0x90
[   12.969892] Code: 00 48 8d 3d b8 92 71 01 67 48 0f b9 3a e9 2e cd
ad 00 48 8d 3d b7 92 71 01 67 48 0f b9 3a e9 1d cd ad 00 48 8d 3d b6
92 71 01 <67> 48 0f b9 3a e9 0c cd ad 00 48 8d 3d b5 92 71 01 67 48 0f
b9 3a
[   12.975114] RSP: 0018:ffffb4c9c03a7838 EFLAGS: 00010246
[   12.976742] RAX: 0000000000000001 RBX: ffff8c96fdc32688 RCX: 0000000000000000
[   12.979085] RDX: 0000000000000000 RSI: 0000000000000002 RDI: ffffffff8b703940
[   12.981349] RBP: ffff8c96848a1738 R08: 000000000000000c R09: 0000000000000200
[   12.983382] R10: ffff8c96848a1738 R11: 000000000000001f R12: ffff8c96848a3a10
[   12.985415] R13: 0000000000000000 R14: 0000000000000008 R15: ffff8c96848a3a48
[   12.987455] FS:  00007f2995026780(0000) GS:ffff8c9771c6f000(0000)
knlGS:0000000000000000
[   12.989751] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.991296] CR2: 00005593d8eb6640 CR3: 0000000001919000 CR4: 0000000000350ef0
[   12.992879] Call Trace:
[   12.993483]  <TASK>
[   12.993982]  ext4_mb_use_preallocated.constprop.0+0x346/0x3d0
[   12.995293]  ext4_mb_new_blocks+0x15b/0xe80
[   12.996246]  ? srso_return_thunk+0x5/0x5f
[   12.997178]  ? ext4_find_extent+0xc8/0x320
[   12.998117]  ext4_ext_map_blocks+0x3f1/0x17e0
[   12.999121]  ? srso_return_thunk+0x5/0x5f
[   13.000067]  ? mpage_prepare_extent_to_map+0x47d/0x4c0
[   13.001224]  ext4_map_create_blocks+0x40/0x170
[   13.002239]  ext4_map_blocks+0x193/0x3c0
[   13.003142]  ext4_do_writepages+0x7e7/0xf20
[   13.004109]  ? ext4_writepages+0xbb/0x180
[   13.004694]  ext4_writepages+0xbb/0x180
[   13.005275]  do_writepages+0xb6/0x150
[   13.005826]  filemap_writeback+0xa1/0xc0
[   13.006435]  ext4_release_file+0x6c/0xa0
[   13.007041]  __fput+0xe1/0x2b0
[   13.007503]  task_work_run+0x57/0x80
[   13.008046]  exit_to_user_mode_loop+0x139/0x4e0
[   13.008705]  ? srso_return_thunk+0x5/0x5f
[   13.009319]  do_syscall_64+0x42c/0x540
[   13.009878]  ? srso_return_thunk+0x5/0x5f
[   13.010477]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[   13.011235] RIP: 0033:0x7f2995128887
[   13.011765] Code: 73 01 c3 48 8b 0d 89 55 0e 00 f7 d8 64 89 01 48
83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 21 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 01 c3 48 8b 15 59 55 0e 00 f7 d8 64 89
02 b8
[   13.014433] RSP: 002b:00007ffe0ef0ac48 EFLAGS: 00000246 ORIG_RAX:
0000000000000021
[   13.015542] RAX: 0000000000000001 RBX: 0000000000000001 RCX: 00007f2995128887
[   13.016591] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 000000000000000c
[   13.017635] RBP: 00005593d8ebbef0 R08: 0000000000000000 R09: 0000000000000000
[   13.018685] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[   13.019737] R13: 0000000000000001 R14: 0000000000000000 R15: 00005593d8ebced0
[   13.020798]  </TASK>
[   13.021158] ---[ end trace 0000000000000000 ]---
After reading the code a little closer, I realized that pa_count is
intentionally decremented to 0.
This of course would trigger the warning above when using refcount_t
since it treats 0 as
"object is completely dead". I assumed it was because a lock was not
held and so I added the
locks around the atomic_inc(&cpa->pa_count);

The reference count is being dropped to 0 on purpose right?

-Rafael