Re: [PATCH v3] RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails

[Yazen Ghannam]

On Fri, Sep 25, 2026 at 11:09:18AM +0800, Rui Qi wrote:
> When erst_get_record_id_begin() returns an error, get_saved_records()
> jumps to the out_end label and unconditionally calls
> erst_get_record_id_end(). But begin() only bumps the
> erst_record_id_cache refcount on success, so calling end() after a
> failed begin() underflows it.
> 
> This is reachable when fmpm is built as a module: if the cache lock
> is contended (by pstore, erst-dbg, or apei_read_mce) and a signal
> arrives, mutex_lock_interruptible() in begin() returns -EINTR without
> incrementing the refcount. end() then takes the lock, decrements the
> refcount below zero, and BUG_ON() fires while still holding it. The
> loader dies with the mutex held, so every later ERST user blocks
> indefinitely.
> 
> Built-in fmpm is not affected, since its initcall runs before
> userspace and mutex_lock_interruptible() can never see a signal.
> 
> Fix it by jumping to the out label on begin() failure, skipping
> erst_get_record_id_end(). kfree(old) moves to that shared label so
> it still runs on every path.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@xxxxxxxxxxxxx>
> ---
> Changes in v3:
> - Reword the commit message per review feedback from Yazen Ghannam.
> - No code changes.
> 
> Patches 1-3 of the series are unchanged and already carry Reviewed-by
> on v2; they are not resent.
> 
> Link: https://lore.kernel.org/r/20260924161746.GI1080284@xxxxxxxxxxxxxxxxx
> 
>  drivers/ras/amd/fmpm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index c13db1f743e5..48a437042953 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -673,7 +673,7 @@ static int get_saved_records(void)
>  
>  	ret = erst_get_record_id_begin(&pos);
>  	if (ret < 0)
> -		goto out_end;
> +		goto out;
>  
>  	while (!erst_get_record_id_next(&pos, &record_id)) {
>  		if (record_id == APEI_ERST_INVALID_RECORD_ID)
> @@ -714,8 +714,8 @@ static int get_saved_records(void)
>  
>  out_end:
>  	erst_get_record_id_end();
> -	kfree(old);
>  out:
> +	kfree(old);
>  	return ret;
>  }
>  
> -- 

Looks good to me.

Reviewed-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>

Thanks,
Yazen