Re: [PATCH v2 2/4] mm: internal: fix inconsistent in folio_pte_batch_flags() for shadow stack

[Yeoreum Yun]

On Mon, Nov 02, 2026 at 01:10:21PM +0100, David Hildenbrand (Arm) wrote:
> On 11/2/26 12:30, Yeoreum Yun wrote:
> > In x86, shadow stack PTE is marked with PAGE_RW=0 and PAGE_DIRTY=1.
> > If the shadow stack pte entry is consequence with the read-only pte
> > like PAGE_RW=0 and PAGE_DIRTY=0, folio_pte_batch_flags()
> > with FPB_RESPECT_WRITE without FPB_RESPECT_DIRTY considers both are
> > the same.
> > 
> > To address this, save the first pte's writable and compare subsequence
> > pte's writablity with pte_write() when FPB_RESPECT_WRITE is set
> > without FPB_RESPECT_DIRTY.
> 
> 
> I think this should be patch #1 and likely carry a Fixes: tag.

I see. I'll move and add Fixes tag.

> 
> I think we can already create an inconsistency by using a mixture of
> 
> * MADV_HUGEPAGE (overwrite for nohugepage for shadow stack)
> * MADV_DONTFORK + fork() + MADV_DOFORK
> * FPB_RESPECT_WRITE users
> 
> Not that it would matter in practice.

True. but it's possible anyway... without alternative suggestion.

> 
> > 
> > Signed-off-by: Yeoreum Yun <yeoreum.yun@xxxxxxx>
> > ---
> >  mm/internal.h | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 35016b1afc8bf..b4d0df4c35adf 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -418,7 +418,7 @@ static inline unsigned int folio_pte_batch_flags(struct folio *folio,
> >  		struct vm_area_struct *vma, pte_t *ptep, pte_t *ptentp,
> >  		unsigned int max_nr, fpb_t flags)
> >  {
> > -	bool any_writable = false, any_young = false, any_dirty = false;
> > +	bool any_writable = false, any_young = false, any_dirty = false, writable;
> >  	pte_t expected_pte, pte = *ptentp;
> >  	unsigned int nr, cur_nr;
> >  
> > @@ -438,6 +438,7 @@ static inline unsigned int folio_pte_batch_flags(struct folio *folio,
> >  
> >  	nr = pte_batch_hint(ptep, pte);
> >  	expected_pte = __pte_batch_clear_ignored(pte_advance_pfn(pte, nr), flags);
> > +	writable = !!pte_write(pte);
> >  	ptep = ptep + nr;
> >  
> >  	while (nr < max_nr) {
> > @@ -446,6 +447,16 @@ static inline unsigned int folio_pte_batch_flags(struct folio *folio,
> >  		if (!pte_same(__pte_batch_clear_ignored(pte, flags), expected_pte))
> >  			break;
> >  
> > +		/*
> > +		 * x86 shadow-stack PTEs encode writability as RW=0, DIRTY=1.
> > +		 * Without FPB_RESPECT_DIRTY, __pte_batch_clear_ignored()
> > +		 * considers shadow-stack PTE as the same with subsequent
> > +		 * ordinary read-only PTEs. Therefore, check whether pte writable.
> > +		 */
> 
> That information doesn't really belong in here in that detail level.
> "/* pte_mkclean() can remove the writable bit in some cases. */"
> 

I see. I'll change with simple as you suggest.

> > +		if (unlikely((flags & (FPB_RESPECT_WRITE | FPB_RESPECT_DIRTY)) == FPB_RESPECT_WRITE &&
> > +			     writable != !!pte_write(pte)))
> 
> It might be nicer and more efficient to track if any was readonly/not-writable and
> 
> 	any_unwritable = !pte_write(pte);
> 
> And checking once after the loop.

Sorry. But I couldn't follow. If we accumalate with any_unwritable
and check it out of loop once only, we couldn't properly nr value or
Am I missing something?

> 
> 
> I checked and we cannot really fix x86 without an additional sw bit to remember
> "shadow stack PTE".

Agree.

> 
> As discussed, the simpler alternative would be to just disallow large folios in
> shadow stacks entirely for now, given that vm_mmap_shadow_stack() already
> disables large folios explicitly.
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 4b71d010b2aff..9a2dc90ef9690 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -367,6 +367,9 @@ int hugepage_madvise(struct vm_area_struct *vma,
>  {
>         switch (advice) {
>         case MADV_HUGEPAGE:
> +               /* PTE batching doesn't work reliably. */
> +               if (vma_test_single_mask(vma, VMA_SHADOW_STACK))
> +                       break;
>                 *vm_flags &= ~VM_NOHUGEPAGE;
>                 *vm_flags |= VM_HUGEPAGE;
>                 break;
> 

Yeap. once we do this, we don't need to this fix patch anyway.
But I don't know whether there is practical case changing the
VMA_SHADOW_STACK with huge though it is initailised with NOHUGE.

> -- 
> Cheers,
> 
> David

-- 
Sincerely,
Yeoreum Yun