[PATCH v1 0/2] hwpoison: fixes signaling on memory error

[PATCH v1 0/2] hwpoison: fixes signaling on memory error

[Naoya Horiguchi]

Hi,

This is a small patchset to solve issues in memory error handler to
send SIGBUS to proper process/thread as expected in configuration.
Please see descriptions in individual patches for more details.

This patchset depends on commit 872e9a205c84 ("mm, memory_failure: don't send
BUS_MCEERR_AO for action required error") currently in upstream.

Thanks,
Naoya Horiguchi
---
Summary:

Naoya Horiguchi (2):
      mm/memory-failure: prioritize prctl(PR_MCE_KILL) over vm.memory_failure_early_kill
      mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread

 mm/memory-failure.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

[PATCH v1 1/2] mm/memory-failure: prioritize prctl(PR_MCE_KILL) over vm.memory_failure_early_kill

[Naoya Horiguchi]

Early-kill policy is controlled from two types of settings, one is
per-process setting prctl(PR_MCE_KILL) and the other is system-wide setting
vm.memory_failure_early_kill. Users expect per-process setting to override
system-wide setting as many other settings do, but early-kill setting doesn't
work as such. For example, if a system configures
vm.memory_failure_early_kill to 1 (enabled), a process receives SIGBUS even
if it's configured to explicitly disable PF_MCE_KILL by prctl(). That's not
desirable for applications with their own policies.

This patch is suggesting to change the priority of these two types of settings,
by checking sysctl_memory_failure_early_kill only when a given process has the
default kill policy.

Note that this patch is solving a thread choice issue too. Originally,
collect_procs() always chooses the main thread when
vm.memory_failure_early_kill is 1, even if the process has a dedicated
thread for memory error handling.  SIGBUS should be sent to the dedicated
thread if early-kill is enabled via vm.memory_failure_early_kill as we
are doing for PR_MCE_KILL_EARLY processes.

Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git v5.7/mm/memory-failure.c v5.7_patched/mm/memory-failure.c
index dd3862f..339c07d 100644
--- v5.7/mm/memory-failure.c
+++ v5.7_patched/mm/memory-failure.c
@@ -402,9 +402,15 @@ static struct task_struct *find_early_kill_thread(struct task_struct *tsk)
 {
 	struct task_struct *t;
 
-	for_each_thread(tsk, t)
-		if ((t->flags & PF_MCE_PROCESS) && (t->flags & PF_MCE_EARLY))
-			return t;
+	for_each_thread(tsk, t) {
+		if (t->flags & PF_MCE_PROCESS) {
+			if (t->flags & PF_MCE_EARLY)
+				return t;
+		} else {
+			if (sysctl_memory_failure_early_kill)
+				return t;
+		}
+	}
 	return NULL;
 }
 
@@ -417,17 +423,11 @@ static struct task_struct *find_early_kill_thread(struct task_struct *tsk)
 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;
+	return find_early_kill_thread(tsk);
 }
 
 /*
-- 
2.7.0

[PATCH v1 2/2] mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread

[Naoya Horiguchi]

Action Required memory error should happen only when a processor is
about to access to a corrupted memory, so it's synchronous and only
affects current process/thread.  Recently commit 872e9a205c84 ("mm,
memory_failure: don't send BUS_MCEERR_AO for action required error")
fixed the issue that Action Required memory could unnecessarily send
SIGBUS to the processes which share the error memory. But we still have
another issue that we could send SIGBUS to a wrong thread.

This is because collect_procs() and task_early_kill() fails to add the
current process to "to-kill" list.  So this patch is suggesting to fix
it.  With this fix, SIGBUS(BUS_MCEERR_AR) is never sent to non-current
process/thread.

Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git v5.7/mm/memory-failure.c v5.7_patched/mm/memory-failure.c
index 339c07d..fa4f9cd 100644
--- v5.7/mm/memory-failure.c
+++ v5.7_patched/mm/memory-failure.c
@@ -212,15 +212,13 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
 	short addr_lsb = tk->size_shift;
 	int ret = 0;
 
-	if ((t->mm == current->mm) || !(flags & MF_ACTION_REQUIRED))
-		pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
+	pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
 			pfn, t->comm, t->pid);
 
 	if (flags & MF_ACTION_REQUIRED) {
-		if (t->mm == current->mm)
-			ret = force_sig_mceerr(BUS_MCEERR_AR,
+		WARN_ON_ONCE(t != current);
+		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
@@ -419,14 +417,25 @@ static struct task_struct *find_early_kill_thread(struct task_struct *tsk)
  * to be signaled when some page under the process is hwpoisoned.
  * Return task_struct of the dedicated thread (main thread unless explicitly
  * specified) if the process is "early kill," and otherwise returns NULL.
+ *
+ * Note that the above is true for Action Optional case, but not for Action
+ * Required case where SIGBUS should sent only to the current thread.
  */
 static struct task_struct *task_early_kill(struct task_struct *tsk,
 					   int force_early)
 {
 	if (!tsk->mm)
 		return NULL;
-	if (force_early)
-		return tsk;
+	if (force_early) {
+		/*
+		 * Comparing ->mm here because current task might represent
+		 * a subthread, while tsk always points to the main thread.
+		 */
+		if (tsk->mm == current->mm)
+			return current;
+		else
+			return NULL;
+	}
 	return find_early_kill_thread(tsk);
 }
 
-- 
2.7.0

Re: [PATCH v1 0/2] hwpoison: fixes signaling on memory error

[HORIGUCHI NAOYA(堀口 直也)]

Sorry, recipients are not enough. I meant to send/CC this to reviewers.

https://lore.kernel.org/linux-mm/1591321039-22141-1-git-send-email-naoya.horiguchi@nec.com/T/#t

Any comment is welcomed.

Thanks,
Naoya Horiguchi

On Fri, Jun 05, 2020 at 10:37:17AM +0900, Naoya Horiguchi wrote:
> Hi,
> 
> This is a small patchset to solve issues in memory error handler to
> send SIGBUS to proper process/thread as expected in configuration.
> Please see descriptions in individual patches for more details.
> 
> This patchset depends on commit 872e9a205c84 ("mm, memory_failure: don't send
> BUS_MCEERR_AO for action required error") currently in upstream.
> 
> Thanks,
> Naoya Horiguchi
> ---
> Summary:
> 
> Naoya Horiguchi (2):
>       mm/memory-failure: prioritize prctl(PR_MCE_KILL) over vm.memory_failure_early_kill
>       mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread
> 
>  mm/memory-failure.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 

Re: [PATCH v1 0/2] hwpoison: fixes signaling on memory error

[Andrew Morton]

On Fri, 5 Jun 2020 01:46:46 +0000 HORIGUCHI NAOYA(堀口　直也) <naoya.horiguchi@nec.com> wrote:

> Sorry, recipients are not enough. I meant to send/CC this to reviewers.
> 
> https://lore.kernel.org/linux-mm/1591321039-22141-1-git-send-email-naoya.horiguchi@nec.com/T/#t
> 
> Any comment is welcomed.

How serious are these issues?  Do you think serious enough to justify
adding these to 5.8?

Re: [PATCH v1 0/2] hwpoison: fixes signaling on memory error

[HORIGUCHI NAOYA(堀口 直也)]

On Sun, Jun 07, 2020 at 05:53:18PM -0700, Andrew Morton wrote:
> On Fri, 5 Jun 2020 01:46:46 +0000 HORIGUCHI NAOYA(堀口　直也) <naoya.horiguchi@nec.com> wrote:
> 
> > Sorry, recipients are not enough. I meant to send/CC this to reviewers.
> > 
> > https://lore.kernel.org/linux-mm/1591321039-22141-1-git-send-email-naoya.horiguchi@nec.com/T/#t
> > 
> > Any comment is welcomed.
> 
> How serious are these issues?  Do you think serious enough to justify
> adding these to 5.8?

The issue of 1/2 is not serious. But the issue of 2/2 is a little severer,
where we could wrongly kill the main thread when the subthread access to
the error in multithread applications. So I want it to be merged for 5.8.

Thanks,
Naoya Horiguchi

Re: [PATCH v1 2/2] mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread

[Luck, Tony]

On Fri, Jun 05, 2020 at 10:37:19AM +0900, Naoya Horiguchi wrote:
> Action Required memory error should happen only when a processor is
> about to access to a corrupted memory, so it's synchronous and only
> affects current process/thread.  Recently commit 872e9a205c84 ("mm,
> memory_failure: don't send BUS_MCEERR_AO for action required error")
> fixed the issue that Action Required memory could unnecessarily send
> SIGBUS to the processes which share the error memory. But we still have
> another issue that we could send SIGBUS to a wrong thread.
> 
> This is because collect_procs() and task_early_kill() fails to add the
> current process to "to-kill" list.  So this patch is suggesting to fix
> it.  With this fix, SIGBUS(BUS_MCEERR_AR) is never sent to non-current
> process/thread.

Does the new code now send SIGBUS(BUS_MCEERR_AO) to all the other threads
of a multi-threaded process?

It looks like it might (and I don't have some handy multi-threaded test
case to try it out).

If it does, is that what we want?

-Tony

Re: [PATCH v1 2/2] mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread

[HORIGUCHI NAOYA(堀口 直也)]

On Mon, Jun 08, 2020 at 03:17:59PM -0700, Luck, Tony wrote:
> On Fri, Jun 05, 2020 at 10:37:19AM +0900, Naoya Horiguchi wrote:
> > Action Required memory error should happen only when a processor is
> > about to access to a corrupted memory, so it's synchronous and only
> > affects current process/thread.  Recently commit 872e9a205c84 ("mm,
> > memory_failure: don't send BUS_MCEERR_AO for action required error")
> > fixed the issue that Action Required memory could unnecessarily send
> > SIGBUS to the processes which share the error memory. But we still have
> > another issue that we could send SIGBUS to a wrong thread.
> > 
> > This is because collect_procs() and task_early_kill() fails to add the
> > current process to "to-kill" list.  So this patch is suggesting to fix
> > it.  With this fix, SIGBUS(BUS_MCEERR_AR) is never sent to non-current
> > process/thread.
> 
> Does the new code now send SIGBUS(BUS_MCEERR_AO) to all the other threads
> of a multi-threaded process?

No, it doesn't. This patch should not change anything for Action Optional
case, and find_early_kill_thread() chooses one thread per process, so
SIGBUS(BUS_MCEERR_AO) (as well as SIGBUS(BUS_MCEERR_AR)) should be sent only
to the chosen thread.

- Naoya

> 
> It looks like it might (and I don't have some handy multi-threaded test
> case to try it out).
> 
> If it does, is that what we want?
> 
> -Tony
> 

RE: [PATCH v1 2/2] mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread

[Luck, Tony]

>> Does the new code now send SIGBUS(BUS_MCEERR_AO) to all the other threads
>> of a multi-threaded process?
>
> No, it doesn't. This patch should not change anything for Action Optional
> case, and find_early_kill_thread() chooses one thread per process, so
> SIGBUS(BUS_MCEERR_AO) (as well as SIGBUS(BUS_MCEERR_AR)) should be sent only
> to the chosen thread.

Ok. I think I see it now.

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

-Tony

Re: [PATCH v1 2/2] mm/memory-failure: send SIGBUS(BUS_MCEERR_AR) only to current thread

[Pankaj Gupta]

> Action Required memory error should happen only when a processor is
> about to access to a corrupted memory, so it's synchronous and only
> affects current process/thread.  Recently commit 872e9a205c84 ("mm,
> memory_failure: don't send BUS_MCEERR_AO for action required error")
> fixed the issue that Action Required memory could unnecessarily send
> SIGBUS to the processes which share the error memory. But we still have
> another issue that we could send SIGBUS to a wrong thread.
>
> This is because collect_procs() and task_early_kill() fails to add the
> current process to "to-kill" list.  So this patch is suggesting to fix
> it.  With this fix, SIGBUS(BUS_MCEERR_AR) is never sent to non-current
> process/thread.
>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  mm/memory-failure.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git v5.7/mm/memory-failure.c v5.7_patched/mm/memory-failure.c
> index 339c07d..fa4f9cd 100644
> --- v5.7/mm/memory-failure.c
> +++ v5.7_patched/mm/memory-failure.c
> @@ -212,15 +212,13 @@ static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
>         short addr_lsb = tk->size_shift;
>         int ret = 0;
>
> -       if ((t->mm == current->mm) || !(flags & MF_ACTION_REQUIRED))
> -               pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
> +       pr_err("Memory failure: %#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
>                         pfn, t->comm, t->pid);
>
>         if (flags & MF_ACTION_REQUIRED) {
> -               if (t->mm == current->mm)
> -                       ret = force_sig_mceerr(BUS_MCEERR_AR,
> +               WARN_ON_ONCE(t != current);
> +               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
> @@ -419,14 +417,25 @@ static struct task_struct *find_early_kill_thread(struct task_struct *tsk)
>   * to be signaled when some page under the process is hwpoisoned.
>   * Return task_struct of the dedicated thread (main thread unless explicitly
>   * specified) if the process is "early kill," and otherwise returns NULL.
> + *
> + * Note that the above is true for Action Optional case, but not for Action
> + * Required case where SIGBUS should sent only to the current thread.
>   */
>  static struct task_struct *task_early_kill(struct task_struct *tsk,
>                                            int force_early)
>  {
>         if (!tsk->mm)
>                 return NULL;
> -       if (force_early)
> -               return tsk;
> +       if (force_early) {
> +               /*
> +                * Comparing ->mm here because current task might represent
> +                * a subthread, while tsk always points to the main thread.
> +                */
> +               if (tsk->mm == current->mm)
> +                       return current;
> +               else
> +                       return NULL;
> +       }
>         return find_early_kill_thread(tsk);
>  }
>
Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com>