[PATCH] x86/mce: Check that memory address is usable for recovery

[PATCH] x86/mce: Check that memory address is usable for recovery

[Tony Luck]

uc_decode_notifier() includes a check that "struct mce"
contains a valid address for recovery. But the machine
check recovery code does not include a similar check.

Use mce_usable_address() to check that there is a valid
address.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2eec60f50057..fa28b3f7d945 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1533,7 +1533,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		/* If this triggers there is no way to recover. Die hard. */
 		BUG_ON(!on_thread_stack() || !user_mode(regs));
 
-		if (kill_current_task)
+		if (kill_current_task || !mce_usable_address(&m))
 			queue_task_work(&m, msg, kill_me_now);
 		else
 			queue_task_work(&m, msg, kill_me_maybe);
-- 
2.39.2

Re: [PATCH] x86/mce: Check that memory address is usable for recovery

[Yazen Ghannam]

On 3/21/23 20:51, Tony Luck wrote:
> uc_decode_notifier() includes a check that "struct mce"
> contains a valid address for recovery. But the machine
> check recovery code does not include a similar check.
> 
> Use mce_usable_address() to check that there is a valid
> address.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 2eec60f50057..fa28b3f7d945 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1533,7 +1533,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
>  		/* If this triggers there is no way to recover. Die hard. */
>  		BUG_ON(!on_thread_stack() || !user_mode(regs));
>  
> -		if (kill_current_task)
> +		if (kill_current_task || !mce_usable_address(&m))
>  			queue_task_work(&m, msg, kill_me_now);
>  		else
>  			queue_task_work(&m, msg, kill_me_maybe);

I think it should be like this:

	if (mce_usable_address(&m))
		queue_task_work(&m, msg, kill_me_maybe);
	else
		queue_task_work(&m, msg, kill_me_now);

A usable address should always go through memory_failure() so that the page is
marked as poison. If !RIPV, then memory_failure() will get the MF_MUST_KILL
flag and try to kill all processes after the page is poisoned.

I had a similar patch a while back:
https://lore.kernel.org/linux-edac/20210504174712.27675-3-Yazen.Ghannam@amd.com/

We could also get rid of kill_me_now() like you had suggested.

Thanks,
Yazen

Re: [PATCH] x86/mce: Check that memory address is usable for recovery

[Tony Luck]

On Tue, Apr 18, 2023 at 12:41:17PM -0400, Yazen Ghannam wrote:
> On 3/21/23 20:51, Tony Luck wrote:
> > uc_decode_notifier() includes a check that "struct mce"
> > contains a valid address for recovery. But the machine
> > check recovery code does not include a similar check.
> > 
> > Use mce_usable_address() to check that there is a valid
> > address.
> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  arch/x86/kernel/cpu/mce/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> > index 2eec60f50057..fa28b3f7d945 100644
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> > @@ -1533,7 +1533,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
> >  		/* If this triggers there is no way to recover. Die hard. */
> >  		BUG_ON(!on_thread_stack() || !user_mode(regs));
> >  
> > -		if (kill_current_task)
> > +		if (kill_current_task || !mce_usable_address(&m))
> >  			queue_task_work(&m, msg, kill_me_now);
> >  		else
> >  			queue_task_work(&m, msg, kill_me_maybe);
> 
> I think it should be like this:
> 
> 	if (mce_usable_address(&m))
> 		queue_task_work(&m, msg, kill_me_maybe);
> 	else
> 		queue_task_work(&m, msg, kill_me_now);
> 
> A usable address should always go through memory_failure() so that the page is
> marked as poison. If !RIPV, then memory_failure() will get the MF_MUST_KILL
> flag and try to kill all processes after the page is poisoned.
> 
> I had a similar patch a while back:
> https://lore.kernel.org/linux-edac/20210504174712.27675-3-Yazen.Ghannam@amd.com/
> 
> We could also get rid of kill_me_now() like you had suggested.

Can we also get rid of "kill_current_task"? It is only set when there is
no valid return address:

        if (!(m.mcgstatus & MCG_STATUS_RIPV))
                kill_current_task = 1;

kill_me_maybe() does an equivalent check:

	if (!p->mce_ripv)
                flags |= MF_MUST_KILL;

Which only leaves this check to resolve in some way:

        if (worst != MCE_AR_SEVERITY && !kill_current_task)
                goto out;

-Tony

Re: [PATCH] x86/mce: Check that memory address is usable for recovery

[Yazen Ghannam]

On 4/18/23 13:27, Tony Luck wrote:
> On Tue, Apr 18, 2023 at 12:41:17PM -0400, Yazen Ghannam wrote:
>> On 3/21/23 20:51, Tony Luck wrote:
>>> uc_decode_notifier() includes a check that "struct mce"
>>> contains a valid address for recovery. But the machine
>>> check recovery code does not include a similar check.
>>>
>>> Use mce_usable_address() to check that there is a valid
>>> address.
>>>
>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>> ---
>>>  arch/x86/kernel/cpu/mce/core.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>>> index 2eec60f50057..fa28b3f7d945 100644
>>> --- a/arch/x86/kernel/cpu/mce/core.c
>>> +++ b/arch/x86/kernel/cpu/mce/core.c
>>> @@ -1533,7 +1533,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
>>>  		/* If this triggers there is no way to recover. Die hard. */
>>>  		BUG_ON(!on_thread_stack() || !user_mode(regs));
>>>  
>>> -		if (kill_current_task)
>>> +		if (kill_current_task || !mce_usable_address(&m))
>>>  			queue_task_work(&m, msg, kill_me_now);
>>>  		else
>>>  			queue_task_work(&m, msg, kill_me_maybe);
>>
>> I think it should be like this:
>>
>> 	if (mce_usable_address(&m))
>> 		queue_task_work(&m, msg, kill_me_maybe);
>> 	else
>> 		queue_task_work(&m, msg, kill_me_now);
>>
>> A usable address should always go through memory_failure() so that the page is
>> marked as poison. If !RIPV, then memory_failure() will get the MF_MUST_KILL
>> flag and try to kill all processes after the page is poisoned.
>>
>> I had a similar patch a while back:
>> https://lore.kernel.org/linux-edac/20210504174712.27675-3-Yazen.Ghannam@amd.com/
>>
>> We could also get rid of kill_me_now() like you had suggested.
> 
> Can we also get rid of "kill_current_task"? It is only set when there is
> no valid return address:
> 
>         if (!(m.mcgstatus & MCG_STATUS_RIPV))
>                 kill_current_task = 1;
> 
> kill_me_maybe() does an equivalent check:
> 
> 	if (!p->mce_ripv)
>                 flags |= MF_MUST_KILL;
> 
> Which only leaves this check to resolve in some way:
> 
>         if (worst != MCE_AR_SEVERITY && !kill_current_task)
>                 goto out;
> 

I agree. And I think all these checks should be baked into the severity.
We'll need additional, fine-grained severity levels though.

The "m.cs" and "m.kflags" checks could also be baked in.

Instead of just one AR severity:
	...
	MCE_AR_SEVERITY,
	MCE_PANIC_SEVERITY,

replace it with specific cases:
	...
	MCE_AR_USER_RECOV,
	MCE_AR_USER_KILL,
	MCE_AR_KERNEL_COPYIN,
	MCE_AR_KERNEL_RECOV,
	MCE_PANIC_SEVERITY,

Then the #MC handler can look like this:

	if (worst < MCE_AR_USER_RECOV)
		goto out;

	if (severity == MCE_AR_USER_RECOV)
		queue_task_work(&m, msg, kill_me_maybe);

	if (severity == MCE_AR_USER_KILL)
		force_sig(SIGBUS);

	if (severity == MCE_AR_KERNEL_COPYIN)
		queue_task_work(&m, msg, kill_me_never);
	
	if (severity == MCE_AR_KERNEL_RECOV) {
		if (!fixup_exception(regs, X86_TRAP_MC, 0, 0))
			mce_panic("Failed kernel mode recovery");
	}

I can take a shot at this if it seems reasonable.

What do you think?

Thanks,
Yazen

[PATCH v2] x86/mce: Always call memory_failure() when there is a valid address

[Tony Luck]

Linux should always take poisoned pages offline when there is an error
report with a valid physcal address.

Note1: that call_me_maybe() will correctly handle the case currently
covered by the test of "kill_current_task" that is deleted by this
change because it will set the MF_MUST_KILL flag when p->mce_ripv is
not set.

Note2: This also provides defense against the case where the logged
error doesn't provide a physical address.

Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2eec60f50057..f72c97860524 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1533,7 +1533,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		/* If this triggers there is no way to recover. Die hard. */
 		BUG_ON(!on_thread_stack() || !user_mode(regs));
 
-		if (kill_current_task)
+		if (mce_usable_address(&m))
 			queue_task_work(&m, msg, kill_me_now);
 		else
 			queue_task_work(&m, msg, kill_me_maybe);
-- 
2.39.2

Re: [PATCH v2] x86/mce: Always call memory_failure() when there is a valid address

[Yazen Ghannam]

On 4/18/23 14:03, Tony Luck wrote:
> Linux should always take poisoned pages offline when there is an error
> report with a valid physcal address.
> 
> Note1: that call_me_maybe() will correctly handle the case currently
> covered by the test of "kill_current_task" that is deleted by this
> change because it will set the MF_MUST_KILL flag when p->mce_ripv is
> not set.
> 
> Note2: This also provides defense against the case where the logged
> error doesn't provide a physical address.
> 
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 2eec60f50057..f72c97860524 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1533,7 +1533,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
>  		/* If this triggers there is no way to recover. Die hard. */
>  		BUG_ON(!on_thread_stack() || !user_mode(regs));
>  
> -		if (kill_current_task)
> +		if (mce_usable_address(&m))

This should be !mce_usable_address().

>  			queue_task_work(&m, msg, kill_me_now);
>  		else
>  			queue_task_work(&m, msg, kill_me_maybe);

Thanks,
Yazen

P.S. I had the exact change in mind. :)

Copying old patch here. Feel free to reuse any of the commit message if
it helps.


From 1123d883470c49babe7c390c67e604b658acb913 Mon Sep 17 00:00:00 2001
From: Yazen Ghannam <yazen.ghannam@amd.com>
Date: Fri, 8 Jan 2021 04:00:35 +0000
Subject: [PATCH] x86/MCE: Call kill_me_maybe() for errors with usable address

Call kill_me_maybe() for machine check errors with a usable address.
This ensures that any memory associated with the error is properly
marked as poison.

This is needed for errors that occur on memory, but that do not have
MCG_STATUS[RIPV] set. One example is data poison consumption through the
instruction fetch units on AMD Zen-based systems.

The MF_MUST_KILL flag is passed to memory_failure() when
MCG_STATUS[RIPV] is not set. So the associated process will still be
killed.

The scenario described above occurs when hardware can precisely identify
the address of poisoned memory, but execution cannot reliably continue
for the interrupted hardware thread.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mce/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 89c81e9992d4..bc2523384357 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1555,7 +1555,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 		/* If this triggers there is no way to recover. Die hard. */
 		BUG_ON(!on_thread_stack() || !user_mode(regs));
 
-		if (kill_current_task)
+		if (!mce_usable_address(&m))
 			queue_task_work(&m, msg, kill_me_now);
 		else
 			queue_task_work(&m, msg, kill_me_maybe);
-- 
2.34.1

Re: [PATCH] x86/mce: Check that memory address is usable for recovery

[Tony Luck]

On Tue, Apr 18, 2023 at 01:51:37PM -0400, Yazen Ghannam wrote:
> I agree. And I think all these checks should be baked into the severity.
> We'll need additional, fine-grained severity levels though.
> 
> The "m.cs" and "m.kflags" checks could also be baked in.
> 
> Instead of just one AR severity:
> 	...
> 	MCE_AR_SEVERITY,
> 	MCE_PANIC_SEVERITY,
> 
> replace it with specific cases:
> 	...
> 	MCE_AR_USER_RECOV,
> 	MCE_AR_USER_KILL,
> 	MCE_AR_KERNEL_COPYIN,
> 	MCE_AR_KERNEL_RECOV,
> 	MCE_PANIC_SEVERITY,
> 
> Then the #MC handler can look like this:
> 
> 	if (worst < MCE_AR_USER_RECOV)
> 		goto out;
> 
> 	if (severity == MCE_AR_USER_RECOV)
> 		queue_task_work(&m, msg, kill_me_maybe);
> 
> 	if (severity == MCE_AR_USER_KILL)
> 		force_sig(SIGBUS);
> 
> 	if (severity == MCE_AR_KERNEL_COPYIN)
> 		queue_task_work(&m, msg, kill_me_never);
> 	
> 	if (severity == MCE_AR_KERNEL_RECOV) {
> 		if (!fixup_exception(regs, X86_TRAP_MC, 0, 0))
> 			mce_panic("Failed kernel mode recovery");
> 	}
> 
> I can take a shot at this if it seems reasonable.

That looks much cleaner. There may be some extra MCE_AR_KERNEL* options
in the future (I'd like someday to address COPYOUT when the corrupt
kernel data is in the page cache). But I don't think the number of cases
is going to explode into dozens of cases.

> What do you think?

Brave person ... you are going to have to tinker with
arch/x86/kernel/cpu/mce/severity.c !  Good luck.

-Tony

RE: [PATCH v2] x86/mce: Always call memory_failure() when there is a valid address

[Luck, Tony]

>> +		if (mce_usable_address(&m))
>
> This should be !mce_usable_address().

> Copying old patch here. Feel free to reuse any of the commit message if
> it helps.

Might as well just take your version. The commit message seems fine.

Reviewed-by: Tony Luck <tony.luck@intel.com>


> From: Yazen Ghannam <yazen.ghannam@amd.com>
> Date: Fri, 8 Jan 2021 04:00:35 +0000

2021 - wow!

> Subject: [PATCH] x86/MCE: Call kill_me_maybe() for errors with usable address

-Tony

Re: [PATCH v2] x86/mce: Always call memory_failure() when there is a valid address

[Yazen Ghannam]

On 4/18/23 15:34, Luck, Tony wrote:
>>> +		if (mce_usable_address(&m))
>>
>> This should be !mce_usable_address().
> 
>> Copying old patch here. Feel free to reuse any of the commit message if
>> it helps.
> 
> Might as well just take your version. The commit message seems fine.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> 

Thanks!

> 
>> From: Yazen Ghannam <yazen.ghannam@amd.com>
>> Date: Fri, 8 Jan 2021 04:00:35 +0000
> 
> 2021 - wow!
> 

Yeah, I've been distracted lately with other things. >_>

-Yazen