Hello,
[email protected], le lun. 16 juin 2025 23:58:06 +0100, a ecrit:
> diff --git a/tests/test-vm.c b/tests/test-vm.c
> index 4ece792e..8e4ad884 100644
> --- a/tests/test-vm.c
> +++ b/tests/test-vm.c
> @@ -75,11 +75,96 @@ static void test_wire()
> // TODO check that all memory is actually wired or unwired
> }
>
> +void test_vm_limit()
> +{
[...]
> + /* check that allocations demoted to VM_PROT_NONE no longer counts towards
> the VM limit */
> + err = vm_allocate(mach_task_self(), &mem, M_512M, TRUE);
> + ASSERT_RET(err, "allocating memory below the limit must succeed");
> + err = vm_allocate(mach_task_self(), &mem2, M_128M, TRUE);
> + /* the current limit is M_512M + M_128M, this allocation should hit the
> limit */
> + ASSERT(err == KERN_NO_SPACE, "allocation must fail with KERN_NO_SPACE");
> + err = vm_protect(mach_task_self(), mem, M_512M, TRUE, VM_PROT_NONE);
> + ASSERT_RET(err, "could not drop protection to VM_PROT_NONE");
> + /* after dropping the protection there should be enough space again */
> + err = vm_allocate(mach_task_self(), &mem2, M_128M, TRUE);
> + ASSERT_RET(err, "allocating memory below the limit must succeed");
> + /* this allocation purpose is showing the failure message to check
> size_none value */
> + err = vm_allocate(mach_task_self(), &mem3, M_512M, TRUE);
> + ASSERT(err == KERN_NO_SPACE, "going above the limit should still fail");
> + err = vm_deallocate(mach_task_self(), mem2, M_128M);
> + ASSERT_RET(err, "deallocation failed");
> + err = vm_deallocate(mach_task_self(), mem, M_512M);
> + ASSERT_RET(err, "deallocation failed");
It would be also useful to check that trying to vm_protect the PROT_NONE
into PROT_ALL fails when that comes above the limit.
> diff --git a/vm/vm_kern.c b/vm/vm_kern.c
> index 51223d98..6b03d014 100644
> --- a/vm/vm_kern.c
> +++ b/vm/vm_kern.c
> @@ -144,6 +144,19 @@ projected_buffer_allocate(
> u_entry->protection = protection;
> u_entry->max_protection = protection;
> u_entry->inheritance = inheritance;
> +
> + /*
> + * vm_map_find_entry allocated an entry of size `size`
> + * without knowing its protection.
Mmmm, then better make vm_map_find_entry aware of the protection? We can
add protection and max_protection parameters to vm_map_find_entry and
make it use that instead of hardcoding VM_PROT_DEFAULT and VM_PROT_ALL.
So that we really have all code modifying map->size that gets a
potential map->size_none update (or comment why not), making it more
obvious to further contributors how it's supposed to be managed.
> + * For this to work, the VM limit must not be reached
> + * in the allocation, even though we are going to free
> + * the chunk from being accounted for the current map
> + * limit by increasing map->size_none here.
> + */
> + if (protection == VM_PROT_NONE)
> + map->size_none += size;
> +
> vm_map_unlock(map);
> *user_p = addr;
>
> diff --git a/vm/vm_map.c b/vm/vm_map.c
> index 2e243b78..51a8dc79 100644
> --- a/vm/vm_map.c
> +++ b/vm/vm_map.c
> @@ -268,6 +277,49 @@ void vm_map_unlock(struct vm_map *map)
> lock_write_done(&map->lock);
> }
>
> +/*
> + * Enforces the VM limit of a target map.
> + */
> +static kern_return_t
> +vm_map_enforce_limit(
> + vm_map_t map,
> + vm_size_t size,
> + const char *fn_name)
> +{
> + /* Limit is ignored for the kernel map */
> + if (vm_map_pmap(map) == kernel_pmap) {
> + return KERN_SUCCESS;
> + }
> +
> + /* Avoid taking into account the total VM_PROT_NONE virtual memory */
> + vm_size_t usable_size = map->size - map->size_none;
"usable" is ambiguous here, it's not obvious whether it's the area that
the task has allocated non-NONE, or the addressing space that is still
available for more allocation. Better call it "really_allocated_size".
> + vm_size_t new_size = size + usable_size;
Better use usable_size + size to explicit the way augmentation works
here.
> + /* Check for integer overflow */
> + if (new_size < size) {
> + return KERN_INVALID_ARGUMENT;
> + }
> +
> + if (new_size > map->size_cur_limit) {
> +#if MACH_KDB
> + SoftDebugger("attempt to allocate above VM limit");
> +#endif
The debugger is useful for now, but we don't want to put it in master.
> + return KERN_NO_SPACE;
> + }
> +
> + return KERN_SUCCESS;
> +}
> +
> /*
> * vm_map_entry_create: [ internal use only ]
> *
> @@ -5168,6 +5272,8 @@ void vm_map_print(db_expr_t addr, boolean_t have_addr,
> db_expr_t count, const ch
> printf("ref=%d,nentries=%d\n", map->ref_count, map->hdr.nentries);
> printf("size=%lu,resident:%lu,wired=%lu\n", map->size,
> pmap_resident_count(map->pmap) * PAGE_SIZE, map->size_wired);
> + printf("max_limit=%lu,cur_limit=%lu,size_none=%lu\n",
> + map->size_max_limit, map->size_cur_limit, map->size_none);
Better integrate size_none=%lu in the line above, as none=%lu.
> printf("version=%d\n", map->timestamp);
> indent += 1;
> for (entry = vm_map_first_entry(map);
> diff --git a/vm/vm_user.c b/vm/vm_user.c
> index 7f706d71..0c9d11c6 100644
> --- a/vm/vm_user.c
> +++ b/vm/vm_user.c
> @@ -808,3 +808,72 @@ kern_return_t vm_pages_phys(
>
> return KERN_SUCCESS;
> }
> +
> +/*
> + * vm_set_size_limit
> + *
> + * Sets the current/maximum virtual adress space limits
> + * of the `target_task`.
> + *
> + * The host privileged port must be provided to increase
> + * the max limit.
> + */
> +kern_return_t
> +vm_set_size_limit(
> + const ipc_port_t host_port,
> + vm_map_t map,
> + vm_size_t current_limit,
> + vm_size_t max_limit)
> +{
[...]
> + vm_map_lock(map);
> + if (max_limit > map->size_max_limit && ikot_host != IKOT_HOST_PRIV) {
> + vm_map_unlock(map);
> + return KERN_INVALID_ARGUMENT;
I'd say KERN_NO_ACCESS?
> + }
> +
> + map->size_cur_limit = current_limit;
> + map->size_max_limit = max_limit;
> + vm_map_unlock(map);
> +
> + return KERN_SUCCESS;
> +}
Samuel
