Re: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails
From: =?gb18030?b?c2hlbmdtaW5naHU1MTI=?=
Date: Wed Sep 24 2025 - 09:39:56 EST
Jiaqi Yan <jiaqiyan@xxxxxxxxxx> writes:
> On Tue, Sep 23, 2025 at 7:57 AM shengminghu512 <shengminghu512@xxxxxx> wrote:
> >
> > From: Shengming Hu <shengminghu512@xxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx></shengminghu512@xxxxxx></shengminghu512@xxxxxx></jiaqiyan@xxxxxxxxxx>