Re: [PATCH 3/4] mm/sparse-vmemmap: drop vmemmap_can_optimize()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/sparse-vmemmap: dropping vmemmap_can_optimize() bypasses DAX arch config
- [Medium] mm/sparse-vmemmap: section_activate() stores non-optimizable compound order
- [Low] mm/sparse-vmemmap: missing header include breaks powerpc init_64.c build
--

commit 4171e4b9fc2abb84bf0e245a60563f33f94a263d
Author: Muchun Song <muchun.song@linux.dev>

mm/sparse-vmemmap: drop vmemmap_can_optimize()

This commit removes the DAX-specific vmemmap_can_optimize() helper from
include/linux/mm.h and relies on section_vmemmap_optimizable() to determine
whether a section uses vmemmap optimization.

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e875f73b0c6ff..4fb48fbe712d2 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -38,7 +38,6 @@
>  #include <linux/bitops.h>
>  #include <linux/iommu-debug-pagealloc.h>
>  #include <linux/kcsan-checks.h>
> -#include <linux/vmemmap-optimization.h>

[Severity: Low]
Does removing this include break the 64-bit PowerPC build when
CONFIG_SPARSEMEM_VMEMMAP=y is enabled?

In arch/powerpc/mm/init_64.c, vmemmap_populate() calls
section_set_compound_order():

    section_set_compound_order(__pfn_to_section(page_to_pfn((struct page *)start)), 0);

Previously, arch/powerpc/mm/init_64.c got the declaration of
section_set_compound_order() transitively via include/linux/mm.h. Without
including linux/vmemmap-optimization.h in arch/powerpc/mm/init_64.c,
compiling it will fail with an implicit function declaration error.

>  struct mempolicy;
>  struct anon_vma;
> @@ -5395,42 +5394,6 @@ static inline void vmem_altmap_free(struct vmem_altmap *altmap,
>  }
>  #endif
>  
> -#ifdef CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP
> -static inline bool __vmemmap_can_optimize(struct vmem_altmap *altmap,
> -					  struct dev_pagemap *pgmap)
> -{
> -	unsigned long nr_pages;
> -	unsigned long nr_vmemmap_pages;
> -
> -	if (!IS_ENABLED(CONFIG_VMEMMAP_OPTIMIZATION))
> -		return false;
> -
> -	if (!pgmap || !is_power_of_2(sizeof(struct page)))
> -		return false;
> -
> -	nr_pages = pgmap_vmemmap_nr(pgmap);
> -	nr_vmemmap_pages = ((nr_pages * sizeof(struct page)) >> PAGE_SHIFT);
> -	/*
> -	 * For vmemmap optimization with DAX we need minimum 2 vmemmap
> -	 * pages. See layout diagram in Documentation/mm/vmemmap_dedup.rst
> -	 */
> -	return !altmap && (nr_vmemmap_pages > VMEMMAP_OPTIMIZATION_PAGES);
> -}
> -/*
> - * If we don't have an architecture override, use the generic rule
> - */
> -#ifndef vmemmap_can_optimize
> -#define vmemmap_can_optimize __vmemmap_can_optimize
> -#endif
> -
> -#else
> -static inline bool vmemmap_can_optimize(struct vmem_altmap *altmap,
> -					   struct dev_pagemap *pgmap)
> -{
> -	return false;
> -}
> -#endif

[Severity: High]
With vmemmap_can_optimize() removed, nothing in C code checks
CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP anymore. Could this unintendedly
enable DAX vmemmap optimization on architectures that only select
ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP, such as 64-bit LoongArch?

When CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP=y and CONFIG_ZONE_DEVICE=y are
both enabled on 64-bit LoongArch, CONFIG_VMEMMAP_OPTIMIZATION is selected.
In section_activate(), order is now set from pgmap->vmemmap_shift, and
section_vmemmap_optimizable() -> vmemmap_optimizable_order() only checks
IS_ENABLED(CONFIG_VMEMMAP_OPTIMIZATION).

As a result, activating a DAX section with pgmap->vmemmap_shift >=
VMEMMAP_OPTIMIZATION_MIN_ORDER on LoongArch will map read-only shared tail
PTEs and grab references to vmemmap_shared_tail_page via vmemmap_alloc_pte().
However, LoongArch's vmemmap_free() in arch/loongarch/mm/init.c is an empty
no-op stub:

    void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *altmap)
    {
    }

On section_deactivate(), those shared tail page references are leaked and
the read-only PTEs remain mapped, causing subsequent section activations to
either trigger WARN_ON_ONCE(vmemmap_optimizable_pfn(pfn)) and fail with
-ENOMEM in vmemmap_pte_populate(), or fault in kernel mode when writing to
struct pages through the stale read-only PTEs.

[ ... ]

> diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> index 3e9d6617c4bb2..43559593107c9 100644
> --- a/mm/sparse-vmemmap.c
> +++ b/mm/sparse-vmemmap.c

[ ... ]

> @@ -699,10 +707,9 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn,
>  	struct mem_section *ms = __pfn_to_section(pfn);
>  	struct mem_section_usage *usage = NULL;
>  	struct page *memmap;
> -	unsigned int order;
> +	unsigned int order = pgmap ? pgmap->vmemmap_shift : 0;
>  	int rc;
>  
> -	order = vmemmap_can_optimize(altmap, pgmap) ? pgmap->vmemmap_shift : 0;
>  	if (nr_pages < PAGES_PER_SECTION && section_compound_order(ms))
>  		return ERR_PTR(-EOPNOTSUPP);

[Severity: Medium]
What happens in section_activate() when pgmap->vmemmap_shift is non-zero,
but is not actually optimizable (for example, when 0 < pgmap->vmemmap_shift
< VMEMMAP_OPTIMIZATION_MIN_ORDER, or when !is_power_of_2(sizeof(struct page)))?

Previously, order was forced to 0 when vmemmap_can_optimize() returned
false. Now, section_activate() passes the raw pgmap->vmemmap_shift to
section_set_compound_order_range(pfn, nr_pages, order).

Because __populate_section_memmap() only resets the order when both altmap
and section_vmemmap_optimizable(ms) are true:

    if (altmap && section_vmemmap_optimizable(ms))
            section_set_compound_order(ms, 0);

ms->compound_page_order remains non-zero for unoptimizable non-zero orders.
Since section_activate() checks section_compound_order(ms) rather than
section_vmemmap_optimizable(ms), wouldn't a subsequent partial-section
activation (nr_pages < PAGES_PER_SECTION) in that section be erroneously
rejected with -EOPNOTSUPP even though vmemmap optimization was never used?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030140720.1256062-1-songmuchun@bytedance.com?part=3