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

[Rui Qi]

On 9/25/26 12:17 AM, Yazen Ghannam wrote:
> 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.
> 
> 

Thanks Yazen. The commit message is reworded in v3 to lead with the
reachable -EINTR case and to spell out the deadlock (loader dies
holding erst_record_id_cache.lock); the -ENODEV case is dropped.
No code changes.

v3:
https://lore.kernel.org/r/20260925030918.3428029-1-qirui.001@xxxxxxxxxxxxx/