Add pidfd support to the fanotify API

With the idea of returning pidfds from the fanotify API, we need to expose a mechanism for creating pidfds. We drop the static qualifier from pidfd_create() and add its declaration to linux/pid.h so that the pidfd_create() helper can be called from other kernel subsystems i.e. fanotify.

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

---

By adding the pidfd_create() declaration to linux/pid.h, we effectively expose this function to the rest of the kernel. In order to avoid any unintended behaviour, or set false expectations upon this function, ensure that constraints are forced upon each of the passed parameters. This includes the checking of whether the passed struct pid is a thread-group leader as pidfd creation is currently limited to such pid types.

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

---

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.

Currently, the usage of FAN_REPORT_TID is not permitted along with FAN_REPORT_PIDFD as the pidfd API only supports the creation of pidfds for thread-group leaders. Additionally, the FAN_REPORT_PIDFD is limited to privileged processes only i.e. listeners that are running with the CAP_SYS_ADMIN capability. Attempting to supply either of these initialisation flags with FAN_REPORT_PIDFD will result with EINVAL being returned to the caller.

In the event of a pidfd creation error, there are two types of error values that can be reported back to the listener. There is FAN_NOPIDFD, which will be reported in cases where the process responsible for generating the event has terminated prior to fanotify being able to create pidfd for event->pid via pidfd_create(). The there is FAN_EPIDFD, which will be reported if a more generic pidfd creation error occurred when calling pidfd_create().

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

---

Changes since v1:

* Explicit checks added to copy_event_to_user() for unprivileged listeners via FANOTIFY_UNPRIV. Only processes running with the CAP_SYS_ADMIN capability can receive pidfds for events.

* The pidfd creation via pidfd_create() has been taken out from copy_pidfd_info_to_user() and put into copy_event_to_user() so that proper clean up of the installed file descriptor can take place in the event that we error out during one of the info copying routines.

* Before pidfd creation is done via pidfd_create(), we perform an explicit check using pid_has_task() to make sure that the process responsible for generating the event in the first place hasn't been terminated. If it has, we supply the FAN_NOPIDFD error to the listener which explicitly indicates this was the case. All other pidfd creation errors are represented by FAN_EPIDFD.

* An additional check has been implemented before calling into pidfd_create() to see whether pid_vnr() had returned 0 for event->pid. In such cases, we also return FAN_NOPIDFD within the pidfd info record as returning metadata->pid = 0 with a valid pidfd doesn't make much sense and could lead to possible security problem.

 fs/notify/fanotify/fanotify_user.c | 98 ++++++++++++++++++++++++++++--
 include/linux/fanotify.h           |  3 +-
 include/uapi/linux/fanotify.h      | 13 ++++
 3 files changed, 107 insertions(+), 7 deletions(-)

fs/notify/fanotify/fanotify_user.c +37 -1

index 85d6eea8d45d..1ce66bcfd9b5 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)
 {

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

@@ -401,13 +406,34 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        return info_len;
 }
 
+static int copy_pidfd_info_to_user(int pidfd,
+                                  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;
+
+       if (copy_to_user(buf, &info, info_len))
+               return -EFAULT;
+
+       return info_len;
+}
+
 static int copy_info_records_to_user(struct fanotify_event *event,
                                     struct fanotify_info *info,
-                                    unsigned int info_mode,
+                                    unsigned int info_mode, int pidfd,
                                     char __user *buf, size_t count)
 {
        int ret, total_bytes = 0, 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.

@@ -478,6 +504,16 @@ static int copy_info_records_to_user(struct fanotify_event *event,
                total_bytes += ret;
        }
 
+       if (pidfd_mode) {
+               ret = copy_pidfd_info_to_user(pidfd, buf, count);
+               if (ret < 0)
+                       return ret;
+
+               buf += ret;
+               count -= ret;
+               total_bytes += ret;
+       }
+
        return total_bytes;
 }
 

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

        pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 

@@ -524,6 +561,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       /*
+        * Currently, reporting a pidfd to an unprivileged listener is not
+        * supported. The FANOTIFY_UNPRIV flag is to be kept here so that a
+        * pidfd is not accidentally leaked to an unprivileged listener.
+        */
+       if (pidfd_mode && !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV)) {
+               /*
+                * The PIDTYPE_TGID check for an event->pid is performed
+                * preemptively in attempt to catch those rare instances
+                * where the process responsible for generating the event has
+                * terminated prior to calling into pidfd_create() and
+                * acquiring a valid pidfd. Report FAN_NOPIDFD to the listener
+                * in those cases.
+                */
+               if (metadata.pid == 0 ||

fs/notify/fanotify/fanotify_user.c +3 -1

+               }
+       }
+
        ret = -EFAULT;
        /*
         * Sanity check copy size in case get_one_event() and

@@ -545,10 +610,19 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
                fd_install(fd, f);
 
        if (info_mode) {
-               ret = copy_info_records_to_user(event, info, info_mode,

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 530

Quoted 8 lines · Matthew Bobrowski

fs/notify/fanotify/fanotify_user.c +2 -1

@@ -489,8 +525,9 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        struct path *path = fanotify_event_path(event);
        struct fanotify_info *info = fanotify_event_info(event);
        unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
+       unsigned int pidfd_mode = info_mode & FAN_REPORT_PIDFD;
        struct file *f = NULL;
-       int ret, fd = FAN_NOFD;
+       int ret, pidfd = 0, fd = FAN_NOFD;

It feels like this should be pidfd = FAN_NOPIDFD?

Quoted 30 lines

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

        pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 

@@ -524,6 +561,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       /*
+        * Currently, reporting a pidfd to an unprivileged listener is not
+        * supported. The FANOTIFY_UNPRIV flag is to be kept here so that a
+        * pidfd is not accidentally leaked to an unprivileged listener.
+        */
+       if (pidfd_mode && !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV)) {
+               /*
+                * The PIDTYPE_TGID check for an event->pid is performed
+                * preemptively in attempt to catch those rare instances
+                * where the process responsible for generating the event has
+                * terminated prior to calling into pidfd_create() and
+                * acquiring a valid pidfd. Report FAN_NOPIDFD to the listener
+                * in those cases.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               /*
+                                * All other pidfd creation errors are reported
+                                * as FAN_EPIDFD to the listener.

Review static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 588

Quoted 2 lines

(quoted diff) +2 -0

+                                */
+                               pidfd = FAN_EPIDFD;

That's an anti pattern. a multi-line statement, even due to comment should be inside {}, but in this case, I think it is better to put this comment as another
line in the big comment above which explains both the if and the else, because it is in fact a continuation of the comment above.

Quoted 11 lines

fs/notify/fanotify/fanotify_user.c +3 -1

+               }
+       }
+
        ret = -EFAULT;
        /*
         * Sanity check copy size in case get_one_event() and

@@ -545,10 +610,19 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
                fd_install(fd, f);
 
        if (info_mode) {
-               ret = copy_info_records_to_user(event, info, info_mode,

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 621

Quoted 10 lines

fs/notify/fanotify/fanotify_user.c +9 -1

-                                               buf, count);
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incorporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(pidfd_mode &&
+                            FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+

This WARN_ON, if needed at all, would be better places inside if (pidfd_mode && code block above where you would only need to

     WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));

as close as possible to PIDTYPE_TGID line.

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 625

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +3 -1

+               ret = copy_info_records_to_user(event, info, info_mode, pidfd,
+                                               buf, count);
                if (ret < 0)
-                       return ret;
+                       goto out_close_fd;

This looks like a bug in upstream.

It should have been goto out_close_fd to begin with. We did already copy metadata.fd to user, but the read() call returns an error.
You should probably fix it before the refactoring patch, so it can be applied to stable kernels.

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 636

Quoted 9 lines

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

        }
 
        return metadata.event_len;

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

That condition is reversed.
We do not seem to have any test coverage for this error handling Not so surprising that upstream had a bug...

Quoted 7 lines

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 625

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +3 -1

+               ret = copy_info_records_to_user(event, info, info_mode, pidfd,
+                                               buf, count);
                if (ret < 0)
-                       return ret;
+                       goto out_close_fd;

This looks like a bug in upstream.

Yes, I'm glad this was picked up and I was actually wondering why it was acceptable to directly return without jumping to the out_close_fd label in the case of an error. I felt like it may have been a burden to raise the question in the first place because I thought that this got picked up in the review already and there was a good reason for having it, despite not really making much sense.

Quoted 5 lines

It should have been goto out_close_fd to begin with. We did already copy metadata.fd to user, but the read() call returns an error.
You should probably fix it before the refactoring patch, so it can be applied to stable kernels.

Sure, I will send through a patch fixing this before submitting the next version of this series though. How do I tag the patch so that it's picked up an back ported accordingly?

Quoted 13 lines

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 636

Quoted 9 lines

        }

        return metadata.event_len;

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

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

That condition is reversed.
We do not seem to have any test coverage for this error handling Not so surprising that upstream had a bug...

Sorry Amir, I don't quite understand what you mean by "That condition is reversed". Presumably you're referring to the fd != FAN_NOFD check and not pidfd < 0 here.

Quoted 21 lines

Quoted 7 lines

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 625

Quoted 5 lines

fs/notify/fanotify/fanotify_user.c +3 -1

+               ret = copy_info_records_to_user(event, info, info_mode, pidfd,
+                                               buf, count);
                if (ret < 0)
-                       return ret;
+                       goto out_close_fd;

This looks like a bug in upstream.

Yes, I'm glad this was picked up and I was actually wondering why it was acceptable to directly return without jumping to the out_close_fd label in the case of an error. I felt like it may have been a burden to raise the question in the first place because I thought that this got picked up in the review already and there was a good reason for having it, despite not really making much sense.

Quoted 5 lines

It should have been goto out_close_fd to begin with. We did already copy metadata.fd to user, but the read() call returns an error.
You should probably fix it before the refactoring patch, so it can be applied to stable kernels.

Quoted 4 lines

Sure, I will send through a patch fixing this before submitting the next version of this series though. How do I tag the patch so that it's picked up an back ported accordingly?

The best option, in case this is a regression (it probably is) is the Fixes: tag which is both a clear indication for stale candidate patch tells the bots exactly which stable kernel the patch should be applied to.

Otherwise, you can Cc: stable (see examples in git)
and generally any commit title with the right keywords 'fix' 'regression' 'bug' should be caught but the stable AI bots.

Quoted 14 lines

Quoted 13 lines

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 636

Quoted 9 lines

        }

        return metadata.event_len;

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

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

That condition is reversed.
We do not seem to have any test coverage for this error handling Not so surprising that upstream had a bug...

Quoted 4 lines

Sorry Amir, I don't quite understand what you mean by "That condition is reversed". Presumably you're referring to the fd != FAN_NOFD check and not pidfd < 0 here.

IDGI, why is the init/cleanup code not as simple as

    int pidfd = FAN_NOPIDFD;
...
out_close_fd:
...
       if (pidfd >= 0)
                 put_unused_fd(fd);

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.

Currently, the usage of FAN_REPORT_TID is not permitted along with FAN_REPORT_PIDFD as the pidfd API only supports the creation of pidfds for thread-group leaders. Additionally, the FAN_REPORT_PIDFD is limited to privileged processes only i.e. listeners that are running with the CAP_SYS_ADMIN capability. Attempting to supply either of these initialisation flags with FAN_REPORT_PIDFD will result with EINVAL being returned to the caller.

In the event of a pidfd creation error, there are two types of error values that can be reported back to the listener. There is FAN_NOPIDFD, which will be reported in cases where the process responsible for generating the event has terminated prior to fanotify being able to create pidfd for event->pid via pidfd_create(). The there is FAN_EPIDFD, which will be reported if a more generic pidfd creation error occurred when calling pidfd_create().

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

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

+		/*
+		 * The PIDTYPE_TGID check for an event->pid is performed
+		 * preemptively in attempt to catch those rare instances
+		 * where the process responsible for generating the event has
+		 * terminated prior to calling into pidfd_create() and
+		 * acquiring a valid pidfd. Report FAN_NOPIDFD to the listener
+		 * in those cases.
+		 */
+		if (metadata.pid == 0 ||
+		    !pid_has_task(event->pid, PIDTYPE_TGID)) {
+			pidfd = FAN_NOPIDFD;
+		} else {
+			pidfd = pidfd_create(event->pid, 0);
+			if (pidfd < 0)
+				/*
+				 * All other pidfd creation errors are reported
+				 * as FAN_EPIDFD to the listener.
+				 */
+				pidfd = FAN_EPIDFD;
+		}
+	}
+
 	ret = -EFAULT;
 	/*
 	 * Sanity check copy size in case get_one_event() and

Quoted 4 lines

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

@@ -524,6 +561,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	}
 	metadata.fd = fd;
 

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 569

Quoted 6 lines

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

+	/*
+	 * Currently, reporting a pidfd to an unprivileged listener is not
+	 * supported. The FANOTIFY_UNPRIV flag is to be kept here so that a
+	 * pidfd is not accidentally leaked to an unprivileged listener.
+	 */
+	if (pidfd_mode && !FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV)) {

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

Quoted 4 lines

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

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

fs/notify/fanotify/fanotify_user.c static ssize_t copy_event_to_user(struct fsnotify_group *group, · line 638

Quoted 4 lines

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

+
+	if (pidfd < 0)
+		put_unused_fd(pidfd);
+

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

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.

Currently, the usage of FAN_REPORT_TID is not permitted along with FAN_REPORT_PIDFD as the pidfd API only supports the creation of pidfds for thread-group leaders. Additionally, the FAN_REPORT_PIDFD is limited to privileged processes only i.e. listeners that are running with the CAP_SYS_ADMIN capability. Attempting to supply either of these initialisation flags with FAN_REPORT_PIDFD will result with EINVAL being returned to the caller.

In the event of a pidfd creation error, there are two types of error values that can be reported back to the listener. There is FAN_NOPIDFD, which will be reported in cases where the process responsible for generating the event has terminated prior to fanotify being able to create pidfd for event->pid via pidfd_create(). The there is FAN_EPIDFD, which will be reported if a more generic pidfd creation error occurred when calling pidfd_create().

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

Quoted 32 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.

Currently, the usage of FAN_REPORT_TID is not permitted along with FAN_REPORT_PIDFD as the pidfd API only supports the creation of pidfds for thread-group leaders. Additionally, the FAN_REPORT_PIDFD is limited to privileged processes only i.e. listeners that are running with the CAP_SYS_ADMIN capability. Attempting to supply either of these initialisation flags with FAN_REPORT_PIDFD will result with EINVAL being returned to the caller.

In the event of a pidfd creation error, there are two types of error values that can be reported back to the listener. There is FAN_NOPIDFD, which will be reported in cases where the process responsible for generating the event has terminated prior to fanotify being able to create pidfd for event->pid via pidfd_create(). The there is FAN_EPIDFD, which will be reported if a more generic pidfd creation error occurred when calling pidfd_create().

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

[...]

include/uapi/linux/fanotify.h line 59

Quoted 10 lines

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

index fbf9c5c7dd59..5cb3e2369b96 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 */
 

Matthew,

One very minor comment.
I have a patch in progress to add FAN_REPORT_CHILD_FID (for reporting fid of created inode) and it would be nice if we can reserve the flag space in the same block with the rest of the FID flags.

If its not a problem, maybe we could move FAN_REPORT_PIDFD up to 0x80 right above FAN_REPORT_TID, which also happen to be related flags.

Hey Jan/Amir/Christian,

Sending through v2 of the fanotify pidfd patch series. This series contains the necessary fixes/suggestions that had come out of the previous discussions, which can be found here [0], here [1], and here [3].

The main difference in this series is that we perform pidfd creation a little earlier on i.e. in copy_event_to_user() so that clean up of the pidfd can be performed nicely in the event of an info generation/copying error. Additionally, we introduce two errors. One being FAN_NOPIDFD, which is supplied to the listener in the event that a pidfd cannot be created due to early process termination. The other being FAN_EPIDFD, which will be supplied in the event that an error was encountered during pidfd creation.

Please let me know what you think.

[0]
https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\
[email protected]/

[1]
https://lore.kernel.org/linux-fsdevel/24c761bd0bd1618c911a392d0c310c24da7d8\
[email protected]/

[2]
https://lore.kernel.org/linux-fsdevel/48d18055deb4617d97c695a08dca77eb57309\
[email protected]/

Matthew Bobrowski (5):
kernel/pid.c: remove static qualifier from pidfd_create() kernel/pid.c: implement additional checks upon pidfd_create()
parameters
fanotify/fanotify_user.c: minor cosmetic adjustments to fid labels fanotify/fanotify_user.c: introduce a generic info record copying
helper

fanotify: add pidfd support to the fanotify API

Quoted 17 lines · Matthew Bobrowski

Hey Jan/Amir/Christian,

Sending through v2 of the fanotify pidfd patch series. This series contains the necessary fixes/suggestions that had come out of the previous discussions, which can be found here [0], here [1], and here [3].

The main difference in this series is that we perform pidfd creation a little earlier on i.e. in copy_event_to_user() so that clean up of the pidfd can be performed nicely in the event of an info generation/copying error. Additionally, we introduce two errors. One being FAN_NOPIDFD, which is supplied to the listener in the event that a pidfd cannot be created due to early process termination. The other being FAN_EPIDFD, which will be supplied in the event that an error was encountered during pidfd creation.

Quoted 6 lines

kernel/pid.c: remove static qualifier from pidfd_create() kernel/pid.c: implement additional checks upon pidfd_create()
parameters
fanotify/fanotify_user.c: minor cosmetic adjustments to fid labels fanotify/fanotify_user.c: introduce a generic info record copying
helper

Above fanotify commits look good to me.
Please remove /fanotify_user.c from commit titles and use 'pidfd:' for the pidfd commit titles.

Quoted 2 lines

fanotify: add pidfd support to the fanotify API

This one looks mostly fine. Gave some minor comments.

The biggest thing I am missing is a link to an LTP test draft and man page update draft.

In general, I think it is good practice to provide a test along with any fix, but for UAPI changes we need to hold higher standards - both the test and man page draft should be a must before merge IMO.

We already know there is going to be a clause about FAN_NOPIDFD and so on... I think it is especially hard for people on linux-api list to review a UAPI change without seeing the contract in a user manual format. Yes, much of the information is in the commit message, but it is not the same thing as reading a user manual and verifying that the contract makes sense to a programmer.

Quoted 7 lines

Quoted 2 lines

fanotify: add pidfd support to the fanotify API

This one looks mostly fine. Gave some minor comments.

The biggest thing I am missing is a link to an LTP test draft and man page update draft.

Fair point, the way I approached it was that I'd get the ACK from all of you on the overall implementation and then go ahead with providing additional things like LTP and man-pages drafts, before the merge is performed.

Quoted 3 lines

In general, I think it is good practice to provide a test along with any fix, but for UAPI changes we need to hold higher standards - both the test and man page draft should be a must before merge IMO.

Agree, moving forward I will take this approach.

Quoted 6 lines

We already know there is going to be a clause about FAN_NOPIDFD and so on... I think it is especially hard for people on linux-api list to review a UAPI change without seeing the contract in a user manual format. Yes, much of the information is in the commit message, but it is not the same thing as reading a user manual and verifying that the contract makes sense to a programmer.

Makes sense.

Quoted 6 lines · Matthew Bobrowski

Quoted 3 lines

In general, I think it is good practice to provide a test along with any fix, but for UAPI changes we need to hold higher standards - both the test and man page draft should be a must before merge IMO.

Agree, moving forward I will take this approach.

Quoted 8 lines

Quoted 6 lines

We already know there is going to be a clause about FAN_NOPIDFD and so on... I think it is especially hard for people on linux-api list to review a UAPI change without seeing the contract in a user manual format. Yes, much of the information is in the commit message, but it is not the same thing as reading a user manual and verifying that the contract makes sense to a programmer.

Makes sense.

I agree with Amir that before your patches can get merged we need a manpage update & LTP coverage. But I fully understand your approach of trying to figure out how things will look like before writing the tests and manpage to save some adaptation of tests & doc as the code changes. For relatively simple changes like this one that approach is fine by me as well (for more complex API changes it's often easier to actually *start* with a manpage to get an idea where we are actually heading). I just want the tests & doc to be part of at least one submission so that e.g. people on linux-api have a good chance to review stuff without having to dive into code details.