Re: [PATCH 2/2] clone.2: added clone3() set_tid information

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

Hello Christian, and Adrian,

Christian, thanks for the review!

On 11/28/19 6:24 PM, Christian Brauner wrote:
> On Thu, Nov 28, 2019 at 01:46:50PM +0100, Adrian Reber wrote:
>> Signed-off-by: Adrian Reber <areber@redhat.com>
>> ---
>>   man2/clone.2 | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>
>> diff --git a/man2/clone.2 b/man2/clone.2
>> index 076b9258e..59c13ec35 100644
>> --- a/man2/clone.2
>> +++ b/man2/clone.2
>> @@ -195,6 +195,8 @@ struct clone_args {
>>       u64 stack;        /* Pointer to lowest byte of stack */
>>       u64 stack_size;   /* Size of stack */
>>       u64 tls;          /* Location of new TLS */
>> +    u64 set_tid;      /* Pointer to a \fIpid_t\fP array */
>> +    u64 set_tid_size; /* Number of elements in \fIset_tid\fP */
>>   };
>>   .EE
>>   .in
>> @@ -262,6 +264,8 @@ flags & 0xff	exit_signal
>>   stack	stack
>>   \fP---\fP	stack_size
>>   tls	tls	See CLONE_SETTLS
>> +\fP---\fP	set_tid	See below for details
>> +\fP---\fP	set_tid_size
>>   .TE
>>   .RE
>>   .\"
>> @@ -285,6 +289,74 @@ options when waiting for the child with
>>   If no signal (i.e., zero) is specified, then the parent process is not signaled
>>   when the child terminates.
>>   .\"
>> +.SS The set_tid array
>> +.PP
>> +The
>> +.I set_tid
>> +array is used to select a certain PID for the process to be created by
>> +.BR clone3 ().
>> +If the PID of the newly created process should only be set for the current
>> +PID namespace or in the newly created PID namespace (if
>> +.I flags
>> +contains
>> +.BR CLONE_NEWPID )
>> +then the first element in the
>> +.I set_tid
>> +array has to be the desired PID and
>> +.I set_tid_size
>> +needs to be 1.
>> +.PP
>> +If the PID of the newly created process should have a certain value in
>> +multiple PID namespaces the
>> +.I set_tid
>> +array can have multiple entries. The first entry defines the PID in the most
>> +nested PID namespace and all following entries contain the PID of the
>> +corresponding parent PID namespace. The number of PID namespaces in which a PID
>> +should be set is defined by
>> +.I set_tid_size
>> +which cannot be larger than the number of currently nested PID namespaces.
> 
> "It's upper cap is the kernel-enforced general nesting limit."
> or sm like that

See my comments in a separate mail where Adrian replied to the above.

>> +.PP
>> +To create a process with the following PIDs:
> 
> in a pid namespace hierarchy

(okay)

>> +.RS
>> +.TS
>> +lb lb
>> +l l .
>> +PID NS level	Requested PID
>> +0 (host)	31496
>> +1	42
>> +2	7
>> +.TE
>> +.RE
>> +.PP
> 

Christian, you trim away the piece:

[[
     +The
     +.I set_tid
     +array would need to be filled with:
]]

that you suggest replacing with:

 > Set the array to:

I think the suggested change is fine, but when you trim the text
in this way (as you do a few times below, it makes it overly
difficult to review your suggestions, since I must separately
refer back to Adrian's original mail to see what is being
replaced (which is not comfortable on my 13" laptop
11km above Belgium :-)).

>> +.PP
>> +.EX
>> +	set_tid[0] = 7;
>> +	set_tid[1] = 42;
>> +	set_tid[2] = 31496;
>> +	set_tid_size = 3;
>> +.EE
>> +.PP
>> +If only the PID of the two innermost PID namespaces

s/PID of/PIDs in/

> 
> need to be specified set the array to:

(okay; but see my comment about trimming above.)

>> +.PP
>> +.EX
>> +	set_tid[0] = 7;
>> +	set_tid[1] = 42;
>> +	set_tid_size = 2;
>> +.EE
>> +.PP
>> +The PID in the PID namespaces outside the two innermost PID namespaces
>> +is then selected the same way as any other PID is selected.
> 
> s/is then/will be/

(okay)

>> +.PP
>> +Only a privileged process
>> +.RB ( CAP_SYS_ADMIN )
> 
> Maybe more specific:
> 
> The set_tid feature requires CAP_SYS_ADMIN in all owning user namespaces
> of the target pid namespaces.

Excellent addition.

>> +can set
>> +.I set_tid
>> +to select a PID for the process to be created.
>> +.\"
>>   .SS The flags mask
>>   .PP
>>   Both
>> @@ -1379,6 +1451,16 @@ in the
>>   .I flags
>>   mask.
>>   .TP
>> +.BR EINVAL " (" clone3 "() only)"
>> +.I set_tid_size
>> +larger than current number of nested PID namespaces or maximum number of
>> +nested PID namespaces was specified.

The last piece of that sentence feels redundant to me, and it
could/should  be removed, I think. The current number of nested PID
namespaces can't exceed the maximum number of nested namespaces, so
mentioning the latter as a limit seems unnecessary (and even a little
confusing, since it leaves the reader wondering whether "current"
could exceed "maximum".)

Or have I misunderstood what you intend to convey, Adrian?

>> +.TP
>> +.BR EINVAL " (" clone3 "() only)"
>> +If one of the PIDs specified in
>> +.I set_tid
>> +was an invalid PID.
>> +.TP
>>   .B ENOMEM
>>   Cannot allocate sufficient memory to allocate a task structure for the
>>   child, or to copy those parts of the caller's context that need to be
>> @@ -1450,6 +1532,14 @@ mask and the caller is in a chroot environment
>>   (i.e., the caller's root directory does not match the root directory
>>   of the mount namespace in which it resides).
>>   .TP
>> +.BR EPERM " (" clone3 "() only)"
>> +If
>> +.I set_tid
>> +with
>> +.I set_tid_size
>> +larger than 0 was specified by an unprivileged process (process without
>> +\fBCAP_SYS_ADMIN\fP).
> 
> without CAP_SYS_ADMIN in _one_ of the owning user namespaces of the
> target pid namespace
> 
> maybe?

Adrian, I think there's no need to mention 'set_tid'. That's implied
by 'set_tid_size' being greater than 0.

How about:

[[
.BR EPERM " (" clone3 "() only)"
.I set_tid_size
was greater than zero, and the caller lacks the
.B CAP_SYS_ADMIN
capability in one or more of the user namespaces that own the
corresponding PID namespaces.
]]

?

> Where's EEXIST?

Indeed!

I speculate (without having looked at the kernel code):

[[
.BR EPERM " (" clone3 "() only)"
One or more of the PIDs specified in
.I set_tid
already exists in the corresponding namespace.
]]

Thanks,

Michael