Re: [PATCH v2 1/4] pidfd: support PIDFD_NONBLOCK in pidfd_open()

On Thu, Sep 03, 2020 at 04:58:09PM +0200, Oleg Nesterov wrote:

Christian, off-topic question...

On 09/02, Christian Brauner wrote:

quoted

-static int pidfd_create(struct pid *pid)
+static int pidfd_create(struct pid *pid, unsigned int flags)
 {
 	int fd;

 	fd = anon_inode_getfd("[pidfd]", &pidfd_fops, get_pid(pid),
-			      O_RDWR | O_CLOEXEC);
+			      flags | O_RDWR | O_CLOEXEC);

I just noticed this comment above pidfd_create:

	 * Note, that this function can only be called after the fd table has
	 * been unshared to avoid leaking the pidfd to the new process.

what does it mean?

Of course, if fd table is shared then pidfd can "leak" to another process,
but this is true for any file and sys_pidfd_open() doesn't do any check?

It's the same comment we added in kernel/fork.c to make callers aware
that they can leak a pidfd to another process unintentionally. Sure,
this is true of any fd but since pidfds were a new type of handle and on
another process at that we felt that this was important to spell out. The
"can only" should've arguably been "should probably".

In fact I think this helper buys nothing but adds the unnecessary get/put_pid,
we can kill it and change pidfd_open() to do

	SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
	{
		int fd;
		struct pid *p;

		if (flags & ~PIDFD_NONBLOCK)
			return -EINVAL;

		if (pid <= 0)
			return -EINVAL;

		p = find_get_pid(pid);
		if (!p)
			return -ESRCH;

		fd = -EINVAL;
		if (pid_has_task(p, PIDTYPE_TGID)) {
			fd = anon_inode_getfd("[pidfd]", &pidfd_fops, pid,
						flags | O_RDWR | O_CLOEXEC);
		}
		if (fd < 0)
			put_pid(p);
		return fd;
	}

Sure, I'd totally take a patch like that!

but this is cosmetic and off-topic too.

No, much appreciated. Good-looking code is important. :)

Christian