Re: [PATCH 2/2] fanotify: Add pidfd support to the fanotify API

On Mon, Apr 19, 2021 at 12:21:39PM +0200, Jan Kara wrote:

On Fri 16-04-21 09:22:25, Matthew Bobrowski wrote:

quoted

Introduce a new flag FAN_REPORT_PIDFD for fanotify_init(2) which
allows userspace applications to control whether a pidfd is to be
returned instead of a pid for `struct fanotify_event_metadata.pid`.

FAN_REPORT_PIDFD is mutually exclusive with FAN_REPORT_TID as the
pidfd API is currently restricted to only support pidfd generation for
thread-group leaders. Attempting to set them both when calling
fanotify_init(2) will result in -EINVAL being returned to the
caller. As the pidfd API evolves and support is added for tids, this
is something that could be relaxed in the future.

If pidfd creation fails, the pid in struct fanotify_event_metadata is
set to FAN_NOPIDFD(-1). Falling back and providing a pid instead of a
pidfd on pidfd creation failures was considered, although this could
possibly lead to confusion and unpredictability within userspace
applications as distinguishing between whether an actual pidfd or pid
was returned could be difficult, so it's best to be explicit.

Signed-off-by: Matthew Bobrowski <redacted>

Overall this looks OK to me. Just one style nit & one question below in
addition to what Amir wrote.

Thanks for the quick review Jan!

quoted

---
 fs/notify/fanotify/fanotify_user.c | 33 +++++++++++++++++++++++++++---
 include/linux/fanotify.h           |  2 +-
 include/uapi/linux/fanotify.h      |  2 ++
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 9e0c1afac8bd..fd8ae88796a8 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c

@@ -329,7 +329,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	struct fanotify_info *info = fanotify_event_info(event);
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct file *f = NULL;
-	int ret, fd = FAN_NOFD;
+	int ret, pidfd, fd = FAN_NOFD;
 	int info_type = 0;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata.pid = pid_vnr(event->pid);
+
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+		pid_has_task(event->pid, PIDTYPE_TGID)) {

Please align the rest of the condition to the opening brace. I.e., like:

	if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
	    pid_has_task(event->pid, PIDTYPE_TGID)) {

ACK.

BTW, why is the pid_has_task() check here?

My thought was that we add a means of ensuring that event->pid holds a
reference to a thread-group leader as pidfds aren't supported for
individual threads just yet. The same check is implemented in
pidfd_open(), so I thought to make the preliminary checks consistent.

Actually, now that I've writeten that, perhaps the pid_has_task()
check can be rolled up into pidfd_create()?

And why is it OK to fall back to returning pid if pid_has_task() is
false?

Ah, I see, it's not OK. Good catch Jan! I will need to fix this up in
the follow up series.

/M