[PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Tony Luck]

In commit dad1743e5993f19b3d7e7bd0fb35dc45b5326626
  x86/mce: Only restart instruction after machine check recovery if it is safe
we fixed mce_notify_process() to force a signal to the current process
if it was not restartable (RIPV bit not set in MCG_STATUS). But doing
it here means that the process doesn't get told the virtual address of
the fault via siginfo_t->si_addr. This would prevent application level
recovery from the fault.

Make a new MF_MUST_KILL flag bit for memory_failure() et. al. to use
so that we will provide the right information with the signal.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 4 ++--
 include/linux/mm.h               | 1 +
 mm/memory-failure.c              | 8 +++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index da27c5d..43f918d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1200,8 +1200,8 @@ void mce_notify_process(void)
 	 * doomed. We still need to mark the page as poisoned and alert any
 	 * other users of the page.
 	 */
-	if (memory_failure(pfn, MCE_VECTOR, MF_ACTION_REQUIRED) < 0 ||
-			   mi->restartable == 0) {
+	if (memory_failure(pfn, MCE_VECTOR,
+			   MF_ACTION_REQUIRED|MF_MUST_KILL) < 0) {
 		pr_err("Memory error not recovered");
 		force_sig(SIGBUS, current);
 	}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..f9f279c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1591,6 +1591,7 @@ void vmemmap_populate_print_last(void);
 enum mf_flags {
 	MF_COUNT_INCREASED = 1 << 0,
 	MF_ACTION_REQUIRED = 1 << 1,
+	MF_MUST_KILL = 1 << 2,
 };
 extern int memory_failure(unsigned long pfn, int trapno, int flags);
 extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ab1e714..e3e0045 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -858,7 +858,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	struct address_space *mapping;
 	LIST_HEAD(tokill);
 	int ret;
-	int kill = 1;
+	int kill = 1, doit;
 	struct page *hpage = compound_head(p);
 	struct page *ppage;
 
@@ -965,12 +965,14 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * Now that the dirty bit has been propagated to the
 	 * struct page and all unmaps done we can decide if
 	 * killing is needed or not.  Only kill when the page
-	 * was dirty, otherwise the tokill list is merely
+	 * was dirty or the process is not restartable,
+	 * otherwise the tokill list is merely
 	 * freed.  When there was a problem unmapping earlier
 	 * use a more force-full uncatchable kill to prevent
 	 * any accesses to the poisoned memory.
 	 */
-	kill_procs(&tokill, !!PageDirty(ppage), trapno,
+	doit = !!PageDirty(ppage) || (flags & MF_MUST_KILL) != 0;
+	kill_procs(&tokill, doit, trapno,
 		      ret != SWAP_SUCCESS, p, pfn, flags);
 
 	return ret;
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Borislav Petkov]

On Fri, Jul 06, 2012 at 02:33:15PM -0700, Tony Luck wrote:
> In commit dad1743e5993f19b3d7e7bd0fb35dc45b5326626
>   x86/mce: Only restart instruction after machine check recovery if it is safe
> we fixed mce_notify_process() to force a signal to the current process
> if it was not restartable (RIPV bit not set in MCG_STATUS). But doing
> it here means that the process doesn't get told the virtual address of
> the fault via siginfo_t->si_addr. This would prevent application level
> recovery from the fault.

Ok, this makes sense, we want to kill all the processes mapping that
page.

> Make a new MF_MUST_KILL flag bit for memory_failure() et. al. to use
> so that we will provide the right information with the signal.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c | 4 ++--
>  include/linux/mm.h               | 1 +
>  mm/memory-failure.c              | 8 +++++---
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index da27c5d..43f918d 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1200,8 +1200,8 @@ void mce_notify_process(void)
>  	 * doomed. We still need to mark the page as poisoned and alert any
>  	 * other users of the page.
>  	 */
> -	if (memory_failure(pfn, MCE_VECTOR, MF_ACTION_REQUIRED) < 0 ||
> -			   mi->restartable == 0) {
> +	if (memory_failure(pfn, MCE_VECTOR,
> +			   MF_ACTION_REQUIRED|MF_MUST_KILL) < 0) {

This makes mi->restartable unused?

And more specifically, we're not looking at RIPV anymore. I'm guessing
when we've reached this point, we always MUST_KILL?

>  		pr_err("Memory error not recovered");
>  		force_sig(SIGBUS, current);
>  	}
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b36d08c..f9f279c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1591,6 +1591,7 @@ void vmemmap_populate_print_last(void);
>  enum mf_flags {
>  	MF_COUNT_INCREASED = 1 << 0,
>  	MF_ACTION_REQUIRED = 1 << 1,
> +	MF_MUST_KILL = 1 << 2,
>  };
>  extern int memory_failure(unsigned long pfn, int trapno, int flags);
>  extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index ab1e714..e3e0045 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -858,7 +858,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  	struct address_space *mapping;
>  	LIST_HEAD(tokill);
>  	int ret;
> -	int kill = 1;
> +	int kill = 1, doit;
>  	struct page *hpage = compound_head(p);
>  	struct page *ppage;
>  
> @@ -965,12 +965,14 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  	 * Now that the dirty bit has been propagated to the
>  	 * struct page and all unmaps done we can decide if
>  	 * killing is needed or not.  Only kill when the page
> -	 * was dirty, otherwise the tokill list is merely
> +	 * was dirty or the process is not restartable,
> +	 * otherwise the tokill list is merely
>  	 * freed.  When there was a problem unmapping earlier
>  	 * use a more force-full uncatchable kill to prevent
>  	 * any accesses to the poisoned memory.
>  	 */
> -	kill_procs(&tokill, !!PageDirty(ppage), trapno,
> +	doit = !!PageDirty(ppage) || (flags & MF_MUST_KILL) != 0;

Maybe

				     !!(flags & MF_MUST_KILL)
?

> +	kill_procs(&tokill, doit, trapno,
>  		      ret != SWAP_SUCCESS, p, pfn, flags);
>  
>  	return ret;
> -- 

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

RE: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Luck, Tony]

> This makes mi->restartable unused?

It does ... but it's not what I meant ... somehow I lost the code that set MF_MUST_KILL
based on mi->restartable. Doh!

>> +	doit = !!PageDirty(ppage) || (flags & MF_MUST_KILL) != 0;
>
> Maybe
>
>				     !!(flags & MF_MUST_KILL)

That fits stylistically with the other half of the "||" expression.

Thanks for the review. I'll update and resend.

-Tony

[PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Tony Luck]

In commit dad1743e5993f19b3d7e7bd0fb35dc45b5326626
  x86/mce: Only restart instruction after machine check recovery if it is safe

we fixed mce_notify_process() to force a signal to the current process
if it was not restartable (RIPV bit not set in MCG_STATUS). But doing
it here means that the process doesn't get told the virtual address of
the fault via siginfo_t->si_addr. This would prevent application level
recovery from the fault.

Make a new MF_MUST_KILL flag bit for memory_failure() et. al. to use
so that we will provide the right information with the signal.

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

v2: Fix brainfart where I forgot to check mi->restartable to decide whether
    to pass in the new MF_MUST_KILL bit [Thanks Boris for spotting this!]
    Use same style syntax !!(flags & MF_MUST_KILL) [Also from Boris]
    Faked tests with RIPV set and not set ... and found that we need one
    more check on MF_MUST_KILL earlier in hwpoison_user_mappings() to
    make sure it doesn't think the clean page case is recoverable.

 arch/x86/kernel/cpu/mcheck/mce.c |  6 ++++--
 include/linux/mm.h               |  1 +
 mm/memory-failure.c              | 10 ++++++----
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index da27c5d..9f9ed4f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1186,6 +1186,7 @@ void mce_notify_process(void)
 {
 	unsigned long pfn;
 	struct mce_info *mi = mce_find_info();
+	int flags = MF_ACTION_REQUIRED;
 
 	if (!mi)
 		mce_panic("Lost physical address for unconsumed uncorrectable error", NULL, NULL);
@@ -1200,8 +1201,9 @@ void mce_notify_process(void)
 	 * doomed. We still need to mark the page as poisoned and alert any
 	 * other users of the page.
 	 */
-	if (memory_failure(pfn, MCE_VECTOR, MF_ACTION_REQUIRED) < 0 ||
-			   mi->restartable == 0) {
+	if (mi->restartable == 0)
+		flags |= MF_MUST_KILL;
+	if (memory_failure(pfn, MCE_VECTOR, flags) < 0) {
 		pr_err("Memory error not recovered");
 		force_sig(SIGBUS, current);
 	}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b36d08c..f9f279c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1591,6 +1591,7 @@ void vmemmap_populate_print_last(void);
 enum mf_flags {
 	MF_COUNT_INCREASED = 1 << 0,
 	MF_ACTION_REQUIRED = 1 << 1,
+	MF_MUST_KILL = 1 << 2,
 };
 extern int memory_failure(unsigned long pfn, int trapno, int flags);
 extern void memory_failure_queue(unsigned long pfn, int trapno, int flags);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ab1e714..83cc9ef 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -858,7 +858,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	struct address_space *mapping;
 	LIST_HEAD(tokill);
 	int ret;
-	int kill = 1;
+	int kill = 1, doit;
 	struct page *hpage = compound_head(p);
 	struct page *ppage;
 
@@ -888,7 +888,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * be called inside page lock (it's recommended but not enforced).
 	 */
 	mapping = page_mapping(hpage);
-	if (!PageDirty(hpage) && mapping &&
+	if (!(flags & MF_MUST_KILL) && !PageDirty(hpage) && mapping &&
 	    mapping_cap_writeback_dirty(mapping)) {
 		if (page_mkclean(hpage)) {
 			SetPageDirty(hpage);
@@ -965,12 +965,14 @@ static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * Now that the dirty bit has been propagated to the
 	 * struct page and all unmaps done we can decide if
 	 * killing is needed or not.  Only kill when the page
-	 * was dirty, otherwise the tokill list is merely
+	 * was dirty or the process is not restartable,
+	 * otherwise the tokill list is merely
 	 * freed.  When there was a problem unmapping earlier
 	 * use a more force-full uncatchable kill to prevent
 	 * any accesses to the poisoned memory.
 	 */
-	kill_procs(&tokill, !!PageDirty(ppage), trapno,
+	doit = !!PageDirty(ppage) || !!(flags & MF_MUST_KILL);
+	kill_procs(&tokill, doit, trapno,
 		      ret != SWAP_SUCCESS, p, pfn, flags);
 
 	return ret;
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Borislav Petkov]

On Mon, Jul 09, 2012 at 01:34:36PM -0700, Tony Luck wrote:
> In commit dad1743e5993f19b3d7e7bd0fb35dc45b5326626
>   x86/mce: Only restart instruction after machine check recovery if it is safe
> 
> we fixed mce_notify_process() to force a signal to the current process
> if it was not restartable (RIPV bit not set in MCG_STATUS). But doing
> it here means that the process doesn't get told the virtual address of
> the fault via siginfo_t->si_addr. This would prevent application level
> recovery from the fault.
> 
> Make a new MF_MUST_KILL flag bit for memory_failure() et. al. to use
> so that we will provide the right information with the signal.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> v2: Fix brainfart where I forgot to check mi->restartable to decide whether
>     to pass in the new MF_MUST_KILL bit [Thanks Boris for spotting this!]
>     Use same style syntax !!(flags & MF_MUST_KILL) [Also from Boris]
>     Faked tests with RIPV set and not set ... and found that we need one
>     more check on MF_MUST_KILL earlier in hwpoison_user_mappings() to
>     make sure it doesn't think the clean page case is recoverable.

Ok, now it makes pretty good sense.

Acked-by: Borislav Petkov <borislav.petkov@amd.com>

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Tony Luck]

On Tue, Jul 10, 2012 at 8:44 AM, Borislav Petkov <bp@amd64.org> wrote:
> Acked-by: Borislav Petkov <borislav.petkov@amd.com>

Thanks for the Ack.

+       doit = !!PageDirty(ppage) || !!(flags & MF_MUST_KILL);

Thinking about this some more, the "!!" are redundant
and are an impediment to readability. We started with
!!PageDirty(ppage) when we were passing an argument
directly to kill_procs() and wanted to make sure we had a
nice boolean value. But the result of the "||" is boolean,
so we don't need to double negate.

I'm going to change it to:
       doit = PageDirty(ppage) || (flags & MF_MUST_KILL);

-Tony

Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process

[Borislav Petkov]

On Tue, Jul 10, 2012 at 10:12:17AM -0700, Tony Luck wrote:
> On Tue, Jul 10, 2012 at 8:44 AM, Borislav Petkov <bp@amd64.org> wrote:
> > Acked-by: Borislav Petkov <borislav.petkov@amd.com>
> 
> Thanks for the Ack.
> 
> +       doit = !!PageDirty(ppage) || !!(flags & MF_MUST_KILL);
> 
> Thinking about this some more, the "!!" are redundant
> and are an impediment to readability. We started with
> !!PageDirty(ppage) when we were passing an argument
> directly to kill_procs() and wanted to make sure we had a
> nice boolean value. But the result of the "||" is boolean,
> so we don't need to double negate.
> 
> I'm going to change it to:
>        doit = PageDirty(ppage) || (flags & MF_MUST_KILL);

Ok, so out of curiosity I took a look at compiler output and both
versions are identical:


> +       doit = !!PageDirty(ppage) || !!(flags & MF_MUST_KILL);

	.loc 1 974 0
	movl	$1, %edx	#, doit
	testb	$16, %al	#, D.28886
	jne	.L196	#,
	movl	-196(%rbp), %ecx	# %sfp,
	xorl	%edx, %edx	# doit
	testl	%ecx, %ecx	#
	setne	%dl	#, doit

>        doit = PageDirty(ppage) || (flags & MF_MUST_KILL);

	.loc 1 974 0
	movl	$1, %edx	#, doit
	testb	$16, %al	#, D.28886
	jne	.L196	#,
	movl	-196(%rbp), %ecx	# %sfp,
	xorl	%edx, %edx	# doit
	testl	%ecx, %ecx	#
	setne	%dl	#, doit

In both cases you get your standard shortcutting OR logic:

.loc 1 974 0
	movl	$1, %edx	#, doit		<--- preset doit
	testb	$16, %al	#, D.28886	<--- PG_dirty
	jne	.L196	#,			<--- ZF=0, shortcut out to kill_procs
	movl	-196(%rbp), %ecx	# %sfp, <--- get value of (flags & MF_MUST_KILL) which we computed earlier in the function and saved on stack
	xorl	%edx, %edx	# doit		<--- clear doit
	testl	%ecx, %ecx	#		<--- check above (flags & MF_MUST_KILL) is not 0
	setne	%dl	#, doit			<--- prep doit for kill_procs

so the compiler is pretty smart already.

You could go another step further by declaring

	bool doit;

and changing kill_procs() argument to bool too so that the booleanness
is really explicit.

This saves you the xor:

        .loc 1 975 0
        movl    $1, %edx        #, prephitmp.124
        testb   $16, %al        #, D.28891
        jne     .L196   #,
        movl    -196(%rbp), %ecx        # %sfp,
        testl   %ecx, %ecx      #
        setne   %dl     #, prephitmp.124

because we're using explicitly bools which are u8s, apparently, in this
case and we're reusing the 1 we wrote into edx at the beginning of the
block.

Oh well, enough fun.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551