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

["David Hildenbrand (Arm)" <david@kernel.org>]

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 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.

> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
>  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. */"

> +		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.


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

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;

-- 
Cheers,

David