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

On Tue, Aug 04, 2026 at 12:22:19PM +0100, Lorenzo Stoakes (ARM) wrote:
> 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 <[email protected]>
> 
> With comment updated as below and nits addressed, LGTM so:
> 
> Reviewed-by: Lorenzo Stoakes (ARM) <[email protected]>
> 
> > ---
> >  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?

Yep, I'll do 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?

I was going to say "yes but then pmdp_get() also needs to be constfified" but
actually no, it can't:

pte_t *__pte_offset_map(pmd_t *pmd, unsigned long addr, pmd_t *pmdvalp) {
        if (unlikely(pmd_bad(pmdval))) {
                pmd_clear_bad(pmd);
                goto nomap;
        }
}

so PTE mapping actually needs to write to the pmdp if the pmd looks bad.
Tricky stuff :)

-- 
Pedro