Changeset View
Standalone View
vm_map.c
Show First 20 Lines • Show All 2,447 Lines • ▼ Show 20 Lines | |||||
*/ | */ | ||||
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, boolean_t set_max) | ||||
{ | { | ||||
vm_map_entry_t current, entry, in_tran; | vm_map_entry_t current, entry, in_tran; | ||||
vm_object_t obj; | vm_object_t obj; | ||||
struct ucred *cred; | struct ucred *cred; | ||||
vm_ooffset_t to_reserve; | |||||
vm_prot_t old_prot; | vm_prot_t old_prot; | ||||
if (start == end) | if (start == end) | ||||
return (KERN_SUCCESS); | return (KERN_SUCCESS); | ||||
again: | again: | ||||
in_tran = NULL; | in_tran = NULL; | ||||
to_reserve = 0; | |||||
vm_map_lock(map); | vm_map_lock(map); | ||||
/* | /* | ||||
* 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 | ||||
* doing so, and the VM object may end up in an inconsistent state if we | * doing so, and the VM object may end up in an inconsistent state if we | ||||
* update the protection on the map entry in between faults. | * update the protection on the map entry in between faults. | ||||
*/ | */ | ||||
vm_map_wait_busy(map); | vm_map_wait_busy(map); | ||||
VM_MAP_RANGE_CHECK(map, start, end); | VM_MAP_RANGE_CHECK(map, start, end); | ||||
if (vm_map_lookup_entry(map, start, &entry)) { | if (!vm_map_lookup_entry(map, start, &entry)) { | ||||
kib: {} are not needed. | |||||
vm_map_clip_start(map, entry, start); | |||||
} else { | |||||
entry = entry->next; | entry = entry->next; | ||||
} | } | ||||
/* | /* | ||||
* Make a first pass to check for protection violations. | * Make a first pass to check for protection violations. | ||||
*/ | */ | ||||
for (current = entry; current->start < end; current = current->next) { | for (current = entry; current->start < end; current = current->next) { | ||||
if ((current->eflags & MAP_ENTRY_GUARD) != 0) | if ((current->eflags & MAP_ENTRY_GUARD) != 0) | ||||
continue; | continue; | ||||
if (current->eflags & MAP_ENTRY_IS_SUB_MAP) { | if (current->eflags & MAP_ENTRY_IS_SUB_MAP) { | ||||
vm_map_unlock(map); | vm_map_unlock(map); | ||||
return (KERN_INVALID_ARGUMENT); | return (KERN_INVALID_ARGUMENT); | ||||
} | } | ||||
if ((new_prot & current->max_protection) != new_prot) { | if ((new_prot & current->max_protection) != new_prot) { | ||||
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; | ||||
if (set_max || | |||||
((new_prot & ~(current->protection)) & VM_PROT_WRITE) == 0 || | |||||
ENTRY_CHARGED(current)) { | |||||
continue; | |||||
} | } | ||||
obj = current->object.vm_object; | |||||
if (obj == NULL ? (current->eflags & MAP_ENTRY_NEEDS_COPY) : | |||||
Done Inline Actions(...eflags & MAP_ENTRY_NEEDS_COPY) != 0 You can check that in_tran == NULL there, I am not sure. kib: (...eflags & MAP_ENTRY_NEEDS_COPY) != 0
You can check that in_tran == NULL there, I am not… | |||||
(obj->type == OBJT_DEFAULT || obj->type == OBJT_SWAP)) { | |||||
to_reserve += ulmin(end, current->end) - | |||||
ulmax(start, current->start); | |||||
} | |||||
} | |||||
Not Done Inline ActionsSuppose an entry spans end. That entry will be clipped below, but we will reserve space for the new entry created by that clipping without charging that entry for the space. markj: Suppose an entry spans `end`. That entry will be clipped below, but we will reserve space for… | |||||
if (!swap_reserve(to_reserve)) { | |||||
vm_map_unlock(map); | |||||
return (KERN_RESOURCE_SHORTAGE); | |||||
} | |||||
/* | /* | ||||
* Postpone the operation until all in transition map entries | * Postpone the operation until all in transition map entries | ||||
* are stabilized. In-transition entry might already have its | * are stabilized. In-transition entry might already have its | ||||
* pages wired and wired_count incremented, but | * pages wired and wired_count incremented, but | ||||
* MAP_ENTRY_USER_WIRED flag not yet set, and visible to other | * MAP_ENTRY_USER_WIRED flag not yet set, and visible to other | ||||
* threads because the map lock is dropped. In this case we | * threads because the map lock is dropped. In this case we | ||||
* would miss our call to vm_fault_copy_entry(). | * would miss our call to vm_fault_copy_entry(). | ||||
*/ | */ | ||||
if (in_tran != NULL) { | if (in_tran != NULL) { | ||||
in_tran->eflags |= MAP_ENTRY_NEEDS_WAKEUP; | in_tran->eflags |= MAP_ENTRY_NEEDS_WAKEUP; | ||||
vm_map_unlock_and_wait(map, 0); | vm_map_unlock_and_wait(map, 0); | ||||
goto again; | goto again; | ||||
} | } | ||||
/* | /* | ||||
* Do an accounting pass for private read-only mappings that | * Do an accounting pass for private read-only mappings that | ||||
* now will do cow due to allowed write (e.g. debugger sets | * now will do cow due to allowed write (e.g. debugger sets | ||||
* breakpoint on text segment) | * breakpoint on text segment) | ||||
*/ | */ | ||||
if (entry->start < start) | |||||
Not Done Inline ActionsIsn't this test redundant? In other words, doesn't vm_map_clip_start() perform the same test? alc: Isn't this test redundant? In other words, doesn't vm_map_clip_start() perform the same test? | |||||
vm_map_clip_start(map, entry, start); | |||||
for (current = entry; current->start < end; current = current->next) { | for (current = entry; current->start < end; current = current->next) { | ||||
vm_map_clip_end(map, current, end); | vm_map_clip_end(map, current, end); | ||||
if (set_max || | if (set_max || | ||||
((new_prot & ~(current->protection)) & VM_PROT_WRITE) == 0 || | ((new_prot & ~(current->protection)) & VM_PROT_WRITE) == 0 || | ||||
ENTRY_CHARGED(current) || | ENTRY_CHARGED(current) || | ||||
(current->eflags & MAP_ENTRY_GUARD) != 0) { | (current->eflags & MAP_ENTRY_GUARD) != 0) { | ||||
continue; | continue; | ||||
} | } | ||||
cred = curthread->td_ucred; | cred = curthread->td_ucred; | ||||
obj = current->object.vm_object; | obj = current->object.vm_object; | ||||
Done Inline Actions!= 0 kib: != 0 | |||||
if (obj == NULL || (current->eflags & MAP_ENTRY_NEEDS_COPY)) { | if (obj == NULL || (current->eflags & MAP_ENTRY_NEEDS_COPY)) { | ||||
if (!swap_reserve(current->end - current->start)) { | |||||
vm_map_unlock(map); | |||||
return (KERN_RESOURCE_SHORTAGE); | |||||
Done Inline ActionsShould we be accumulating the total swap reserved in this loop and releasing it on an error return? dougm: Should we be accumulating the total swap reserved in this loop and releasing it on an error… | |||||
Not Done Inline ActionsIdeally, I think so. Note though that you would also need to go back and release any acquired cred refs. If you do as you suggest below and defer assignment of current->cred to the second loop, you wouldn't have to worry about this. markj: Ideally, I think so. Note though that you would also need to go back and release any acquired… | |||||
} | |||||
crhold(cred); | crhold(cred); | ||||
current->cred = cred; | current->cred = cred; | ||||
Done Inline ActionsShould we be delaying the assignment of current->cred until the next loop, since otherwise we may cause two entries to become mergeable, and return early with KERN_RESOURCE_SHORTAGE without either restoring them to their original states or merging them? dougm: Should we be delaying the assignment of current->cred until the next loop, since otherwise we… | |||||
continue; | continue; | ||||
} | } | ||||
VM_OBJECT_WLOCK(obj); | VM_OBJECT_WLOCK(obj); | ||||
if (obj->type != OBJT_DEFAULT && obj->type != OBJT_SWAP) { | if (obj->type != OBJT_DEFAULT && obj->type != OBJT_SWAP) { | ||||
VM_OBJECT_WUNLOCK(obj); | VM_OBJECT_WUNLOCK(obj); | ||||
continue; | continue; | ||||
} | } | ||||
/* | /* | ||||
* Charge for the whole object allocation now, since | * Charge for the whole object allocation now, since | ||||
* we cannot distinguish between non-charged and | * we cannot distinguish between non-charged and | ||||
* charged clipped mapping of the same object later. | * charged clipped mapping of the same object later. | ||||
*/ | */ | ||||
KASSERT(obj->charge == 0, | KASSERT(obj->charge == 0, | ||||
("vm_map_protect: object %p overcharged (entry %p)", | ("vm_map_protect: object %p overcharged (entry %p)", | ||||
obj, current)); | obj, current)); | ||||
if (!swap_reserve(ptoa(obj->size))) { | |||||
VM_OBJECT_WUNLOCK(obj); | |||||
vm_map_unlock(map); | |||||
return (KERN_RESOURCE_SHORTAGE); | |||||
} | |||||
crhold(cred); | crhold(cred); | ||||
Not Done Inline ActionsWe may have reserved swap space for an earlier entry without clipping it. If the entry spanned one of start or end we will have marked the entry as charged by setting entry->cred, but the amount of reserved swap space will be less than entry->end - entry->start. So when the entry is deleted, we may release more swap space than was reserved. At least, I don't see anything preventing this scenario. markj: We may have reserved swap space for an earlier entry without clipping it. If the entry spanned… | |||||
Not Done Inline ActionsThis is still not done, right ? kib: This is still not done, right ? | |||||
Done Inline ActionsI'm not clear about whether this comment expresses a concern about a bug that this patch would introduce, or a bug that's already there that this patch is being asked to fix. So far, I'm not trying to fix anything except the cases where vm_map_protect clips or modifies entries and then fails without undoing those changes. Is this about using entry->end - entry->size instead of ptoa(obj->size)? I'm happy to go back to ptoa(obj->size). Somebody told me that they were the same. If that's not it, then I'm not sure what we're talking about. dougm: I'm not clear about whether this comment expresses a concern about a bug that this patch would… | |||||
Not Done Inline ActionsLook at how the clip_start entry is handled, for instance for NEED_COPY case. You reserve the amount of swap needed for non-clipped entry, then set cred for clipped. This means that the swap reserved is bigger than the size of the entry, and the difference would be never released. kib: Look at how the clip_start entry is handled, for instance for NEED_COPY case. You reserve the… | |||||
Done Inline ActionsThat's right now. But back up to diff 58892, where the critical line was: to_reserve += ulmin(end, current->end) - ulmax(start, current->start); so I was reserving just the amount needed for clipped entries. But Mark's first comment was made with this line in place, so I didn't understand it to be about accounting-for-clipping. Why did I change this line 12 hours ago? Because a Peter Holm crash suggested I wasn't reserving enough swap space, and I thought this line might be the problem. dougm: That's right now. But back up to diff 58892, where the critical line was:
to_reserve… | |||||
Done Inline ActionsIsn't this what happens now? That is, clip_start calls vm_entry_charge_object, which leaves the two halves of the clipped entry both with the same underlying object and the same charge. Isn't that range being charged twice *now*? I don't see where clip_start is later making two objects, one for each of the two map entries, with reduced charges. dougm: Isn't this what happens now? That is, clip_start calls vm_entry_charge_object, which leaves… | |||||
Not Done Inline ActionsI was pointing out a bug in the initial version of the patch, which is still present and related to my comment above. Basically, with your patch we are reserving swap space based on the size of unclipped map entries, and I believe that that is incorrect. I will try to provide more context in my comments in the future. markj: I was pointing out a bug in the initial version of the patch, which is still present and… | |||||
Done Inline ActionsThanks for all the help you can give me. But, as explained in the response to Kostik, I was accounting for clipping at the time you made your comment. I'm just not doing it now. dougm: Thanks for all the help you can give me. But, as explained in the response to Kostik, I was… | |||||
Not Done Inline ActionsThe initial version had a different variant of the problem: it accounted for the clipping when reserving swap space, but the first loop could return an error before any clipping actually happened. So the amount of swap space reserved may not have matched the entry size. markj: The initial version had a different variant of the problem: it accounted for the clipping when… | |||||
obj->cred = cred; | obj->cred = cred; | ||||
obj->charge = ptoa(obj->size); | obj->charge = ptoa(obj->size); | ||||
VM_OBJECT_WUNLOCK(obj); | VM_OBJECT_WUNLOCK(obj); | ||||
} | } | ||||
/* | /* | ||||
* Go back and fix up protections. [Note that clipping is not | * Go back and fix up protections. [Note that clipping is not | ||||
* necessary the second time.] | * necessary the second time.] | ||||
▲ Show 20 Lines • Show All 2,345 Lines • Show Last 20 Lines |
{} are not needed.