Re: [PATCH v2 5/5] fanotify: add pidfd support to the fanotify API

On Thu, Jun 10, 2021 at 01:23:31PM +0200, Jan Kara wrote:

quoted

@@ -524,6 +561,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	}
 	metadata.fd = fd;
 
+	/*
+	 * Currently, reporting a pidfd to an unprivileged listener is not
+	 * supported. The FANOTIFY_UNPRIV flag is to be kept here so that a
+	 * pidfd is not accidentally leaked to an unprivileged listener.
+	 */
+	if (pidfd_mode && !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV)) {

Hum, you've added FAN_REPORT_PIDFD to FANOTIFY_ADMIN_INIT_FLAGS so this
condition should be always true? I don't think we need to be that much
defensive and would just drop the check here.

Yes, that's right, so dropping this check is also fine with me.

quoted

@@ -558,6 +632,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		put_unused_fd(fd);
 		fput(f);
 	}
+
+	if (pidfd < 0)
+		put_unused_fd(pidfd);
+

put_unused_fd() is not enough to destroy the pidfd you have. That will just
mark 'pidfd' as free in the fd table. You rather need to call close_fd()
here to fully close open file.

Ah, I see, put_unused_fd() doesn't free up the file instance. I will swap
this out with close_fd() instead.

Thanks for the suggestions Jan!

/M