* [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process @ 2020-05-26 7:06 Wetp Zhang 2020-05-28 2:22 ` HORIGUCHI NAOYA(堀口 直也) 0 siblings, 1 reply; 6+ messages in thread From: Wetp Zhang @ 2020-05-26 7:06 UTC (permalink / raw) To: n-horiguchi, akpm; +Cc: linux-mm, linux-kernel From: Zhang Yi <wetpzy@gmail.com> If a process don't need early-kill, it may not care the BUS_MCEERR_AO. Let the process to be killed when it really access the corrupted memory. Signed-off-by: Zhang Yi <wetpzy@gmail.com> --- mm/memory-failure.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/mm/memory-failure.c b/mm/memory-failure.c index a96364be8ab4..2db13d48865c 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -210,7 +210,7 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) { struct task_struct *t = tk->tsk; short addr_lsb = tk->size_shift; - int ret; + int ret = 0; pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n", pfn, t->comm, t->pid); @@ -225,8 +225,9 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) * This could cause a loop when the user sets SIGBUS * to SIG_IGN, but hopefully no one will do that? */ - ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, - addr_lsb, t); /* synchronous? */ + if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY)) + ret = send_sig_mceerr(BUS_MCEERR_AO, + (void __user *)tk->addr, addr_lsb, t); } if (ret < 0) pr_info("Memory failure: Error sending signal to %s:%d: %d\n", -- 2.14.3 (Apple Git-98) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process 2020-05-26 7:06 [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process Wetp Zhang @ 2020-05-28 2:22 ` HORIGUCHI NAOYA(堀口 直也) 2020-05-28 6:50 ` wetp 0 siblings, 1 reply; 6+ messages in thread From: HORIGUCHI NAOYA(堀口 直也) @ 2020-05-28 2:22 UTC (permalink / raw) To: Wetp Zhang; +Cc: n-horiguchi, akpm, linux-mm, linux-kernel Hi Zhang, Sorry for my late response. On Tue, May 26, 2020 at 03:06:41PM +0800, Wetp Zhang wrote: > From: Zhang Yi <wetpzy@gmail.com> > > If a process don't need early-kill, it may not care the BUS_MCEERR_AO. > Let the process to be killed when it really access the corrupted memory. > > Signed-off-by: Zhang Yi <wetpzy@gmail.com> Thank you for pointing this. This looks to me a bug (per-process flag is ignored when system-wide flag is set). > --- > mm/memory-failure.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index a96364be8ab4..2db13d48865c 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -210,7 +210,7 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) > { > struct task_struct *t = tk->tsk; > short addr_lsb = tk->size_shift; > - int ret; > + int ret = 0; > > pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n", > pfn, t->comm, t->pid); > @@ -225,8 +225,9 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) > * This could cause a loop when the user sets SIGBUS > * to SIG_IGN, but hopefully no one will do that? > */ > - ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, > - addr_lsb, t); /* synchronous? */ > + if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY)) > + ret = send_sig_mceerr(BUS_MCEERR_AO, > + (void __user *)tk->addr, addr_lsb, t); kill_proc() could be called only for processes that are selected by collect_procs() with task_early_kill(). So I think that we should fix task_early_kill(), maybe by reordering sysctl_memory_failure_early_kill check and find_early_kill_thread() check. static struct task_struct *task_early_kill(struct task_struct *tsk, int force_early) { struct task_struct *t; if (!tsk->mm) return NULL; if (force_early) return tsk; t = find_early_kill_thread(tsk); if (t) return t; if (sysctl_memory_failure_early_kill) return tsk; return NULL; } One subtleness is to make sure that find_early_kill_thread() should distinguish default value and explicitly set value, so we might need some modification on find_early_kill_thread(). Can you try that? Thanks, Naoya Horiguchi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process 2020-05-28 2:22 ` HORIGUCHI NAOYA(堀口 直也) @ 2020-05-28 6:50 ` wetp 2020-05-29 2:12 ` HORIGUCHI NAOYA(堀口 直也) 0 siblings, 1 reply; 6+ messages in thread From: wetp @ 2020-05-28 6:50 UTC (permalink / raw) To: HORIGUCHI NAOYA(堀口 直也) Cc: n-horiguchi, akpm, linux-mm, linux-kernel On 2020/5/28 上午10:22, HORIGUCHI NAOYA(堀口 直也) wrote: > Hi Zhang, > > Sorry for my late response. > > On Tue, May 26, 2020 at 03:06:41PM +0800, Wetp Zhang wrote: >> From: Zhang Yi <wetpzy@gmail.com> >> >> If a process don't need early-kill, it may not care the BUS_MCEERR_AO. >> Let the process to be killed when it really access the corrupted memory. >> >> Signed-off-by: Zhang Yi <wetpzy@gmail.com> > Thank you for pointing this. This looks to me a bug (per-process flag > is ignored when system-wide flag is set). The flag is not problem for me. In my case, two processes share memory with no any flag setting, both will be killed when only one access the fail memory. >> --- >> mm/memory-failure.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >> index a96364be8ab4..2db13d48865c 100644 >> --- a/mm/memory-failure.c >> +++ b/mm/memory-failure.c >> @@ -210,7 +210,7 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) >> { >> struct task_struct *t = tk->tsk; >> short addr_lsb = tk->size_shift; >> - int ret; >> + int ret = 0; >> >> pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n", >> pfn, t->comm, t->pid); >> @@ -225,8 +225,9 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) >> * This could cause a loop when the user sets SIGBUS >> * to SIG_IGN, but hopefully no one will do that? >> */ >> - ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, >> - addr_lsb, t); /* synchronous? */ >> + if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY)) >> + ret = send_sig_mceerr(BUS_MCEERR_AO, >> + (void __user *)tk->addr, addr_lsb, t); > kill_proc() could be called only for processes that are selected by > collect_procs() with task_early_kill(). So I think that we should fix > task_early_kill(), maybe by reordering sysctl_memory_failure_early_kill > check and find_early_kill_thread() check. > > static struct task_struct *task_early_kill(struct task_struct *tsk, > int force_early) > { > struct task_struct *t; > if (!tsk->mm) > return NULL; > if (force_early) > return tsk; The force_early is rely the flag MF_ACTION_REQUIRED, so it is always true when MCE occurs. This leads always sending SIGBUS to processes even if those are not current or no flag setting. I think it could keep the non-current processes which has no flag setting running. Besides, base on your recommendation I reorder the force_early check and find_early_kill_thread() check, to send the signal to the right thread. diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 2db13d48865c..33a87d7b3e61 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -399,11 +402,11 @@ static struct task_struct *task_early_kill(struct task_struct *tsk, struct task_struct *t; if (!tsk->mm) return NULL; - if (force_early) - return tsk; t = find_early_kill_thread(tsk); if (t) return t; + if (force_early && tsk->mm == current->mm) + return tsk; if (sysctl_memory_failure_early_kill) return tsk; return NULL; > t = find_early_kill_thread(tsk); > if (t) > return t; > if (sysctl_memory_failure_early_kill) > return tsk; > return NULL; > } > > One subtleness is to make sure that find_early_kill_thread() should distinguish > default value and explicitly set value, so we might need some modification > on find_early_kill_thread(). > > Can you try that? > > Thanks, > Naoya Horiguchi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process 2020-05-28 6:50 ` wetp @ 2020-05-29 2:12 ` HORIGUCHI NAOYA(堀口 直也) 2020-05-29 5:56 ` wetp 0 siblings, 1 reply; 6+ messages in thread From: HORIGUCHI NAOYA(堀口 直也) @ 2020-05-29 2:12 UTC (permalink / raw) To: wetp; +Cc: n-horiguchi, akpm, linux-mm, linux-kernel On Thu, May 28, 2020 at 02:50:09PM +0800, wetp wrote: > > On 2020/5/28 上午10:22, HORIGUCHI NAOYA(堀口 直也) wrote: > > Hi Zhang, > > > > Sorry for my late response. > > > > On Tue, May 26, 2020 at 03:06:41PM +0800, Wetp Zhang wrote: > > > From: Zhang Yi <wetpzy@gmail.com> > > > > > > If a process don't need early-kill, it may not care the BUS_MCEERR_AO. > > > Let the process to be killed when it really access the corrupted memory. > > > > > > Signed-off-by: Zhang Yi <wetpzy@gmail.com> > > Thank you for pointing this. This looks to me a bug (per-process flag > > is ignored when system-wide flag is set). > > The flag is not problem for me. > > In my case, two processes share memory with no any flag setting, both will > be killed when only one > > access the fail memory. Thanks, now your problem seems clearer. It seems that this happens because in "Action Required" case kill_proc() takes the first branch for current process, while it takes the else branch for other affected processes: static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) { ... if ((flags & MF_ACTION_REQUIRED) && t->mm == current->mm) { ret = force_sig_mceerr(BUS_MCEERR_AR, (void __user *)tk->addr, addr_lsb); } else { /* * Don't use force here, it's convenient if the signal * can be temporarily blocked. * This could cause a loop when the user sets SIGBUS * to SIG_IGN, but hopefully no one will do that? */ ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, addr_lsb, t); /* synchronous? */ } Sending SIGBUS with BUS_MCEERR_AO for action optional error is strange, so maybe this logic should be like this: if (flags & MF_ACTION_REQUIRED) { if (t->mm == current->mm) ret = force_sig_mceerr(BUS_MCEERR_AR, (void __user *)tk->addr, addr_lsb); /* send no signal to non-current processes */ } else { /* * Don't use force here, it's convenient if the signal * can be temporarily blocked. * This could cause a loop when the user sets SIGBUS * to SIG_IGN, but hopefully no one will do that? */ ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, addr_lsb, t); /* synchronous? */ } > > > > --- > > > mm/memory-failure.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > > > index a96364be8ab4..2db13d48865c 100644 > > > --- a/mm/memory-failure.c > > > +++ b/mm/memory-failure.c > > > @@ -210,7 +210,7 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) > > > { > > > struct task_struct *t = tk->tsk; > > > short addr_lsb = tk->size_shift; > > > - int ret; > > > + int ret = 0; > > > > > > pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n", > > > pfn, t->comm, t->pid); > > > @@ -225,8 +225,9 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) > > > * This could cause a loop when the user sets SIGBUS > > > * to SIG_IGN, but hopefully no one will do that? > > > */ > > > - ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, > > > - addr_lsb, t); /* synchronous? */ > > > + if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY)) > > > + ret = send_sig_mceerr(BUS_MCEERR_AO, > > > + (void __user *)tk->addr, addr_lsb, t); > > kill_proc() could be called only for processes that are selected by > > collect_procs() with task_early_kill(). So I think that we should fix > > task_early_kill(), maybe by reordering sysctl_memory_failure_early_kill > > check and find_early_kill_thread() check. > > > > static struct task_struct *task_early_kill(struct task_struct *tsk, > > int force_early) > > { > > struct task_struct *t; > > if (!tsk->mm) > > return NULL; > > if (force_early) > > return tsk; > > The force_early is rely the flag MF_ACTION_REQUIRED, so it is always true > when MCE occurs. > > This leads always sending SIGBUS to processes even if those are not current > or no flag setting. > > I think it could keep the non-current processes which has no flag setting > running. > > > Besides, base on your recommendation I reorder the force_early check and > find_early_kill_thread() > > check, to send the signal to the right thread. Sorry, my previous comment around task_early_kill() is for a separate problem, so I'll try some fix on this later. Thanks, Naoya Horiguchi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process 2020-05-29 2:12 ` HORIGUCHI NAOYA(堀口 直也) @ 2020-05-29 5:56 ` wetp 2020-05-29 6:43 ` HORIGUCHI NAOYA(堀口 直也) 0 siblings, 1 reply; 6+ messages in thread From: wetp @ 2020-05-29 5:56 UTC (permalink / raw) To: HORIGUCHI NAOYA(堀口 直也) Cc: n-horiguchi, akpm, linux-mm, linux-kernel On 2020/5/29 上午10:12, HORIGUCHI NAOYA(堀口 直也) wrote: > On Thu, May 28, 2020 at 02:50:09PM +0800, wetp wrote: >> On 2020/5/28 上午10:22, HORIGUCHI NAOYA(堀口 直也) wrote: >>> Hi Zhang, >>> >>> Sorry for my late response. >>> >>> On Tue, May 26, 2020 at 03:06:41PM +0800, Wetp Zhang wrote: >>>> From: Zhang Yi <wetpzy@gmail.com> >>>> >>>> If a process don't need early-kill, it may not care the BUS_MCEERR_AO. >>>> Let the process to be killed when it really access the corrupted memory. >>>> >>>> Signed-off-by: Zhang Yi <wetpzy@gmail.com> >>> Thank you for pointing this. This looks to me a bug (per-process flag >>> is ignored when system-wide flag is set). >> The flag is not problem for me. >> >> In my case, two processes share memory with no any flag setting, both will >> be killed when only one >> >> access the fail memory. > Thanks, now your problem seems clearer. > > It seems that this happens because in "Action Required" case kill_proc() > takes the first branch for current process, while it takes the else branch > for other affected processes: > > static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) > { > ... > > if ((flags & MF_ACTION_REQUIRED) && t->mm == current->mm) { > ret = force_sig_mceerr(BUS_MCEERR_AR, (void __user *)tk->addr, > addr_lsb); > } else { > /* > * Don't use force here, it's convenient if the signal > * can be temporarily blocked. > * This could cause a loop when the user sets SIGBUS > * to SIG_IGN, but hopefully no one will do that? > */ > ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, > addr_lsb, t); /* synchronous? */ > } > > Sending SIGBUS with BUS_MCEERR_AO for action optional error is strange, so > maybe this logic should be like this: > > > if (flags & MF_ACTION_REQUIRED) { > if (t->mm == current->mm) > ret = force_sig_mceerr(BUS_MCEERR_AR, (void __user *)tk->addr, > addr_lsb); > /* send no signal to non-current processes */ Ok, this can solve my problem. > } else { > /* > * Don't use force here, it's convenient if the signal > * can be temporarily blocked. > * This could cause a loop when the user sets SIGBUS > * to SIG_IGN, but hopefully no one will do that? > */ > ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, > addr_lsb, t); /* synchronous? */ > } > >>>> --- >>>> mm/memory-failure.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c >>>> index a96364be8ab4..2db13d48865c 100644 >>>> --- a/mm/memory-failure.c >>>> +++ b/mm/memory-failure.c >>>> @@ -210,7 +210,7 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) >>>> { >>>> struct task_struct *t = tk->tsk; >>>> short addr_lsb = tk->size_shift; >>>> - int ret; >>>> + int ret = 0; >>>> >>>> pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n", >>>> pfn, t->comm, t->pid); >>>> @@ -225,8 +225,9 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) >>>> * This could cause a loop when the user sets SIGBUS >>>> * to SIG_IGN, but hopefully no one will do that? >>>> */ >>>> - ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, >>>> - addr_lsb, t); /* synchronous? */ >>>> + if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY)) >>>> + ret = send_sig_mceerr(BUS_MCEERR_AO, >>>> + (void __user *)tk->addr, addr_lsb, t); >>> kill_proc() could be called only for processes that are selected by >>> collect_procs() with task_early_kill(). So I think that we should fix >>> task_early_kill(), maybe by reordering sysctl_memory_failure_early_kill >>> check and find_early_kill_thread() check. >>> >>> static struct task_struct *task_early_kill(struct task_struct *tsk, >>> int force_early) >>> { >>> struct task_struct *t; >>> if (!tsk->mm) >>> return NULL; >>> if (force_early) >>> return tsk; >> The force_early is rely the flag MF_ACTION_REQUIRED, so it is always true >> when MCE occurs. >> >> This leads always sending SIGBUS to processes even if those are not current >> or no flag setting. >> >> I think it could keep the non-current processes which has no flag setting >> running. >> >> >> Besides, base on your recommendation I reorder the force_early check and >> find_early_kill_thread() >> >> check, to send the signal to the right thread. > Sorry, my previous comment around task_early_kill() is for a separate problem, > so I'll try some fix on this later. Thanks. Should me send the patch V2 for my problem alone? Or you will fix it with task_early_kill() together ？ > Thanks, > Naoya Horiguchi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process 2020-05-29 5:56 ` wetp @ 2020-05-29 6:43 ` HORIGUCHI NAOYA(堀口 直也) 0 siblings, 0 replies; 6+ messages in thread From: HORIGUCHI NAOYA(堀口 直也) @ 2020-05-29 6:43 UTC (permalink / raw) To: wetp; +Cc: n-horiguchi, akpm, linux-mm, linux-kernel On Fri, May 29, 2020 at 01:56:25PM +0800, wetp wrote: > On 2020/5/29 上午10:12, HORIGUCHI NAOYA(堀口 直也) wrote: ... > > > > > @@ -225,8 +225,9 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags) > > > > > * This could cause a loop when the user sets SIGBUS > > > > > * to SIG_IGN, but hopefully no one will do that? > > > > > */ > > > > > - ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr, > > > > > - addr_lsb, t); /* synchronous? */ > > > > > + if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY)) > > > > > + ret = send_sig_mceerr(BUS_MCEERR_AO, > > > > > + (void __user *)tk->addr, addr_lsb, t); > > > > kill_proc() could be called only for processes that are selected by > > > > collect_procs() with task_early_kill(). So I think that we should fix > > > > task_early_kill(), maybe by reordering sysctl_memory_failure_early_kill > > > > check and find_early_kill_thread() check. > > > > > > > > static struct task_struct *task_early_kill(struct task_struct *tsk, > > > > int force_early) > > > > { > > > > struct task_struct *t; > > > > if (!tsk->mm) > > > > return NULL; > > > > if (force_early) > > > > return tsk; > > > The force_early is rely the flag MF_ACTION_REQUIRED, so it is always true > > > when MCE occurs. > > > > > > This leads always sending SIGBUS to processes even if those are not current > > > or no flag setting. > > > > > > I think it could keep the non-current processes which has no flag setting > > > running. > > > > > > > > > Besides, base on your recommendation I reorder the force_early check and > > > find_early_kill_thread() > > > > > > check, to send the signal to the right thread. > > Sorry, my previous comment around task_early_kill() is for a separate problem, > > so I'll try some fix on this later. > Thanks. > > Should me send the patch V2 for my problem alone? Or you will fix it with > task_early_kill() together ？ You can send v2 if you like. Please note that patch description should be descriptive enough for future reference. I'm working on related testing now. I guess that action optional cases are not tested well enough from current process context because there's no useful injector. Thanks, Naoya Horiguchi ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-29 6:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-26 7:06 [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process Wetp Zhang 2020-05-28 2:22 ` HORIGUCHI NAOYA(堀口 直也) 2020-05-28 6:50 ` wetp 2020-05-29 2:12 ` HORIGUCHI NAOYA(堀口 直也) 2020-05-29 5:56 ` wetp 2020-05-29 6:43 ` HORIGUCHI NAOYA(堀口 直也)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).