mm/mremap: Honour writable bit in mremap pte batching
Subsystems (3)
PATCH 1/1 mm/mremap: Honour writable bit in mremap pte batching
Currently mremap folio pte batch ignores the writable bit during figuring out a set of similar ptes mapping the same folio. Suppose that the first pte of the batch is writable while the others are not - set_ptes will end up setting the writable bit on the other ptes, which is a violation of mremap semantics. Therefore, use FPB_RESPECT_WRITE to check the writable bit while determining the pte batch.
---
mm-selftests pass. Based on mm-new. Need David H. to confirm whether the repro passes.
mm/mremap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/mremap.c +1 -1
@@ -187,7 +187,7 @@ static int mremap_folio_pte_batch(struct vm_area_struct *vma, unsigned long addrif (!folio || !folio_test_large (folio))return 1 ; -return folio_pte_batch (folio, ptep, pte, max_nr); +return folio_pte_batch_flags (folio,NULL , ptep, &pte, max_nr,FPB_RESPECT_WRITE ); } static int move_ptes(struct pagetable_move_control *pmc,
Signature · 1 line
2.30.2
Quoted 7 lines · Dev Jain
Currently mremap folio pte batch ignores the writable bit during figuring out a set of similar ptes mapping the same folio. Suppose that the first pte of the batch is writable while the others are not -
set_pteswill end up setting the writable bit on the other ptes, which is a violation of mremap semantics. Therefore, useFPB_RESPECT_WRITEto check the writable bit while determining the pte batch.
Hmm, it seems to be like we're doing the wrong thing by default here? I must admit I haven't followed the contpte work as much as I would've liked, but it doesn't make much sense to me why FPB_RESPECT_WRITE would be an option you have to explicitly pass, and where folio_pte_batch (the "simple" interface) doesn't Just Do The Right Thing for naive callers.
Auditing all callers:
- khugepaged clears a variable number of ptes
- memory.c clears a variable number of ptes
- mempolicy.c grabs folios for migrations
- mlock.c steps over nr_ptes - 1 ptes, speeding up traversal
- mremap is borked since we're remapping nr_ptes ptes
- rmap.c TTU unmaps nr_ptes ptes for a given folio
so while the vast majority of callers don't seem to care, it would make sense that folio_pte_batch() works conservatively by default, and folio_pte_batch_flags() would allow for further batching (or maybe we would add a separate folio_pte_batch_clear() or folio_pte_batch_greedy() or whatnot).
Quoted 5 lines
Cc [email protected] #6.17Fixes f822a9a81a31 ("mm: optimize mremap() by PTE batching")Reported-by David Hildenbrand <[email protected]>Debugged-by David Hildenbrand <[email protected]>Signed-off-by Dev Jain <[email protected]>
But the solution itself looks okay to me. so, fwiw:
Signature · 1 line
Pedro
David Hildenbrand
Acked-byQuoted 9 lines · Pedro Falcato
Quoted 7 lines · Dev Jain
Currently mremap folio pte batch ignores the writable bit during figuring out a set of similar ptes mapping the same folio. Suppose that the first pte of the batch is writable while the others are not -
set_pteswill end up setting the writable bit on the other ptes, which is a violation of mremap semantics. Therefore, useFPB_RESPECT_WRITEto check the writable bit while determining the pte batch.
Quoted 5 lines
Hmm, it seems to be like we're doing the wrong thing by default here? I must admit I haven't followed the contpte work as much as I would've liked, but it doesn't make much sense to me why
FPB_RESPECT_WRITEwould be an option you have to explicitly pass, and wherefolio_pte_batch(the "simple" interface) doesn't Just Do The Right Thing for naive callers.
We use the "simple" version to apply to as many callers as possible: the common case, not some "let's be super careful" scenarios.
Quoted 9 lines
Auditing all callers:
- khugepaged clears a variable number of ptes
-memory.cclears a variable number of ptes
-mempolicy.cgrabs folios for migrations
-mlock.csteps overnr_ptes- 1 ptes, speeding up traversal
- mremap is borked since we're remappingnr_ptesptes
-rmap.cTTU unmapsnr_ptesptes for a given folio
Quoted 5 lines
so while the vast majority of callers don't seem to care, it would make sense that
folio_pte_batch()works conservatively by default, andfolio_pte_batch_flags()would allow for further batching (or maybe we would add a separatefolio_pte_batch_clear()orfolio_pte_batch_greedy()or whatnot).
I think really the tricky part is when we'e not only scanning or clearing, but actually want to "set" ptes again based on the result, like we do here.
For that we could consider having a second variant. But if it ends up having only a single caller, it's also not that great.
Quoted 11 lines
Quoted 6 lines
Cc [email protected] #6.17Fixes f822a9a81a31 ("mm: optimize mremap() by PTE batching")Reported-by David Hildenbrand <[email protected]>Debugged-by David Hildenbrand <[email protected]>Signed-off-by Dev Jain <[email protected]>But the solution itself looks okay to me. so, fwiw:
Acked-by Pedro Falcato <[email protected]>
Backport might end up being a bit tricky I suspect.
Signature · 3 lines
Cheers David / dhildenb
You'll probably get a tag just from using the British English spelling of 'honour' from me :P (joking! ;)
Quoted 6 lines · Dev Jain
Currently mremap folio pte batch ignores the writable bit during figuring out a set of similar ptes mapping the same folio. Suppose that the first pte of the batch is writable while the others are not -
set_pteswill end up setting the writable bit on the other ptes, which is a violation of mremap semantics. Therefore, useFPB_RESPECT_WRITEto check the writable bit while determining the pte batch.
Yikes.
Quoted 6 lines
Cc [email protected] #6.17Fixes f822a9a81a31 ("mm: optimize mremap() by PTE batching")Reported-by David Hildenbrand <[email protected]>Debugged-by David Hildenbrand <[email protected]>Signed-off-by Dev Jain <[email protected]>
LGTM, so:
Quoted 3 lines
---
mm-selftests pass. Based on mm-new. Need David H. to confirm whether the repro passes.
Given he A-b'd I assume it did :)
Quoted 20 lines
mm/mremap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/mremap.c +1
-2@@ -187,7 +187,7 @@ static int mremap_folio_pte_batch(struct vm_area_struct *vma, unsigned long addrif (!folio || !folio_test_large (folio))return 1 ; -return folio_pte_batch (folio, ptep, pte, max_nr); +return folio_pte_batch_flags (folio,NULL , ptep, &pte, max_nr,FPB_RESPECT_WRITE ); } static int move_ptes(struct pagetable_move_control *pmc, --2.30.2