amd64 pmap: allocate leaf page table page for wired userspace 2M pages

[bnovkov]

Summary

This patch reverts the changes made in D19670 and fixes the original issue by allocating and prepopulating a leaf page table page for wired userspace 2M pages.

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, which leads to a panic in pmap_demote_pde_locked 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) .

Test Plan

The C repros generated by syzkaller no longer trigger this panic, so I've tested the changes using a pseudodevice module that inserts a wired 2M page mapping in the calling process's address space through an ioctl.
I've attached an archive containing the source files for the module and a test program

wirekmod.tar.gz8 KBDownload

.
Running the test program with D19670 reverted triggers the same panic. The program exits normally when running it with this patch.

[markj]

The C repros generated by syzkaller no longer trigger this panic

hmm, that is surprising. Could you share the program and output from ktrace -tcf ./repro?

sys/amd64/amd64/pmap.c

7535

Further below we handle the case where a mapping already exists at the specified va. In that situation a leaf PTP may already be resident in the trie, for example if the existing mapping was created by a promotion. Then the new pmap_insert_pt_page() call will fail.

We should insert the leaf PTP further below, after any existing mapping has been dealt with.

7537

I suspect that VM_ALLOC_INTERRUPT is inappropriate here, since we can gracefully handle allocation failures. _INTERRUPT should generally be used to minimize the risk of failure in paths where error handling isn't possible.

7544

Local variables should be defined at the beginning of the function or block.

sys/vm/vm_fault.c

391 ↗

(On Diff #124969)

Just a note: this part shouldn't be committed until other pmaps which implement pmap_enter(psind = 1) (i386, arm64, riscv) have been similarly modified.

[bnovkov]

Address @markj 's comments/

[bnovkov]

In D41132#936726, @markj wrote:

The C repros generated by syzkaller no longer trigger this panic

hmm, that is surprising. Could you share the program and output from ktrace -tcf ./repro?

Sure, here is the ktrace output

ktrace.out161 KBDownload

and the link to the program.
Also, for some reason the test program tarball I originally attached wasn't visible, should be accessible now.

sys/vm/vm_fault.c

391 ↗

(On Diff #124969)

Of course, if this approach gets accepted I'll move it into a separate patch.

[kib]

sys/amd64/amd64/pmap.c

7648

It is really strange to return an error in case of page table page allocation failure, but panic on trie insert.

[bnovkov]

Address @kib 's comments.

[bnovkov]

sys/amd64/amd64/pmap.c

7648

thank you for pointing this out, I've reworked the condition and removed the panic.

[kib]

sys/amd64/amd64/pmap.c

7648

If pmap_insert_pt_page() failed, wouldn't you leak the page table page?

[bnovkov]

Address @kib 's comment, free pagetable page when insertion fails.

[bnovkov]

sys/amd64/amd64/pmap.c

7648

Yes, that would cause a leak, I've reworked the code to deal with this.

[kib]

sys/amd64/amd64/pmap.c

7633

pv entry must be removed if the ptpage below cannot be mounted. Perhaps PGA_WRITEABLE setting for the constituent pages should be postponed until we committed to return success.

7655

[bnovkov]

sys/amd64/amd64/pmap.c

7633

Do you think it would be more appropriate to move this code block before PV entry creation? That way we only have worry about removing and freeing the newly allocated ptpage if things go awry.

[kib]

sys/amd64/amd64/pmap.c

7633

There are two things you need to worry about allocation failure: pde pv entry and page table page. I do not see much difference if you change order of allocation, either way you need to handle both failures.

[bnovkov]

Address @kib 's comments - remove pde pv entry upon leaf ptpage insertion or allocation failure, defer setting PGA_WRITEABLE for constituent pages.

[alc]

sys/amd64/amd64/pmap.c

7633

Actually, the error handling will be simpler if you move it.

7637

Increase the scope of this variable, and initialize it to NULL.

7642

This does not correctly handle the PAT bit, which is in a different bit position between PDEs and PTEs. That said, there is no reason to initialize the PTEs. Instead, pass false as the last argument to pmap_insert_pt_page(), and the page table page will be initialized on demotion, if demotion ever occurs. (The comment describing the last parameter to pmap_insert_pt_page() is specific to its current usage and can be updated later.)

7643

This has three problems: It's removing from the 4KB PV list when it should be removing from the 2MB PV list. It should only attempt removal if the PDE has the managed bit set. It needs to conditionally perform pmap_abort_ptp().

[alc]

sys/amd64/amd64/pmap.c

7577

On some occasions, the page table page that you are later going to allocate is being freed here. :-)

[bnovkov]

Address @alc 's comments:

• Defer PTE initialization until demotion
• Rearrange code to simplify error handling

I've also excluded the changes to vm_fault.c, as per @markj 's remark.

[alc]

sys/amd64/amd64/pmap.c

7597

As a micro-optimization, because wired mappings are rare and user-space mapping are common, so test the condition to most likely to be false first.

7603

Please note that the surrounding code does not introduce blank lines except around blocks of code preceded by a multiline block comment. This is an idiosyncrasy of style(9) that admittedly isn't always adhered to.

7605

The page is not guaranteed to be zero filled.

7618–7619

We need to undo the pmap_insert_pt_page().

[bnovkov]

Address @alc 's comments.

[alc]

This looks correct now. Thanks!

sys/amd64/amd64/pmap.c

7621

This should only be indented as far as the opening parentheses on the previous line.

[markj]

sys/amd64/amd64/pmap.c

7598

This needs to be pmap_pde_pindex(va). pmap_pde_index() gives you the PDE index for the address, which despite the similar name is completely different. I'll fix this before pushing.