lkml.org 
[lkml]   [2024]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mm,swapops: Update check in is_pfn_swap_entry for hwpoison entries
    On Sun, Apr 07, 2024 at 03:56:17PM +0200, Oscar Salvador wrote:
    > On Sun, Apr 07, 2024 at 03:05:37PM +0200, Oscar Salvador wrote:
    > > Tony reported that the Machine check recovery was broken in v6.9-rc1,
    > > as he was hitting a VM_BUG_ON when injecting uncorrectable memory errors
    > > to DRAM.
    > > After some more digging and debugging on his side, he realized that this
    > > went back to v6.1, with the introduction of 'commit 0d206b5d2e0d ("mm/swap: add
    > > swp_offset_pfn() to fetch PFN from swap entry")'.
    > > That commit, among other things, introduced swp_offset_pfn(), replacing
    > > hwpoison_entry_to_pfn() in its favour.
    > >
    > > The patch also introduced a VM_BUG_ON() check for is_pfn_swap_entry(),
    > > but is_pfn_swap_entry() never got updated to cover hwpoison entries, which
    > > means that we would hit the VM_BUG_ON whenever we would call
    > > swp_offset_pfn() for such entries on environments with CONFIG_DEBUG_VM set.
    > > Fix this by updating the check to cover hwpoison entries as well, and update
    > > the comment while we are it.
    > >
    > > Reported-by: Tony Luck <tony.luck@intel.com>
    > > Closes: https://lore.kernel.org/all/Zg8kLSl2yAlA3o5D@agluck-desk3/
    > > Tested-by: Tony Luck <tony.luck@intel.com>
    > > Fixes: 0d206b5d2e0d ("mm/swap: add swp_offset_pfn() to fetch PFN from swap entry")

    Totally unexpected, as this commit even removed hwpoison_entry_to_pfn().
    Obviously even until now I assumed hwpoison is accounted as pfn swap entry
    but it's just missing..

    Since this commit didn't really change is_pfn_swap_entry() itself, I was
    thinking maybe an older fix tag would apply, but then I noticed the old
    code indeed should work well even if hwpoison entry is missing. For
    example, it's a grey area on whether a hwpoisoned page should be accounted
    in smaps. So I think the Fixes tag is correct, and thanks for fixing this.

    Reviewed-by: Peter Xu <peterx@redhat.com>

    > > Cc: <stable@vger.kernel.org> # 6.1.x
    >
    > I think I need to clarify why the stable.
    >
    > It is my understanding that some distros ship their kernel with
    > CONFIG_DEBUG_VM set by default (I think Fedora comes to my mind?).
    > I am fine with backing down if people think that this is an
    > overreaction.

    Fedora stopped having DEBUG_VM for some time, but not sure about when it's
    still in the 6.1 trees. It looks like cc stable is still reasonable from
    that regard.

    A side note is that when I'm looking at this, I went back and see why in
    some cases we need the pfn maintained for the poisoned, then I saw the only
    user is check_hwpoisoned_entry() who wants to do fast kills in some
    contexts and that includes a double check on the pfns in a poisoned entry.
    Then afaict this path is just too rarely used and buggy.

    A few things we may need fixing, maybe someone in the loop would have time
    to have a look:

    - check_hwpoisoned_entry()
    - pte_none check is missing
    - all the rest swap types are missing (e.g., we want to kill the proc too
    if the page is during migration)
    - check_hwpoisoned_pmd_entry()
    - need similar care like above (pmd_none is covered not others)

    I copied Naoya too.

    Thanks,

    --
    Peter Xu


    \
     
     \ /
      Last update: 2024-05-27 16:27    [W:4.790 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site