vm_map_protect: allow to set prot and max_prot in one go.

[kib]

Summary

Also enable setting wx for max_prot even if map does not allow to set effective wx protection.

[markj]

sys/vm/vm_map.c

2914

Suppose one initially has prot = maxprot = PROT_READ|PROT_WRITE, and we use mprotect() to set prot = PROT_READ|PROT_WRITE and maxprot = PROT_READ. From my reading, this code will leave the entry in a situation where prot & ~maxprot != 0.

[kib]

sys/vm/vm_map.c

2914

Wouldn't be such attempt rejected by the added check at the start of the function, right before 'again' label ?

[markj]

sys/vm/vm_map.c

2914

No, because it checks the new prot against the old maxprot.

[markj]

sys/vm/vm_map.c

2914

Oh sorry, you're right, I was looking at the check in the first loop.

[kib]

Use another mach error code to translate to ENOTSUP for maxprot>prot violation check. Existing selection collided with EINVAL, breaking several tests, e.g. largepage mprotect.

[alc]

sys/vm/vm_map.c

2839–2845

Aren't these two lines pointless? Two "if" statements later we have:

if ((flags & VM_MAP_PROTECT_SET_PROT) == 0 ||
    ...)
        continue;

2841–2842

This also appears pointless. Specifically, I don't see new_maxprot used by the rest of this loop body.

[kib]

sys/vm/vm_map.c

2841–2842

I tried to be protective. While it is easy to see that the last loop, that finally changes protection settings of the map entries, only access new_prot and new_maxprot when guarded by corresponding flags, for the first and second loop it is not that obvious (and for first loop it is not true).

So instead of micro-optimizing, I just added the safety belts. If you prefer the following to be committed, I will do it.

diff --git a/sys/vm/vm_map.c b/sys/vm/vm_map.c
index 4bd0b245a881..b3288fce5114 100644
--- a/sys/vm/vm_map.c
+++ b/sys/vm/vm_map.c
@@ -2836,11 +2836,6 @@ vm_map_protect(vm_map_t map, vm_offset_t start, vm_offset_t end,
 			return (rv);
 		}
 
-		if ((flags & VM_MAP_PROTECT_SET_PROT) == 0)
-			new_prot = entry->protection;
-		if ((flags & VM_MAP_PROTECT_SET_MAXPROT) == 0)
-			new_maxprot = entry->max_protection;
-
 		if ((flags & VM_MAP_PROTECT_SET_PROT) == 0 ||
 		    ((new_prot & ~entry->protection) & VM_PROT_WRITE) == 0 ||
 		    ENTRY_CHARGED(entry) ||

[alc]

sys/vm/vm_map.c

2841–2842

Please do commit it.