I question if there shouldn't be an explicit statement somewhere in this man page that this feature can be used by an application to implement memory "safety" mechanisms, but not "security". Because the RDPKRU and WRPKRU instructions are not privileged, a compromised application could always change the access rights. This is different from the protection key mechanisms on other architectures such as 32-bit ARM.

Ask Peter to test this patch.

In D18058#388822, @pho wrote:

D18058.id50652.diff ran the stress2/misc/stealer.sh test scenario for 11 hours before freezing up: https://people.freebsd.org/~pho/stress/log/dougm015.txt

In D18058#389007, @dougm_rice.edu wrote:

Allow bighint to be updated for a meta-node where the cursor lies ahead of all nonempty children, or even when there are no nonempty children, since blist_alloc depends on a reduction of the root bighint to allow a failed allocation to finish.

Alternatively, we could change the test in blist_alloc to depend on something other than bighint. This is the only case where having a too-large bighint breaks things.

Yes, this is clearly a bug. Hopefully, it is the one that Peter is tripping over.

In D16712#385630, @alc wrote:

In D16712#385624, @kib wrote:

Prefer to remove the inline keyword.

Okay, I'm going to commit this change in a few hours without the inline keyword.

In D16712#385624, @kib wrote:

Prefer to remove the inline keyword.

I like the introduction of the #define, but I'd to rename it to have the suffix _MASK. The point being that when a reader sees a use case as opposed to the definition the reader doesn't think that it's yet another flag.

On a somewhat related note, the version of jemalloc that we use in HEAD and 12.x is frequently allocating small, unaligned chunks from the kernel. In contrast, older versions were (always?) allocating large, aligned chunks. You can see the effects here: P196. Clang reads source files using mmap(2), and between most mapped files there is a sliver of anonymous heap. For "buildworld", I estimate that this has reduced superpage promotions by 1/3. The explicit clustering of anonymous memory allocations by this patch likely cures this problem.

In D5603#168859, @lattera-gmail.com wrote:

In D5603#168856, @emaste wrote:

ASR has a measurable performance hit; ASLR does not.

I'd be quite interested in the detail here if you can provide a link or reference to this.

How expensive is pulling from the entropy pool at least once, at most five times, per call to mmap?

Later today, I'm going to commit the bits here that are orthogonal to the main point of this patch. In other words, the bits that migrated here from an earlier, abandoned review that Kostik had asked about last week (if I recall correctly).

Aside from my one comment, this looks fine. Once that comment is addressed, consider this change "Accepted" by me.

arm64 is missing code from pmap_remove_l2() that reinstalls the (empty) page table page in the kernel pmap.

In regards to

mt = PHYS_TO_VM_PAGE(*pde & PG_FRAME);

the earlier call to pmap_remove_pde() and its helper function, pmap_remove_kernel_pde(), reinstalled the page table page that mt references, so this is correct code.

I would also suggest mentioning the use of this new flag in the comment at the top of the struct vm_map definition where we talk about min and max offset.

I would like to see D14005 completed and committed first.

In D17367#376853, @kib wrote:

In D17367#376752, @alc wrote:

I *really* wish that vm_fault() knew whether the copy-on-write fault was caused by a protection violation or an invalid mapping.  I think that a better heuristic for determining whether to call vm_fault_prefault() would be based on that knowledge.  Specifically, I would call vm_fault_prefault() when the mapping was invalid.

We can try to approximately calculate this information, but it is not reliable even on x86. For instance, we get a nonsensical error code for spurious page faults. But still, translating the bits from x86 exceptional error code into some additional information to vm_fault() is easy.

It might be that vm_fault() should be prepared to not get the hint about the fault cause at all.

This seems plausible.

I *really* wish that vm_fault() knew whether the copy-on-write fault was caused by a protection violation or an invalid mapping. I think that a better heuristic for determining whether to call vm_fault_prefault() would be based on that knowledge. Specifically, I would call vm_fault_prefault() when the mapping was invalid.

This creates an edge-case that we've never had to think about before, that we have overlapping entries in both the list and binary search tree (BST) of entries. Specifically, what I really don't like is that you are performing a splay on such a BST. So, now, I have to think about how splay will handle that edge-case.

Before making any of these changes, I would like to see an argument made as to whether the adj_free field is actually of benefit to us. For example, does it actually reduce the number of map entries that findspace() touches, and thereby reduce the number of cache misses? Or does its small contribution to the size of a map entry actually result in more cache misses?

This can be closed.

My only remaining unarticulated concern is that an errant call to pmap_large_map_wb_large() could allocate page table pages. (I think that function's outer loop should look more like pmap_protect() or pmap_remove().) However, assuming no errant calls, this revision looks good.

These are my last comments.

Reported by: Brett Gutstein <bgutstein@rice.edu>

I'm done.