Hi Yazen, Borislav, Tony,

This series fixes several bugs in the AMD FRU Memory Poison Manager
driver.

Patch 1 fixes an out-of-bounds read in the for_each_fru macro caused by
the comma operator evaluating the array access before the bounds check.
This is technically undefined behavior and would be flagged by UBSan.

Patch 2 fixes an uninitialized stack bitmap in save_new_records() that
could cause the rollback path to clear ERST records that were not created
in the current initialization pass.

Patch 3 makes the max_nr_entries module parameter read-only (0444),
preventing runtime writes that could exceed the allocated flexible array
size.

Patch 4 fixes a spurious BUG when erst_get_record_id_begin() fails,
because the error path unconditionally calls erst_get_record_id_end()
which triggers BUG_ON.

All four bugs have been present since the original introduction of the
AMD FMPM driver.

Rui Qi (4):
  RAS/amd/fmpm: Fix out-of-bounds read in for_each_fru macro
  RAS/amd/fmpm: Clear new records bitmap before rollback
  RAS/amd/fmpm: Make max_nr_entries read-only
  RAS/amd/fmpm: Fix spurious BUG when ERST record enumeration fails

 drivers/ras/amd/fmpm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--
2.20.1

Hi Yazen, Borislav, Tony,

This series fixes several bugs in the AMD FRU Memory Poison Manager
driver.

Patch 1 fixes an out-of-bounds read in the for_each_fru macro caused by
the comma operator evaluating the array access before the bounds check.

Patch 2 fixes an uninitialized stack bitmap in save_new_records() that
could cause the rollback path to clear ERST records that were not created
in the current initialization pass.

Patch 3 makes the max_nr_entries module parameter read-only (0444),
preventing runtime writes that could exceed the allocated flexible array
size.

Patch 4 fixes a spurious BUG when erst_get_record_id_begin() fails,
because the error path unconditionally calls erst_get_record_id_end()
which triggers BUG_ON.

All four bugs have been present since the original introduction of the
AMD FMPM driver.

Changes since v1 [1]:
- All patches: Use RAS/AMD/FMPM: subject prefix to match existing
  convention (Yazen Ghannam)
- Patch 1: Replace UBSan with KASAN in commit message, as KASAN is the
  appropriate sanitizer for out-of-bounds memory accesses (Yazen Ghannam)
- Patch 1: Use ", true" instead of ", 1" in the for_each_fru macro to
  clearly indicate a boolean value (Yazen Ghannam)
- Patch 2: Initialize DECLARE_BITMAP at declaration with = { 0 } instead
  of calling bitmap_zero() separately (Yazen Ghannam)
- Patch 4: Fix commit message to accurately describe the comment in
  erst_get_record_id_end() (Yazen Ghannam)
- Patch 4: Simplify error path by using goto out and moving the out:
  label above kfree(old), removing the out_free label (Yazen Ghannam)

[1] https://lore.kernel.org/r/20260821094748.145394-1-qirui.001@bytedance.com

Rui Qi (4):
  RAS/AMD/FMPM: Fix out-of-bounds read in for_each_fru macro
  RAS/AMD/FMPM: Clear new records bitmap before rollback
  RAS/AMD/FMPM: Make max_nr_entries read-only
  RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails

 drivers/ras/amd/fmpm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

--
2.20.1

On 8/26/26 11:53 AM, Rui Qi wrote:
> Hi Yazen, Borislav, Tony,
> 
> This series fixes several bugs in the AMD FRU Memory Poison Manager
> driver.
> 
> Patch 1 fixes an out-of-bounds read in the for_each_fru macro caused by
> the comma operator evaluating the array access before the bounds check.
> 
> Patch 2 fixes an uninitialized stack bitmap in save_new_records() that
> could cause the rollback path to clear ERST records that were not created
> in the current initialization pass.
> 
> Patch 3 makes the max_nr_entries module parameter read-only (0444),
> preventing runtime writes that could exceed the allocated flexible array
> size.
> 
> Patch 4 fixes a spurious BUG when erst_get_record_id_begin() fails,
> because the error path unconditionally calls erst_get_record_id_end()
> which triggers BUG_ON.
> 
> All four bugs have been present since the original introduction of the
> AMD FMPM driver.
> 
> Changes since v1 [1]:
> - All patches: Use RAS/AMD/FMPM: subject prefix to match existing
>   convention (Yazen Ghannam)
> - Patch 1: Replace UBSan with KASAN in commit message, as KASAN is the
>   appropriate sanitizer for out-of-bounds memory accesses (Yazen Ghannam)
> - Patch 1: Use ", true" instead of ", 1" in the for_each_fru macro to
>   clearly indicate a boolean value (Yazen Ghannam)
> - Patch 2: Initialize DECLARE_BITMAP at declaration with = { 0 } instead
>   of calling bitmap_zero() separately (Yazen Ghannam)
> - Patch 4: Fix commit message to accurately describe the comment in
>   erst_get_record_id_end() (Yazen Ghannam)
> - Patch 4: Simplify error path by using goto out and moving the out:
>   label above kfree(old), removing the out_free label (Yazen Ghannam)
> 
> [1] https://lore.kernel.org/r/20260821094748.145394-1-qirui.001@bytedance.com
> 
> Rui Qi (4):
>   RAS/AMD/FMPM: Fix out-of-bounds read in for_each_fru macro
>   RAS/AMD/FMPM: Clear new records bitmap before rollback
>   RAS/AMD/FMPM: Make max_nr_entries read-only
>   RAS/AMD/FMPM: Fix spurious BUG when ERST record enumeration fails
> 
>  drivers/ras/amd/fmpm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> --
> 2.20.1

Hi Yazen,

Gentle ping on this series.

This v2 incorporates all your feedback on v1, including the subject
prefix updates and the suggested changes to patches 1, 2, and 4.
Could you please take another look when you have a chance?

Thanks,
Rui

The for_each_fru macro evaluates the array access "rec = fru_records[i]"
before the bounds check "i < max_nr_fru" due to the comma operator's
left-to-right evaluation order. When the loop terminates, i equals
max_nr_fru, causing fru_records[max_nr_fru] to be read before the
condition is checked.

While the garbage pointer value assigned to rec is never dereferenced
(the loop exits immediately), this is technically undefined behavior
and would be flagged by KASAN and static analyzers.

Fix by using short-circuit evaluation with && to check the bound first,
only accessing the array when i is within range:

  for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 4ccaaf7b70bf..81d7f02c053d 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -169,7 +169,7 @@ static unsigned int spa_nr_entries;
 static DEFINE_MUTEX(fmpm_update_mutex);
 
 #define for_each_fru(i, rec) \
-	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
+	for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)
 
 static inline u32 get_fmp_len(struct fru_rec *rec)
 {
-- 
2.20.1

On Wed, Aug 26, 2026 at 11:53:11AM +0800, Rui Qi wrote:
> The for_each_fru macro evaluates the array access "rec = fru_records[i]"
> before the bounds check "i < max_nr_fru" due to the comma operator's
> left-to-right evaluation order. When the loop terminates, i equals
> max_nr_fru, causing fru_records[max_nr_fru] to be read before the
> condition is checked.
> 
> While the garbage pointer value assigned to rec is never dereferenced
> (the loop exits immediately), this is technically undefined behavior
> and would be flagged by KASAN and static analyzers.
> 
> Fix by using short-circuit evaluation with && to check the bound first,
> only accessing the array when i is within range:
> 
>   for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 4ccaaf7b70bf..81d7f02c053d 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -169,7 +169,7 @@ static unsigned int spa_nr_entries;
>  static DEFINE_MUTEX(fmpm_update_mutex);
>  
>  #define for_each_fru(i, rec) \
> -	for (i = 0; rec = fru_records[i], i < max_nr_fru; i++)
> +	for (i = 0; i < max_nr_fru && ((rec = fru_records[i]), true); i++)
>  
>  static inline u32 get_fmp_len(struct fru_rec *rec)
>  {
> -- 

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

Thanks,
Yazen

save_new_records() uses a stack bitmap to track which ERST records were
created during the current initialization pass. If a later write fails,
the rollback path tests this bitmap to decide which records should be
removed again.

DECLARE_BITMAP() does not initialize stack storage, so the rollback path
can observe stale bits and attempt to clear records that were not created
by this function. Initialize the bitmap to zero at declaration so that
only records successfully written in the current pass are rolled back.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index 81d7f02c053d..e3f7bd053479 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -528,7 +528,7 @@ static void set_rec_fields(struct fru_rec *rec)
 
 static int save_new_records(void)
 {
-	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU);
+	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU) = { 0 };
 	struct fru_rec *rec;
 	unsigned int i;
 	int ret = 0;
-- 
2.20.1

On Wed, Aug 26, 2026 at 11:53:12AM +0800, Rui Qi wrote:
> save_new_records() uses a stack bitmap to track which ERST records were
> created during the current initialization pass. If a later write fails,
> the rollback path tests this bitmap to decide which records should be
> removed again.
> 
> DECLARE_BITMAP() does not initialize stack storage, so the rollback path
> can observe stale bits and attempt to clear records that were not created
> by this function. Initialize the bitmap to zero at declaration so that
> only records successfully written in the current pass are rolled back.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index 81d7f02c053d..e3f7bd053479 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -528,7 +528,7 @@ static void set_rec_fields(struct fru_rec *rec)
>  
>  static int save_new_records(void)
>  {
> -	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU);
> +	DECLARE_BITMAP(new_records, FMPM_MAX_NR_FRU) = { 0 };
>  	struct fru_rec *rec;
>  	unsigned int i;
>  	int ret = 0;
> -- 

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

Thanks,
Yazen

max_nr_entries is used during module init to calculate max_rec_len.
That length determines the size of each allocated FRU record and is not
resized after init.

Leaving the parameter writable lets a later sysfs write raise the runtime
limit used by update_fru_record(), allowing entries beyond the allocated
flexible array to be written.

Expose the parameter as read-only so it can still be set at module load
time, but cannot diverge from the allocation size afterwards.

Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
---
 drivers/ras/amd/fmpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
index e3f7bd053479..c13db1f743e5 100644
--- a/drivers/ras/amd/fmpm.c
+++ b/drivers/ras/amd/fmpm.c
@@ -138,7 +138,7 @@ static struct dentry *fmpm_dfs_entries;
  * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
  */
 static u8 max_nr_entries;
-module_param(max_nr_entries, byte, 0644);
+module_param(max_nr_entries, byte, 0444);
 MODULE_PARM_DESC(max_nr_entries,
 		 "Maximum number of memory poison descriptor entries per FRU");
 
-- 
2.20.1

On Wed, Aug 26, 2026 at 11:53:13AM +0800, Rui Qi wrote:
> max_nr_entries is used during module init to calculate max_rec_len.
> That length determines the size of each allocated FRU record and is not
> resized after init.
> 
> Leaving the parameter writable lets a later sysfs write raise the runtime
> limit used by update_fru_record(), allowing entries beyond the allocated
> flexible array to be written.
> 
> Expose the parameter as read-only so it can still be set at module load
> time, but cannot diverge from the allocation size afterwards.
> 
> Fixes: 6f15e617cc99 ("RAS: Introduce a FRU memory poison manager")
> Signed-off-by: Rui Qi <qirui.001@bytedance.com>
> ---
>  drivers/ras/amd/fmpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ras/amd/fmpm.c b/drivers/ras/amd/fmpm.c
> index e3f7bd053479..c13db1f743e5 100644
> --- a/drivers/ras/amd/fmpm.c
> +++ b/drivers/ras/amd/fmpm.c
> @@ -138,7 +138,7 @@ static struct dentry *fmpm_dfs_entries;
>   * No input or '0' will default to FMPM_DEFAULT_MAX_NR_ENTRIES.
>   */
>  static u8 max_nr_entries;
> -module_param(max_nr_entries, byte, 0644);
> +module_param(max_nr_entries, byte, 0444);
>  MODULE_PARM_DESC(max_nr_entries,
>  		 "Maximum number of memory poison descriptor entries per FRU");
>  
> -- 

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

Thanks,
Yazen

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@bytedance.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

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@bytedance.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;
>  }
>  
> -- 

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.

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@bytedance.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;
>>  }
>>  
>> -- 
> 
> 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@bytedance.com/