| 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
| ||
|
Message-ID: <f2ec1578-bdc7-414c-9d6c-ae8fbd7cd54f@oracle.com>
Date: Tue, 23 Dec 2025 10:28:13 -0800
From: jane.chu@...cle.com
To: "David Hildenbrand (Red Hat)" <david@...nel.org>,
linux-kernel@...r.kernel.org
Cc: linux-mm@...ck.org, stable@...r.kernel.org, muchun.song@...ux.dev,
osalvador@...e.de, 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 v3 1/2] mm/memory-failure: fix missing ->mf_stats count in
hugetlb poison
On 12/23/2025 12:50 AM, David Hildenbrand (Red Hat) wrote:
> On 12/23/25 02:21, Jane Chu 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.
>>
>> Fixes: 18f41fa616ee4 ("mm: memory-failure: bump memory failure stats
>> to pglist_data")
>> Cc: <stable@...r.kernel.org>
>> Signed-off-by: Jane Chu <jane.chu@...cle.com>
>> ---
>> v2 -> v3:
>> No change.
>> v1 -> v2:
>> adapted David and Liam's comment, define
>> __get_huge_page_for_hwpoison()
>> return values in terms of symbol names instead of naked integers for
>> better
>> readibility. #define instead of enum is used since the function has
>> footprint
>> outside MF, just try to limit the MF specifics local.
>> also renamed 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.
>>
>> ---
>> mm/memory-failure.c | 56 ++++++++++++++++++++++++++-------------------
>> 1 file changed, 33 insertions(+), 23 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index fbc5a01260c8..8b47e8a1b12d 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1883,12 +1883,18 @@ static unsigned long
>> __folio_free_raw_hwp(struct folio *folio, bool move_flag)
>> return count;
>> }
>> -static int folio_set_hugetlb_hwpoison(struct folio *folio, struct
>> page *page)
>> +#define MF_HUGETLB_ALREADY_POISONED 3 /* already poisoned */
>> +#define MF_HUGETLB_ACC_EXISTING_POISON 4 /* accessed existing
>> poisoned page */
>
> What happened to the idea of using an enum?
I briefly mentioned the reason of using #define instead of enum in the
v1 -> v2 comment, more below.
>
>
>> +/*
>> + * Set hugetlb folio as hwpoisoned, update folio private raw hwpoison
>> list
>> + * to keep track of the poisoned pages.
>> + */
>> +static int hugetlb_update_hwpoison(struct folio *folio, struct page
>> *page)
>> {
>> struct llist_head *head;
>> struct raw_hwp_page *raw_hwp;
>> struct raw_hwp_page *p;
>> - int ret = folio_test_set_hwpoison(folio) ? -EHWPOISON : 0;
>> + int ret = folio_test_set_hwpoison(folio) ?
>> MF_HUGETLB_ALREADY_POISONED : 0;
>> /*
>> * Once the hwpoison hugepage has lost reliable raw error info,
>> @@ -1896,20 +1902,18 @@ static int folio_set_hugetlb_hwpoison(struct
>> folio *folio, struct page *page)
>> * so skip to add additional raw error info.
>> */
>> if (folio_test_hugetlb_raw_hwp_unreliable(folio))
>> - return -EHWPOISON;
>> + return MF_HUGETLB_ALREADY_POISONED;
>> +
>> head = raw_hwp_list_head(folio);
>> llist_for_each_entry(p, head->first, node) {
>> if (p->page == page)
>> - return -EHWPOISON;
>> + return MF_HUGETLB_ACC_EXISTING_POISON;
>> }
>> raw_hwp = kmalloc(sizeof(struct raw_hwp_page), GFP_ATOMIC);
>> if (raw_hwp) {
>> raw_hwp->page = page;
>> llist_add(&raw_hwp->node, head);
>> - /* the first error event will be counted in action_result(). */
>> - if (ret)
>> - num_poisoned_pages_inc(page_to_pfn(page));
>> } else {
>> /*
>> * Failed to save raw error info. We no longer trace all
>> @@ -1955,32 +1959,30 @@ void folio_clear_hugetlb_hwpoison(struct folio
>> *folio)
>> folio_free_raw_hwp(folio, true);
>> }
>> +#define MF_HUGETLB_FREED 0 /* freed hugepage */
>> +#define MF_HUGETLB_IN_USED 1 /* in-use hugepage */
>> +#define MF_NOT_HUGETLB 2 /* not a hugepage */
>
> If you're already dealing with negative error codes, "MF_NOT_HUGETLB"
> nicely
> translated to -EINVAL.
Agreed, thanks, will make the change in next round.
>
> But I wonder if it would be cleaner to just define all values in an enum
> and return
> that enum instead of an int from the functions.
>
> enum md_hugetlb_status {
> MF_HUGETLB_INVALID, /* not a hugetlb folio */
> MF_HUGETLB_BUSY, /* busy, retry later */
> MF_HUGETLB_FREED, /* hugetlb folio was freed */
> MF_HUGETLB_IN_USED, /* ??? no idea what that really means */
> MF_HUGETLB_FOLIO_PRE_POISONED, /* folio already poisoned, per-
> page information unclear */
> MF_HUGETLB_PAGE_PRE_POISONED, /* exact page already poisoned */
> }
enum is nicer than #define for sure, the only concern I have is that,
this enum as a return type from _get_huge_page_for_hwpoison()/
get_huge_page_for_hwpoison() will leave footprint in
include/linux/mm.h that declares _get_huge_page_for_hwpoison(),
include/linux/hugetlb.h that declares/defines
get_huge_page_for_hwpoison(),
and mm/hugetlb.c.
Is there a neat way? or, am I nitpicking? :)
thanks,
-jane
>
Powered by blists - more mailing lists