| 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: <9fb04185-5b71-46c0-b62c-0e0e6ee59e6e@arm.com>
Date: Wed, 25 Jun 2025 09:58:31 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: Matthew Wilcox <willy@...radead.org>, David Hildenbrand
<david@...hat.com>, LKML <linux-kernel@...r.kernel.org>,
linux-mm <linux-mm@...ck.org>
Subject: Re: [RFC PATCH 1/1] mm/debug_vm_pgtable: Use a swp_entry_t input
value for swap tests
On 24/06/25 12:13 AM, Gerald Schaefer wrote:
> The various __pte/pmd_to_swp_entry and __swp_entry_to_pte/pmd helper
> functions are expected to operate on swapped PTE/PMD entries, not on
> present and mapped entries.
>
> Reflect this in the swap tests by using a swp_entry_t as input value,
> similar to how it is already done in pte_swap_exclusive_tests().
> Move the swap entry creation to init_args() and store it in args, so
> it can also be used in other functions.
Makes sense.
>
> The pte/pmd_swap_tests() are also changed to compare entries instead of
> pfn values, because pte/pmd_pfn() helpers are not expected to operate on
Swap entries compare is also happening now in pte_swap_exclusive_tests().
> swapped entries. E.g. on s390, pmd_pfn() needs different shifts for leaf
> (large) and non-leaf PMDs.
>
> Also update documentation, to reflect that the helpers operate on
> swapped and not mapped entries, and use correct names, i.e.
> __swp_to_pte/pmd_entry -> __swp_entry_to_pte/pmd.
>
> For consistency, also change pte/pmd_swap_soft_dirty_tests() to use
> args->swp_entry instead of a present and mapped PTE/PMD.
Makes sense.
>
> Signed-off-by: Gerald Schaefer <gerald.schaefer@...ux.ibm.com>
> ---
> Documentation/mm/arch_pgtable_helpers.rst | 8 ++--
> mm/debug_vm_pgtable.c | 55 ++++++++++++++---------
> 2 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/mm/arch_pgtable_helpers.rst b/Documentation/mm/arch_pgtable_helpers.rst
> index af245161d8e7..e2ac76202a85 100644
> --- a/Documentation/mm/arch_pgtable_helpers.rst
> +++ b/Documentation/mm/arch_pgtable_helpers.rst
> @@ -242,13 +242,13 @@ SWAP Page Table Helpers
> ========================
>
> +---------------------------+--------------------------------------------------+
> -| __pte_to_swp_entry | Creates a swapped entry (arch) from a mapped PTE |
> +| __pte_to_swp_entry | Creates a swap entry (arch) from a swapped PTE |
> +---------------------------+--------------------------------------------------+
> -| __swp_to_pte_entry | Creates a mapped PTE from a swapped entry (arch) |
> +| __swp_entry_to_pte | Creates a swapped PTE from a swap entry (arch) |
> +---------------------------+--------------------------------------------------+
> -| __pmd_to_swp_entry | Creates a swapped entry (arch) from a mapped PMD |
> +| __pmd_to_swp_entry | Creates a swap entry (arch) from a swapped PMD |
> +---------------------------+--------------------------------------------------+
> -| __swp_to_pmd_entry | Creates a mapped PMD from a swapped entry (arch) |
> +| __swp_entry_to_pmd | Creates a swapped PMD from a swap entry (arch) |
> +---------------------------+--------------------------------------------------+
> | is_migration_entry | Tests a migration (read or write) swapped entry |
> +-------------------------------+----------------------------------------------+
__pte_to_swp_entry() and __pmd_to_swp_entry() are still being used (and tested)
even after applying this patch. Should not their entries be preserved ?
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 7731b238b534..3b0f83ed6c2e 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -73,6 +73,8 @@ struct pgtable_debug_args {
> unsigned long fixed_pud_pfn;
> unsigned long fixed_pmd_pfn;
> unsigned long fixed_pte_pfn;
> +
> + swp_entry_t swp_entry;
> };
>
> static void __init pte_basic_tests(struct pgtable_debug_args *args, int idx)
> @@ -754,12 +756,15 @@ static void __init pte_soft_dirty_tests(struct pgtable_debug_args *args)
>
> static void __init pte_swap_soft_dirty_tests(struct pgtable_debug_args *args)
> {
> - pte_t pte = pfn_pte(args->fixed_pte_pfn, args->page_prot);
> + pte_t pte;
>
> if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
> return;
>
> pr_debug("Validating PTE swap soft dirty\n");
> + pte = swp_entry_to_pte(args->swp_entry);
> + WARN_ON(!is_swap_pte(pte));
> +
> WARN_ON(!pte_swp_soft_dirty(pte_swp_mksoft_dirty(pte)));
> WARN_ON(pte_swp_soft_dirty(pte_swp_clear_soft_dirty(pte)));
> }
> @@ -793,7 +798,9 @@ static void __init pmd_swap_soft_dirty_tests(struct pgtable_debug_args *args)
> return;
>
> pr_debug("Validating PMD swap soft dirty\n");
> - pmd = pfn_pmd(args->fixed_pmd_pfn, args->page_prot);
> + pmd = swp_entry_to_pmd(args->swp_entry);
> + WARN_ON(!is_swap_pmd(pmd));
> +
> WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
> WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
> }
> @@ -804,17 +811,11 @@ static void __init pmd_swap_soft_dirty_tests(struct pgtable_debug_args *args) {
>
> static void __init pte_swap_exclusive_tests(struct pgtable_debug_args *args)
> {
> - unsigned long max_swap_offset;
> swp_entry_t entry, entry2;
> pte_t pte;
>
> pr_debug("Validating PTE swap exclusive\n");
> -
> - /* See generic_max_swapfile_size(): probe the maximum offset */
> - max_swap_offset = swp_offset(pte_to_swp_entry(swp_entry_to_pte(swp_entry(0, ~0UL))));
> -
> - /* Create a swp entry with all possible bits set */
> - entry = swp_entry((1 << MAX_SWAPFILES_SHIFT) - 1, max_swap_offset);
> + entry = args->swp_entry;
args->swp_entry should be reused here as well.
>
> pte = swp_entry_to_pte(entry);
> WARN_ON(pte_swp_exclusive(pte));
> @@ -838,30 +839,36 @@ static void __init pte_swap_exclusive_tests(struct pgtable_debug_args *args)
>
> static void __init pte_swap_tests(struct pgtable_debug_args *args)
> {
> - swp_entry_t swp;
> - pte_t pte;
> + swp_entry_t entry, arch_entry;
> + pte_t pte, pte2;
A very small nit - s/pte2/pte as the first one is pmd not pte or
make it pte1, pte2 if preferred.
>
> pr_debug("Validating PTE swap\n");
> - pte = pfn_pte(args->fixed_pte_pfn, args->page_prot);
> - swp = __pte_to_swp_entry(pte);
> - pte = __swp_entry_to_pte(swp);
> - WARN_ON(args->fixed_pte_pfn != pte_pfn(pte));
> + entry = args->swp_entry;
Should args->swp_entry be used directly here and 'entry' local variable
be dropped ?
> +
> + pte = swp_entry_to_pte(entry);
> + WARN_ON(!is_swap_pte(pte));
> + arch_entry = __pte_to_swp_entry(pte);
> + pte2 = __swp_entry_to_pte(arch_entry);
> + WARN_ON(memcmp(&pte, &pte2, sizeof(pte)));
> }
>
> #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> static void __init pmd_swap_tests(struct pgtable_debug_args *args)
> {
> - swp_entry_t swp;
> - pmd_t pmd;
> + swp_entry_t entry, arch_entry;
> + pmd_t pmd, pmd2;
A very small nit - s/pmd2/pmd1 as the first one is pmd not pmd1 or
just make it pmd1, pmd2 if preferred.
>
> if (!has_transparent_hugepage())
> return;
>
> pr_debug("Validating PMD swap\n");
> - pmd = pfn_pmd(args->fixed_pmd_pfn, args->page_prot);
> - swp = __pmd_to_swp_entry(pmd);
> - pmd = __swp_entry_to_pmd(swp);
> - WARN_ON(args->fixed_pmd_pfn != pmd_pfn(pmd));
> + entry = args->swp_entry;
> + pmd = swp_entry_to_pmd(entry);
> + WARN_ON(!is_swap_pmd(pmd));
Good to have the WARN_ON() to test whether it is a swap PMD or not.
> +
> + arch_entry = __pmd_to_swp_entry(pmd);
> + pmd2 = __swp_entry_to_pmd(arch_entry);
> + WARN_ON(memcmp(&pmd, &pmd2, sizeof(pmd)));
Sounds good.
> }
> #else /* !CONFIG_ARCH_ENABLE_THP_MIGRATION */
> static void __init pmd_swap_tests(struct pgtable_debug_args *args) { }
> @@ -1166,6 +1173,7 @@ static void __init init_fixed_pfns(struct pgtable_debug_args *args)
>
> static int __init init_args(struct pgtable_debug_args *args)
> {
> + unsigned long max_swap_offset;
> struct page *page = NULL;
> int ret = 0;
>
> @@ -1248,6 +1256,11 @@ static int __init init_args(struct pgtable_debug_args *args)
>
> init_fixed_pfns(args);
>
> + /* See generic_max_swapfile_size(): probe the maximum offset */
> + max_swap_offset = swp_offset(pte_to_swp_entry(swp_entry_to_pte(swp_entry(0, ~0UL))));
Why not directly use generic_max_swapfile_size() which is doing exact same thing.
unsigned long generic_max_swapfile_size(void)
{
return swp_offset(pte_to_swp_entry(
swp_entry_to_pte(swp_entry(0, ~0UL)))) + 1;
}
> + /* Create a swp entry with all possible bits set */
> + args->swp_entry = swp_entry((1 << MAX_SWAPFILES_SHIFT) - 1, max_swap_offset);
> +
Makes sense to use maximum possible bits while creating the swap entry for testing.
> /*
> * Allocate (huge) pages because some of the tests need to access
> * the data in the pages. The corresponding tests will be skipped
Powered by blists - more mailing lists