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

[Yeoreum Yun]

> >>> +		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?
> 
> Ah. The idea was to simply reject the batch entirely if that happens. It would
> only matter if some folio PTEs are writable while others are not ... well, ok,
> there are some cases where that can happen, but it's not really the common case.
> 
> I think we should simply disallow ever getting large folios into shadow stack
> VMAs for now. So I'd prefer the fix below.
> 
> Thoughts?

That's my preference too and we've discussed :)
I'll repost with below unless anyone has any strong comment.


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