Changeset View
Standalone View
sys/vm/vm_map.c
Show First 20 Lines • Show All 2,727 Lines • ▼ Show 20 Lines | if (p_start != NULL) | ||||
pmap_enter_object(map->pmap, start, addr + ptoa(psize), | pmap_enter_object(map->pmap, start, addr + ptoa(psize), | ||||
p_start, prot); | p_start, prot); | ||||
VM_OBJECT_RUNLOCK(object); | VM_OBJECT_RUNLOCK(object); | ||||
} | } | ||||
/* | /* | ||||
* vm_map_protect: | * vm_map_protect: | ||||
* | * | ||||
* Sets the protection of the specified address | * Sets the protection and/or the maximum protection of the | ||||
* region in the target map. If "set_max" is | * specified address region in the target map. | ||||
* specified, the maximum protection is to be set; | |||||
* otherwise, only the current protection is affected. | |||||
*/ | */ | ||||
int | int | ||||
vm_map_protect(vm_map_t map, vm_offset_t start, vm_offset_t end, | vm_map_protect(vm_map_t map, vm_offset_t start, vm_offset_t end, | ||||
vm_prot_t new_prot, boolean_t set_max) | vm_prot_t new_prot, vm_prot_t new_maxprot, int flags) | ||||
{ | { | ||||
vm_map_entry_t entry, first_entry, in_tran, prev_entry; | vm_map_entry_t entry, first_entry, in_tran, prev_entry; | ||||
vm_object_t obj; | vm_object_t obj; | ||||
struct ucred *cred; | struct ucred *cred; | ||||
vm_prot_t old_prot; | vm_prot_t old_prot; | ||||
int rv; | int rv; | ||||
if (start == end) | if (start == end) | ||||
return (KERN_SUCCESS); | return (KERN_SUCCESS); | ||||
if ((flags & (VM_MAP_PROTECT_SET_PROT | VM_MAP_PROTECT_SET_MAXPROT)) == | |||||
(VM_MAP_PROTECT_SET_PROT | VM_MAP_PROTECT_SET_MAXPROT) && | |||||
(new_prot & new_maxprot) != new_prot) | |||||
return (KERN_OUT_OF_BOUNDS); | |||||
again: | again: | ||||
in_tran = NULL; | in_tran = NULL; | ||||
vm_map_lock(map); | vm_map_lock(map); | ||||
if ((map->flags & MAP_WXORX) != 0 && (new_prot & | if ((map->flags & MAP_WXORX) != 0 && | ||||
(VM_PROT_WRITE | VM_PROT_EXECUTE)) == (VM_PROT_WRITE | | (flags & VM_MAP_PROTECT_SET_PROT) != 0 && | ||||
(new_prot & (VM_PROT_WRITE | VM_PROT_EXECUTE)) == (VM_PROT_WRITE | | |||||
VM_PROT_EXECUTE)) { | VM_PROT_EXECUTE)) { | ||||
vm_map_unlock(map); | vm_map_unlock(map); | ||||
return (KERN_PROTECTION_FAILURE); | return (KERN_PROTECTION_FAILURE); | ||||
} | } | ||||
/* | /* | ||||
* Ensure that we are not concurrently wiring pages. vm_map_wire() may | * Ensure that we are not concurrently wiring pages. vm_map_wire() may | ||||
* need to fault pages into the map and will drop the map lock while | * need to fault pages into the map and will drop the map lock while | ||||
Show All 13 Lines | again: | ||||
for (entry = first_entry; entry->start < end; | for (entry = first_entry; entry->start < end; | ||||
entry = vm_map_entry_succ(entry)) { | entry = vm_map_entry_succ(entry)) { | ||||
if ((entry->eflags & MAP_ENTRY_GUARD) != 0) | if ((entry->eflags & MAP_ENTRY_GUARD) != 0) | ||||
continue; | continue; | ||||
if ((entry->eflags & MAP_ENTRY_IS_SUB_MAP) != 0) { | if ((entry->eflags & MAP_ENTRY_IS_SUB_MAP) != 0) { | ||||
vm_map_unlock(map); | vm_map_unlock(map); | ||||
return (KERN_INVALID_ARGUMENT); | return (KERN_INVALID_ARGUMENT); | ||||
} | } | ||||
if ((new_prot & entry->max_protection) != new_prot) { | 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 ((new_prot & entry->max_protection) != new_prot || | |||||
(new_maxprot & entry->max_protection) != new_maxprot) { | |||||
vm_map_unlock(map); | vm_map_unlock(map); | ||||
return (KERN_PROTECTION_FAILURE); | return (KERN_PROTECTION_FAILURE); | ||||
} | } | ||||
if ((entry->eflags & MAP_ENTRY_IN_TRANSITION) != 0) | if ((entry->eflags & MAP_ENTRY_IN_TRANSITION) != 0) | ||||
in_tran = entry; | in_tran = entry; | ||||
} | } | ||||
/* | /* | ||||
Show All 24 Lines | again: | ||||
for (entry = first_entry; entry->start < end; | for (entry = first_entry; entry->start < end; | ||||
entry = vm_map_entry_succ(entry)) { | entry = vm_map_entry_succ(entry)) { | ||||
rv = vm_map_clip_end(map, entry, end); | rv = vm_map_clip_end(map, entry, end); | ||||
if (rv != KERN_SUCCESS) { | if (rv != KERN_SUCCESS) { | ||||
vm_map_unlock(map); | vm_map_unlock(map); | ||||
return (rv); | return (rv); | ||||
} | } | ||||
if (set_max || | if ((flags & VM_MAP_PROTECT_SET_PROT) == 0) | ||||
new_prot = entry->protection; | |||||
alc: Aren't these two lines pointless? Two "if" statements later we have:
```
if ((flags &… | |||||
if ((flags & VM_MAP_PROTECT_SET_MAXPROT) == 0) | |||||
new_maxprot = entry->max_protection; | |||||
alcUnsubmitted Not Done Inline ActionsThis also appears pointless. Specifically, I don't see new_maxprot used by the rest of this loop body. alc: This also appears pointless. Specifically, I don't see `new_maxprot` used by the rest of this… | |||||
kibAuthorUnsubmitted Done Inline ActionsI 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) || kib: I tried to be protective. While it is easy to see that the last loop, that finally changes… | |||||
alcUnsubmitted Not Done Inline ActionsPlease do commit it. alc: Please do commit it. | |||||
if ((flags & VM_MAP_PROTECT_SET_PROT) == 0 || | |||||
((new_prot & ~entry->protection) & VM_PROT_WRITE) == 0 || | ((new_prot & ~entry->protection) & VM_PROT_WRITE) == 0 || | ||||
ENTRY_CHARGED(entry) || | ENTRY_CHARGED(entry) || | ||||
(entry->eflags & MAP_ENTRY_GUARD) != 0) { | (entry->eflags & MAP_ENTRY_GUARD) != 0) | ||||
continue; | continue; | ||||
} | |||||
cred = curthread->td_ucred; | cred = curthread->td_ucred; | ||||
obj = entry->object.vm_object; | obj = entry->object.vm_object; | ||||
if (obj == NULL || | if (obj == NULL || | ||||
(entry->eflags & MAP_ENTRY_NEEDS_COPY) != 0) { | (entry->eflags & MAP_ENTRY_NEEDS_COPY) != 0) { | ||||
if (!swap_reserve(entry->end - entry->start)) { | if (!swap_reserve(entry->end - entry->start)) { | ||||
rv = KERN_RESOURCE_SHORTAGE; | rv = KERN_RESOURCE_SHORTAGE; | ||||
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines | for (prev_entry = vm_map_entry_pred(first_entry), entry = first_entry; | ||||
vm_map_try_merge_entries(map, prev_entry, entry), | vm_map_try_merge_entries(map, prev_entry, entry), | ||||
prev_entry = entry, entry = vm_map_entry_succ(entry)) { | prev_entry = entry, entry = vm_map_entry_succ(entry)) { | ||||
if (rv != KERN_SUCCESS || | if (rv != KERN_SUCCESS || | ||||
(entry->eflags & MAP_ENTRY_GUARD) != 0) | (entry->eflags & MAP_ENTRY_GUARD) != 0) | ||||
continue; | continue; | ||||
old_prot = entry->protection; | old_prot = entry->protection; | ||||
if (set_max) | if ((flags & VM_MAP_PROTECT_SET_MAXPROT) != 0) { | ||||
entry->protection = | entry->max_protection = new_maxprot; | ||||
(entry->max_protection = new_prot) & | entry->protection = new_maxprot & old_prot; | ||||
old_prot; | } | ||||
else | if ((flags & VM_MAP_PROTECT_SET_PROT) != 0) | ||||
entry->protection = new_prot; | entry->protection = new_prot; | ||||
Not Done Inline ActionsSuppose 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. markj: Suppose one initially has prot = maxprot = PROT_READ|PROT_WRITE, and we use mprotect() to set… | |||||
Done Inline ActionsWouldn't be such attempt rejected by the added check at the start of the function, right before 'again' label ? kib: Wouldn't be such attempt rejected by the added check at the start of the function, right before… | |||||
Not Done Inline ActionsNo, because it checks the new prot against the old maxprot. markj: No, because it checks the new prot against the old maxprot. | |||||
Not Done Inline ActionsOh sorry, you're right, I was looking at the check in the first loop. markj: Oh sorry, you're right, I was looking at the check in the first loop. | |||||
/* | /* | ||||
* For user wired map entries, the normal lazy evaluation of | * For user wired map entries, the normal lazy evaluation of | ||||
* write access upgrades through soft page faults is | * write access upgrades through soft page faults is | ||||
* undesirable. Instead, immediately copy any pages that are | * undesirable. Instead, immediately copy any pages that are | ||||
* copy-on-write and enable write access in the physical map. | * copy-on-write and enable write access in the physical map. | ||||
*/ | */ | ||||
if ((entry->eflags & MAP_ENTRY_USER_WIRED) != 0 && | if ((entry->eflags & MAP_ENTRY_USER_WIRED) != 0 && | ||||
▲ Show 20 Lines • Show All 2,479 Lines • Show Last 20 Lines |
Aren't these two lines pointless? Two "if" statements later we have: