Page MenuHomeFreeBSD

i386 pmap: allocate leaf page table page for wired userspace superpages
ClosedPublic

Authored by bnovkov on Aug 29 2023, 2:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 14, 5:32 PM
Unknown Object (File)
Mon, Apr 8, 2:04 PM
Unknown Object (File)
Sun, Apr 7, 1:03 PM
Unknown Object (File)
Sun, Apr 7, 7:52 AM
Unknown Object (File)
Sun, Apr 7, 6:48 AM
Unknown Object (File)
Sun, Apr 7, 12:16 AM
Unknown Object (File)
Sat, Apr 6, 6:38 PM
Unknown Object (File)
Mon, Apr 1, 2:08 PM
Subscribers

Details

Summary

This patch fixes a syzkaller issue regarding wired userspace superpages by allocating a leaf page table page for wired userspace superpages.

The original issue is an edge case that creates an unmapped, wired region in userspace. Subsequent faults on this region can trigger wired superpage creation, leading to a panic upon demotion as the pmap does not create a leaf page table page for the wired superpage. D19670 fixed this by disallowing preemptive creation of wired superpage mappings, but that fix is currently interfering with an ongoing effort of speeding up vm_map_wire for large, contiguous entries (e.g. bhyve wiring guest memory) .

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Sep 17 2023, 12:45 AM
markj added inline comments.
sys/i386/i386/pmap.c
4035

The amd64 patch doesn't zero the PTP here, and I believe that's correct. Why exactly do we need to zero the page here?

4058

The indentation here should be by four spaces.

This revision now requires review to proceed.Sep 17 2023, 10:02 PM
bnovkov added inline comments.
sys/i386/i386/pmap.c
4035

That was an unintentional slipup, thank you for catching that.

alc requested changes to this revision.Sep 18 2023, 7:27 AM
alc added inline comments.
sys/i386/i386/pmap.c
4035

Because i386 doesn't have 34eeabff5a8636155bb02985c5928c1844fd3178, so zeroing is necessary.

This revision now requires changes to proceed.Sep 18 2023, 7:27 AM

@markj I think that you should apply 34eeabff5a8636155bb02985c5928c1844fd3178 to i386 and riscv. Otherwise, I suspect that there will be assertion failures in places like pmap_remove_pde():

KASSERT(vm_page_all_valid(mpte),
    ("pmap_remove_pde: pte page not promoted"));
bnovkov marked an inline comment as done.

Address @alc 's comments.

In D41635#955565, @bojan.novkovic_fer.hr wrote:

Address @alc 's comments.

Unfortunately, zeroing the page table page won't prevent some of the assertions from misfiring. I've started preparing a variant of 34eeabff5a8636155bb02985c5928c1844fd3178 and some related changes to i386. Once that is committed, you won't need to zero the page table page and assertions will not be a problem.

I've just committed 902ed64fecbe which eliminates the need for zeroing the PTP and the possibility of an assertion failure if you pass the same arguments to pmap_insert_pt_page() as you did on amd64. pmap_insert_pt_page() now has the needed extra argument.

In D41635#954945, @alc wrote:

@markj I think that you should apply 34eeabff5a8636155bb02985c5928c1844fd3178 to i386 and riscv. Otherwise, I suspect that there will be assertion failures in places like pmap_remove_pde():

KASSERT(vm_page_all_valid(mpte),
    ("pmap_remove_pde: pte page not promoted"));

I was away for a few weeks and am only now getting caught up. I can spend some time on the riscv pmap if you aren't already working on it.

@bojan.novkovic_fer.hr if you have time, could you please try the test case I added in commit 8f26ed01bd74aab21309ac04ae1d1368a6346c90? That is how I found the bug in the amd64 patch. Note that for the test to be effective, you need to revert commit 64087fd7f372d70a0aaaeb02ffd3438720923534. Once all of your pmap patches land, I believe we can revert that commit.

In D41635#957353, @alc wrote:

I've just committed 902ed64fecbe which eliminates the need for zeroing the PTP and the possibility of an assertion failure if you pass the same arguments to pmap_insert_pt_page() as you did on amd64. pmap_insert_pt_page() now has the needed extra argument.

That is great, thank you for your effort!

@bojan.novkovic_fer.hr if you have time, could you please try the test case I added in commit 8f26ed01bd74aab21309ac04ae1d1368a6346c90? That is how I found the bug in the amd64 patch. Note that for the test to be effective, you need to revert commit 64087fd7f372d70a0aaaeb02ffd3438720923534. Once all of your pmap patches land, I believe we can revert that commit.

No problem, I'm a bit swamped at the moment but I'll run it within the next couple of days.

In D41635#954945, @alc wrote:

@markj I think that you should apply 34eeabff5a8636155bb02985c5928c1844fd3178 to i386 and riscv. Otherwise, I suspect that there will be assertion failures in places like pmap_remove_pde():

KASSERT(vm_page_all_valid(mpte),
    ("pmap_remove_pde: pte page not promoted"));

I was away for a few weeks and am only now getting caught up. I can spend some time on the riscv pmap if you aren't already working on it.

I am not.

This revision is now accepted and ready to land.Sep 28 2023, 7:02 AM

@markj I tried the new mlock superpage test case, the patch didn't cause a panic.

@markj Are you overloaded? Should I commit this?

In D41635#961050, @alc wrote:

@markj Are you overloaded? Should I commit this?

Done now, I'm sorry for the delay.