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

[Andreas Dilger]

On Jul 20, 2026, at 00:54, rafad900 <rafad900@xxxxxxxxx> 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