Re: [PATCH] new interface: memory_object_get_proxy

[Sergey Bugaev]

On Sat, Oct 30, 2021 at 9:38 AM Joan Lledó <jlledom@mailfence.com> wrote:
> El 24/10/21 a les 19:50, Sergey Bugaev ha escrit:
> > I would expect the request port argument to be a vm_task_t (i.e. a
> > vm_map), not a full task. But I see that you need to pass
> > task->itk_space to memory_object_create_proxy (). But
> > memory_object_create_proxy () doesn't actually need the task either,
> > it just checks it against NULL (as usual) and never uses it again. So
> > maybe it'd be cleaner to make both calls accept a vm_task_t? Not that
> > this matters.
>
> yes, that's true, but if we remove the parameter from
> memory_object_create_proxy() then we must remove it from all calls in
> the code, and in the documentation if there's any. So that's beyond the
> scope of this patch, it's something we can do later on.

That argument is needed simply because it's the request port, so we
can't remove it entirely. What I meant is we could change its type
from ipc_space_t to vm_task_t; then the userspace would still be
passing a task port (task_t), but you'd pass a vm_map in the kernel.
And there aren't any other in-kernel callers of
memory_object_create_proxy () besides the one you're adding, so that
shouldn't be an issue either.

But you're right that this could be done separately if you prefer to
go with the full task_t for now.

> > Should the implementation cap the length to that of the entry
> > silently, or should it return an error if called with an overly long
> > len argument?
> >
>
> I don't know, Samuel, what do you think?

Actually now that I think about it, it would probably make sense for
this to, some day, transparently support proxying multiple regions,
which is something that the memory object proxies API supports in
theory (but it's currently unimplemented). So maybe it should return a
"not yet supported" error for now.

> I'm not familiar with the concept of consuming objects, could you
> explain it to me?

Consuming, as in taking ownership of, or "eating" the passed-in reference.

If a call consuming something, the callee takes ownership of the
passed-in reference:

some_resource_t global_resource;

void
callee (some_resource_t *argument)
{
  /* Take ownership.  */
  global_resource = argument;
}

void
caller ()
{
  some_resource_t *res = make_resource ();
  callee (res);
  /* No need to release the resource -- the callee took ownership of it.  */
}

If the call is *not* consuming something, the caller keeps ownership
of the reference, and the callee has to add another reference if it
wants to keep the resource for longer:

some_resource_t global_resource;

void
callee (some_resource_t *argument)
{
  /* Add a reference since we're storing the resource for longer.  */
  some_resource_ref (argument);
  global_resource = argument;
}

void
caller ()
{
  some_resource_t *res = make_resource ();
  callee (res);
  some_resource_rele (res);
}

For MIG routines, the convention is that the routine implementation
takes ownership of all passed-in resources (except the request port)
if and only if it returns KERN_SUCCESS (aka 0) or MIG_NO_REPLY. In
other cases (so, for error returns) MIG runtime (or technically not
even MIG but rather mach_msg_server ()) will destroy any resources in
the message. If we look at mach_ports_register () in kern/ipc_tt.c, we
can see it copies the ports from the 'memory' array and takes
ownership of them (doesn't add any references). But
memory_object_create_proxy () does add a reference to the memory
object by doing ipc_port_copy_send (object[0]). It would seem that it
should not do that; but then I'm not sure why this doesn't cause a
leak — I know Samuel has ensured that proxies don't cause leaks, at
least not immediately visible ones.

So my point is, *if* we decide that memory_object_create_proxy () is
indeed supposed to consume the passed-in- object reference, then your
routine should create that reference before calling
memory_object_create_proxy (), i.e. you should pass ipc_port_copy_send
(entry->object.vm_object->pager) instead of passing
entry->object.vm_object->pager directly.

I hope this makes sense; I'd be happy to expand if not.

Sergey