Re: [PATCH v2 0/5] mm/hwpoison: Fix regressions in memory failure handling

[Shuai Xue <xueshuai@linux.alibaba.com>]

在 2025/2/18 16:27, Borislav Petkov 写道:
> On Mon, Feb 17, 2025 at 02:33:30PM +0800, Shuai Xue wrote:
>> changes singce v1:
>> - Patch 1: Fix cur_sev and sev type to `int` per Tony
>> - Patch 4: Fix return value to 0 for clean pages per Miaohe
>> - Patch 5: pick return value comments of memory-failure()
>>
>> This patch addresses three regressions identified in memory failure
>> handling, as discovered using ras-tools[1]:
>>
>> - `./einj_mem_uc copyin -f`
>> - `./einj_mem_uc futex -f`
>> - `./einj_mem_uc instr`
> 
> This is not how you write a problem statement and explain why your patches
> exist.
> 
> You need to state:
> 
> 1. What are you trying to do
> 2. What is the expected outcome and why
> 3. What actually happens and why
> 4. The fix, in your opinion, should be X or Y
> 
> Not quote some ras tools commands. Show me that you actually know what you're
> doing and explain the problem in human understandable way.  And then we can
> talk fixes.
> 
> Thx.
> 

Sorry for the confusion.

> 1. What are you trying to do

I am tring to fix two memory failure regression in upstream kernel compared
with 5.10 LTS.

- copyin case: poison found while copying from user space.
- instr case: poison found while instruction fetching in user space

> 2. What is the expected outcome and why

For copyin case:

Kernel can recover from poison found while copying from user space.  MCE check
the fixup handler type to decide whether an in kernel #MC can be recovered.
When EX_TYPE_UACCESS is found, the PC jumps to recovery code specified in
_ASM_EXTABLE_FAULT() and return a -EFAULT to user space.

For instr case:

If a poison found while instruction fetching in user space, full recovery is
possible. User process takes #PF, Linux allocates a new page and fills by
reading from storage.

> 3. What actually happens and why

For copyin case: kernel panic since v5.17

Commit 4c132d1d844a ("x86/futex: Remove .fixup usage") introduced a new extable
fixup type, EX_TYPE_EFAULT_REG, and later patches updated the extable fixup
type for copy-from-user operations, changing it from EX_TYPE_UACCESS to
EX_TYPE_EFAULT_REG.

For instr case: user process is killed by a SIGBUS signal

Commit 046545a661af ("mm/hwpoison: fix error page recovered but reported "not
recovered"") introduced a bug that kill_accessing_process() return -EHWPOISON
for instr case, as result, kill_me_maybe() send a SIGBUS to user process.

> 4. The fix, in your opinion, should be X or Y

For copyin case: add EX_TYPE_EFAULT_REG as a recovery type.
For instr case: let kill_accessing_process return 0 to prevent a SIGBUS.

For patch 1 and 2:

While debuging the two regression, I found `msg` in predefined `severities`, e.g.

     MCESEV(
         AO, "Action optional: last level cache writeback error",
         SER, MASK(MCI_UC_AR|MCACOD, MCI_STATUS_UC|MCACOD_L3WB)
         ),

is helpful for me to know what kind of MCE is happened. For a fatal machine
check, kernel panic use the message and I want to extend to collect the message
and print it out for non-fatal one.

For patch 5:

The return value of memory_failure() is quite important while discussed instr
case regression with Tony and Miaohe for patch 4, so move comment to the place
it belongs to.

I hope the information provided above effectively addresses your concerns.
Please feel free to let me know if you have any further questions or need
additional clarification.

Thanks.
Shuai