Re: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when unmap fails

[=?gb18030?B?c2hlbmdtaW5naHU1MTI=?= <shengminghu512@qq.com>]

[-- 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:
&gt; On Tue, Sep 23, 2025 at 7:576§2AM shengminghu512 <shengminghu512@qq.com> wrote:
&gt; &gt;
&gt; &gt; From: Shengming Hu <shengminghu512@qq.com>
&gt; &gt; Date: Tue, 23 Sep 2025 20:56:28 +0800
&gt; &gt; Subject: [PATCH V2] mm/memory-failure: Ensure collect_procs is retried when
&gt; &gt;  unmap fails
&gt; &gt;
&gt; &gt; In the memory_failure process, if collect_procs is not executed with the
&gt; &gt; flag set, the to_kill list may be empty. Even if there are pages that fail
&gt; 
&gt; Hi Shengming,
&gt; 
&gt; I am trying to figure out what your code is for.
&gt; 
&gt; If we get into hwpoison_user_mappings with MF_ACTION_REQUIRED *not*
&gt; set in flags, force_early should not be set and it is totally valid
&gt; that collect_procs add nothing to to kill...
&gt; 

Hi Jiaqi

Thank you for the review and for the explanation. I¡¯d like to double-check 
my understanding and ask for your guidance.

&gt; &gt; to be unmapped, SIGKILL or SIGBUS cannot be sent to the process via
&gt; 
&gt; unless some process opt-ed in MCE_KILL_EARLY and collect_procs will
&gt; find that process if it mapped the poisoned page, regardless if
&gt; force_early is 1 or 0.
&gt; 
&gt; IOW I don't think there is any reason (no bug to fix and nothing to
&gt; improve) for what you are trying to do here.
&gt; 

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 -&gt; unmap_poisoned_folio -&gt; kill_procs

  - For kill_procs to send signals, three conditions must be satisfied:  
        forcekill = folio_test_dirty(folio) || (flags &amp; 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.

&gt; &gt; collect_procs.
&gt; &gt;
&gt; &gt; This patch fixes the issue by re-executing collect_procs when the to_kill
&gt; &gt; list is empty and unmap fails. This collects processes with unmap failures
&gt; &gt; into the to_kill list, allowing SIGBUS or SIGKILL to terminate them in
&gt; &gt; subsequent code.
&gt; &gt;
&gt; &gt; V2:
&gt; &gt;   - Resent as plain text (previous version was HTML).
&gt; &gt;   - No functional changes.
&gt; &gt;
&gt; &gt; Signed-off-by: Shengming Hu <hu.shengming@zte.com.cn>
&gt; &gt; ---
&gt; &gt;  mm/memory-failure.c | 5 ++++-
&gt; &gt;  1 file changed, 4 insertions(+), 1 deletion(-)
&gt; &gt;
&gt; &gt; diff --git a/mm/memory-failure.c b/mm/memory-failure.c
&gt; &gt; index a24806bb8e82..8157823c7fb7 100644
&gt; &gt; --- a/mm/memory-failure.c
&gt; &gt; +++ b/mm/memory-failure.c
&gt; &gt; @@ -1600,9 +1600,12 @@ static bool hwpoison_user_mappings(struct folio *folio, struct page *p,
&gt; &gt;         collect_procs(folio, p, &amp;tokill, flags &amp; MF_ACTION_REQUIRED);
&gt; &gt;
&gt; &gt;         unmap_success = !unmap_poisoned_folio(folio, pfn, flags &amp; MF_MUST_KILL);
&gt; &gt; -       if (!unmap_success)
&gt; &gt; +       if (!unmap_success) {
&gt; &gt;                 pr_err("%#lx: failed to unmap page (folio mapcount=%d)\n",
&gt; &gt;                        pfn, folio_mapcount(folio));
&gt; &gt; +               if (list_empty(&amp;tokill))
&gt; &gt; +                       collect_procs(folio, p, &amp;tokill, 1);
&gt; &gt; +       }
&gt; &gt;
&gt; &gt;         /*
&gt; &gt;          * try_to_unmap() might put mlocked page in lru cache, so call
&gt; &gt; --
&gt; &gt; 2.25.1

Best regards,  
Shengming Hu</hu.shengming@zte.com.cn></shengminghu512@qq.com></shengminghu512@qq.com></jiaqiyan@google.com>