bhyve: Use MAP_GUARD guard pages

[lattera-gmail.com]

Summary

The memory space of virtual machines is protected by guard pages (one guard mapping at the beginning of the VM space and one guard mapping at the end of the VM space) . Instead of relying solely on PROT_NONE mappings, use MAP_GUARD mappings as the guard pages. Note that the size of the guard mapping (4MB) is left unchanged.

Sponsored by: HardendBSD and G2, Inc

Test Plan

1. Apply patch
2. Run your bhyve VMs as you normally do

[jhb]

Adding @kib since he added MAP_GUARD. I think you should instead MAP_GUARD the entire range first and then remap the middle. The middle is already remapped on line 518, so instead of adding new mmap()'s just for the guards, you should replace the mmap() on line 496 with this:

base = mmap(NULL, len2, PROT_NONE, MAP_GUARD | MAP_ALIGNED_SUPER, -1, 0);

[lattera-gmail.com]

In D16822#361106, @jhb wrote:

Adding @kib since he added MAP_GUARD. I think you should instead MAP_GUARD the entire range first and then remap the middle. The middle is already remapped on line 518, so instead of adding new mmap()'s just for the guards, you should replace the mmap() on line 496 with this:

base = mmap(NULL, len2, PROT_NONE, MAP_GUARD | MAP_ALIGNED_SUPER, -1, 0);

Thanks for the feedback! I'll give that a shot and report back soon.

[lattera-gmail.com]

Update the patch to use John Baldwin's suggestion on mapping the entire range first with MAP_GUARD.

[lattera-gmail.com]

Missed a spot. Cover another mapping with MAP_GUARD.

[kib]

The only useful feature of the phabricator is to easily see context around the patch, which you successfully botched.

lib/libvmmapi/vmmapi.c

392 ↗

(On Diff #47401)

flags is single-use and thus pointless.

402 ↗

(On Diff #47401)

I munmap(2) fails, the interesting errno value is obliterated.

503 ↗

(On Diff #47401)

Again flags use is pointless. Before your patch, it was used to avoid line wrapping in the mmap() line, most likely.

510 ↗

(On Diff #47401)

Weird line space use.

[jhb]

You should only be replacing the existing MAP_ANON / PROT_NONE mmap() calls with MAP_GUARD instead. You shouldn't be adding new mmap() calls. The "real" thing is already being mmap()'d later. As @kib notes, it's good to use 'svn diff -x -U 99999' or the like to generate context when uploading to phabricator.

[lattera-gmail.com]

Reflect changes requested by both kib and jhb.

[lattera-gmail.com]

In D16822#361129, @kib wrote:

The only useful feature of the phabricator is to easily see context around the patch, which you successfully botched.

I'm still getting used to Phabricator. Please be patient.

[jhb]

lib/libvmmapi/vmmapi.c

397 ↗

(On Diff #47406)

You don't need this second call to mmap(). setup_memory_segment() maps the appropriate regions of memory via mmap() with MAP_FIXED below. You should just keep the existing line that does 'baseaddr = ptr + VM_MMAP_GUARD_SIZE;'.

499 ↗

(On Diff #47406)

I would probably leave the existing comment as it is already accurate. I would also probably leave the len2 helper variable to minimize the diff.

[lattera-gmail.com]

lib/libvmmapi/vmmapi.c

397 ↗

(On Diff #47406)

Ah, okay. I didn't realize setup_memory_segment did that. I'll remove this call to mmap.

499 ↗

(On Diff #47406)

I'll add the comment back in.

As far as len2 is concerned, I figured that since kib complained about the already-existing flags variable being single-use, I'd also get rid of len2, since it, too, is single-use. I can revert that change, though.

[lattera-gmail.com]

Minimize diff with suggestions by jhb.

[jhb]

This looks right to me now. I'll try to test it locally in the next day or so.

[lattera-gmail.com]

In D16822#361197, @jhb wrote:

This looks right to me now. I'll try to test it locally in the next day or so.

Thanks a lot for the review and the suggestions! For what it's worth, I've been running with this patch with various operating systems (Win10, FreeBSD, HardenedBSD, and CentOS) since the last revision. No issues to report on my end.

[araujo]

I have tested it with FreeBSD HEAD as a guest running for couple days.