Re: Task server thread and task allocation/deallocation interfaces proposal

[Marcus Brinkmann]

At Fri, 04 Mar 2005 17:33:22 +0000,
Neal H. Walfield wrote:
> > ***task_create(hurd_cap_handle_t current_task_handle,
> >              hurd_cap_handle_t *new_task_handle)
> > 
> > This create a new task, with just one thread, and creates an handle
> > associated with it so you can act on the task.
> 
> The first argument should be a task server root capability found in
> the hurd_startup_data.

I disagree.  We need task groups, so we can kill off child tasks which
did not register themselves with the proc server.  This is not only
important for sub-hurds like boot, or similar applications where a
process doesn't register child tasks with the proc server, but also to
clean up after a partial fork() or exec(), which otherwise could leave
behind unregistered tasks in a broken state.

The task group ID can just be a protected word that can only be set by
a privileged server, for example proc, and which is inherited at task
creation.  proc could set this word to something as simple as the PID.

Killing a task should kill all tasks with the same task group ID, too.
This ensures that no stranded unregistered tasks are left behind.

So, yes, the task_create RPC should go to the "parent task"
capability.  Furthermore, we need a privileged "task_set_group_id" RPC
that sets the protected word.  And task_destroy should kill all tasks
in the same group.

BTW, one important thing to consider is how suid exec will work.  If
we want proper accounting, suid tasks should be donated by the starter
to the filesystem, but the filesystem needs total control over it.  So
some revoke-and-reparent operation will be needed: revoke to remove
access by the original starter, and reparent to associate the donated
task to the filesystem doing the suid exec.  So, the task_set_group_id
should also be permitted for non-privileged users in the special case
that you want to set the group ID to the group ID of your own task
(rather than any arbitrary group ID).  Ie, you claim a task you have
control over (you have the task capability) as belonging to your own
task group (there are no security problems with this I think - there
_are_ security issues with donating your own tasks to somebody else's
task group).

BTW, using a protected word as task group ID is just an optimization -
you could generalize this and use task group capabilities.  But I
think that the optimization is worthwhile here - task group as
distinct cap objects don't offer us enough to be worth the overhead I
think.  (IE, I don't see any value in giving another task group
control over your own task group, except for proc which has "control"
over all task groups, ie can arbitrary change the task group ID).

No "root task server" capability seems to be needed at all for what I
know.  The thing that comes next to it is the "control" or "master"
capability (or whatever it will be) that is used by proc.

> Second, allocating a task should not allocate a thread.  Those are
> separate operations there is no need to artificially merge them at
> this level.  This is easy enough to do at the POSIX level.

Well, maybe.  But not because they are separate operations, but
because there are optimizations possible then.  In particular, you can
allocate the address space lazily at first thread creation.

Consider the suid exec: The starter creates a task object and donates
it to the filesystem.  The filesystem revokes all access except its
own.  To be secure, all threads in the task must be trashed, and the
address space has to be purged (this means destroying the address
space and creating a new one).  This is most efficient if you didn't
create an address space in the first place and the task is still
"pristine".

However, if you are not donating the task, I don't see a point in
making this two operations.  A task without threads is pretty much
useless in L4.  So, as an optimization, I would not object in allowing
to request creation of N initial threads in the task.  IE, folding the
functionality of thread creation into the task creation RPC.  In fact,
this could even actually be one and the same RPC (with a flag saying
if you want the threads in the same or a new task).

> > -By a separate RPC, task_set_pager(hurd_cap_handle_t task_handle,
> >                                    l4_thread_id_t pager,
> >                                    l4_thread_id_t thread).
> 
> This isn't needed if we set the pager when creating the thread.  Then
> it automatically starts waiting for a message from the pager.

Yes.  I think that it should be part of the creation call.

> > *** task_thread_alloc(hurd_cap_handle_t task_handle,
> >                       l4_thread_id_t pager,
> >                       l4_word_t number_requested,
> >                       l4_word_t all_or_nothing,
> >                       l4_word_t *number_allocated,
> >                       l4_thread_id_t *thread_ids)
> > 
> > Neal wanted to alloc several threads at one time, so:
> > 
> > This allocates NUMBER threads, or less if the server cannot or does
> > not want (unless all_or_nothing is set, in which case no thread is
> > allocated). Every created thread has its pager set to PAGER.
> 
> This looks good.  But rename all_or_nothing to flags and create a
> macro called TASK_THREAD_ALLOW_PARTIAL.  This way, we can add more
> flags later.

Yes.  Allocating multiple threads at once and feed them into a local
cache is a good optimization.

However, I wonder why you need to fail if you can't allocate N
threads.  You could also just always default to "allow partial" and
then deallocate the threads if you didn't get as many as you wanted,
in case you really don't want them.

So, unless somebody shows me why such a flag is needed, I would just
vote for not having such a flag at all.
 
> > *** task_thread_dealloc(hurd_cap_handle_t task_handle,
> >                         l4_word_t number,
> >                         l4_thread_id_t *thread_ids)
> > 
> > This destroy the threads and put the thread ids back in the free
> > thread ids list.
> 
> Good.  But you need to know how many were successfully deallocated.
> If I pass 3 tids and the second is bad (but the first and third are
> valid) then only the first should be deallocated and an error, such as
> EINVAL, should be returned and the number of deallocations set to 1.

I don't think that's a smart interface.  If you want to allow to
return RPC "out" arguments _and_ an error code, then you should
realize that this means that:

1. All our RPC server stubs must return valid return arguments, even
   on errors.  (ie, it must make sure that returned caps are
   HURD_CAP_NULL, all "nr" types are 0, pointers are 0 or -1 or
   similar, etc).  In particular, for all return arguments there must
   be a value that indicates that the return argument is not valid.

2. All client RPC stubs must go through all return arguments and clean
   them up, even on errors.

I think that this makes the RPC stub code generation a lot harder and
increases interface complexity enormously.  And this for a small
number of interfaces that potentially work this way.

Instead, I would suggest that you change the interface to return the
failure reason as part of the return arguments, not as the error code
of the whole RPC, and just return success.  Then only the caller of
this particular RPC must be changed to not only check the return code,
but also how many threads were actually deallocated and do something
in case this was not the whole list.  But, the caller of such an
interface must already be aware of the possibility of partial success,
so this is not a cost that is imposed by my suggestion.  Rather, my
suggestion aims at not introducing costs for _all other_ RPCs.

There is a semantical issue here.  I think for simplicity, it is
worthwhile to consider that anything but "0" returned from an IPC
indicates complete failure - in particular that no arguments are
returned.  Otherwise, we will have to make sure that _all_ return
arguments are valid whenever we return an error.  This is important to
make the RPC stub code work properly.

So, for example, you could return N error codes, one for each thread
for which deallocation was attempted.  Or you could just return the
number X of the deallocated threads starting from the first one, and
an error code that gives a reason for the failure of the X+1 one.

> > When there is only one thread left, the thread is not destroyed but
> > just inactivated.
> 
> This is an interesting case.  I think it might be better to just
> implicitly deallocate the address space.

I agree with Neal here.  I think that there is no need to keep tasks
with an address space with mappings around that doesn't contain any
threads.

Dealing with the need to keep one thread around to keep the address
space intact can happily be punted to the user code.

> The rest of this patch looks fine as do the other 2 patches.  Marcus
> can check them in if he is happy with them.

I'll look at it later...

Marcus