linux-kernel - Re: [PATCH v4 1/2] mm/memory-failure: fix missing ->mf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260114153749.3004663-1-clm@meta.com>
Date: Wed, 14 Jan 2026 07:37:47 -0800
From: Chris Mason <clm@...a.com>
To: Jane Chu <jane.chu@...cle.com>
CC: Chris Mason <clm@...a.com>, <linux-kernel@...r.kernel.org>,
        <linux-mm@...ck.org>, <stable@...r.kernel.org>,
        <muchun.song@...ux.dev>, <osalvador@...e.de>, <david@...nel.org>,
        <linmiaohe@...wei.com>, <jiaqiyan@...gle.com>,
        <william.roche@...cle.com>, <rientjes@...gle.com>,
        <akpm@...ux-foundation.org>, <lorenzo.stoakes@...cle.com>,
        <Liam.Howlett@...cle.com>, <rppt@...nel.org>, <surenb@...gle.com>,
        <mhocko@...e.com>, <willy@...radead.org>
Subject: Re: [PATCH v4 1/2] mm/memory-failure: fix missing ->mf_stats count in hugetlb poison

On Tue, 13 Jan 2026 01:07:50 -0700 Jane Chu <jane.chu@...cle.com> wrote:

> When a newly poisoned subpage ends up in an already poisoned hugetlb
> folio, 'num_poisoned_pages' is incremented, but the per node ->mf_stats
> is not. Fix the inconsistency by designating action_result() to update
> them both.
> 
> While at it, define __get_huge_page_for_hwpoison() return values in terms
> of symbol names for better readibility. Also rename
> folio_set_hugetlb_hwpoison() to hugetlb_update_hwpoison() since the
> function does more than the conventional bit setting and the fact
> three possible return values are expected.
> 

Hi everyone,

I ran mm-new through the AI patch review prompts, and this commit was flagged.
The review below looks right to me:

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c

[ ... ]

> @@ -2029,22 +2037,29 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
>  	*hugetlb = 1;
>  retry:
>  	res = get_huge_page_for_hwpoison(pfn, flags, &migratable_cleared);
> -	if (res == 2) { /* fallback to normal page handling */
> +	switch (res) {
> +	case -EINVAL:	/* fallback to normal page handling */
>  		*hugetlb = 0;
>  		return 0;
> -	} else if (res == -EHWPOISON) {
> -		if (flags & MF_ACTION_REQUIRED) {
> -			folio = page_folio(p);
> -			res = kill_accessing_process(current, folio_pfn(folio), flags);
> -		}
> -		action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
> -		return res;
> -	} else if (res == -EBUSY) {
> +	case -EBUSY:
>  		if (!(flags & MF_NO_RETRY)) {
>  			flags |= MF_NO_RETRY;
>  			goto retry;
>  		}
>  		return action_result(pfn, MF_MSG_GET_HWPOISON, MF_IGNORED);
> +	case MF_HUGETLB_FOLIO_PRE_POISONED:
> +	case MF_HUGETLB_PAGE_PRE_POISON:
> +		if (flags & MF_ACTION_REQUIRED) {
> +			folio = page_folio(p);
> +			res = kill_accessing_process(current, folio_pfn(folio), flags);
> +		}
> +		if (res == MF_HUGETLB_FOLIO_PRE_POISONED)

When MF_ACTION_REQUIRED is set, kill_accessing_process() overwrites res
before this check. kill_accessing_process() returns either 0 or
-EHWPOISON, so the check for MF_HUGETLB_FOLIO_PRE_POISONED (which is 3)
will always be false in that path.

This means action_result() will be called with MF_MSG_HUGE instead of
MF_MSG_ALREADY_POISONED. Looking at action_result():

    if (type != MF_MSG_ALREADY_POISONED && type != MF_MSG_PFN_MAP) {
        num_poisoned_pages_inc(pfn);
        update_per_node_mf_stats(pfn, result);
    }

Does this cause incorrect counter increments for already-poisoned pages
when MF_ACTION_REQUIRED is set? The original code called action_result()
unconditionally with MF_MSG_ALREADY_POISONED regardless of whether
kill_accessing_process() was called.

Should a separate variable preserve the original res value before the
call to kill_accessing_process()?

> +			action_result(pfn, MF_MSG_ALREADY_POISONED, MF_FAILED);
> +		else
> +			action_result(pfn, MF_MSG_HUGE, MF_FAILED);
> +		return res;