Page MenuHomeFreeBSD
Differential D27956

amd64 pmap: do not sleep in _pmap_allocpte() with zero referenced page table page.
ClosedPublic
Actions

Authored by kib on Jan 4 2021, 7:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 3:45 AM
Unknown Object (File)
Tue, Nov 19, 3:37 AM
Unknown Object (File)
Tue, Nov 19, 3:32 AM
Unknown Object (File)
Tue, Nov 19, 3:31 AM
Unknown Object (File)
Tue, Nov 19, 3:17 AM
Unknown Object (File)
Tue, Nov 19, 1:24 AM
Unknown Object (File)
Sat, Nov 9, 10:21 PM
Unknown Object (File)
Sun, Nov 3, 10:59 PM
View All Files
Subscribers
pho

Details

Reviewers
alc
markj
Commits
rG4174e45fb432: amd64 pmap: do not sleep in pmap_allocpte_alloc() with zero referenced page…
rG9a8f5f5cf5ba: amd64 pmap: rename _pmap_allocpte() to pmap_allocpte_alloc().
rGf67064e592ce: amd64 pmap: Remove wrong __unused annotation from the va argument.
rG9658d9c71ae5: amd64 pmap: fix NULL deref in pmap_mincore().
Summary

Otherwise parallel _pmap_allocpte() for nearby va might also fail allocating page table page and free the page under us. The end result is that we could dereference unmapped pte when doing cleanup after sleep.




Instead, on allocation failure, first free everything, then sleep right before returning to caller.  Due to recursive nature of _pmap_allocpte(), additional argument indicates is this the first call level, or deeper recursive call.



Reported by:    pho
Diff Detail
Repository 
rG FreeBSD src repository 
Lint 
Lint Skipped
 
Unit 
Tests Skipped
 
Event Timeline
kib requested review of this revision.Jan 4 2021, 7:38 PM
kib created this revision.
alc added inline comments.Jan 4 2021, 11:57 PM
sys/amd64/amd64/pmap.c


4274
Why is va labeled __unused?
kib marked an inline comment as done.Jan 5 2021, 12:11 AM
kib added inline comments.
sys/amd64/amd64/pmap.c


4274
Right now it should be not.  It came from some intermediate version of LA57 patch where va was only used to improve asserts.
kib updated this revision to Diff 81654.Jan 5 2021, 12:14 AM
kib marked an inline comment as done.
Remove __unused
kib updated this revision to Diff 81668.Jan 5 2021, 11:54 AM
Reword the comment.
pho added a comment.Jan 5 2021, 9:46 PM
I ran a full stress2 test on mercat1.
markj added inline comments.Jan 8 2021, 3:11 PM
sys/amd64/amd64/pmap.c


4274
Why not split it into two functions, one for handling the base case of the recursion (try to alloc and sleep if that fails), and a second recursive function that never sleeps?
kib marked an inline comment as done.Jan 8 2021, 7:16 PM
kib added inline comments.
sys/amd64/amd64/pmap.c


4274
Done, with the note that alloc cannot go into non-recursive part, it must occur in the recursive helper when needed.
kib marked an inline comment as done.Jan 8 2021, 7:17 PM
kib updated this revision to Diff 81906.Jan 8 2021, 7:19 PM
Instead of the first_level arg to _pmap_allocpte, split non-sleepable recursive part into pmap_allocpte_nosleep().



Updated diff contains unrelated fix for pmap_mincore() dereferencing NULL pdpep, reported by Peter while testing this patch. (It is separate commit, of course).
markj added inline comments.Jan 9 2021, 4:33 PM
sys/amd64/amd64/pmap.c


4236
The comment is stale now.


4274
I would call it _pmap_allocpte_nosleep() or __pmap_allocpte(), to make it clearer that it is an internal helper for _pmap_allocpte().
kib marked 2 inline comments as done.Jan 9 2021, 8:30 PM
kib added inline comments.
sys/amd64/amd64/pmap.c


4236
I also moved first two paragraphs to the pmap_allocpte_alloc() herald.  It is more logical.


4274
I renamed it to pmap_allocpte_alloc().  I quite dislike the name that pretends on belonging to the language implementation namespace.
kib updated this revision to Diff 81970.Jan 9 2021, 8:31 PM
kib marked 2 inline comments as done.
_pmap_allocpte()->pmap_allocpte_alloc()

Rearrange and split comment.
markj accepted this revision.Jan 11 2021, 3:26 PM
Do riscv or arm64 have the same bug?
sys/amd64/amd64/pmap.c


4402
s/from/to/
This revision is now accepted and ready to land.Jan 11 2021, 3:26 PM
kib marked an inline comment as done.Jan 11 2021, 4:52 PM
kib added a comment.Jan 11 2021, 4:56 PM


In D27956#627736, @markj wrote:


Do riscv or arm64 have the same bug?





No, this bug was introduced with LA57 rewrite of pmap_allocpte().  riscv and arm64 forked pmap.c before LA57.
pho added a comment.Jan 11 2021, 8:41 PM
I ran a two-hour test on two different hosts with D27956.81970.diff added.

No problems seen.
Closed by commit rG9658d9c71ae5: amd64 pmap: fix NULL deref in pmap_mincore(). (authored by kib).  ·  Explain WhyJan 11 2021, 8:58 PM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rG9658d9c71ae5: amd64 pmap: fix NULL deref in pmap_mincore()..
kib added a commit: rGf67064e592ce: amd64 pmap: Remove wrong __unused annotation from the va argument..
kib added a commit: rG9a8f5f5cf5ba: amd64 pmap: rename _pmap_allocpte() to pmap_allocpte_alloc()..
kib added a commit: rG4174e45fb432: amd64 pmap: do not sleep in pmap_allocpte_alloc() with zero referenced page….
alc added a comment.Jan 15 2021, 8:07 PM


In D27956#627791, @kib wrote:




In D27956#627736, @markj wrote:


Do riscv or arm64 have the same bug?





No, this bug was introduced with LA57 rewrite of pmap_allocpte().  riscv and arm64 forked pmap.c before LA57.





Previously, our "strategy" was to allocate all required page table pages before linking any of them into the page table.  My impression from a brief examination is that with the LA57 changes that is no longer true, which led to this problem.  Specifically, under LA57, a pml4 page might be allocated and linked into the page table before we had successfully allocated all of the required lower-level page table pages, yes?  More generally, what was the reason for the change in strategy?
kib added a comment.Jan 15 2021, 8:51 PM


In D27956#629875, @alc wrote:




In D27956#627791, @kib wrote:




In D27956#627736, @markj wrote:


Do riscv or arm64 have the same bug?





No, this bug was introduced with LA57 rewrite of pmap_allocpte().  riscv and arm64 forked pmap.c before LA57.





Previously, our "strategy" was to allocate all required page table pages before linking any of them into the page table.  My impression from a brief examination is that with the LA57 changes that is no longer true, which led to this problem.  Specifically, under LA57, a pml4 page might be allocated and linked into the page table before we had successfully allocated all of the required lower-level page table pages, yes?  More generally, what was the reason for the change in strategy?





The biggest issue was that rollback becomes too unwieldy with 5-level (in fact when simultaneously 4 and 5 levels should be handled).  Making allocpte() reasonably structured was the most time-consuming part of the whole work.  In the current allocpte, rollback is 'local' so to say (local to each page tree level).



Then I realized that we do not reuse higher pages if lower vm_page_alloc() fails.  This makes the sleep rude, since we do not free pages that probably needed by somebody else, while sleeping.  After that realization, a new structure for allocpte() was naturally written, which I found satisfactory.



The missed detail was the location of the sleep, but with it moved out of the recursive part it is even more clean IMO.
Revision Contents
Changeset List
PathSize
sys/
amd64/
amd64/
82 lines
Diff 81668
View Options
sys/amd64/amd64/pmap.c
Loading...