Re: [PATCH gnumach v1] Implement per task virtual memory limit

[Sergey Bugaev]

Merry Christmas :)

On Tue, Dec 24, 2024 at 8:11 PM Diego Nieto Cid <dnietoc@gmail.com> wrote:
>      *      HOST_PORT must be the privileged host control port
>      *      if the caller desires to increase the current max limit.
>      *
>      *      On the other hand, if the max limit is being decreased, the
>      *      unprivileged host name (as returned by mach_host_self()) can
>      *      be provided.

"host control port" rather than "host name"

>      *
>      *      Returns:
>      *        - KERN_SUCESS when the size limit has been set
>      *        - KERN_INVALID_ARGUMENT in the following cases:
>      *            * current_limit > max_limit
>      *            * map is NULL
>      *            * attemt to increase max limi without providing
>      *              the privileged control host port.

Here, too, "privileged host control port" (just English things...)

> Should I still return KERN_INVALID_HOST in case the "host port dance" fails?
> Or is it ok to map every error to KERN_INVALID_ARGUMENT?

It doesn't actually really matter for anything, I doubt anyone in
userland is going to care precisely which error this returns if it
fails. But that being said, if an invalid host port is supplied, it's
a good idea to return KERN_INVALID_HOST, KERN_INVALID_TASK for an
invalid map (task), and if something is off about the limits,
KERN_INVALID_ARGUMENT. For the specific case where a valid host port
(but not a host priv port) is supplied but we're trying to increase
the limit, I think I would prefer KERN_INVALID_ARGUMENT.

So:

+kern_return_t
+vm_set_size_limit(...)
+{
+       ipc_kobject_type_t ikot_host = IKOT_NONE;
+
+       if (limit checks)
+               return KERN_INVALID_ARGUMENT;
+
+       if (map checks)
+               return KERN_INVALID_TASK;
+
+       if (host checks)
+               return KERN_INVALID_HOST;
+
+       vm_map_lock(map);
+       if (max_limit > map->size_max_limit && ikot_host != IKOT_HOST_PRIV) {
+               vm_map_unlock(map);
+               return KERN_INVALID_ARGUMENT;
+       }
...

> > Perhaps also skip this for kernel maps?
> >
>
> Like this?
>
>     if (pmap != kernel_pmap) {
>         map->size_cur_limit = vm_page_mem_size() / 2;
>         map->size_max_limit = vm_page_mem_size() / 2;
>     } else {
>         map->size_cur_limit = (~0UL);
>         map->size_max_limit = (~0UL);
>     }

Yeah.

> Also, I couldn't find any callers to projected_buffer_allocate :/

Me neither, projected buffers and whether they're being used for
anything still remains a little mystery to me.

> > > +kern_return_t
> > > +vm_get_size_limit(
> > > +       vm_map_t        map,
> > > +       vm_size_t       *current_limit,
> > > +       vm_size_t       *max_limit)
> > > +{
> >
> > Missing the check for map == VM_MAP_NULL.
> >
>
> Hmm, I've got an error from mach_msg when the receiver is NULL.
> But I'll add it just in case.

If you send the RPC to MACH_PORT_NULL (or other invalid port) you'll
get an error at message-send time, and the kernel-side
vm_get_size_limit won't even get invoked, indeed. The map ==
VM_MAP_NULL case is for when you send the RPC to a valid kernel port
that does not correspond to a map, so basically anything but a task
port: a thread port, a host port, a device port, etc. In that case the
message-send succeeds and the incoming RPC is accepted by the overall
RPC handling machinery, and it's up to the actual routine
implementation (which is your code here) to notice that the request
object is invalid and refuse the call.

In the Hurd userland, it's typical to return EOPNOTSUPP in cases like
this, because the request object is valid but doesn't support this
operation (since it's of a different type). In Mach we don't have
that, but instead there are special-case KERN_INVALID_FOOBAR codes for
a few types. And if we don't have a code for a specific type,
KERN_INVALID_ARGUMENT (or something) could be used as a catch-all. But
for this case we do, it's KERN_INVALID_TASK.

> > And remember, you still have to handle the cases of:
> > - deallocating a VM_PROT_NONE mapping,
>
> Isn't this hunk enough?
>
>     @@ -2042,6 +2104,7 @@ void vm_map_entry_delete(
>
>             vm_map_entry_unlink(map, entry);
>             map->size -= size;
>     +       map->size_none -= ((entry->max_protection == VM_PROT_NONE) ? size 
> : 0);
>
>             vm_map_entry_dispose(map, entry);
>      }

Ah right, it likely is.

> > - downgrading an existing mapping to VM_PROT_NONE.
>
> Hmm, maybe I should start tracking the protection instead of the 
> max_protection?
> Or do you mean calls to vm_map_protect with set_max == TRUE?

The latter, yes. And you need to check whether it's actually changing
things, so if max protection is already VM_PROT_NONE, don't subtract.

Sergey