| Message ID | 20240501232458.3919593-2-jane.chu@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enhance soft hwpoison handling and injection | expand |
On Wed, May 01, 2024 at 05:24:56PM -0600, Jane Chu wrote: > For years when it comes down to kill a process due to hwpoison, > a SIGBUS is delivered only if unmap has been successful. > Otherwise, a SIGKILL is delivered. And the reason for that is > to prevent the involved process from accessing the hwpoisoned > page again. > > Since then a lot has changed, a hwpoisoned page is marked and > upon being re-accessed, the process will be killed immediately. > So let's take out the '!unmap_success' factor and try to deliver > SIGBUS if possible. I am missing some details here. An unmapped hwpoison page will trigger a fault and will return VM_FAULT_HWPOISON all the way down and then deliver SIGBUS, but if the page was not unmapped, how will this be catch upon re-accessing? Will the system deliver a MCE event?
On 5/7/2024 2:02 AM, Oscar Salvador wrote: > On Wed, May 01, 2024 at 05:24:56PM -0600, Jane Chu wrote: >> For years when it comes down to kill a process due to hwpoison, >> a SIGBUS is delivered only if unmap has been successful. >> Otherwise, a SIGKILL is delivered. And the reason for that is >> to prevent the involved process from accessing the hwpoisoned >> page again. >> >> Since then a lot has changed, a hwpoisoned page is marked and >> upon being re-accessed, the process will be killed immediately. >> So let's take out the '!unmap_success' factor and try to deliver >> SIGBUS if possible. > I am missing some details here. > An unmapped hwpoison page will trigger a fault and will return > VM_FAULT_HWPOISON all the way down and then deliver SIGBUS, > but if the page was not unmapped, how will this be catch upon > re-accessing? Will the system deliver a MCE event? > I actually managed to hit the re-access case with an older version of Linux - MCE occurred, but unmap failed, no SIGBUS and test process re-access the same address over and over (hence MCE after MCE), as the CPU was unable to make forward progress. In reality, this issue is fixed with kill_accessing_processes(). The comment for this patch refers to comment made about '!unmap_access' long time ago. thanks, -jane
On 2024/5/2 7:24, Jane Chu wrote: > For years when it comes down to kill a process due to hwpoison, > a SIGBUS is delivered only if unmap has been successful. > Otherwise, a SIGKILL is delivered. And the reason for that is > to prevent the involved process from accessing the hwpoisoned > page again. > > Since then a lot has changed, a hwpoisoned page is marked and > upon being re-accessed, the process will be killed immediately. > So let's take out the '!unmap_success' factor and try to deliver > SIGBUS if possible. > > Signed-off-by: Jane Chu <jane.chu@oracle.com> > --- > mm/memory-failure.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 9e62a00b46dd..7fcf182abb96 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p, > * Also when FAIL is set do a force kill because something went > * wrong earlier. Since @fail is removed, above comment should be removed too. Thanks. .
On Tue, May 07, 2024 at 10:54:10AM -0700, Jane Chu wrote: > I actually managed to hit the re-access case with an older version of Linux > - > > MCE occurred, but unmap failed, no SIGBUS and test process re-access > > the same address over and over (hence MCE after MCE), as the CPU > > was unable to make forward progress. In reality, this issue is fixed with > > kill_accessing_processes(). The comment for this patch refers to comment > made So we get a faulty page and we try to unmap it from all processes that might have it mapped in their pgtables. Prior to this patch we would kill the processes right away and now we deliver a SIGBUS. Seems safe as upon-reaccesing kill_accessing_process() will be called for already hwpoisoned pages. I think the changelog could be made more explicit about this scenario and state the role of kill_accessing_process more clear. With that: Reviewed-by: Oscar Salvador <osalvador@suse.de>
On 5/8/2024 5:06 AM, Oscar Salvador wrote: > On Tue, May 07, 2024 at 10:54:10AM -0700, Jane Chu wrote: >> I actually managed to hit the re-access case with an older version of Linux >> - >> >> MCE occurred, but unmap failed, no SIGBUS and test process re-access >> >> the same address over and over (hence MCE after MCE), as the CPU >> >> was unable to make forward progress. In reality, this issue is fixed with >> >> kill_accessing_processes(). The comment for this patch refers to comment >> made > So we get a faulty page and we try to unmap it from all processes that > might have it mapped in their pgtables. > Prior to this patch we would kill the processes right away and now we > deliver a SIGBUS. > > Seems safe as upon-reaccesing kill_accessing_process() will be called > for already hwpoisoned pages. > > I think the changelog could be made more explicit about this scenario > and state the role of kill_accessing_process more clear. > > With that: Reviewed-by: Oscar Salvador <osalvador@suse.de> > I will revise the changelog and mention kill_accessing_process(). Thanks! -jane >
On 5/8/2024 12:47 AM, Miaohe Lin wrote: > On 2024/5/2 7:24, Jane Chu wrote: >> For years when it comes down to kill a process due to hwpoison, >> a SIGBUS is delivered only if unmap has been successful. >> Otherwise, a SIGKILL is delivered. And the reason for that is >> to prevent the involved process from accessing the hwpoisoned >> page again. >> >> Since then a lot has changed, a hwpoisoned page is marked and >> upon being re-accessed, the process will be killed immediately. >> So let's take out the '!unmap_success' factor and try to deliver >> SIGBUS if possible. >> >> Signed-off-by: Jane Chu <jane.chu@oracle.com> >> --- >> mm/memory-failure.c | 13 ++++--------- >> 1 file changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 9e62a00b46dd..7fcf182abb96 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p, >> * Also when FAIL is set do a force kill because something went >> * wrong earlier. > Since @fail is removed, above comment should be removed too. > Thanks. > . Good catch, will do. Thanks! -jane
On 2024/5/2 7:24, Jane Chu wrote: > For years when it comes down to kill a process due to hwpoison, > a SIGBUS is delivered only if unmap has been successful. > Otherwise, a SIGKILL is delivered. And the reason for that is > to prevent the involved process from accessing the hwpoisoned > page again. > > Since then a lot has changed, a hwpoisoned page is marked and > upon being re-accessed, the process will be killed immediately. > So let's take out the '!unmap_success' factor and try to deliver > SIGBUS if possible. > > Signed-off-by: Jane Chu <jane.chu@oracle.com> > --- > mm/memory-failure.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 9e62a00b46dd..7fcf182abb96 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p, > * Also when FAIL is set do a force kill because something went > * wrong earlier. > */ > -static void kill_procs(struct list_head *to_kill, int forcekill, bool fail, > +static void kill_procs(struct list_head *to_kill, int forcekill, > unsigned long pfn, int flags) > { > struct to_kill *tk, *next; > > list_for_each_entry_safe(tk, next, to_kill, nd) { > if (forcekill) { > - /* > - * In case something went wrong with munmapping > - * make sure the process doesn't catch the > - * signal and then access the memory. Just kill it. > - */ > - if (fail || tk->addr == -EFAULT) { > + if (tk->addr == -EFAULT) { > pr_err("%#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n", > pfn, tk->tsk->comm, tk->tsk->pid); > do_send_sig_info(SIGKILL, SEND_SIG_PRIV, > @@ -1666,7 +1661,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, > */ There is comment above the forcekill saying: When there was a problem unmapping earlier use a more force-full uncatchable kill to prevent any accesses to the poisoned memory. This might need to be changed too. Thanks. . > forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) || > !unmap_success; > - kill_procs(&tokill, forcekill, !unmap_success, pfn, flags); > + kill_procs(&tokill, forcekill, pfn, flags); > > return unmap_success; > } > @@ -1730,7 +1725,7 @@ static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn, > unmap_mapping_range(mapping, start, size, 0); > } > > - kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags); > + kill_procs(to_kill, flags & MF_MUST_KILL, pfn, flags); > } > > /* >
On 5/8/2024 7:54 PM, Miaohe Lin wrote: > On 2024/5/2 7:24, Jane Chu wrote: >> For years when it comes down to kill a process due to hwpoison, >> a SIGBUS is delivered only if unmap has been successful. >> Otherwise, a SIGKILL is delivered. And the reason for that is >> to prevent the involved process from accessing the hwpoisoned >> page again. >> >> Since then a lot has changed, a hwpoisoned page is marked and >> upon being re-accessed, the process will be killed immediately. >> So let's take out the '!unmap_success' factor and try to deliver >> SIGBUS if possible. >> >> Signed-off-by: Jane Chu <jane.chu@oracle.com> >> --- >> mm/memory-failure.c | 13 ++++--------- >> 1 file changed, 4 insertions(+), 9 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index 9e62a00b46dd..7fcf182abb96 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p, >> * Also when FAIL is set do a force kill because something went >> * wrong earlier. >> */ >> -static void kill_procs(struct list_head *to_kill, int forcekill, bool fail, >> +static void kill_procs(struct list_head *to_kill, int forcekill, >> unsigned long pfn, int flags) >> { >> struct to_kill *tk, *next; >> >> list_for_each_entry_safe(tk, next, to_kill, nd) { >> if (forcekill) { >> - /* >> - * In case something went wrong with munmapping >> - * make sure the process doesn't catch the >> - * signal and then access the memory. Just kill it. >> - */ >> - if (fail || tk->addr == -EFAULT) { >> + if (tk->addr == -EFAULT) { >> pr_err("%#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n", >> pfn, tk->tsk->comm, tk->tsk->pid); >> do_send_sig_info(SIGKILL, SEND_SIG_PRIV, >> @@ -1666,7 +1661,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, >> */ > There is comment above the forcekill saying: > > When there was a problem unmapping earlier use a more force-full > uncatchable kill to prevent any accesses to the poisoned memory. > > This might need to be changed too. Yes, will do. thanks! -jane > Thanks. > . > >> forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) || >> !unmap_success; >> - kill_procs(&tokill, forcekill, !unmap_success, pfn, flags); >> + kill_procs(&tokill, forcekill, pfn, flags); >> >> return unmap_success; >> } >> @@ -1730,7 +1725,7 @@ static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn, >> unmap_mapping_range(mapping, start, size, 0); >> } >> >> - kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags); >> + kill_procs(to_kill, flags & MF_MUST_KILL, pfn, flags); >> } >> >> /* >> >
diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 9e62a00b46dd..7fcf182abb96 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p, * Also when FAIL is set do a force kill because something went * wrong earlier. */ -static void kill_procs(struct list_head *to_kill, int forcekill, bool fail, +static void kill_procs(struct list_head *to_kill, int forcekill, unsigned long pfn, int flags) { struct to_kill *tk, *next; list_for_each_entry_safe(tk, next, to_kill, nd) { if (forcekill) { - /* - * In case something went wrong with munmapping - * make sure the process doesn't catch the - * signal and then access the memory. Just kill it. - */ - if (fail || tk->addr == -EFAULT) { + if (tk->addr == -EFAULT) { pr_err("%#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n", pfn, tk->tsk->comm, tk->tsk->pid); do_send_sig_info(SIGKILL, SEND_SIG_PRIV, @@ -1666,7 +1661,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn, */ forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) || !unmap_success; - kill_procs(&tokill, forcekill, !unmap_success, pfn, flags); + kill_procs(&tokill, forcekill, pfn, flags); return unmap_success; } @@ -1730,7 +1725,7 @@ static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn, unmap_mapping_range(mapping, start, size, 0); } - kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags); + kill_procs(to_kill, flags & MF_MUST_KILL, pfn, flags); } /*
For years when it comes down to kill a process due to hwpoison, a SIGBUS is delivered only if unmap has been successful. Otherwise, a SIGKILL is delivered. And the reason for that is to prevent the involved process from accessing the hwpoisoned page again. Since then a lot has changed, a hwpoisoned page is marked and upon being re-accessed, the process will be killed immediately. So let's take out the '!unmap_success' factor and try to deliver SIGBUS if possible. Signed-off-by: Jane Chu <jane.chu@oracle.com> --- mm/memory-failure.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)