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

[David Hildenbrand (Arm)]

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

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