Linux-man Archive on lore.kernel.org
 help / color / Atom feed
From: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
To: Christian Brauner <christian.brauner@ubuntu.com>,
	Adrian Reber <areber@redhat.com>
Cc: linux-man@vger.kernel.org
Subject: Re: [PATCH 2/2] clone.2: added clone3() set_tid information
Date: Sun, 1 Dec 2019 13:18:52 +0100
Message-ID: <49be41bf-f6e5-c0ea-e6fb-22eeea9656e8@gmail.com> (raw)
In-Reply-To: <20191128172404.xan6vzaoofjeysq2@wittgenstein>

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

  parent reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-28 12:46 [PATCH 1/2] clone.2: tfix Adrian Reber
2019-11-28 12:46 ` [PATCH 2/2] clone.2: added clone3() set_tid information Adrian Reber
2019-11-28 17:24   ` Christian Brauner
2019-11-29 13:56     ` Adrian Reber
2019-12-01 12:17       ` Michael Kerrisk (man-pages)
2019-12-01 12:18     ` Michael Kerrisk (man-pages) [this message]
2019-12-02 14:25       ` Adrian Reber
2019-12-01 12:18   ` Michael Kerrisk (man-pages)
2019-11-28 13:30 ` [PATCH 1/2] clone.2: tfix Christian Brauner
2019-12-01 12:16 ` Michael Kerrisk (man-pages)

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49be41bf-f6e5-c0ea-e6fb-22eeea9656e8@gmail.com \
    --to=mtk.manpages@gmail.com \
    --cc=areber@redhat.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=linux-man@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-man Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-man/0 linux-man/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-man linux-man/ https://lore.kernel.org/linux-man \
		linux-man@vger.kernel.org
	public-inbox-index linux-man

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-man


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git