* [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
@ 2012-07-06 21:33 Tony Luck
2012-07-07 4:38 ` Borislav Petkov
0 siblings, 1 reply; 7+ messages in thread
From: Tony Luck @ 2012-07-06 21:33 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Borislav Petkov, Chen Gong, Huang, Ying, Hidetoshi Seto
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
2012-07-06 21:33 [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process Tony Luck
@ 2012-07-07 4:38 ` Borislav Petkov
2012-07-09 16:22 ` Luck, Tony
0 siblings, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2012-07-07 4:38 UTC (permalink / raw)
To: Tony Luck
Cc: linux-kernel, Ingo Molnar, Borislav Petkov, Chen Gong, Huang,
Ying, Hidetoshi Seto
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
2012-07-07 4:38 ` Borislav Petkov
@ 2012-07-09 16:22 ` Luck, Tony
2012-07-09 20:34 ` Tony Luck
0 siblings, 1 reply; 7+ messages in thread
From: Luck, Tony @ 2012-07-09 16:22 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, Ingo Molnar, Chen Gong, Huang, Ying, Hidetoshi Seto
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
2012-07-09 16:22 ` Luck, Tony
@ 2012-07-09 20:34 ` Tony Luck
2012-07-10 15:44 ` Borislav Petkov
0 siblings, 1 reply; 7+ messages in thread
From: Tony Luck @ 2012-07-09 20:34 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Borislav Petkov, Chen Gong, Huang, Ying, Hidetoshi Seto
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
2012-07-09 20:34 ` Tony Luck
@ 2012-07-10 15:44 ` Borislav Petkov
2012-07-10 17:12 ` Tony Luck
0 siblings, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2012-07-10 15:44 UTC (permalink / raw)
To: Tony Luck
Cc: linux-kernel, Ingo Molnar, Chen Gong, Huang, Ying, Hidetoshi Seto
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
2012-07-10 15:44 ` Borislav Petkov
@ 2012-07-10 17:12 ` Tony Luck
2012-07-10 18:36 ` Borislav Petkov
0 siblings, 1 reply; 7+ messages in thread
From: Tony Luck @ 2012-07-10 17:12 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, Ingo Molnar, Chen Gong, Huang, Ying, Hidetoshi Seto
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process
2012-07-10 17:12 ` Tony Luck
@ 2012-07-10 18:36 ` Borislav Petkov
0 siblings, 0 replies; 7+ messages in thread
From: Borislav Petkov @ 2012-07-10 18:36 UTC (permalink / raw)
To: Tony Luck
Cc: linux-kernel, Ingo Molnar, Chen Gong, Huang, Ying, Hidetoshi Seto
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-07-10 18:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-06 21:33 [PATCH] x86/mce: Need to let kill_proc() send signal to doomed process Tony Luck
2012-07-07 4:38 ` Borislav Petkov
2012-07-09 16:22 ` Luck, Tony
2012-07-09 20:34 ` Tony Luck
2012-07-10 15:44 ` Borislav Petkov
2012-07-10 17:12 ` Tony Luck
2012-07-10 18:36 ` Borislav Petkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.