linux-kernel - Re: [PATCH 2/2] kpageflags: fix wrong KPF_THP on non-pmd-mappable compound pages

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]

Date: Thu, 27 Jun 2024 15:54:17 +0200
From: David Hildenbrand <david@...hat.com>
To: ran xiaokai <ranxiaokai627@....com>, akpm@...ux-foundation.org,
 willy@...radead.org
Cc: vbabka@...e.cz, svetly.todorov@...verge.com, ran.xiaokai@....com.cn,
 baohua@...nel.org, ryan.roberts@....com, peterx@...hat.com, ziy@...dia.com,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 2/2] kpageflags: fix wrong KPF_THP on non-pmd-mappable
 compound pages

On 26.06.24 04:49, ran xiaokai wrote:
> From: Ran Xiaokai <ran.xiaokai@....com.cn>
> 
> KPF_COMPOUND_HEAD and KPF_COMPOUND_TAIL are set on "common" compound
> pages, which means of any order, but KPF_THP should only be set
> when the folio is a 2M pmd mappable THP. Since commit 19eaf44954df

"should only be set" -- who says that? :)

The documentation only talks about "Contiguous pages which construct 
transparent hugepages". Sure, when it was added there were only PMD ones.


> ("mm: thp: support allocation of anonymous multi-size THP"),
> multiple orders of folios can be allocated and mapped to userspace,
> so the folio_test_large() check is not sufficient here,
> replace it with folio_test_pmd_mappable() to fix this.
> 

A couple of points:

1) If I am not daydreaming, ever since we supported non-PMD THP in the
    pagecache (much longer than anon mTHP), we already indicate KPF_THP
    for them here. So this is not anon specific? Or am I getting the
    PG_lru check all wrong?

2) Anon THP are disabled as default. If some custom tool cannot deal
    with that "extension" we did with smaller THP, it shall be updated if
    it really has to special-case PMD-mapped THP, before enabled by the
    admin.


I think this interface does exactly what we want, as it is right now. 
Unless there is *good* reason, we should keep it like that.

So I suggest

a) Extend the documentation to just state "THP of any size and any 
mapping granularity" or sth like that.

b) Maybe using folio_test_large_rmappable() instead of "(k & (1 <<
    PG_lru)) || is_anon", so even isolated-from-LRU THPs are indicated
    properly.

c) Whoever is interested in getting the folio size, use this flag along
    with a scan for the KPF_COMPOUND_HEAD.


I'll note that, scanning documentation, PAGE_IS_HUGE currently only 
applies to PMD *mapped* THP. It doesn't consider PTE-mapped THP at all 
(including PMD-ones). Likely, documentation should be updated to state 
"PMD-mapped THP or any hugetlb page".

> Also kpageflags is not only for userspace memory but for all valid pfn
> pages,including slab pages or drivers used pages, so the PG_lru and
> is_anon check are unnecessary here.

I'm completely confused about that statements. We do have KPF_OFFLINE, 
KPF_PGTABLE, KPF_SLAB, ... I'm missing something important.

> 
> Signed-off-by: Ran Xiaokai <ran.xiaokai@....com.cn>
> ---
>   fs/proc/page.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/proc/page.c b/fs/proc/page.c
> index 2fb64bdb64eb..3e7b70449c2f 100644
> --- a/fs/proc/page.c
> +++ b/fs/proc/page.c
> @@ -146,19 +146,13 @@ u64 stable_page_flags(const struct page *page)
>   		u |= kpf_copy_bit(k, KPF_COMPOUND_HEAD, PG_head);
>   	else
>   		u |= 1 << KPF_COMPOUND_TAIL;
> +
>   	if (folio_test_hugetlb(folio))
>   		u |= 1 << KPF_HUGE;
> -	/*
> -	 * We need to check PageLRU/PageAnon
> -	 * to make sure a given page is a thp, not a non-huge compound page.
> -	 */
> -	else if (folio_test_large(folio)) {
> -		if ((k & (1 << PG_lru)) || is_anon)
> -			u |= 1 << KPF_THP;
> -		else if (is_huge_zero_folio(folio)) {
> +	else if (folio_test_pmd_mappable(folio)) {

folio_test_pmd_mappable() would not be future safe. It includes anything 
 >= PMD_ORDER as well.


-- 
Cheers,

David / dhildenb