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

Michael Kerrisk (man-pages)
2019-12-01T12:18:58Z

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