Re: [PATCH v2 03/12] mm: Call ctor/dtor for kernel PTEs

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Tue, Apr 08, 2025 at 10:52:13AM +0100, Kevin Brodsky wrote:
> Since [1], constructors/destructors are expected to be called for
> all page table pages, at all levels and for both user and kernel
> pgtables. There is however one glaring exception: kernel PTEs are
> managed via separate helpers (pte_alloc_kernel/pte_free_kernel),
> which do not call the [cd]tor, at least not in the generic
> implementation.
> 
> The most obvious reason for this anomaly is that init_mm is
> special-cased not to use split page table locks. As a result calling
> ptlock_init() for PTEs associated with init_mm would be wasteful,
> potentially resulting in dynamic memory allocation. However, pgtable
> [cd]tors perform other actions - currently related to
> accounting/statistics, and potentially more functionally significant
> in the future.
> 
> Now that pagetable_pte_ctor() is passed the associated mm, we can
> make it skip the call to ptlock_init() for init_mm; this allows us
> to call the ctor from pte_alloc_one_kernel() too. This is matched by
> a call to the pgtable destructor in pte_free_kernel(); no
> special-casing is needed on that path, as ptlock_free() is already
> called unconditionally. (ptlock_free() is a no-op unless a ptlock
> was allocated for the given PTP.)
> 
> This patch ensures that all architectures that rely on
> <asm-generic/pgalloc.h> call the [cd]tor for kernel PTEs.
> pte_free_kernel() cannot be overridden so changing the generic
> implementation is sufficient. pte_alloc_one_kernel() can be
> overridden using __HAVE_ARCH_PTE_ALLOC_ONE_KERNEL, and a few
> architectures implement it by calling the page allocator directly.
> We amend those so that they call the generic
> __pte_alloc_one_kernel() instead, if possible, ensuring that the
> ctor is called.
> 
> A few architectures do not use <asm-generic/pgalloc.h>; those will
> be taken care of separately.
> 
> [1] https://lore.kernel.org/linux-mm/20250103184415.2744423-1-kevin.brodsky@arm.com/
> 
> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
> ---
>  arch/csky/include/asm/pgalloc.h | 2 +-
>  arch/microblaze/mm/pgtable.c    | 2 +-
>  arch/openrisc/mm/ioremap.c      | 2 +-
>  include/asm-generic/pgalloc.h   | 7 ++++++-
>  include/linux/mm.h              | 2 +-
>  5 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/csky/include/asm/pgalloc.h b/arch/csky/include/asm/pgalloc.h
> index 11055c574968..9ed2b15ffd94 100644
> --- a/arch/csky/include/asm/pgalloc.h
> +++ b/arch/csky/include/asm/pgalloc.h
> @@ -29,7 +29,7 @@ static inline pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
>  	pte_t *pte;
>  	unsigned long i;
>  
> -	pte = (pte_t *) __get_free_page(GFP_KERNEL);
> +	pte = __pte_alloc_one_kernel(mm);
>  	if (!pte)
>  		return NULL;
>  
> diff --git a/arch/microblaze/mm/pgtable.c b/arch/microblaze/mm/pgtable.c
> index 9f73265aad4e..e96dd1b7aba4 100644
> --- a/arch/microblaze/mm/pgtable.c
> +++ b/arch/microblaze/mm/pgtable.c
> @@ -245,7 +245,7 @@ unsigned long iopa(unsigned long addr)
>  __ref pte_t *pte_alloc_one_kernel(struct mm_struct *mm)
>  {
>  	if (mem_init_done)
> -		return (pte_t *)__get_free_page(GFP_KERNEL | __GFP_ZERO);
> +		return __pte_alloc_one_kernel(mm);
>  	else
>  		return memblock_alloc_try_nid(PAGE_SIZE, PAGE_SIZE,
>  					      MEMBLOCK_LOW_LIMIT,
> diff --git a/arch/openrisc/mm/ioremap.c b/arch/openrisc/mm/ioremap.c
> index 8e63e86251ca..3b352f97fecb 100644
> --- a/arch/openrisc/mm/ioremap.c
> +++ b/arch/openrisc/mm/ioremap.c
> @@ -36,7 +36,7 @@ pte_t __ref *pte_alloc_one_kernel(struct mm_struct *mm)
>  	pte_t *pte;
>  
>  	if (likely(mem_init_done)) {
> -		pte = (pte_t *)get_zeroed_page(GFP_KERNEL);
> +		pte = __pte_alloc_one_kernel(mm);
>  	} else {
>  		pte = memblock_alloc_or_panic(PAGE_SIZE, PAGE_SIZE);
>  	}
> diff --git a/include/asm-generic/pgalloc.h b/include/asm-generic/pgalloc.h
> index e164ca66f0f6..3c8ec3bfea44 100644
> --- a/include/asm-generic/pgalloc.h
> +++ b/include/asm-generic/pgalloc.h
> @@ -23,6 +23,11 @@ static inline pte_t *__pte_alloc_one_kernel_noprof(struct mm_struct *mm)
>  
>  	if (!ptdesc)
>  		return NULL;
> +	if (!pagetable_pte_ctor(mm, ptdesc)) {
> +		pagetable_free(ptdesc);
> +		return NULL;
> +	}
> +
>  	return ptdesc_address(ptdesc);
>  }
>  #define __pte_alloc_one_kernel(...)	alloc_hooks(__pte_alloc_one_kernel_noprof(__VA_ARGS__))
> @@ -48,7 +53,7 @@ static inline pte_t *pte_alloc_one_kernel_noprof(struct mm_struct *mm)
>   */
>  static inline void pte_free_kernel(struct mm_struct *mm, pte_t *pte)
>  {
> -	pagetable_free(virt_to_ptdesc(pte));
> +	pagetable_dtor_free(virt_to_ptdesc(pte));
>  }
>  
>  /**
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index f9b793cce2c1..3f48e449574a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3103,7 +3103,7 @@ static inline void pagetable_dtor_free(struct ptdesc *ptdesc)
>  static inline bool pagetable_pte_ctor(struct mm_struct *mm,
>  				      struct ptdesc *ptdesc)
>  {
> -	if (!ptlock_init(ptdesc))
> +	if (mm != &init_mm && !ptlock_init(ptdesc))
>  		return false;
>  	__pagetable_ctor(ptdesc);
>  	return true;

Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com> # s390