Add pidfd support to the fanotify API

The copy_info_to_user() function has been repurposed with the idea to

Quoted 2 lines · Matthew Bobrowski

The copy_info_to_user() function has been repurposed with the idea to

Sorry I gave comment on patch 5 before I knew you repurposed copy_info_to_user().
Perhaps it would be better to give a non ambiguous name like copy_info_records_to_user() or

copy_fid_info_records_to_user() and leave pidfd out of this helper. If you do wish to keep pidfd copy inside this helper and follow my suggestion on patch 5 you will have to pass in pidfd as another argument.

Introduce a new flag FAN_REPORT_PIDFD for fanotify_init(2) which allows userspace applications to control whether a pidfd info record containing a pidfd is to be returned with each event.

If FAN_REPORT_PIDFD is enabled for a notification group, an additional struct fanotify_event_info_pidfd object will be supplied alongside the generic struct fanotify_event_metadata within a single event. This functionality is analogous to that of FAN_REPORT_FID in terms of how the event structure is supplied to the userspace application. Usage of FAN_REPORT_PIDFD with FAN_REPORT_FID/FAN_REPORT_DFID_NAME is permitted, and in this case a struct fanotify_event_info_pidfd object will follow any struct fanotify_event_info_fid object.

Usage of FAN_REPORT_TID is not permitted with FAN_REPORT_PIDFD as the pidfd API only supports the creation of pidfds for thread-group leaders. Attempting to do so will result with a -EINVAL being returned when calling fanotify_init(2).

If pidfd creation fails via pidfd_create(), the pidfd field within struct fanotify_event_info_pidfd is set to FAN_NOPIDFD.

Signed-off-by Matthew Bobrowski <[email protected]>

---
 fs/notify/fanotify/fanotify_user.c | 65 +++++++++++++++++++++++++++---
 include/linux/fanotify.h           |  3 +-
 include/uapi/linux/fanotify.h      | 12 ++++++
 3 files changed, 74 insertions(+), 6 deletions(-)

fs/notify/fanotify/fanotify_user.c +14 -0

index 1e15f3222eb2..bba61988f4a0 100644
@@ -106,6 +106,8 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 #define FANOTIFY_EVENT_ALIGN 4
 #define FANOTIFY_FID_INFO_HDR_LEN \
 	(sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))
+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+	sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
 	if (fh_len)
 		info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+	if (info_mode & FAN_REPORT_PIDFD)
+		info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
 	return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
 	return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+				   char __user *buf,
+				   size_t count)
+{
+	struct fanotify_event_info_pidfd info = { };
+	size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+	if (WARN_ON_ONCE(info_len > count))
+		return -EFAULT;

fs/notify/fanotify/fanotify_user.c +36 -5

+
+	return info_len;
+}
+
 static int copy_info_to_user(struct fanotify_event *event,
 			     struct fanotify_info *info,
 			     unsigned int info_mode,

@@ -408,9 +436,12 @@ static int copy_info_to_user(struct fanotify_event *event,
 {
 	int ret, info_type = 0;
 	unsigned int fid_mode = info_mode & FANOTIFY_FID_BITS;
+	unsigned int pidfd_mode = info_mode & FAN_REPORT_PIDFD;
 
 	/*
 	 * Event info records order is as follows: dir fid + name, child fid.
+	 * If FAN_REPORT_PIDFD has been specified, then a pidfd info record
+	 * will follow the fid info records.
 	 */
 	if (fanotify_event_dir_fh_len(event)) {
 		info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :

@@ -465,10 +496,18 @@ static int copy_info_to_user(struct fanotify_event *event,
 		}
 
 		ret = copy_fid_info_to_user(fanotify_event_fsid(event),
-					    fanotify_event_object_fh(event),
-					    info_type, dot, dot_len,
-					    buf, count);
-	}
+					    fanotify_event_object_fh(event),
+					    info_type, dot, dot_len,
+					    buf, count);
+		if (ret < 0)
+			return ret;
+
+		buf += ret;
+		count -= ret;
+	}
+
+	if (pidfd_mode)
+		return copy_pidfd_info_to_user(event->pid, buf, count);
 
 	return ret;
 }

@@ -530,6 +569,15 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		fd_install(fd, f);
 
 	if (info_mode) {
+		/*
+		 * Complain if FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+		 * exclusion is ever lifted. At the time of implementing
+		 * FAN_REPORT_PIDFD, the pidfd API only supported the creation
+		 * of pidfds on thread-group leaders.
+		 */
+		WARN_ON_ONCE((info_mode & FAN_REPORT_PIDFD) &&
+			     FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
 		ret = copy_info_to_user(event, info, info_mode, buf, count);
 		if (ret < 0)
 			return ret;

@@ -1079,6 +1127,13 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 #endif
 		return -EINVAL;
 
+	/*
+	 * A pidfd can only be returned for a thread-group leader; thus
+	 * FAN_REPORT_TID and FAN_REPORT_PIDFD need to be mutually exclusive.
+	 */
+	if ((flags & FAN_REPORT_PIDFD) && (flags & FAN_REPORT_TID))
+		return -EINVAL;
+
 	if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_BITS)
 		return -EINVAL;
 

@@ -1477,7 +1532,7 @@ static int __init fanotify_user_setup(void)
 	max_marks = clamp(max_marks, FANOTIFY_OLD_DEFAULT_MAX_MARKS,
 				     FANOTIFY_DEFAULT_MAX_USER_MARKS);
 
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 10);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 11);
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,

include/linux/fanotify.h +2 -1

index f76c7635efc8..bb2898240e5a 100644
@@ -27,7 +27,7 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */
 
 #define FANOTIFY_FID_BITS	(FAN_REPORT_FID | FAN_REPORT_DFID_NAME)
 
-#define FANOTIFY_INFO_MODES	(FANOTIFY_FID_BITS)
+#define FANOTIFY_INFO_MODES	(FANOTIFY_FID_BITS | FAN_REPORT_PIDFD)
 
 /*
  * fanotify_init() flags that require CAP_SYS_ADMIN.

@@ -37,6 +37,7 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */
  */
 #define FANOTIFY_ADMIN_INIT_FLAGS	(FANOTIFY_PERM_CLASSES | \
 					 FAN_REPORT_TID | \
+					 FAN_REPORT_PIDFD | \
 					 FAN_UNLIMITED_QUEUE | \
 					 FAN_UNLIMITED_MARKS)
 

include/uapi/linux/fanotify.h +12 -0

index fbf9c5c7dd59..36c3bddcf690 100644
@@ -55,6 +55,7 @@
 #define FAN_REPORT_FID		0x00000200	/* Report unique file id */
 #define FAN_REPORT_DIR_FID	0x00000400	/* Report unique directory id */
 #define FAN_REPORT_NAME		0x00000800	/* Report events with name */
+#define FAN_REPORT_PIDFD	0x00001000	/* Report pidfd for event->pid */
 
 /* Convenience macro - FAN_REPORT_NAME requires FAN_REPORT_DIR_FID */
 #define FAN_REPORT_DFID_NAME	(FAN_REPORT_DIR_FID | FAN_REPORT_NAME)

@@ -123,6 +124,7 @@ struct fanotify_event_metadata {
 #define FAN_EVENT_INFO_TYPE_FID		1
 #define FAN_EVENT_INFO_TYPE_DFID_NAME	2
 #define FAN_EVENT_INFO_TYPE_DFID	3
+#define FAN_EVENT_INFO_TYPE_PIDFD	4
 
 /* Variable length info record following event metadata */
 struct fanotify_event_info_header {

@@ -148,6 +150,15 @@ struct fanotify_event_info_fid {
 	unsigned char handle[0];
 };
 
+/*
+ * This structure is used for info records of type FAN_EVENT_INFO_TYPE_PIDFD.
+ * It holds a pidfd for the pid responsible for generating an event.
+ */
+struct fanotify_event_info_pidfd {
+	struct fanotify_event_info_header hdr;
+	__s32 pidfd;
+};
+
 struct fanotify_response {
 	__s32 fd;
 	__u32 response;

@@ -160,6 +171,7 @@ struct fanotify_response {
 
 /* No fd set in event */
 #define FAN_NOFD	-1
+#define FAN_NOPIDFD	FAN_NOFD
 
 /* Helper functions to deal with fanotify_event_metadata buffers */
 #define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

Signature · 3 lines

2.31.1.751.gd2f1c929bd-goog

/M

Quoted 69 lines · Matthew Bobrowski

Introduce a new flag FAN_REPORT_PIDFD for fanotify_init(2) which allows userspace applications to control whether a pidfd info record containing a pidfd is to be returned with each event.

If FAN_REPORT_PIDFD is enabled for a notification group, an additional struct fanotify_event_info_pidfd object will be supplied alongside the generic struct fanotify_event_metadata within a single event. This functionality is analogous to that of FAN_REPORT_FID in terms of how the event structure is supplied to the userspace application. Usage of FAN_REPORT_PIDFD with FAN_REPORT_FID/FAN_REPORT_DFID_NAME is permitted, and in this case a struct fanotify_event_info_pidfd object will follow any struct fanotify_event_info_fid object.

Usage of FAN_REPORT_TID is not permitted with FAN_REPORT_PIDFD as the pidfd API only supports the creation of pidfds for thread-group leaders. Attempting to do so will result with a -EINVAL being returned when calling fanotify_init(2).

If pidfd creation fails via pidfd_create(), the pidfd field within struct fanotify_event_info_pidfd is set to FAN_NOPIDFD.

Signed-off-by Matthew Bobrowski <[email protected]>

---
 fs/notify/fanotify/fanotify_user.c | 65 +++++++++++++++++++++++++++---
 include/linux/fanotify.h           |  3 +-
 include/uapi/linux/fanotify.h      | 12 ++++++
 3 files changed, 74 insertions(+), 6 deletions(-)

fs/notify/fanotify/fanotify_user.c +19 -0

index 1e15f3222eb2..bba61988f4a0 100644
@@ -106,6 +106,8 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 #define FANOTIFY_EVENT_ALIGN 4
 #define FANOTIFY_FID_INFO_HDR_LEN \
      (sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))
+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 33 lines · Christian Brauner

Quoted 33 lines

fs/notify/fanotify/fanotify_user.c +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Good catch!

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

Anyway, something like:

        if (!capable(CAP_SYS_ADMIN) &&
            task_tgid(current) != event->pid)
                metadata.pid = 0;

(quoted diff) +4 -0

+      else if (pidfd_mode)
+              pidfd = pidfd_create(pid, 0);
 
[...]
 
+       if (pidfd_mode)
+               ret = copy_pidfd_info_to_user(pidfd, buf, count);
 
        return metadata.event_len;

out_close_fd:
+ if (pidfd != FAN_NOPIDFD) {
...

And in any case, it wrong to call copy_pidfd_info_to_user() from copy_info_to_user(). It needs to be called once from copy_event_to_user() because copy_pidfd_info_to_user() may be called twice to report both FAN_EVENT_INFO_TYPE_FID and FAN_EVENT_INFO_TYPE_DFID
records for the same event.

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

fs/notify/fanotify/fanotify_user.c +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 62 lines · Matthew Bobrowski

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

fs/notify/fanotify/fanotify_user.c +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

Quoted 73 lines · Amir Goldstein

Quoted 62 lines · Matthew Bobrowski

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

fs/notify/fanotify/fanotify_user.c +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 6 lines

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

I have no problems with being more defensive (it's certainly better than being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged process, that process is also free to update all the passwords. Traditionally permission checks in Unix are performed on open and then who has fd can do whatever that fd allows... I've tried to follow similar philosophy with fanotify as well and e.g. open happening as a result of fanotify path events does not check permissions either.

Quoted 82 lines · Jan Kara

Quoted 73 lines · Amir Goldstein

Quoted 62 lines · Matthew Bobrowski

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

(quoted diff) +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

Review static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

(quoted diff) +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 6 lines

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

Quoted 9 lines

I have no problems with being more defensive (it's certainly better than being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged process, that process is also free to update all the passwords. Traditionally permission checks in Unix are performed on open and then who has fd can do whatever that fd allows... I've tried to follow similar philosophy with fanotify as well and e.g. open happening as a result of fanotify path events does not check permissions either.

Agreed.

However, because we had this issue with no explicit FAN_REPORT_PID we added the CAP_SYS_ADMIN check for reporting event->pid as next best thing. So now that becomes weird if priv process created fanotify fd and passes it to unpriv process, then unpriv process gets events with pidfd but without event->pid.

We can change the code to:

        if (!capable(CAP_SYS_ADMIN) && !pidfd_mode &&
            task_tgid(current) != event->pid)
                metadata.pid = 0;

So the case I decscribed above ends up reporting both pidfd and event->pid to unpriv user, but that is a bit inconsistent...

Quoted 101 lines · Amir Goldstein

Quoted 82 lines · Jan Kara

Quoted 73 lines · Amir Goldstein

Quoted 62 lines · Matthew Bobrowski

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

(quoted diff) +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

Review static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

(quoted diff) +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 6 lines

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

Quoted 9 lines

I have no problems with being more defensive (it's certainly better than being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged process, that process is also free to update all the passwords. Traditionally permission checks in Unix are performed on open and then who has fd can do whatever that fd allows... I've tried to follow similar philosophy with fanotify as well and e.g. open happening as a result of fanotify path events does not check permissions either.

Agreed.

However, because we had this issue with no explicit FAN_REPORT_PID we added the CAP_SYS_ADMIN check for reporting event->pid as next best thing. So now that becomes weird if priv process created fanotify fd and passes it to unpriv process, then unpriv process gets events with pidfd but without event->pid.

Quoted 8 lines

We can change the code to:

        if (!capable(CAP_SYS_ADMIN) && !pidfd_mode &&
            task_tgid(current) != event->pid)
                metadata.pid = 0;

So the case I decscribed above ends up reporting both pidfd and event->pid to unpriv user, but that is a bit inconsistent...

Oh, now I see where you are coming from :) Thanks for explanation. And remind me please, cannot we just have internal FAN_REPORT_PID flag that gets set on notification group when priviledged process creates it and then test for that instead of CAP_SYS_ADMIN in copy_event_to_user()? It is mostly equivalent but I guess more in the spirit of how fanotify traditionally does things. Also FAN_REPORT_PIDFD could then behave in the same way...

Quoted 112 lines · Jan Kara

Quoted 101 lines · Amir Goldstein

Quoted 82 lines · Jan Kara

Quoted 73 lines · Amir Goldstein

Quoted 62 lines · Matthew Bobrowski

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

(quoted diff) +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

Review static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

(quoted diff) +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 6 lines

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

Quoted 9 lines

I have no problems with being more defensive (it's certainly better than being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged process, that process is also free to update all the passwords. Traditionally permission checks in Unix are performed on open and then who has fd can do whatever that fd allows... I've tried to follow similar philosophy with fanotify as well and e.g. open happening as a result of fanotify path events does not check permissions either.

Agreed.

However, because we had this issue with no explicit FAN_REPORT_PID we added the CAP_SYS_ADMIN check for reporting event->pid as next best thing. So now that becomes weird if priv process created fanotify fd and passes it to unpriv process, then unpriv process gets events with pidfd but without event->pid.

Quoted 8 lines

We can change the code to:

        if (!capable(CAP_SYS_ADMIN) && !pidfd_mode &&
            task_tgid(current) != event->pid)
                metadata.pid = 0;

So the case I decscribed above ends up reporting both pidfd and event->pid to unpriv user, but that is a bit inconsistent...

Quoted 7 lines

Oh, now I see where you are coming from :) Thanks for explanation. And remind me please, cannot we just have internal FAN_REPORT_PID flag that gets set on notification group when priviledged process creates it and then test for that instead of CAP_SYS_ADMIN in copy_event_to_user()? It is mostly equivalent but I guess more in the spirit of how fanotify traditionally does things. Also FAN_REPORT_PIDFD could then behave in the same way...

Yes, we can. In fact, we should call the internal flag FANOTIFY_UNPRIV as it described the situation better than FAN_REPORT_PID. This happens to be how I implemented it in the initial RFC [1].

It's not easy to follow our entire discussion on this thread, but I think we can resurrect the FANOTIFY_UNPRIV internal flag and use it in this case instead of CAP_SYS_ADMIN.

Quoted 125 lines · Amir Goldstein

Quoted 112 lines · Jan Kara

Quoted 101 lines · Amir Goldstein

Quoted 82 lines · Jan Kara

Quoted 73 lines · Amir Goldstein

Quoted 62 lines · Matthew Bobrowski

Hey Amir/Christian,

Quoted 44 lines · Amir Goldstein

Quoted 33 lines · Christian Brauner

Quoted 33 lines

(quoted diff) +19 -0

+#define FANOTIFY_PIDFD_INFO_HDR_LEN \
+     sizeof(struct fanotify_event_info_pidfd)
 
 static int fanotify_fid_info_len(int fh_len, int name_len)
 {

@@ -141,6 +143,9 @@ static int fanotify_event_info_len(unsigned int info_mode,
      if (fh_len)
              info_len += fanotify_fid_info_len(fh_len, dot_len);
 
+     if (info_mode & FAN_REPORT_PIDFD)
+             info_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
+
      return info_len;
 }
 

@@ -401,6 +406,29 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid,
      return info_len;
 }
 
+static int copy_pidfd_info_to_user(struct pid *pid,
+                                char __user *buf,
+                                size_t count)
+{
+     struct fanotify_event_info_pidfd info = { };
+     size_t info_len = FANOTIFY_PIDFD_INFO_HDR_LEN;
+
+     if (WARN_ON_ONCE(info_len > count))
+             return -EFAULT;
+
+     info.hdr.info_type = FAN_EVENT_INFO_TYPE_PIDFD;
+     info.hdr.len = info_len;
+
+     info.pidfd = pidfd_create(pid, 0);

Quoted 8 lines

Review static int copy_fid_info_to_user(__kernel_fsid_t *fsid, · line 427

Quoted 5 lines

(quoted diff) +5 -0

+     if (info.pidfd < 0)
+             info.pidfd = FAN_NOPIDFD;
+
+     if (copy_to_user(buf, &info, info_len))
+             return -EFAULT;

Hm, well this kinda sucks. The caller can end up with a pidfd in their fd table and when the copy_to_user() failed they won't know what fd it

Quoted 1 line

Good catch!

Super awesome catch Christian, thanks pulling this up!

Quoted 3 lines

But I prefer to solve it differently, because moving fd_install() to the end of this function does not guarantee that copy_event_to_user() won't return an error one day with dangling pidfd in fd table.

I can see the angle you're approaching this from...

Quoted 4 lines

It might be simpler to do pidfd_create() next to create_fd() in copy_event_to_user() and pass pidfd to copy_pidfd_info_to_user(). pidfd can be closed on error along with fd on out_close_fd label.

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 6 lines

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

Quoted 9 lines

I have no problems with being more defensive (it's certainly better than being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged process, that process is also free to update all the passwords. Traditionally permission checks in Unix are performed on open and then who has fd can do whatever that fd allows... I've tried to follow similar philosophy with fanotify as well and e.g. open happening as a result of fanotify path events does not check permissions either.

Agreed.

However, because we had this issue with no explicit FAN_REPORT_PID we added the CAP_SYS_ADMIN check for reporting event->pid as next best thing. So now that becomes weird if priv process created fanotify fd and passes it to unpriv process, then unpriv process gets events with pidfd but without event->pid.

Quoted 8 lines

We can change the code to:

        if (!capable(CAP_SYS_ADMIN) && !pidfd_mode &&
            task_tgid(current) != event->pid)
                metadata.pid = 0;

So the case I decscribed above ends up reporting both pidfd and event->pid to unpriv user, but that is a bit inconsistent...

Quoted 7 lines

Oh, now I see where you are coming from :) Thanks for explanation. And remind me please, cannot we just have internal FAN_REPORT_PID flag that gets set on notification group when priviledged process creates it and then test for that instead of CAP_SYS_ADMIN in copy_event_to_user()? It is mostly equivalent but I guess more in the spirit of how fanotify traditionally does things. Also FAN_REPORT_PIDFD could then behave in the same way...

Yes, we can. In fact, we should call the internal flag FANOTIFY_UNPRIV as it described the situation better than FAN_REPORT_PID. This happens to be how I implemented it in the initial RFC [1].

Quoted 3 lines

It's not easy to follow our entire discussion on this thread, but I think we can resurrect the FANOTIFY_UNPRIV internal flag and use it in this case instead of CAP_SYS_ADMIN.

I think at that time we were discussing how to handle opening of fds and we decided to not depend on FANOTIFY_UNPRIV and then I didn't see a value of that flag because I forgot about pids... Anyway now I agree to go for that flag. :)

Quoted 56 lines

Quoted 56 lines

Quoted 44 lines

Quoted 35 lines

Quoted 17 lines

Quoted 10 lines

Quoted 9 lines

Quoted 2 lines

You also forgot to add CAP_SYS_ADMIN check before pidfd_create() (even though fanotify_init() does check for that).

I didn't really understand the need for this check here given that the administrative bits are already being checked for in fanotify_init() i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener; thus never walking any of the pidfd_mode paths. Is this just a defense in depth approach here, or is it something else that I'm missing?

Quoted 6 lines

We want to be extra careful not to create privilege escalations, so even if the fanotify fd is leaked or intentionally passed to a less privileged user, it cannot get an open pidfd.

IOW, it is *much* easier to be defensive in this case than to prove that the change cannot introduce any privilege escalations.

Quoted 9 lines

I have no problems with being more defensive (it's certainly better than being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged process, that process is also free to update all the passwords. Traditionally permission checks in Unix are performed on open and then who has fd can do whatever that fd allows... I've tried to follow similar philosophy with fanotify as well and e.g. open happening as a result of fanotify path events does not check permissions either.

Agreed.

However, because we had this issue with no explicit FAN_REPORT_PID we added the CAP_SYS_ADMIN check for reporting event->pid as next best thing. So now that becomes weird if priv process created fanotify fd and passes it to unpriv process, then unpriv process gets events with pidfd but without event->pid.

Quoted 8 lines

We can change the code to:

        if (!capable(CAP_SYS_ADMIN) && !pidfd_mode &&
            task_tgid(current) != event->pid)
                metadata.pid = 0;

So the case I decscribed above ends up reporting both pidfd and event->pid to unpriv user, but that is a bit inconsistent...

Quoted 7 lines

Oh, now I see where you are coming from :) Thanks for explanation. And remind me please, cannot we just have internal FAN_REPORT_PID flag that gets set on notification group when priviledged process creates it and then test for that instead of CAP_SYS_ADMIN in copy_event_to_user()? It is mostly equivalent but I guess more in the spirit of how fanotify traditionally does things. Also FAN_REPORT_PIDFD could then behave in the same way...

Yes, we can. In fact, we should call the internal flag FANOTIFY_UNPRIV as it described the situation better than FAN_REPORT_PID. This happens to be how I implemented it in the initial RFC [1].

Quoted 8 lines

Quoted 3 lines

It's not easy to follow our entire discussion on this thread, but I think we can resurrect the FANOTIFY_UNPRIV internal flag and use it in this case instead of CAP_SYS_ADMIN.

I think at that time we were discussing how to handle opening of fds and we decided to not depend on FANOTIFY_UNPRIV and then I didn't see a value of that flag because I forgot about pids... Anyway now I agree to go for that flag. :)

Resurrection of this flag SGTM! However, it also sounds like we need to land that series before this PIDFD series or simply incorporate the UNPRIV flag into this one.

Will chat with Amir to get this done.

Hey Jan/Amir/Christian,

This is the updated patch series for adding pidfd support to the fanotify API. It incorporates all the suggestions that had come out of the initial RFC patch series [0].

The main difference with this patch series is that FAN_REPORT_PIDFD results in an additional info record object supplied alongside the generic event metadata object instead of overloading metadata->pid. If any of the fid flavoured init flags are specified, then the pidfd info record object will follow any fid info record objects.

[0] https://www.spinics.net/lists/linux-fsdevel/msg193296.html

Matthew Bobrowski (5):
kernel/pid.c: remove static qualifier from pidfd_create() kernel/pid.c: implement checks on parameters passed to pidfd_create() fanotify_user.c: minor cosmetic adjustments to fid labels fanotify/fanotify_user.c: introduce a generic info record copying
function
fanotify: Add pidfd info record support to the fanotify API

 fs/notify/fanotify/fanotify_user.c | 216 +++++++++++++++++++----------
 include/linux/fanotify.h           |   3 +
 include/linux/pid.h                |   1 +
 include/uapi/linux/fanotify.h      |  12 ++
 kernel/pid.c                       |  15 +-
 5 files changed, 170 insertions(+), 77 deletions(-)

Signature · 3 lines

2.31.1.751.gd2f1c929bd-goog

/M

Quoted 12 lines · Matthew Bobrowski

Hey Jan/Amir/Christian,

This is the updated patch series for adding pidfd support to the fanotify API. It incorporates all the suggestions that had come out of the initial RFC patch series [0].

The main difference with this patch series is that FAN_REPORT_PIDFD results in an additional info record object supplied alongside the generic event metadata object instead of overloading metadata->pid. If any of the fid flavoured init flags are specified, then the pidfd info record object will follow any fid info record objects.

Quoted 1 line

[0] https://www.spinics.net/lists/linux-fsdevel/msg193296.html

Overall the series looks fine to me - modulo the problems Christian & Amir found. Do you have any tests for this? Preferably for LTP so that we can extend the coverage there?

Hey Jan!

Quoted 13 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

Hey Jan/Amir/Christian,

This is the updated patch series for adding pidfd support to the fanotify API. It incorporates all the suggestions that had come out of the initial RFC patch series [0].

The main difference with this patch series is that FAN_REPORT_PIDFD results in an additional info record object supplied alongside the generic event metadata object instead of overloading metadata->pid. If any of the fid flavoured init flags are specified, then the pidfd info record object will follow any fid info record objects.

Quoted 5 lines

Quoted 1 line

[0] https://www.spinics.net/lists/linux-fsdevel/msg193296.html

Overall the series looks fine to me - modulo the problems Christian & Amir found. Do you have any tests for this? Preferably for LTP so that we can extend the coverage there?

Cool and thanks for glancing over this series.

I've written some simple programs to verify this functionality works in FID and non-FID modes. I definitely plan on writing LTP tests, although it's something I'll do once we've agreed on the approach and I've received an ACK from yourself, Amir and Christian. This series passes all current LTP regressions. Also, I guess I'll need to write some patches for man-pages given this is an ABI change.

There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

/M

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Also opening pidfd in the context of event generation is problematic for two reasons:

1) Technically, the context under which events are generated can be rather constrained (various locks held etc.). Adding relatively complex operations such as pidfd creation is going to introduce strange lock dependencies, possibly deadlocks.

2) Doing pidfd generation in the context of the process generating event is problematic - you don't know in which fd_table the fd will live. Also that process is unfairly penalized (performance wise) because someone else is listening. We try to keep overhead of event generation as low as possible for this reason.

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 43 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Quoted 4 lines

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
	   	 return -EINVAL;

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

1) Implement an additional check in pidfd_create() that effectively checks whether provided pid still holds reference to a struct pid that isn't in the process of being cleaned up. If it is being cleaned up, then return something like -ESRCH instead of -EINVAL so that the caller, in this case fanotify, can check and set FAN_NOPIDFD if -ESRCH is returned from pidfd_create(). I definitely don't feel as though returning -ESRCH from the !pid_has_task(pid, PIDTYPE_TGID) would be appropriate. In saying that, I'm not aware of a helper by which would allow us to perform such an in-flight check? Perhaps something needs to be introduced here, IDK...

2) Refrain from performing any further changes to pidfd_create() i.e. as proposed in option 1), and manually perform the pidfd creation from some kind of new fanotify helper, as suggested by you here [0]. However, I'm not convinved that I like this approach as we may end up slowly drifting away from pidfd creation semantics over time.

[0] https://www.spinics.net/lists/linux-fsdevel/msg195556.html

/M

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
	   	 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

(I think if would be interesting to see perf numbers and how high you need to bump the number of open fds sysctl limit to make this useable on systems with a lot of events. I'd be interested in that just in case you have something there.)

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
	   	 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
	   	 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

WDYT?

Quoted 111 lines · Matthew Bobrowski

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
	   	 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

Quoted 1 line

WDYT?

Hm, why would you have to drop the pid_has_task() check again?

Quoted 112 lines · Christian Brauner

Quoted 111 lines · Matthew Bobrowski

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

        if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
                 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

Quoted 3 lines

Quoted 1 line

WDYT?

Hm, why would you have to drop the pid_has_task() check again?

Because of the race that I brielfy decscribed here [0]. The race exists

because we perform the pidfd creation during the notification queue processing and not in the event allocation stages (for reasons that Jan has already covered here [1]). So, tl;dr there is the case where the fanotify calls pidfd_create() and the check for pid_has_task() fails because the struct pid that we're hanging onto within an event no longer contains a task of type PIDTYPE_TGID...

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

Quoted 114 lines · Matthew Bobrowski

Quoted 112 lines · Christian Brauner

Quoted 111 lines · Matthew Bobrowski

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

        if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
                 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

WDYT?

Hm, why would you have to drop the pid_has_task() check again?

Because of the race that I brielfy decscribed here [0]. The race exists

Sorry for being thich. I still don't understand what's racy about this. Won't the event reader get a valid pidfd?
Can't the event reader verify that the pidfd points to a dead process? I don't mind returning FAN_NOPIDFD for convenience, but user will have to check the pidfd that it got anyway, because process can die at any time between reading the event and acting on the pidfd.

Quoted 10 lines

because we perform the pidfd creation during the notification queue processing and not in the event allocation stages (for reasons that Jan has already covered here [1]). So, tl;dr there is the case where the fanotify calls pidfd_create() and the check for pid_has_task() fails because the struct pid that we're hanging onto within an event no longer contains a task of type PIDTYPE_TGID...

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

I warmly recommend that you use lore.kernel.org for archive links.

Thanks,
Amir.

Quoted 121 lines · Amir Goldstein

Quoted 114 lines · Matthew Bobrowski

Quoted 112 lines · Christian Brauner

Quoted 111 lines · Matthew Bobrowski

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

        if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
                 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

WDYT?

Hm, why would you have to drop the pid_has_task() check again?

Because of the race that I brielfy decscribed here [0]. The race exists

Quoted 7 lines

Sorry for being thich. I still don't understand what's racy about this. Won't the event reader get a valid pidfd?
Can't the event reader verify that the pidfd points to a dead process? I don't mind returning FAN_NOPIDFD for convenience, but user will have to check the pidfd that it got anyway, because process can die at any time between reading the event and acting on the pidfd.

(Replying to this part of the thread so we don't have to many parallel replies.)

Hm, so quoting from link [0] Matthew posted so we all have some context here:
"Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record."

I don't think that's a race and even if I don't think that it's a meaningful one. So when the event is queued the process is still alive but when the notification is actually delivered the process is dead.

And your point, Matthew, seems to be that the caller should always get a pidfd back even if the process has already exited _and_ been reaped, i.e. is dead because it was alive when the event was generated.

I think that's no how it needs to work and I have a hard time seeing this as a good argument. What's problematic about just returning FAN_NOPIDFD in that case? After all the process is gone. All the caller can do with such a pidfd is to send it a signal and immediately realize that the process is gone, i.e. -ESRCH anyway.

Quoted 9 lines

Quoted 9 lines

because we perform the pidfd creation during the notification queue processing and not in the event allocation stages (for reasons that Jan has already covered here [1]). So, tl;dr there is the case where the fanotify calls pidfd_create() and the check for pid_has_task() fails because the struct pid that we're hanging onto within an event no longer contains a task of type PIDTYPE_TGID...

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

Quoted 151 lines · Christian Brauner

Quoted 121 lines · Amir Goldstein

Quoted 114 lines · Matthew Bobrowski

Quoted 112 lines · Christian Brauner

Quoted 111 lines · Matthew Bobrowski

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 20 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in
regards to the overall approach we've taken that I'm not particularly
happy about and I'd like to hear all your thoughts. Basically, with
this approach the pidfd creation is done only once an event has been
queued and the notification worker wakes up and picks up the event
from the queue processes it. There's a subtle latency introduced when
taking such an approach which at times leads to pidfd creation
failures. As in, by the time pidfd_create() is called the struct pid
has already been reaped, which then results in FAN_NOPIDFD being
returned in the pidfd info record.

Having said that, I'm wondering what the thoughts are on doing pidfd
creation earlier on i.e. in the event allocation stages? This way, the
struct pid is pinned earlier on and rather than FAN_NOPIDFD being
returned in the pidfd info record because the struct pid has been
already reaped, userspace application will atleast receive a valid
pidfd which can be used to check whether the process still exists or
not. I think it'll just set the expectation better from an API
perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

        if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
                 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

WDYT?

Hm, why would you have to drop the pid_has_task() check again?

Because of the race that I brielfy decscribed here [0]. The race exists

Quoted 7 lines

Sorry for being thich. I still don't understand what's racy about this. Won't the event reader get a valid pidfd?
Can't the event reader verify that the pidfd points to a dead process? I don't mind returning FAN_NOPIDFD for convenience, but user will have to check the pidfd that it got anyway, because process can die at any time between reading the event and acting on the pidfd.

(Replying to this part of the thread so we don't have to many parallel replies.)

Hm, so quoting from link [0] Matthew posted so we all have some context here:
"Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record."

I don't think that's a race and even if I don't think that it's a meaningful one. So when the event is queued the process is still alive but when the notification is actually delivered the process is dead.

And your point, Matthew, seems to be that the caller should always get a pidfd back even if the process has already exited _and_ been reaped, i.e. is dead because it was alive when the event was generated.

Quoted 5 lines

I think that's no how it needs to work and I have a hard time seeing this as a good argument. What's problematic about just returning FAN_NOPIDFD in that case? After all the process is gone. All the caller can do with such a pidfd is to send it a signal and immediately realize that the process is gone, i.e. -ESRCH anyway.

To get things straight, there's no argument here. There's a discussion

about what the best approach is for communicating to the event listener that a process has been killed prior to a pidfd being created by/from fanotify. I have no issues with communicating FAN_NOPIDFD to the event listener in such cases. I just want to make sure everyone else is OK with it.

Quoted 114 lines · Matthew Bobrowski

Quoted 112 lines · Christian Brauner

Quoted 111 lines · Matthew Bobrowski

Quoted 75 lines · Christian Brauner

Quoted 67 lines · Matthew Bobrowski

Quoted 50 lines · Christian Brauner

Quoted 49 lines · Jan Kara

Quoted 47 lines · Matthew Bobrowski

Quoted 22 lines · Jan Kara

Quoted 12 lines · Matthew Bobrowski

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record.

Quoted 8 lines

Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective.

Quoted 3 lines

Yes, there's this race. OTOH if FAN_NOPIDFD is returned, the listener can be sure the original process doesn't exist anymore. So is it useful to still receive pidfd of the dead process?

Well, you're absolutely right. However, FWIW I was approaching this from two different angles:

1) I wanted to keep the pattern in which the listener checks for the existence/recycling of the process consistent. As in, the listener would receive the pidfd, then send the pidfd a signal via pidfd_send_signal() and check for -ESRCH which clearly indicates that the target process has terminated.

2) I didn't want to mask failed pidfd creation because of early process termination and other possible failures behind a single FAN_NOPIDFD. IOW, if we take the -ESRCH approach above, the listener can take clear corrective branches as what's to be done next if a race is to have been detected, whereas simply returning FAN_NOPIDFD at this stage can mean multiple things.

Now that I've written the above and keeping in mind that we'd like to refrain from doing anything in the event allocation stages, perhaps we could introduce a different error code for detecting early process termination while attempting to construct the info record. WDYT?

Quoted 6 lines

Quoted 3 lines

Sure, I wouldn't like to overengineer it but having one special fd value for "process doesn't exist anymore" and another for general "creating pidfd failed" looks OK to me.

FAN_EPIDFD -> "creation failed"
FAN_NOPIDFD -> "no such process"

Yes, I was thinking something along the lines of this...

With the approach that I've proposed in this series, the pidfd creation failure trips up in pidfd_create() at the following condition:

        if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
                 return -EINVAL;

Quoted 6 lines

Specifically, the following check:
!pid_has_task(pid, PIDTYPE_TGID)

In order to properly report either FAN_NOPIDFD/FAN_EPIDFD to userspace, AFAIK I'll have to do one of either two things to better distinguish between why the pidfd creation had failed:

Quoted 8 lines

Ok, I see. You already do have a reference to a struct pid and in that case we should just always return a pidfd to the caller. For pidfd_open() for example we only report an error when find_get_pid(<pidnr>) doesn't find a struct pid to refer to. But in your case here you already have a struct pid so I think we should just keep this simple and always return a pidfd to the caller and in fact do burden them with figuring out that the process is gone via pidfd_send_signal() instead of complicating our lives here.

Ah, actually Christian... Before, I go ahead and send through the updated series. Given what you've mentioned above I'm working with the assumption that you're OK with dropping the pid_has_task() check from pidfd_create() [0]. Is that right?

If so, I don't know how I feel about this given that pidfd_create() is now to be exposed to the rest of the kernel and the pidfd API, as it stands, doesn't support the creation of pidfds for non-thread-group leaders. I suppose what I don't want is other kernel subsystems, if any, thinking it's OK to call pidfd_create() with an arbitrary struct pid and setting the expectation that a fully functional pidfd will be returned.

The way I see it, I think we've got two options here:

1) Leave the pid_has_task() check within pidfd_create() and perform another explicit pid_has_task() check from the fanotify code before calling pidfd_create(). If it returns false, we set something like FAN_NOPIDFD indicating to userspace that there's no such process when the event was created.

2) Scrap using pidfd_create() all together and implement a fanotify specific pidfd creation wrapper which would allow for more control. Something along the lines of what you've done in kernel/fork.c [1]. Not the biggest fan of this idea just yet given the possibility of it leading to an API drift over time.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

WDYT?

Hm, why would you have to drop the pid_has_task() check again?

Because of the race that I brielfy decscribed here [0]. The race exists

Sorry for being thich.

I still don't understand what's racy about this. Won't the event reader get a valid pidfd?

Can't the event reader verify that the pidfd points to a dead process?

I don't mind returning FAN_NOPIDFD for convenience, but user will have to check the pidfd that it got anyway, because process can die at any time between reading the event and acting on the pidfd.

Quoted 9 lines

because we perform the pidfd creation during the notification queue processing and not in the event allocation stages (for reasons that Jan has already covered here [1]). So, tl;dr there is the case where the fanotify calls pidfd_create() and the check for pid_has_task() fails because the struct pid that we're hanging onto within an event no longer contains a task of type PIDTYPE_TGID...

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

Quoted 2 lines

I still don't understand what's racy about this. Won't the event reader get a valid pidfd?

I guess this depends, right?

As the logic/implementation currently stands in this specific patch series, pidfd_create() will _NOT_ return a valid pidfd unless the struct pid still holds reference to a task of type PIDTYPE_TGID. This is thanks to the extra pid_hash_task() check that I thought was appropriate to incorporate into pidfd_create() seeing as though:

- With the pidfd_create() declaration now being added to linux/pid.h, we effectively are giving the implicit OK for it to be called from other kernel subsystems, and hence why the caller should be subject to the same restrictions/verifications imposed by the API specification i.e. "Currently, the process identified by @pid must be a thread-group leader...". Not enforcing the pid_has_task() check in pidfd_create() effectively says that the pidfd creation can be done for any struct pid types i.e. PIDTYPE_PID, PIDTYPE_PGID, etc. This leads to assumptions being made by the callers, which effectively then could lead to undefined/unexpected behavior.

There definitely can be cases whereby the underlying task(s) associated with a struct pid have been freed as a result of process being killed early. As in, the process is killed before the pid_has_task() check is performed from within pidfd_create() when called from fanotify. This is precisely the race that I'm referring to here, and in such cases as the code currently stands, the event listener will _NOT_ receive a valid pidfd.

Quoted 1 line

Can't the event reader verify that the pidfd points to a dead process?

This was the idea, as in, the burden of checking whether a process has been killed before the event listener receives the event should be on the event listener. However, we're trying to come up with a good way to effectively communicate that the above race I've attempted to articulate has actually occurred. As in, do we:

a) Drop the pid_has_task() check in pidfd_create() so that a pidfd can be returned for all passed struct pids? That way, even if the above race is experienced the caller will still receive a pidfd and the event listener can do whatever it needs to with it.

b) Before calling into pidfd_create(), perform an explicit pid_has_task() check for PIDTYPE_TGID and if that returns false, then set FAN_NOPIDFD and save ourselves from calling into pidfd_create() all together. The event listener in this case doesn't have to perform the signal check to determine whether the process has already been killed.

c) Scrap calling into pidfd_create() all together and write a simple fanotify wrapper that contains the pidfd creation logic we need.

Quoted 4 lines

I don't mind returning FAN_NOPIDFD for convenience, but user will have to check the pidfd that it got anyway, because process can die at any time between reading the event and acting on the pidfd.

Well sort of, as it depends on the approach that we decide to go ahead with to report such early process termination cases. If we return FAN_NOPIDFD, which signifies that the process died before a pidfd could be created, then there's no point for the listener to step into checking the pidfd because the pidfd already == FAN_NOPIDFD. If we simply return a pidfd regardless of the early termination of the process, then sure the event listener will need to check each pidfd that is supplied.

Quoted 7 lines

Quoted 7 lines

because we perform the pidfd creation during the notification queue processing and not in the event allocation stages (for reasons that Jan has already covered here [1]). So, tl;dr there is the case where the fanotify calls pidfd_create() and the check for pid_has_task() fails because the struct pid that we're hanging onto within an event no longer contains a task of type PIDTYPE_TGID...

Quoted 2 lines

Quoted 2 lines

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

Maybe I'm going down a rabbit hole and overthinking this whole thing, IDK... :(

Quoted 56 lines

Quoted 2 lines

I still don't understand what's racy about this. Won't the event reader get a valid pidfd?

I guess this depends, right?

As the logic/implementation currently stands in this specific patch series, pidfd_create() will _NOT_ return a valid pidfd unless the struct pid still holds reference to a task of type PIDTYPE_TGID. This is thanks to the extra pid_hash_task() check that I thought was appropriate to incorporate into pidfd_create() seeing as though:

- With the pidfd_create() declaration now being added to linux/pid.h, we effectively are giving the implicit OK for it to be called from other kernel subsystems, and hence why the caller should be subject to the same restrictions/verifications imposed by the API specification i.e. "Currently, the process identified by @pid must be a thread-group leader...". Not enforcing the pid_has_task() check in pidfd_create() effectively says that the pidfd creation can be done for any struct pid types i.e. PIDTYPE_PID, PIDTYPE_PGID, etc. This leads to assumptions being made by the callers, which effectively then could lead to undefined/unexpected behavior.

There definitely can be cases whereby the underlying task(s) associated with a struct pid have been freed as a result of process being killed early. As in, the process is killed before the pid_has_task() check is performed from within pidfd_create() when called from fanotify. This is precisely the race that I'm referring to here, and in such cases as the code currently stands, the event listener will _NOT_ receive a valid pidfd.

Quoted 1 line

Can't the event reader verify that the pidfd points to a dead process?

This was the idea, as in, the burden of checking whether a process has been killed before the event listener receives the event should be on the event listener. However, we're trying to come up with a good way to effectively communicate that the above race I've attempted to articulate has actually occurred. As in, do we:

a) Drop the pid_has_task() check in pidfd_create() so that a pidfd can be returned for all passed struct pids? That way, even if the above race is experienced the caller will still receive a pidfd and the event listener can do whatever it needs to with it.

b) Before calling into pidfd_create(), perform an explicit pid_has_task() check for PIDTYPE_TGID and if that returns false, then set FAN_NOPIDFD and save ourselves from calling into pidfd_create() all together. The event listener in this case doesn't have to perform the signal check to determine whether the process has already been killed.

c) Scrap calling into pidfd_create() all together and write a simple fanotify wrapper that contains the pidfd creation logic we need.

Quoted 4 lines

I don't mind returning FAN_NOPIDFD for convenience, but user will have to check the pidfd that it got anyway, because process can die at any time between reading the event and acting on the pidfd.

Quoted 8 lines

Well sort of, as it depends on the approach that we decide to go ahead with to report such early process termination cases. If we return FAN_NOPIDFD, which signifies that the process died before a pidfd could be created, then there's no point for the listener to step into checking the pidfd because the pidfd already == FAN_NOPIDFD. If we simply return a pidfd regardless of the early termination of the process, then sure the event listener will need to check each pidfd that is supplied.

I don't see any problem with the fact that the listener would have to check the reported pidfd. I don't see how or why that should be avoided.
If we already know there is no process, we can be nice and return NOPIDFD, because that doesn't add any complexity.

Not to mention that if pid_vnr() returns 0 (process is outside of pidns of caller)
then I think you MUST either report FAN_NOPIDFD or no pid info record, because getting 0 event->pid with a valid pidfd is weird IMO and could be considered as a security breach.

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

because we perform the pidfd creation during the notification queue processing and not in the event allocation stages (for reasons that Jan has already covered here [1]). So, tl;dr there is the case where the fanotify calls pidfd_create() and the check for pid_has_task() fails because the struct pid that we're hanging onto within an event no longer contains a task of type PIDTYPE_TGID...

[0] https://www.spinics.net/lists/linux-api/msg48630.html
[1] https://www.spinics.net/lists/linux-api/msg48632.html

Quoted 3 lines

Maybe I'm going down a rabbit hole and overthinking this whole thing, IDK... :(

That is the feeling I get as well.
Suggestion: write the man page - that will make it clear to yourself and to code reviewers if the API is sane and if it is going to end up being confusing to end users.