Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process
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
Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process
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
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
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
Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process
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
> > >
> > > 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
> > 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
Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process
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
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
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
Re: [PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process
Hi Zhang,
Sorry for my late response.
On Tue, May 26, 2020 at 03:06:41PM +0800, Wetp Zhang wrote:
> From: Zhang Yi
>
> 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
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
[PATCH] mm, memory_failure: only send BUS_MCEERR_AO to early-kill process
From: Zhang Yi
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
---
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)
