linux-kernel - Re: [PATCH v3] kpageflags: respect folio head-page flag placement

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: Wed, 20 Mar 2024 16:40:43 -0700
From: Svetly Todorov <svetly.todorov@...verge.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 gregory.price@...verge.com, wangkefeng.wang@...wei.com,
 akpm@...ux-foundation.org, david@...hat.com, vbabka@...e.cz,
 naoya.horiguchi@...ux.dev
Subject: Re: [PATCH v3] kpageflags: respect folio head-page flag placement


Hi Matthew,

> I have a somewhat different patch for this.  Let me know what you think.
> It depends on a few other patches in my tree, so probably won't compile
> for you.
I don't have extensive experience with folios or anything but on the
whole it looks good to me. I like the use of `mapping` to dodge the
compound_head() checks. Beyond that, only a few things caught my eye.

> -	if (PageKsm(page))
> +	if (mapping & PAGE_MAPPING_KSM)
>   		u |= 1 << KPF_KSM;
This might need an #ifdef?
Say mapping is movable and anon -- then (mapping & PAGE_MAPPING_KSM) is
true. Before, we called PageKsm, which falls through to a PG_ksm check.
If !CONFIG_KSM then that flag is always false. But now, we're liable to
report KPF_KSM even if !CONFIG_KSM.

>   	/*
>   	 * compound pages: export both head/tail info
>   	 * they together define a compound page's start/end pos and order
>   	 */
> -	if (PageHead(page))
> -		u |= 1 << KPF_COMPOUND_HEAD;
> -	if (PageTail(page))
> +	if (page == &folio->page)
> +		u |= kpf_copy_bit(k, KPF_COMPOUND_HEAD, PG_head);
> +	else
>   		u |= 1 << KPF_COMPOUND_TAIL;
This makes sense but it'd require changes to the documentation.
I ran a python3 memhog to see if anonymous pages are currently reported
as COMPOUND_HEAD or COMPOUND_TAIL and it seems to be a no on both.
But with this, I think every pfn will have one of the two set.
Unless you can have a page outside of a folio -- not sure.

Also, in
> -	if (page_is_idle(page))
> +#if defined(CONFIG_PAGE_IDLE_FLAG) && defined(CONFIG_64BIT)
> +	u |= kpf_copy_bit(k, KPF_IDLE,          PG_idle);
> +#else
> +	if (folio_test_idle(folio))
>   		u |= 1 << KPF_IDLE;
> +#endif
> 
and
> -	if (PageSwapCache(page))
> +#define SWAPCACHE ((1 << PG_swapbacked) | (1 << PG_swapcache))
> +	if ((k & SWAPCACHE) == SWAPCACHE)
>   		u |= 1 << KPF_SWAPCACHE;
>   	u |= kpf_copy_bit(k, KPF_SWAPBACKED,	PG_swapbacked);
it seems to me like the #ifdef/#define could be supplanted by
folio_test_idle and folio_test_swapcache. But I guess those would
require extra folio_flags queries and an #include <page_idle.h>.
So if this is more performant, I can understand the design.

Best,
Svetly