Re: [PATCH v2 6/6] mm: constify the pte_offset_map_ro_nolock() return value

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Aug 03, 2026 at 05:44:00PM +0100, Pedro Falcato wrote:
> Constify the pte_t * retval from pte_offset_map_ro_nolock(), for which it is
> already pledged that accesses must be read-only. With it, convert the three
> treewide users to use const pte_t *.
>
> khugepaged passes the result right down to fault code (do_swap_page()). This
> leads to a complicated set of conditions that, in order to be correct, must
> not install anything into *vmf->pte. This is not trivial to work around in
> fault code, and as such just trivially cast to non-const pte_t* in the
> meantime.
>
> The other users are far more trivial and the conversion is equally
> trivially simple.

Ah finally more words! :)

>
> Signed-off-by: Pedro Falcato <pfalcato@suse.de>

With comment updated as below and nits addressed, LGTM so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---
>  arch/powerpc/mm/pgtable.c | 2 +-
>  include/linux/mm.h        | 4 ++--
>  include/linux/pgtable.h   | 2 +-
>  mm/filemap.c              | 2 +-
>  mm/khugepaged.c           | 2 +-
>  mm/pgtable-generic.c      | 4 ++--
>  6 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index a9be337be3e4..e29db41b6043 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -390,7 +390,7 @@ void assert_pte_locked(struct mm_struct *mm, unsigned long addr)
>  	p4d_t *p4d;
>  	pud_t *pud;
>  	pmd_t *pmd;
> -	pte_t *pte;
> +	const pte_t *pte;
>  	spinlock_t *ptl;
>
>  	if (mm == &init_mm)
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7fabe6c66b4b..acf5a5e31d34 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3885,8 +3885,8 @@ static inline pte_t *pte_offset_map(pmd_t *pmd, unsigned long addr)
>  pte_t *pte_offset_map_lock(struct mm_struct *mm, pmd_t *pmd,
>  			   unsigned long addr, spinlock_t **ptlp);
>
> -pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd,
> -				unsigned long addr, spinlock_t **ptlp);
> +const pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd,
> +				      unsigned long addr, spinlock_t **ptlp);
>  pte_t *pte_offset_map_rw_nolock(struct mm_struct *mm, pmd_t *pmd,
>  				unsigned long addr, pmd_t *pmdvalp,
>  				spinlock_t **ptlp);
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index dc418553e57a..dd51e722c535 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -112,7 +112,7 @@ static inline pte_t *__pte_map(pmd_t *pmd, unsigned long address)
>  {
>  	return pte_offset_kernel(pmd, address);
>  }
> -static inline void pte_unmap(pte_t *pte)
> +static inline void pte_unmap(const pte_t *pte)

I was going to question this based on whether the contract holds for
CONFIG_HIGHPTE but actually:

#define pte_unmap(pte)	do {	\
	kunmap_local((pte));	\
	rcu_read_unlock();	\
} while (0)

#define kunmap_local(__addr)					\
do {								\
	BUILD_BUG_ON(__same_type((__addr), struct page *));	\
	__kunmap_local(__addr);					\
} while (0)

static inline void __kunmap_local(const void *vaddr) <-- const!
{
	kunmap_local_indexed(vaddr);
}

So nice (CONFIG_HIGHPTE is going to go away at some point though, right? I
hope... :)


>  {
>  	rcu_read_unlock();
>  }
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 6afec636881f..af5d3fcd1b05 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3490,7 +3490,7 @@ static vm_fault_t filemap_fault_recheck_pte_none(struct vm_fault *vmf)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	vm_fault_t ret = 0;
> -	pte_t *ptep;
> +	const pte_t *ptep;
>
>  	/*
>  	 * We might have COW'ed a pagecache folio and might now have an mlocked
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index b237f6e7662a..09efac93a8c6 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1170,7 +1170,7 @@ static enum scan_result __collapse_huge_page_swapin(struct mm_struct *mm,
>  			 * Here the ptl is only used to check pte_same() in
>  			 * do_swap_page(), so readonly version is enough.
>  			 */
> -			pte = pte_offset_map_ro_nolock(mm, pmd, addr, &ptl);
> +			pte = (pte_t *) pte_offset_map_ro_nolock(mm, pmd, addr, &ptl);

Hmm yeah this is nasty, but you explain why in the commit message. Could you
extend the comment to explain it?

>  			if (!pte) {
>  				mmap_read_unlock(mm);
>  				result = SCAN_NO_PTE_TABLE;
> diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c
> index b91b1a98029c..2cfc6e608ef4 100644
> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -308,8 +308,8 @@ pte_t *__pte_offset_map(pmd_t *pmd, unsigned long addr, pmd_t *pmdvalp)
>  	return NULL;
>  }
>
> -pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd,
> -				unsigned long addr, spinlock_t **ptlp)
> +const pte_t *pte_offset_map_ro_nolock(struct mm_struct *mm, pmd_t *pmd,

Can pmd be const too?

> +				      unsigned long addr, spinlock_t **ptlp)
>  {
>  	pmd_t pmdval;
>  	pte_t *pte;
> --
> 2.55.0
>

--
Cheers, Lorenzo