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

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

[Rui Qi]

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@bytedance.com>
---
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@yaz-khff2.amd.com

 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;
 }
 
-- 
2.20.1

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@bytedance.com>
> ---
> 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@yaz-khff2.amd.com
> 
>  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@amd.com>

Thanks,
Yazen