* [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails
@ 2025-09-23 14:57 shengminghu512
2025-09-23 22:17 ` Jiaqi Yan
0 siblings, 1 reply; 4+ messages in thread
From: shengminghu512 @ 2025-09-23 14:57 UTC (permalink / raw)
To: linmiaohe, nao.horiguchi, akpm
Cc: linux-mm, linux-kernel, hu.shengming, shengminghu512, zhang.run
From: Shengming Hu <shengminghu512@qq.com>
Date: Tue, 23 Sep 2025 20:56:28 +0800
Subject: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when
unmap fails
In the memory_failure process, if collect_procs is not executed with the
flag set, the to_kill list may be empty. Even if there are pages that fail
to be unmapped, SIGKILL or SIGBUS cannot be sent to the process via
collect_procs.
This patch fixes the issue by re-executing collect_procs when the to_kill
list is empty and unmap fails. This collects processes with unmap failures
into the to_kill list, allowing SIGBUS or SIGKILL to terminate them in
subsequent code.
V2:
- Resent as plain text (previous version was HTML).
- No functional changes.
Signed-off-by: Shengming Hu <hu.shengming@zte.com.cn>
---
mm/memory-failure.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a24806bb8e82..8157823c7fb7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1600,9 +1600,12 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
unmap_success = !unmap_poisoned_folio(folio, pfn, flags & MF_MUST_KILL);
- if (!unmap_success)
+ if (!unmap_success) {
pr_err("%#lx: failed to unmap page (folio mapcount=%d)\n",
pfn, folio_mapcount(folio));
+ if (list_empty(&tokill))
+ collect_procs(folio, p, &tokill, 1);
+ }
/*
* try_to_unmap() might put mlocked page in lru cache, so call
--
2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails
2025-09-23 14:57 [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails shengminghu512
@ 2025-09-23 22:17 ` Jiaqi Yan
2025-09-24 13:33 ` =?gb18030?B?c2hlbmdtaW5naHU1MTI=?=
2025-09-24 13:55 ` [PATCH V2] mm/memory-failure: Ensure collect_procs is retriedwhen " shengminghu512
0 siblings, 2 replies; 4+ messages in thread
From: Jiaqi Yan @ 2025-09-23 22:17 UTC (permalink / raw)
To: shengminghu512
Cc: linmiaohe, nao.horiguchi, akpm, linux-mm, linux-kernel,
hu.shengming, zhang.run
On Tue, Sep 23, 2025 at 7:57 AM shengminghu512 <shengminghu512@qq.com> wrote:
>
> From: Shengming Hu <shengminghu512@qq.com>
> Date: Tue, 23 Sep 2025 20:56:28 +0800
> Subject: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when
> unmap fails
>
> In the memory_failure process, if collect_procs is not executed with the
> flag set, the to_kill list may be empty. Even if there are pages that fail
Hi Shengming,
I am trying to figure out what your code is for.
If we get into hwpoison_user_mappings with MF_ACTION_REQUIRED *not*
set in flags, force_early should not be set and it is totally valid
that collect_procs add nothing to to kill...
> to be unmapped, SIGKILL or SIGBUS cannot be sent to the process via
unless some process opt-ed in MCE_KILL_EARLY and collect_procs will
find that process if it mapped the poisoned page, regardless if
force_early is 1 or 0.
IOW I don't think there is any reason (no bug to fix and nothing to
improve) for what you are trying to do here.
> collect_procs.
>
> This patch fixes the issue by re-executing collect_procs when the to_kill
> list is empty and unmap fails. This collects processes with unmap failures
> into the to_kill list, allowing SIGBUS or SIGKILL to terminate them in
> subsequent code.
>
> V2:
> - Resent as plain text (previous version was HTML).
> - No functional changes.
>
> Signed-off-by: Shengming Hu <hu.shengming@zte.com.cn>
> ---
> mm/memory-failure.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index a24806bb8e82..8157823c7fb7 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1600,9 +1600,12 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
> collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>
> unmap_success = !unmap_poisoned_folio(folio, pfn, flags & MF_MUST_KILL);
> - if (!unmap_success)
> + if (!unmap_success) {
> pr_err("%#lx: failed to unmap page (folio mapcount=%d)\n",
> pfn, folio_mapcount(folio));
> + if (list_empty(&tokill))
> + collect_procs(folio, p, &tokill, 1);
> + }
>
> /*
> * try_to_unmap() might put mlocked page in lru cache, so call
> --
> 2.25.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails
2025-09-23 22:17 ` Jiaqi Yan
@ 2025-09-24 13:33 ` =?gb18030?B?c2hlbmdtaW5naHU1MTI=?=
2025-09-24 13:55 ` [PATCH V2] mm/memory-failure: Ensure collect_procs is retriedwhen " shengminghu512
1 sibling, 0 replies; 4+ messages in thread
From: =?gb18030?B?c2hlbmdtaW5naHU1MTI=?= @ 2025-09-24 13:33 UTC (permalink / raw)
To: =?gb18030?B?SmlhcWkgWWFu?=
Cc: =?gb18030?B?bGlubWlhb2hl?=, =?gb18030?B?bmFvLmhvcmlndWNoaQ==?=,
=?gb18030?B?YWtwbQ==?=, =?gb18030?B?bGludXgtbW0=?=,
=?gb18030?B?bGludXgta2VybmVs?=, =?gb18030?B?aHUuc2hlbmdtaW5n?=,
=?gb18030?B?emhhbmcucnVu?=
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030", Size: 5122 bytes --]
Jiaqi Yan <jiaqiyan@google.com> writes:
> On Tue, Sep 23, 2025 at 7:576§2AM shengminghu512 <shengminghu512@qq.com> wrote:
> >
> > From: Shengming Hu <shengminghu512@qq.com>
> > Date: Tue, 23 Sep 2025 20:56:28 +0800
> > Subject: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when
> > unmap fails
> >
> > In the memory_failure process, if collect_procs is not executed with the
> > flag set, the to_kill list may be empty. Even if there are pages that fail
>
> Hi Shengming,
>
> I am trying to figure out what your code is for.
>
> If we get into hwpoison_user_mappings with MF_ACTION_REQUIRED *not*
> set in flags, force_early should not be set and it is totally valid
> that collect_procs add nothing to to kill...
>
Hi Jiaqi
Thank you for the review and for the explanation. I¡¯d like to double-check
my understanding and ask for your guidance.
> > to be unmapped, SIGKILL or SIGBUS cannot be sent to the process via
>
> unless some process opt-ed in MCE_KILL_EARLY and collect_procs will
> find that process if it mapped the poisoned page, regardless if
> force_early is 1 or 0.
>
> IOW I don't think there is any reason (no bug to fix and nothing to
> improve) for what you are trying to do here.
>
Your explanation of the collect_procs collection flow was extremely detailed
and helpful. After diving deeper into the code myself, I¡¯d like to discuss
with you a few scenarios I¡¯m particularly worried about where things might
go wrong.
From reading the code, my understanding of the flow is:
- hwpoison_user_mappings handles poisoned pages in two ways:
(1) mark PTEs with hwpoison so that later accesses trigger SIGBUS, or
(2) proactively send SIGBUS/SIGKILL to terminate processes.
- The sequence is:
collect_procs -> unmap_poisoned_folio -> kill_procs
- For kill_procs to send signals, three conditions must be satisfied:
forcekill = folio_test_dirty(folio) || (flags & MF_MUST_KILL) || !unmap_success
and `tokill` must not be empty.
My concern is the following corner case:
* If unmap_poisoned_folio() fails on a poisoned page, it may not install
a hwpoison PTE entry.
* As you note If collect_procs() earlier ran without MF_ACTION_REQUIRED (and
some processes did not opt into MCE_KILL_EARLY), `tokill` can remain empty.
* In this situation, kill_procs() will not deliver any signal, and a
process can still run while using the poisoned page.
My patch retries collect_procs() (with force_early = 1) when
unmap_poisoned_folio() fails and `tokill` is empty, ensuring processes that
still hold the mapping are collected and can receive SIGBUS/SIGKILL. That
is the motivation for the change.
My question is:
Is there already a guarantee in the current design that either (a) a
hwpoison PTE entry will always be installed, or (b) a process will always
be collected into `tokill` in this unmap failure case?
If such a guarantee exists, I may have misunderstood the intended flow ¡ª
could you help clarify where that happens in the code? If not, does my
approach of retrying collect_procs make sense?
Thanks a lot for the feedback and guidance ¡ª I¡¯d like to align with the
intended semantics and update the patch accordingly.
> > collect_procs.
> >
> > This patch fixes the issue by re-executing collect_procs when the to_kill
> > list is empty and unmap fails. This collects processes with unmap failures
> > into the to_kill list, allowing SIGBUS or SIGKILL to terminate them in
> > subsequent code.
> >
> > V2:
> > - Resent as plain text (previous version was HTML).
> > - No functional changes.
> >
> > Signed-off-by: Shengming Hu <hu.shengming@zte.com.cn>
> > ---
> > mm/memory-failure.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index a24806bb8e82..8157823c7fb7 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -1600,9 +1600,12 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
> > collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
> >
> > unmap_success = !unmap_poisoned_folio(folio, pfn, flags & MF_MUST_KILL);
> > - if (!unmap_success)
> > + if (!unmap_success) {
> > pr_err("%#lx: failed to unmap page (folio mapcount=%d)\n",
> > pfn, folio_mapcount(folio));
> > + if (list_empty(&tokill))
> > + collect_procs(folio, p, &tokill, 1);
> > + }
> >
> > /*
> > * try_to_unmap() might put mlocked page in lru cache, so call
> > --
> > 2.25.1
Best regards,
Shengming Hu</hu.shengming@zte.com.cn></shengminghu512@qq.com></shengminghu512@qq.com></jiaqiyan@google.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] mm/memory-failure: Ensure collect_procs is retriedwhen unmap fails
2025-09-23 22:17 ` Jiaqi Yan
2025-09-24 13:33 ` =?gb18030?B?c2hlbmdtaW5naHU1MTI=?=
@ 2025-09-24 13:55 ` shengminghu512
1 sibling, 0 replies; 4+ messages in thread
From: shengminghu512 @ 2025-09-24 13:55 UTC (permalink / raw)
To: Jiaqi Yan
Cc: linmiaohe, nao.horiguchi, akpm, linux-mm, linux-kernel,
hu.shengming, zhang.run
Jiaqi Yan <jiaqiyan@google.com> writes:
> On Tue, Sep 23, 2025 at 7:57 AM shengminghu512 <shengminghu512@qq.com> wrote:
> >
> > From: Shengming Hu <shengminghu512@qq.com>
> > Date: Tue, 23 Sep 2025 20:56:28 +0800
> > Subject: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when
> > unmap fails
> >
> > In the memory_failure process, if collect_procs is not executed with the
> > flag set, the to_kill list may be empty. Even if there are pages that fail
>
> Hi Shengming,
>
> I am trying to figure out what your code is for.
>
> If we get into hwpoison_user_mappings with MF_ACTION_REQUIRED *not*
> set in flags, force_early should not be set and it is totally valid
> that collect_procs add nothing to to kill...
>
Hi Jiaqi
Thank you for the review and for the explanation. I’d like to double-check
my understanding and ask for your guidance.
> > to be unmapped, SIGKILL or SIGBUS cannot be sent to the process via
>
> unless some process opt-ed in MCE_KILL_EARLY and collect_procs will
> find that process if it mapped the poisoned page, regardless if
> force_early is 1 or 0.
>
> IOW I don't think there is any reason (no bug to fix and nothing to
> improve) for what you are trying to do here.
>
Your explanation of the collect_procs collection flow was extremely detailed
and helpful. After diving deeper into the code myself, I’d like to discuss
with you a few scenarios I’m particularly worried about where things might
go wrong.
From reading the code, my understanding of the flow is:
- hwpoison_user_mappings handles poisoned pages in two ways:
(1) mark PTEs with hwpoison so that later accesses trigger SIGBUS, or
(2) proactively send SIGBUS/SIGKILL to terminate processes.
- The sequence is:
collect_procs -> unmap_poisoned_folio -> kill_procs
- For kill_procs to send signals, three conditions must be satisfied:
forcekill = folio_test_dirty(folio) || (flags & MF_MUST_KILL) || !unmap_success
and `tokill` must not be empty.
My concern is the following corner case:
* If unmap_poisoned_folio() fails on a poisoned page, it may not install
a hwpoison PTE entry.
* As you note If collect_procs() earlier ran without MF_ACTION_REQUIRED (and
some processes did not opt into MCE_KILL_EARLY), `tokill` can remain empty.
* In this situation, kill_procs() will not deliver any signal, and a
process can still run while using the poisoned page.
My patch retries collect_procs() (with force_early = 1) when
unmap_poisoned_folio() fails and `tokill` is empty, ensuring processes that
still hold the mapping are collected and can receive SIGBUS/SIGKILL. That
is the motivation for the change.
My question is:
Is there already a guarantee in the current design that either (a) a
hwpoison PTE entry will always be installed, or (b) a process will always
be collected into `tokill` in this unmap failure case?
If such a guarantee exists, I may have misunderstood the intended flow —
could you help clarify where that happens in the code? If not, does my
approach of retrying collect_procs make sense?
Thanks a lot for the feedback and guidance — I’d like to align with the
intended semantics and update the patch accordingly.
> > collect_procs.
> >
> > This patch fixes the issue by re-executing collect_procs when the to_kill
> > list is empty and unmap fails. This collects processes with unmap failures
> > into the to_kill list, allowing SIGBUS or SIGKILL to terminate them in
> > subsequent code.
> >
> > V2:
> > - Resent as plain text (previous version was HTML).
> > - No functional changes.
> >
> > Signed-off-by: Shengming Hu <hu.shengming@zte.com.cn>
> > ---
> > mm/memory-failure.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index a24806bb8e82..8157823c7fb7 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -1600,9 +1600,12 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
> > collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
> >
> > unmap_success = !unmap_poisoned_folio(folio, pfn, flags & MF_MUST_KILL);
> > - if (!unmap_success)
> > + if (!unmap_success) {
> > pr_err("%#lx: failed to unmap page (folio mapcount=%d)\n",
> > pfn, folio_mapcount(folio));
> > + if (list_empty(&tokill))
> > + collect_procs(folio, p, &tokill, 1);
> > + }
> >
> > /*
> > * try_to_unmap() might put mlocked page in lru cache, so call
> > --
> > 2.25.1
Best regards,
Shengming Hu
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-24 14:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-09-23 14:57 [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails shengminghu512
2025-09-23 22:17 ` Jiaqi Yan
2025-09-24 13:33 ` =?gb18030?B?c2hlbmdtaW5naHU1MTI=?=
2025-09-24 13:55 ` [PATCH V2] mm/memory-failure: Ensure collect_procs is retriedwhen " shengminghu512
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox