Re: [PATCH] Linux: Implement per-thread user and group IDs

[Carlos O'Donell <carlos at redhat dot com>]

On 6/30/19 5:36 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>>> (Build-tested on all architectures with build-many-glibcs.py.
>>> Run-time-tested on x86-64, by running the new xtest as root.  I still
>>> need to implement the inheritance test Carlos requested.  Due to the
>>> similarities, I plan to write one separate test for both kinds of
>>> properties.)
>>
>> Right, the test for per-process is missing, we have only test for
>> per-thread inheritance, and null attr default, but not per-process
>> inheritance. I assume you'll fix it like you are going to fix the
>> other-thread fs test case?
> 
> You've already seen the new separate test.

Yes I have. I've been reviewing these somewhat in parallel.

>> The other thing I was thinking about is: Should a thread created
>> with per-thread uid/gid block SIGSETXID permanently from startup?
>> This way there can be no accident that the function sighandler_setxid
>> is called? If you don't like that then at least some belt-and-suspenders
>> code in sighandler_setxid to assert that a thread per-thread uid/gid
>> will not have this handler ever called? Even better... do both! :-)
> 
> We might have to use SIGSETXID for something else in the future (or
> perhaps we already do).  I would leave this as it is today.

Can you expand on this?

Today SIGSETXID is reserved explicitly for this use, and it's a per-process
signal handler, so it's always active until you remove the last use of
SIGSETXID by replacing it with a kernel mechanism for transitioning IDs.

It seems dangerous to me to leave it unblocked or not to check the thread's
attribute flag to exclude it from ever participating.

Given that this is an internal implementation detail we could change it later.

>>> diff --git a/NEWS b/NEWS
>>> index e63b69b930..83bd54b5e8 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -45,6 +45,12 @@ Major new features:
>>>    pthread_attr_setperthreadfs_np and pthread_attr_getperthreadfs_np have
>>>    been added in support of that.
>>>  
>>> +* Likewise, on Linux, threads can now be created in such a way that they
>>
>> s/Likewise, on/On/g
> 
> There's a preceding NEWS entry with similar wording, so I thought the
> “Likewise” would make sense here.

Each news entry should stand on its own so it can be quoted, or so it makes
sense when reading just one entry.

>>
>>> +  retain per-thread user and group IDs.  (This has always been the way how
>>> +  the Linux kernel implements threads.)  The functions
>>
>> Drop the parenthetical.
> 
> I will do that.
> 
>>> +
>>> +@deftypefun int pthread_attr_getperthreadid_np (pthread_attr_t
>> *restrict @var{attr}, int *restrict @var{scope})
>>
>> Typo. s/pthread_attr_getperthreadid_np/pthread_attr_getperthreadids_np/g
> 
> Will fix.
> 
>>> +static const struct test_case test_cases[] =
>>> +  {
>>> +   { OP_NONE, { 0, }, { 0, 0, 0}, {0, 0, 0} },
>>> +
>>> +   { OP_SETUID, { 1, }, { 1, 1, 1 }, { 0, } },
>>> +   { OP_SETEUID, { 2, }, { 0, 2, }, { 0, } },
>>> +   { OP_SETREUID, { 3, 4, }, { 3, 4, 4 }, { 0, } },
>>> +   { OP_SETRESUID, { 3, 4, 5 }, { 3, 4, 5 }, { 0, } },
>>> +
>>> +   { OP_SETGID, { 6, }, { 0, }, { 6, 6, 6 } },
>>> +   { OP_SETEGID, { 7, }, { 0, }, { 0, 7, } },
>>> +   { OP_SETREGID, { 8, 9, }, { 0, }, { 8, 9, 9 } },
>>> +   { OP_SETRESGID, { 10, 11, 12 }, { 0, }, { 10, 11, 12 } },
>>> +
>>> +   { OP_SETGROUPS, { -1, }, { 0, }, { 0, } },
>>> +   { OP_SETGROUPS, { 13, -1 }, { 0, }, { 0, } },
>>> +   { OP_SETGROUPS, { 13, 14, -1 }, { 0, }, { 0, } },
>>> +   { OP_SETGROUPS, { 13, 14, 15 }, { 0, }, { 0, } },
>>> +
>>> +   /* Final round of checks.  */
>>> +   { OP_NONE, { 0, }, { 0, }, {0, } },
>>
>> I find it more difficult to read and edit when elements of the
>> struct are elided. Could you please fill them all in?
> 
> I can do this, although it will add quite a bit of cruft.
> 

I disagree that this is cruft.

As they are now, the columns don't line up and reading the test
input at a glance is difficult.

>>> +/* An actual test thread.  CLOSURE is a pointer to struct
>>> +   thread_argument, and perform the test according to this
>>> +   information.  */
>>> +static void *
>>> +threadfunc (void *closure)
>>> +{
>>> +  struct thread_argument *arg = closure;
>>> +
>>> +  TEST_COMPARE (perthread_flag (pthread_self ()), arg->perthread);
>>> +
>>> +  if (arg->indirect)
>>> +    {
>>> +      /* Only indirect once.  */
>>> +      arg->indirect = false;
>>> +      /* Use the default attributes (NULL).  This verifies that the
>>> +         per-thread scope is sticky.  */
>>> +      pthread_t thr = xpthread_create (NULL, threadfunc, arg);
>>> +      TEST_COMPARE (perthread_flag (thr), arg->perthread);
>>> +      return xpthread_join (thr);
>>> +    }
>>> +
>>> +  const struct test_case *expected = &test_cases[0];
>>> +  gid_t expected_supplementary[3] = { -1, -1, -1 };
>>> +  int expected_supplementary_count = 0;
>>> +
>>> +  xpthread_barrier_wait (&barrier);
>>
>> All of these barrier waits should have notes about what they are
>> synchronizing with, and which point if possible, thta way a future
>> editor can know what they are wiating for. Perhaps numbering them
>> with comments might help here.
>>
>> I realized late that I had the same problem while reviewing the
>> other test case and it only occurred to me that we should just
>> comment the phases.
> 
> Hmm.  I'll see what I can do about this.
> 
>>> +  /* Main thread and two other shared threads are extras.  One
>>> +     per-thread test is skipped, so there are two extra threads.  */
>>
>> ^^^ This comment needs clarification. Is it trying to rationalize why it's
>> array_length (test_cases) + 2?
> 
> Yes, exactly.  I will expand the comment.
> 
>>> +  xpthread_barrier_init (&barrier, NULL, array_length (test_cases) + 2);
>>> +
>>> +  pthread_t perthread_threads[array_length (test_cases)];
>>> +  /* Use thread index zero for no-op checking.  */
>>
>> What is no-op checking?
> 
> It uses OP_NONE.  In general, this is when the thread observes the UID
> change from another thread and verifies that.  I will try to make this
> more explicit.

Thanks, this wasn't clear to me when reading through the test the first
time.

> 
>> This needs a block comment explaining what it is doing and why.
> 
> What about this?
> 
>   /* Test both the direct and indirect case.  Direct is when test
>      threads are launched with a per-thread attribute.  Indirect is
>      when a thread is first launched with the a per-thread attribute,
>      but the actual test runs in another sub-thread, created with a
>      NULL attribute.  */

OK.

> 
>>> +  for (int indirect = 0; indirect < 2; ++indirect)
>>> +    {
>>> +      check_perthread (indirect);
>>> +
> 
>     /* Test two scenarios, one where set*id function is called from
>        the main thread, and other one where it is called from another
>        thread.  */

OK.

> 
>>> +      for (int broadcast_from_main = 0; broadcast_from_main < 2;
>>> +           ++broadcast_from_main)
> 
>       /* Perform each of the test cases once, using a broadcast set*id
>          operation across multiple threads (those with per-process
>          scope).  */

OK.

> 
>>> +/* Set *SCOPE to PTHREAD_PER_PROCESS_NP or PTHREAD_PER_THREAD_NP,
>>> +   depending on the state of *ATTR.  */
>>
>> Wrong comment.
>>
>>> +int pthread_attr_getperthreadids_np (const pthread_attr_t *__restrict __attr,
>>> +				     int *__restrict __scope)
>>> +  __THROW __nonnull ((1, 2));
>>> +
> 
> Sorry, wrong comment in what way?

It says "Set ..." but this is the getter.

> 


-- 
Cheers,
Carlos.