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

[Yazen Ghannam]

On Wed, Aug 26, 2026 at 11:53:14AM +0800, Rui Qi wrote:
> When erst_get_record_id_begin() returns an error, get_saved_records()
> jumps to the out_end label which unconditionally calls
> erst_get_record_id_end(). This is wrong because:
> 
> - If erst_disable is true, begin() returns -ENODEV without
>   incrementing the refcount. Then end() hits BUG_ON(erst_disable)
>   and panics.
> 
> - If mutex_lock_interruptible() is interrupted, begin() returns
>   -EINTR without incrementing the refcount. Then end() decrements
>   refcount below zero, hitting BUG_ON(refcount < 0).
> 
> The comment in erst_get_record_id_end() warns that it should not be
> called when erst_disable is true, so callers must not invoke it after
> begin() fails.
> 
> Fix by jumping to the out label when begin() fails, skipping the
> erst_get_record_id_end() call. This is safe because kfree() handles
> NULL pointers.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@xxxxxxxxxxxxx>
> ---
>  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;
>  }
>  
> -- 

The patch is okay, but the 'erst_disable' part didn't make sense to me.
So I went over it with an AI assistant. Response is below.

Basically, the commit message needs to be reworded to cover the actual
issue.

Thanks,
Yazen

=========================

`erst_get_record_id_begin()` has two ways to fail, and the commit
message describes both. Only one of them can happen when fmpm calls it.

```c
int erst_get_record_id_begin(int *pos)
{
	if (erst_disable)
		return -ENODEV;			/* case 1 */

	rc = mutex_lock_interruptible(&erst_record_id_cache.lock);
	if (rc)
		return rc;			/* case 2: -EINTR */
	erst_record_id_cache.refcount++;
	...
```

**Case 1 (`-ENODEV`) can't happen from `get_saved_records()`:**
- `fru_mem_poison_init()` already returns `-ENODEV` when `erst_disable`
  is set, before it calls `get_saved_records()`.
- `erst_disable` has only two writers: the `erst_disable` boot parameter
  (`__setup`) and the error path of `erst_init()`.
- `erst_init()` is a `device_initcall` in `drivers/acpi/`, which links
  ahead of `drivers/ras/`. When fmpm is built in, `erst_init()` has
  already run by the time fmpm's initcall runs. When fmpm is a module,
  it loads later still.
- So `erst_disable` can't change between fmpm's check and the `begin()`
  call, and the `BUG_ON(erst_disable)` in `end()` can't fire here.

**Case 2 (`-EINTR`) can happen, but only under narrow conditions:**
- The lock has to be contended. `mutex_lock_interruptible()` takes an
  uncontended lock without checking for signals. Other code that takes
  `erst_record_id_cache.lock` includes the other `begin()` callers:
  `erst_open_pstore()`, `erst_dbg_open()` and `apei_read_mce()`.
- A signal has to be pending while the task waits. That's realistic when
  fmpm is a module, for example Ctrl-C or SIGKILL sent to `modprobe`
  while pstore or erst-dbg holds the lock. When fmpm is built in, its
  init runs in the `kernel_init` thread before userspace starts, so no
  signal can reach it.
- Before the patch, this path calls `end()`. The refcount drops to -1
  and hits `BUG_ON(refcount < 0)` while `erst_record_id_cache.lock` is
  held. The oops kills the task with the mutex still locked, so every
  later ERST user blocks on it forever.

In short, "reachable" was loose wording. The `-ENODEV` case can't happen
from fmpm at all. The `-EINTR` case can happen, but only when fmpm is a
module, the lock is contended, and the load is interrupted by a signal.
The patch fixes that case, which is real. The commit message just leads
with the case that can't happen from fmpm and understates the
consequence of the one that can.