Message-ID: <tencent_E424DB6EA2E9CFD8CD43EE3596DC69506009@qq.com>
Date: Wed, 24 Sep 2025 21:55:38 +0800
From: "shengminghu512" <shengminghu512@...com>
To: "Jiaqi Yan" <jiaqiyan@...gle.com>
Cc: "linmiaohe" <linmiaohe@...wei.com>, "nao.horiguchi" <nao.horiguchi@...il.com>, "akpm" <akpm@...ux-foundation.org>, "linux-mm" <linux-mm@...ck.org>, "linux-kernel" <linux-kernel@...r.kernel.org>, "hu.shengming" <hu.shengming@....com.cn>, "zhang.run" <zhang.run@....com.cn>
Subject: Re: [PATCH V2] mm/memory-failure: Ensure collect_procs is retriedwhen unmap fails

Jiaqi Yan <jiaqiyan@...gle.com> writes:
> On Tue, Sep 23, 2025 at 7:57 AM shengminghu512 <shengminghu512@...com> wrote:
> >
> > From: Shengming Hu <shengminghu512@...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@....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

Powered by blists - more mailing lists