Re: [PATCH v3 1/2] pidfd: show pids for nested pid namespaces in fdinfo

On Fri, Oct 11, 2019 at 05:30:03PM +0200, Jann Horn wrote:

On Fri, Oct 11, 2019 at 5:17 PM Christian Brauner
[off-list ref] wrote:

quoted

On Fri, Oct 11, 2019 at 04:55:59PM +0200, Jann Horn wrote:

quoted

On Fri, Oct 11, 2019 at 2:23 PM Christian Kellner [off-list ref] wrote:

quoted

The fdinfo file for a process file descriptor already contains the
pid of the process in the callers namespaces. Additionally, if pid
namespaces are configured, show the process ids of the process in
all nested namespaces in the same format as in the procfs status
file, i.e. "NSPid:\t%d\%d...". This allows the easy identification
of the processes in nested namespaces.

[...]

quoted

 #ifdef CONFIG_PROC_FS
+static inline void print_pidfd_nspid(struct seq_file *m, struct pid *pid,
+                                    struct pid_namespace *ns)

`ns` is the namespace of the PID namespace of the procfs instance
through which the file descriptor is being viewed.

quoted

+{
+#ifdef CONFIG_PID_NS
+       int i;
+
+       seq_puts(m, "\nNSpid:");
+       for (i = ns->level; i <= pid->level; i++) {

ns->level is the level of the PID namespace associated with the procfs
instance through which the file descriptor is being viewed. pid->level
is the level of the PID associated with the pidfd.

quoted

+               ns = pid->numbers[i].ns;
+               seq_put_decimal_ull(m, "\t", pid_nr_ns(pid, ns));
+       }
+#endif
+}

I think you assumed that `ns` is always going to contain `pid`.
However, that's not the case. Consider the following scenario:

 - the init_pid_ns has two child PID namespaces, A and B (each with
its own mount namespace and procfs instance)
 - process P1 lives in A
 - process P2 lives in B
 - P1 opens a pidfd for itself
 - P1 passes the pidfd to P2 (e.g. via a unix domain socket)
 - P2 reads /proc/self/fdinfo/$pidfd

Now the loop will print the ID of P1 in A. I don't think that's what
you intended? You might want to bail out if "pid_nr_ns(pid, ns) == 0",
or something like that.

I assumed the same thing happens when you pass around an fd for
/proc/self/status and that's why I didn't object to this behavior.

I don't see how /proc/$pid/status is relevant. In the
/proc/$pid/status case, the output is the list of PIDs starting at the
PID namespace the procfs is associated with; and the process is always
contained in that namespace, which also means that the first PID
listed is the one in the PID namespace of the procfs instance. In the
pidfd case, the process is not necessarily contained in that
namespace, and the output doesn't make sense.

I might be misreading what you're saying.
(Maybe I'm doing something obviously wrong.)
If I compile the following two programs:
b2: https://paste.ubuntu.com/p/xthMsCXy3s/
c2: https://paste.ubuntu.com/p/y5HSzyMQJr/

Then in shell1
sudo unshare --mount --pid --fork --mount-proc

and in shell2
sudo unshare --mount --pid --fork --mount-proc

and run b2 in shell1 and c2 in shell2 which sends around an fd for
/proc/b2/status to c2. Now c2 reads b2's status file via the fd it
received. The c2 will see the pid of b2 in b2's pid namespace even
though the process is not contained in the pid namespace of c2.

Christian