linux-kernel - Re: [PATCH v4 4/4] selftests/mm: add tests for HWPOISON hugetlbfs read

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: <39b90dce-fe0f-e1d8-3094-75cabbfa38a3@oracle.com>
Date: Thu, 11 Jan 2024 10:11:12 -0800
From: Sidhartha Kumar <sidhartha.kumar@...cle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: Jiaqi Yan <jiaqiyan@...gle.com>,
        Muhammad Usama Anjum <usama.anjum@...labora.com>, linmiaohe@...wei.com,
        mike.kravetz@...cle.com, naoya.horiguchi@....com,
        akpm@...ux-foundation.org, songmuchun@...edance.com,
        shy828301@...il.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        jthoughton@...gle.com, "kernel@...labora.com" <kernel@...labora.com>
Subject: Re: [PATCH v4 4/4] selftests/mm: add tests for HWPOISON hugetlbfs
 read

On 1/11/24 10:03 AM, Matthew Wilcox wrote:
> On Thu, Jan 11, 2024 at 09:51:47AM -0800, Sidhartha Kumar wrote:
>> On 1/11/24 9:34 AM, Jiaqi Yan wrote:
>>>> -                       if (!folio_test_has_hwpoisoned(folio))
>>>> +                       if (!folio_test_hwpoison(folio))
>>>
>>> Sidhartha, just curious why this change is needed? Does
>>> PageHasHWPoisoned change after commit
>>> "a08c7193e4f18dc8508f2d07d0de2c5b94cb39a3"?
>>
>> No its not an issue PageHasHWPoisoned(), the original code is testing for
>> the wrong flag and I realized that has_hwpoison and hwpoison are two
>> different flags. The memory-failure code calls folio_test_set_hwpoison() to
>> set the hwpoison flag and does not set the has_hwpoison flag. When
>> debugging, I realized this if statement was never true despite the code
>> hitting folio_test_set_hwpoison(). Now we are testing the correct flag.
>>
>>  From page-flags.h
>>
>> #ifdef CONFIG_MEMORY_FAILURE
>> 	PG_hwpoison,		/* hardware poisoned page. Don't touch */
>> #endif
>>
>> folio_test_hwpoison() checks this flag ^^^
>>
>> /* At least one page in this folio has the hwpoison flag set */
>> PG_has_hwpoisoned = PG_error,
>>
>> while folio_test_has_hwpoisoned() checks this flag ^^^
> 
> So what you're saying is that hugetlb behaves differently from THP
> with how memory-failure sets the flags?

I think so, in memory_failure() THP goes through this path:
	
	hpage = compound_head(p);
	if (PageTransHuge(hpage)) {
		/*
		 * The flag must be set after the refcount is bumped
		 * otherwise it may race with THP split.
		 * And the flag can't be set in get_hwpoison_page() since
		 * it is called by soft offline too and it is just called
		 * for !MF_COUNT_INCREASED.  So here seems to be the best
		 * place.
		 *
		 * Don't need care about the above error handling paths for
		 * get_hwpoison_page() since they handle either free page
		 * or unhandlable page.  The refcount is bumped iff the
		 * page is a valid handlable page.
		 */
		SetPageHasHWPoisoned(hpage);

which sets has_hwpoisoned flag while hugetlb goes through 
folio_set_hugetlb_hwpoison() which calls folio_test_set_hwpoison().