On Tue, Mar 26, 2019 at 04:55:11PM +0100, Christian Brauner wrote:

The pidctl() syscalls builds on, extends, and improves translate_pid() [4].
I quote Konstantins original patchset first that has already been acked and
picked up by Eric before and whose functionality is preserved in this
syscall:

"Each process have different pids, one for each pid namespace it belongs.
 When interaction happens within single pid-ns translation isn't required.
 More complicated scenarios needs special handling.

 For example:
 - reading pid-files or logs written inside container with pid namespace
 - writing logs with internal pids outside container for pushing them into
 - attaching with ptrace to tasks from different pid namespace

 Generally speaking, any cross pid-ns API with pids needs translation.

 Currently there are several interfaces that could be used here:

 Pid namespaces are identified by device and inode of /proc/[pid]/ns/pid.

 Pids for nested pid namespaces are shown in file /proc/[pid]/status.
 In some cases pid translation could be easily done using this information.
 Backward translation requires scanning all tasks and becomes really
 complicated for deeper namespace nesting.

 Unix socket automatically translates pid attached to SCM_CREDENTIALS.
 This requires CAP_SYS_ADMIN for sending arbitrary pids and entering
 into pid namespace, this expose process and could be insecure."

The original patchset allowed two distinct operations implicitly:
- discovering whether pid namespaces (pidns) have a parent-child
  relationship
- translating a pid from a source pidns into a target pidns

Both tasks are accomplished in the original patchset by passing a pid
along. If the pid argument is passed as 1 the relationship between two pid
namespaces can be discovered.
The syscall will gain a lot clearer syntax and will be easier to use for
userspace if the task it is asked to perform is passed through a
command argument. Additionally, it allows us to remove an intrinsic race
caused by using the pid argument as a way to discover the relationship
between pid namespaces.
This patch introduces three commands:

/* PIDCMD_QUERY_PID */
PIDCMD_QUERY_PID allows to translate a pid between pid namespaces.
Given a source pid namespace fd return the pid of the process in the target
namespace:

Could we call this PIDCMD_TRANSLATE_PID please? QUERY is confusing/ambigious
IMO (see below).

1. pidctl(PIDCMD_QUERY_PID, pid, source_fd, -1, 0)
  - retrieve pidns identified by source_fd
  - retrieve struct pid identifed by pid in pidns identified by source_fd
  - retrieve callers pidns
  - return pid in callers pidns

2. pidctl(PIDCMD_QUERY_PID, pid, -1, target_fd, 0)
  - retrieve callers pidns
  - retrieve struct pid identifed by pid in callers pidns
  - retrieve pidns identified by target_fd
  - return pid in pidns identified by target_fd

3. pidctl(PIDCMD_QUERY_PID, 1, source_fd, -1, 0)
  - retrieve pidns identified by source_fd
  - retrieve struct pid identifed by init task in pidns identified by source_fd
  - retrieve callers pidns
  - return pid of init task of pidns identified by source_fd in callers pidns

4. pidctl(PIDCMD_QUERY_PID, pid, source_fd, target_fd, 0)
  - retrieve pidns identified by source_fd
  - retrieve struct pid identifed by pid in pidns identified by source_fd
  - retrieve pidns identified by target_fd
  - check whether struct pid can be found in pidns identified by target_fd
  - return pid in pidns identified by target_fd

/* PIDCMD_QUERY_PIDNS */
PIDCMD_QUERY_PIDNS allows to determine the relationship between pid
namespaces.
In the original version of the pachset passing pid as 1 would allow to
deterimine the relationship between the pid namespaces. This is inherhently
racy. If pid 1 inside a pid namespace has died it would report false
negatives. For example, if pid 1 inside of the target pid namespace already
died, it would report that the target pid namespace cannot be reached from
the source pid namespace because it couldn't find the pid inside of the
target pid namespace and thus falsely report to the user that the two pid
namespaces are not related. This problem is simple to avoid. In the new
version we simply walk the list of ancestors and check whether the
namespace are related to each other. By doing it this way we can reliably
report what the relationship between two pid namespace file descriptors
looks like.

1. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd1, 0) == 0
   - pidns_of(ns_fd1) and pidns_of(ns_fd2) are unrelated to each other

2. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 1
   - pidns_of(ns_fd1) == pidns_of(ns_fd2)

3. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 2
   - pidns_of(ns_fd1) is ancestor of pidns_of(ns_fd2)

4. pidctl(PIDCMD_QUERY_PIDNS, 0, ns_fd1, ns_fd2, 0) == 3
   - pidns_of(ns_fd2) is ancestor of pidns_of(ns_fd1)

Why not call this PIDCMD_COMPARE_PIDNS, since a comparison is what you're doing.

Again QUERY is ambigious here. Above you called QUERY to translate something,
now you're calling QUERY to mean compare something. I suggest to be explicit
about the operation PIDCMD_<OPERATION>_<OPERAND-TYPE>.

Arguably, 2 syscalls for this is cleaner:
pid_compare_namespaces(ns_fd1, ns_fd2);
pid_translate(pid, ns_fd1, nds_fd2);

These two commands - PIDCMD_QUERY_PID and PIDCMD_QUERY_PIDNS - cover and
improve the functionality expressed implicitly in translate_pid() before.

/* PIDCMD_GET_PIDFD */

And this can be a third syscall:
pidfd_translate(pid, ns_fd1, ns_fd2).

I am actually supportive of Daniel's view that by combining too many
arguments into a single syscall, becomes confusing and sometimes some
arguments have to be forced to 0 in the single shoe-horned syscall. Like you
don't need a pid to compare to pid-ns, so user has to set that to 0.

More comments below...

This command allows to retrieve file descriptors for processes and removes
the dependency of pidfds and thereby the pidfd_send_signal() syscall on
procfs. First, multiple people have expressed a desire to do this even when
pidfd_send_signal() was merged. It is even recorded in the commit message
for pidfd_send_signal() itself
(cf. commit 3eb39f47934f9d5a3027fe00d906a45fe3a15fad):
Q-06: (Andrew Morton [1])
      Is there a cleaner way of obtaining the fd? Another syscall perhaps.
A-06: Userspace can already trivially retrieve file descriptors from procfs
      so this is something that we will need to support anyway. Hence,
      there's no immediate need to add another syscalls just to make
      pidfd_send_signal() not dependent on the presence of procfs. However,
      adding a syscalls to get such file descriptors is planned for a
      future patchset (cf. [1]).
Alexey made a similar request (cf. [2]).
Additionally, Andy made an additional, idependent argument that we should
go forward with non-proc-dirfd file descriptors for the sake of security
and extensibility (cf. [3]).

The pidfds are not associated with a specific pid namespaces but rather
only with struct pid. What the pidctl() syscall enforces is that when a
caller wants to retrieve a pidfd file descriptor for a pid in a given
target pid namespace the caller 
- must have been given access to two file descriptors referring
  to target and source pid namespace
- the source pid namespace must be an ancestor of the target pid
  namespace
- the pid must be translatable from the source pid namespace into the
  target pid namespace

1. pidctl(PIDCMD_GET_PIDFD, pid, source_fd, -1, 0)
  - retrieve pidns identified by source_fd
  - retrieve struct pid identifed by pid in pidns identified by source_fd
  - retrieve callers pidns
  - return pidfd

2. pidctl(PIDCMD_GET_PIDFD, pid, -1, target_fd, 0)
  - retrieve callers pidns
  - retrieve struct pid identifed by pid in callers pidns
  - retrieve pidns identified by target_fd
  - return pidfd

3. pidctl(PIDCMD_GET_PIDFD, 1, source_fd, -1, 0)
  - retrieve pidns identified by source_fd
  - retrieve struct pid identifed by init task in pidns identified by
    source_fd
  - retrieve callers pidns
  - return pidfd

4. pidctl(PIDCMD_GET_PIDFD, pid, source_fd, target_fd, 0)
  - retrieve pidns identified by source_fd
  - retrieve struct pid identifed by pid in pidns identified by source_fd
  - retrieve pidns identified by target_fd
  - check whether struct pid can be found in pidns identified by target_fd
  - return pidfd

These pidfds are allocated using anon_inode_getfd(), are O_CLOEXEC by
default and can be used with the pidfd_send_signal() syscall. They are not
dirfds and as such have the advantage that we can make them pollable or
readable in the future if we see a need to do so (which I'm pretty sure we
will eventually). Currently they do not support any advanced operations.

/* References */
[1]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/ (local)
[2]: https://lore.kernel.org/lkml/20190320203910.GA2842@avx2/ (local)
[3]: https://lore.kernel.org/lkml/CALCETrXO=V=+qEdLDVPf8eCgLZiB9bOTrUfe0V-U-tUZoeoRDA@mail.gmail.com/ (local)
[4]: https://lore.kernel.org/lkml/20181109034919.GA21681@altlinux.org/ (local)

[snip]

flags seems not needed since it is unused, but I get it that you may want to
have flags in the future? If yes, give example of future flags?

+{
+	struct pid_namespace *source_ns = NULL, *target_ns = NULL;

Setting these to NULL is no longer needed.

+	struct pid *struct_pid;
+	pid_t result;
+
+	if (flags)
+		return -EINVAL;
+
+	switch (cmd) {
+	case PIDCMD_QUERY_PID:
+		break;
+	case PIDCMD_QUERY_PIDNS:
+		if (pid)
+			return -EINVAL;
+		break;
+	case PIDCMD_GET_PIDFD:
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	source_ns = get_pid_ns_by_fd(source);
+	if (IS_ERR(source_ns))
+		return PTR_ERR(source_ns);
+
+	target_ns = get_pid_ns_by_fd(target);
+	if (IS_ERR(target_ns)) {
+		put_pid_ns(source_ns);
+		return PTR_ERR(target_ns);
+	}
+
+	if (cmd == PIDCMD_QUERY_PIDNS) {
+		result = pidns_related(source_ns, target_ns);
+	} else {
+		rcu_read_lock();
+		struct_pid = get_pid(find_pid_ns(pid, source_ns));
+		rcu_read_unlock();
+
+		if (struct_pid)
+			result = pid_nr_ns(struct_pid, target_ns);
+		else
+			result = -ESRCH;
+
+		if (cmd == PIDCMD_GET_PIDFD && (result > 0))
+			result = pidfd_create_fd(struct_pid, O_CLOEXEC);

pidfd_create_fd already does put_pid on errors..

+
+		if (!result)
+			result = -ENOENT;
+
+		put_pid(struct_pid);

so on error you would put_pid twice which seems odd..  I would suggest, don't
release the pid ref from within pidfd_create_fd, release the ref from the
caller. Speaking of which, I added to my list to convert the pid->count to
refcount_t at some point :)

+	}
+
+	put_pid_ns(target_ns);
+	put_pid_ns(source_ns);

This part looks more clean than before so good.

thanks,

 - Joel