Add pidfd support to the fanotify API

With the idea to support additional info record types in the future i.e. fanotify_event_info_pidfd, it's a good idea to rename some of the labels assigned to some of the existing fid related functions, parameters, etc which more accurately represent the intent behind their usage.

For example, copy_info_to_user() was defined with a generic function label, which arguably reads as being supportive of different info record types, however the parameter list for this function is explicitly tailored towards the creation and copying of the fanotify_event_info_fid records. This same point applies to the macro defined as FANOTIFY_INFO_HDR_LEN.

With fanotify_event_info_len(), we change the parameter label so that the function implies that it can be extended to calculate the length for additional info record types.

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

---
 fs/notify/fanotify/fanotify_user.c | 33 +++++++++++++++++-------------
 1 file changed, 19 insertions(+), 14 deletions(-)

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

index 64864fb40b40..182fea255376 100644
@@ -104,7 +104,7 @@ struct kmem_cache *fanotify_path_event_cachep __read_mostly;
 struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 
 #define FANOTIFY_EVENT_ALIGN 4
-#define FANOTIFY_INFO_HDR_LEN \
+#define FANOTIFY_FID_INFO_HDR_LEN \
        (sizeof(struct fanotify_event_info_fid) + sizeof(struct file_handle))
 
 static int fanotify_fid_info_len(int fh_len, int name_len)

@@ -114,10 +114,11 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
        if (name_len)
                info_len += name_len + 1;
 
-       return roundup(FANOTIFY_INFO_HDR_LEN + info_len, FANOTIFY_EVENT_ALIGN);
+       return roundup(FANOTIFY_FID_INFO_HDR_LEN + info_len,
+                      FANOTIFY_EVENT_ALIGN);
 }
 
-static int fanotify_event_info_len(unsigned int fid_mode,
+static int fanotify_event_info_len(unsigned int info_mode,
                                   struct fanotify_event *event)
 {
        struct fanotify_info *info = fanotify_event_info(event);

@@ -128,7 +129,8 @@ static int fanotify_event_info_len(unsigned int fid_mode,
 
        if (dir_fh_len) {
                info_len += fanotify_fid_info_len(dir_fh_len, info->name_len);
-       } else if ((fid_mode & FAN_REPORT_NAME) && (event->mask & FAN_ONDIR)) {
+       } else if ((info_mode & FAN_REPORT_NAME) &&
+                  (event->mask & FAN_ONDIR)) {
                /*
                 * With group flag FAN_REPORT_NAME, if name was not recorded in
                 * event on a directory, we will report the name ".".

@@ -303,9 +305,10 @@ static int process_access_response(struct fsnotify_group *group,
        return -ENOENT;
 }
 
-static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
-                            int info_type, const char *name, size_t name_len,
-                            char __user *buf, size_t count)
+static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
+                                int info_type, const char *name,
+                                size_t name_len,
+                                char __user *buf, size_t count)
 {
        struct fanotify_event_info_fid info = { };
        struct file_handle handle = { };

@@ -466,10 +469,11 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        if (fanotify_event_dir_fh_len(event)) {
                info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
                                             FAN_EVENT_INFO_TYPE_DFID;
-               ret = copy_info_to_user(fanotify_event_fsid(event),
-                                       fanotify_info_dir_fh(info),
-                                       info_type, fanotify_info_name(info),
-                                       info->name_len, buf, count);
+               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+                                           fanotify_info_dir_fh(info),
+                                           info_type,
+                                           fanotify_info_name(info),
+                                           info->name_len, buf, count);
                if (ret < 0)
                        goto out_close_fd;
 

@@ -515,9 +519,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
                        info_type = FAN_EVENT_INFO_TYPE_FID;
                }
 
-               ret = copy_info_to_user(fanotify_event_fsid(event),
-                                       fanotify_event_object_fh(event),
-                                       info_type, dot, dot_len, buf, count);
+               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+                                           fanotify_event_object_fh(event),
+                                           info_type, dot, dot_len,
+                                           buf, count);
                if (ret < 0)
                        goto out_close_fd;
 
--

2.32.0.432.gabb21c7263-goog

/M

The copy_info_records_to_user() helper allows for the separation of info record copying routines/conditionals from copy_event_to_user(), which reduces the overall clutter within this function. This becomes especially true as we start introducing additional info records in the future i.e. struct fanotify_event_info_pidfd. On success, this helper returns the total amount of bytes that have been copied into the user supplied buffer and on error, a negative value is returned to the caller.

The newly defined macro FANOTIFY_INFO_MODES can be used to obtain info record types that have been enabled for a specific notification group. This macro becomes useful in the subsequent patch when the FAN_REPORT_PIDFD initialization flag is introduced.

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

---
 fs/notify/fanotify/fanotify_user.c | 155 ++++++++++++++++-------------
 include/linux/fanotify.h           |   2 +
 2 files changed, 90 insertions(+), 67 deletions(-)

fs/notify/fanotify/fanotify_user.c +88 -67

index 182fea255376..d19f70b2c24c 100644
@@ -173,7 +173,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
        size_t event_size = FAN_EVENT_METADATA_LEN;
        struct fanotify_event *event = NULL;
        struct fsnotify_event *fsn_event;
-       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
+       unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
 
        pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
 

@@ -183,8 +183,8 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
                goto out;
 
        event = FANOTIFY_E(fsn_event);
-       if (fid_mode)
-               event_size += fanotify_event_info_len(fid_mode, event);
+       if (info_mode)
+               event_size += fanotify_event_info_len(info_mode, event);
 
        if (event_size > count) {
                event = ERR_PTR(-EINVAL);

@@ -401,6 +401,86 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        return info_len;
 }
 
+static int copy_info_records_to_user(struct fanotify_event *event,
+                                    struct fanotify_info *info,
+                                    unsigned int info_mode,
+                                    char __user *buf, size_t count)
+{
+       int ret, total_bytes = 0, info_type = 0;
+       unsigned int fid_mode = info_mode & FANOTIFY_FID_BITS;
+
+       /*
+        * Event info records order is as follows: dir fid + name, child fid.
+        */
+       if (fanotify_event_dir_fh_len(event)) {
+               info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
+                                            FAN_EVENT_INFO_TYPE_DFID;
+               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+                                           fanotify_info_dir_fh(info),
+                                           info_type,
+                                           fanotify_info_name(info),
+                                           info->name_len, buf, count);
+               if (ret < 0)
+                       return ret;
+
+               buf += ret;
+               count -= ret;
+               total_bytes += ret;
+       }
+
+       if (fanotify_event_object_fh_len(event)) {
+               const char *dot = NULL;
+               int dot_len = 0;
+
+               if (fid_mode == FAN_REPORT_FID || info_type) {
+                       /*
+                        * With only group flag FAN_REPORT_FID only type FID is
+                        * reported. Second info record type is always FID.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_FID;
+               } else if ((fid_mode & FAN_REPORT_NAME) &&
+                          (event->mask & FAN_ONDIR)) {
+                       /*
+                        * With group flag FAN_REPORT_NAME, if name was not
+                        * recorded in an event on a directory, report the name
+                        * "." with info type DFID_NAME.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
+                       dot = ".";
+                       dot_len = 1;
+               } else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
+                          (event->mask & FAN_ONDIR)) {
+                       /*
+                        * With group flag FAN_REPORT_DIR_FID, a single info
+                        * record has type DFID for directory entry
+                        * modificatio\ n event and for event on a directory.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_DFID;
+               } else {
+                       /*
+                        * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
+                        * a single info record has type FID for event on a
+                        * non-directory, when there is no directory to report.
+                        * For example, on FAN_DELETE_SELF event.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_FID;
+               }
+
+               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+                                           fanotify_event_object_fh(event),
+                                           info_type, dot, dot_len,
+                                           buf, count);
+               if (ret < 0)
+                       return ret;
+
+               buf += ret;
+               count -= ret;
+               total_bytes += ret;
+       }
+
+       return total_bytes;
+}
+
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
                                  struct fanotify_event *event,
                                  char __user *buf, size_t count)

@@ -408,15 +488,14 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        struct fanotify_event_metadata metadata;
        struct path *path = fanotify_event_path(event);
        struct fanotify_info *info = fanotify_event_info(event);
-       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
+       unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
        struct file *f = NULL;
        int ret, fd = FAN_NOFD;
-       int info_type = 0;
 
        pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
        metadata.event_len = FAN_EVENT_METADATA_LEN +
-                               fanotify_event_info_len(fid_mode, event);
+                               fanotify_event_info_len(info_mode, event);
        metadata.metadata_len = FAN_EVENT_METADATA_LEN;
        metadata.vers = FANOTIFY_METADATA_VERSION;
        metadata.reserved = 0;

@@ -465,69 +544,11 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        if (f)
                fd_install(fd, f);
 
-       /* Event info records order is: dir fid + name, child fid */
-       if (fanotify_event_dir_fh_len(event)) {
-               info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
-                                            FAN_EVENT_INFO_TYPE_DFID;
-               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
-                                           fanotify_info_dir_fh(info),
-                                           info_type,
-                                           fanotify_info_name(info),
-                                           info->name_len, buf, count);
-               if (ret < 0)
-                       goto out_close_fd;
-
-               buf += ret;
-               count -= ret;
-       }
-
-       if (fanotify_event_object_fh_len(event)) {
-               const char *dot = NULL;
-               int dot_len = 0;
-
-               if (fid_mode == FAN_REPORT_FID || info_type) {
-                       /*
-                        * With only group flag FAN_REPORT_FID only type FID is
-                        * reported. Second info record type is always FID.
-                        */
-                       info_type = FAN_EVENT_INFO_TYPE_FID;
-               } else if ((fid_mode & FAN_REPORT_NAME) &&
-                          (event->mask & FAN_ONDIR)) {
-                       /*
-                        * With group flag FAN_REPORT_NAME, if name was not
-                        * recorded in an event on a directory, report the
-                        * name "." with info type DFID_NAME.
-                        */
-                       info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
-                       dot = ".";
-                       dot_len = 1;
-               } else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
-                          (event->mask & FAN_ONDIR)) {
-                       /*
-                        * With group flag FAN_REPORT_DIR_FID, a single info
-                        * record has type DFID for directory entry modification
-                        * event and for event on a directory.
-                        */
-                       info_type = FAN_EVENT_INFO_TYPE_DFID;
-               } else {
-                       /*
-                        * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
-                        * a single info record has type FID for event on a
-                        * non-directory, when there is no directory to report.
-                        * For example, on FAN_DELETE_SELF event.
-                        */
-                       info_type = FAN_EVENT_INFO_TYPE_FID;
-               }
-
-               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
-                                           fanotify_event_object_fh(event),
-                                           info_type, dot, dot_len,
-                                           buf, count);
+       if (info_mode) {
+               ret = copy_info_records_to_user(event, info, info_mode,
+                                               buf, count);
                if (ret < 0)
                        goto out_close_fd;
-
-               buf += ret;
-               count -= ret;
        }
 
        return metadata.event_len;

include/linux/fanotify.h +2 -1

index a16dbeced152..10a7e26ddba6 100644
@@ -27,6 +27,8 @@ 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)
+
 /*
  * fanotify_init() flags that require CAP_SYS_ADMIN.
  * We do not allow unprivileged groups to request permission events.
--

2.32.0.432.gabb21c7263-goog

/M

Quoted 10 lines · Matthew Bobrowski

The copy_info_records_to_user() helper allows for the separation of info record copying routines/conditionals from copy_event_to_user(), which reduces the overall clutter within this function. This becomes especially true as we start introducing additional info records in the future i.e. struct fanotify_event_info_pidfd. On success, this helper returns the total amount of bytes that have been copied into the user supplied buffer and on error, a negative value is returned to the caller.

Quoted 6 lines

The newly defined macro FANOTIFY_INFO_MODES can be used to obtain info record types that have been enabled for a specific notification group. This macro becomes useful in the subsequent patch when the FAN_REPORT_PIDFD initialization flag is introduced.

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

Reviewed-by Amir Goldstein <[email protected]>

Quoted 78 lines

---
 fs/notify/fanotify/fanotify_user.c | 155 ++++++++++++++++-------------
 include/linux/fanotify.h           |   2 +
 2 files changed, 90 insertions(+), 67 deletions(-)

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

index 182fea255376..d19f70b2c24c 100644
@@ -173,7 +173,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
        size_t event_size = FAN_EVENT_METADATA_LEN;
        struct fanotify_event *event = NULL;
        struct fsnotify_event *fsn_event;
-       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
+       unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
 
        pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
 

@@ -183,8 +183,8 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
                goto out;
 
        event = FANOTIFY_E(fsn_event);
-       if (fid_mode)
-               event_size += fanotify_event_info_len(fid_mode, event);
+       if (info_mode)
+               event_size += fanotify_event_info_len(info_mode, event);
 
        if (event_size > count) {
                event = ERR_PTR(-EINVAL);

@@ -401,6 +401,86 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        return info_len;
 }
 
+static int copy_info_records_to_user(struct fanotify_event *event,
+                                    struct fanotify_info *info,
+                                    unsigned int info_mode,
+                                    char __user *buf, size_t count)
+{
+       int ret, total_bytes = 0, info_type = 0;
+       unsigned int fid_mode = info_mode & FANOTIFY_FID_BITS;
+
+       /*
+        * Event info records order is as follows: dir fid + name, child fid.
+        */
+       if (fanotify_event_dir_fh_len(event)) {
+               info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
+                                            FAN_EVENT_INFO_TYPE_DFID;
+               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+                                           fanotify_info_dir_fh(info),
+                                           info_type,
+                                           fanotify_info_name(info),
+                                           info->name_len, buf, count);
+               if (ret < 0)
+                       return ret;
+
+               buf += ret;
+               count -= ret;
+               total_bytes += ret;
+       }
+
+       if (fanotify_event_object_fh_len(event)) {
+               const char *dot = NULL;
+               int dot_len = 0;
+
+               if (fid_mode == FAN_REPORT_FID || info_type) {
+                       /*
+                        * With only group flag FAN_REPORT_FID only type FID is
+                        * reported. Second info record type is always FID.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_FID;
+               } else if ((fid_mode & FAN_REPORT_NAME) &&
+                          (event->mask & FAN_ONDIR)) {
+                       /*
+                        * With group flag FAN_REPORT_NAME, if name was not
+                        * recorded in an event on a directory, report the name
+                        * "." with info type DFID_NAME.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 456

Quoted 8 lines

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

+                       dot = ".";
+                       dot_len = 1;
+               } else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
+                          (event->mask & FAN_ONDIR)) {
+                       /*
+                        * With group flag FAN_REPORT_DIR_FID, a single info
+                        * record has type DFID for directory entry
+                        * modificatio\ n event and for event on a directory.

Quoted 98 lines · Amir Goldstein

Quoted 10 lines · Matthew Bobrowski

The copy_info_records_to_user() helper allows for the separation of info record copying routines/conditionals from copy_event_to_user(), which reduces the overall clutter within this function. This becomes especially true as we start introducing additional info records in the future i.e. struct fanotify_event_info_pidfd. On success, this helper returns the total amount of bytes that have been copied into the user supplied buffer and on error, a negative value is returned to the caller.

Quoted 6 lines

The newly defined macro FANOTIFY_INFO_MODES can be used to obtain info record types that have been enabled for a specific notification group. This macro becomes useful in the subsequent patch when the FAN_REPORT_PIDFD initialization flag is introduced.

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

Reviewed-by Amir Goldstein <[email protected]>

Quoted 78 lines

---
 fs/notify/fanotify/fanotify_user.c | 155 ++++++++++++++++-------------
 include/linux/fanotify.h           |   2 +
 2 files changed, 90 insertions(+), 67 deletions(-)

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

index 182fea255376..d19f70b2c24c 100644
@@ -173,7 +173,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
        size_t event_size = FAN_EVENT_METADATA_LEN;
        struct fanotify_event *event = NULL;
        struct fsnotify_event *fsn_event;
-       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
+       unsigned int info_mode = FAN_GROUP_FLAG(group, FANOTIFY_INFO_MODES);
 
        pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
 

@@ -183,8 +183,8 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
                goto out;
 
        event = FANOTIFY_E(fsn_event);
-       if (fid_mode)
-               event_size += fanotify_event_info_len(fid_mode, event);
+       if (info_mode)
+               event_size += fanotify_event_info_len(info_mode, event);
 
        if (event_size > count) {
                event = ERR_PTR(-EINVAL);

@@ -401,6 +401,86 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
        return info_len;
 }
 
+static int copy_info_records_to_user(struct fanotify_event *event,
+                                    struct fanotify_info *info,
+                                    unsigned int info_mode,
+                                    char __user *buf, size_t count)
+{
+       int ret, total_bytes = 0, info_type = 0;
+       unsigned int fid_mode = info_mode & FANOTIFY_FID_BITS;
+
+       /*
+        * Event info records order is as follows: dir fid + name, child fid.
+        */
+       if (fanotify_event_dir_fh_len(event)) {
+               info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
+                                            FAN_EVENT_INFO_TYPE_DFID;
+               ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+                                           fanotify_info_dir_fh(info),
+                                           info_type,
+                                           fanotify_info_name(info),
+                                           info->name_len, buf, count);
+               if (ret < 0)
+                       return ret;
+
+               buf += ret;
+               count -= ret;
+               total_bytes += ret;
+       }
+
+       if (fanotify_event_object_fh_len(event)) {
+               const char *dot = NULL;
+               int dot_len = 0;
+
+               if (fid_mode == FAN_REPORT_FID || info_type) {
+                       /*
+                        * With only group flag FAN_REPORT_FID only type FID is
+                        * reported. Second info record type is always FID.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_FID;
+               } else if ((fid_mode & FAN_REPORT_NAME) &&
+                          (event->mask & FAN_ONDIR)) {
+                       /*
+                        * With group flag FAN_REPORT_NAME, if name was not
+                        * recorded in an event on a directory, report the name
+                        * "." with info type DFID_NAME.
+                        */
+                       info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;

Quoted 8 lines

fs/notify/fanotify/fanotify_user.c static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh, · line 456

Quoted 8 lines

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

+                       dot = ".";
+                       dot_len = 1;
+               } else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
+                          (event->mask & FAN_ONDIR)) {
+                       /*
+                        * With group flag FAN_REPORT_DIR_FID, a single info
+                        * record has type DFID for directory entry
+                        * modificatio\ n event and for event on a directory.

Just notices this typo in the copied comment:
modificatio\ n

And for the next posting, please remove the mention of fanotify_user.c from the commit title - it adds no information and clutters the git oneline log. This concerns patch 3/5 as well. It does NOT concern the kernel/pid.c patches.

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 initialization 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 v2:

* The FAN_REPORT_PIDFD flag value has been changed from 0x00001000 to 0x00000080. This was so that future FID related initialization flags could be grouped nicely.

* Fixed pidfd clean up at out_close_fd label in copy_event_to_user(). Reversed the conditional and it now uses the close_fd() helper instead of put_unused_fd() as we also need to close the backing file, not just just mark the pidfd free in the fdtable.

* Shuffled around the WARN_ON_ONCE(FAN_REPORT_TID) within copy_event_to_user() so that it's inside the if (pidfd_mode) branch. It makes more sense to be as close to pidfd creation as possible.

* Fixed up the comment block within the if (pidfd_mode) branch.

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

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

@@ -489,8 +526,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 = FAN_NOPIDFD, fd = FAN_NOFD;
 
        pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd

Quoted 22 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 initialization flags with FAN_REPORT_PIDFD will result with EINVAL being returned to the caller.

Quoted 5 lines

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

I think that "...prior to event listener reading the event..." is a more accurate description of the situation.

Quoted 28 lines

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 v2:

* The FAN_REPORT_PIDFD flag value has been changed from 0x00001000 to 0x00000080. This was so that future FID related initialization flags could be grouped nicely.

* Fixed pidfd clean up at out_close_fd label in copy_event_to_user(). Reversed the conditional and it now uses the close_fd() helper instead of put_unused_fd() as we also need to close the backing file, not just just mark the pidfd free in the fdtable.

* Shuffled around the WARN_ON_ONCE(FAN_REPORT_TID) within copy_event_to_user() so that it's inside the if (pidfd_mode) branch. It makes more sense to be as close to pidfd creation as possible.

* Fixed up the comment block within the if (pidfd_mode) branch.

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

[...]

Quoted 26 lines

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

@@ -489,8 +526,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 = FAN_NOPIDFD, fd = FAN_NOFD;
 
        pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*

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

Quoted 4 lines

fs/notify/fanotify/fanotify_user.c +4 -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

I find the description above to be "over dramatic".
An event listener reading events after generating process has terminated could be quite common in case of one shot tools like mv,touch,etc.

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 initialization 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().

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/status?

Quoted 82 lines · Amir Goldstein

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Quoted 4 lines

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

You mean the FAN_REPORT_PIDFD interface, right?

Quoted 5 lines

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

Yes, the primary use case would be for reliable auditing i.e. what actual process had accessed what filesystem object of interest. Generally, finding what process is a little unreliable at the moment given that the reporting event->pid and crawling through /proc based on that has been observed to lead to certain inaccuracy in the past i.e. reporting an access that was in fact not performed by event->pid.

The permission model doesn't work in this case given that it takes the "blocking" approach and not it's not something that can always be afforded...

Quoted 3 lines

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

The idea is that the event listener, or receiver will use the pidfd_send_signal(2) and specify event->info->pidfd as one of its arguments in order to _reliably_ determine whether the process that generated the event is still around. If so, it can freely ascertain further contextual information from /proc reliably.

Quoted 3 lines

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/stat

TBH, I don't fully understand what you mean by this model...

Quoted 84 lines · Matthew Bobrowski

Quoted 82 lines · Amir Goldstein

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Quoted 6 lines

Quoted 4 lines

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

You mean the FAN_REPORT_PIDFD interface, right?

No, I mean FAN_REPORT_FID.
We have a new interface that does not pollute reader process fd table with fds of event->fd, so maybe let's try to avoiding regressing this use case by polluting the reader process fd table with pidfds.

Quoted 27 lines

Quoted 5 lines

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

Yes, the primary use case would be for reliable auditing i.e. what actual process had accessed what filesystem object of interest. Generally, finding what process is a little unreliable at the moment given that the reporting event->pid and crawling through /proc based on that has been observed to lead to certain inaccuracy in the past i.e. reporting an access that was in fact not performed by event->pid.

The permission model doesn't work in this case given that it takes the "blocking" approach and not it's not something that can always be afforded...

Quoted 3 lines

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

The idea is that the event listener, or receiver will use the pidfd_send_signal(2) and specify event->info->pidfd as one of its arguments in order to _reliably_ determine whether the process that generated the event is still around. If so, it can freely ascertain further contextual information from /proc reliably.

Quoted 6 lines

Quoted 3 lines

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/stat

TBH, I don't fully understand what you mean by this model...

The model is this:

FAN_REPORT_UPID (or something) will report an info record with a unique identifier of the generating process or thread, because there is no restriction imposed by pidfd to support only group leaders.

That unique identifier may be obtained from /proc, e.g.:

$ cat /proc/self/upid
633733.0

In this case .0 represents generation 0.
If pid numbers would wrap around in that pid namespace generation would be bumped and next process to get pid 633733 would have a unique id 633733.1.

There are probably more pid namespace considerations of how that /proc API will be designed exactly.

Quoted 141 lines · Amir Goldstein

Quoted 84 lines · Matthew Bobrowski

Quoted 82 lines · Amir Goldstein

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Quoted 6 lines

Quoted 4 lines

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

You mean the FAN_REPORT_PIDFD interface, right?

No, I mean FAN_REPORT_FID.
We have a new interface that does not pollute reader process fd table with fds of event->fd, so maybe let's try to avoiding regressing this use case by polluting the reader process fd table with pidfds.

Quoted 27 lines

Quoted 5 lines

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

Yes, the primary use case would be for reliable auditing i.e. what actual process had accessed what filesystem object of interest. Generally, finding what process is a little unreliable at the moment given that the reporting event->pid and crawling through /proc based on that has been observed to lead to certain inaccuracy in the past i.e. reporting an access that was in fact not performed by event->pid.

The permission model doesn't work in this case given that it takes the "blocking" approach and not it's not something that can always be afforded...

Quoted 3 lines

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

The idea is that the event listener, or receiver will use the pidfd_send_signal(2) and specify event->info->pidfd as one of its arguments in order to _reliably_ determine whether the process that generated the event is still around. If so, it can freely ascertain further contextual information from /proc reliably.

Quoted 6 lines

Quoted 3 lines

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/stat

TBH, I don't fully understand what you mean by this model...

The model is this:

FAN_REPORT_UPID (or something) will report an info record with a unique identifier of the generating process or thread, because there is no restriction imposed by pidfd to support only group leaders.

That unique identifier may be obtained from /proc, e.g.:

$ cat /proc/self/upid
633733.0

Quoted 7 lines

In this case .0 represents generation 0.
If pid numbers would wrap around in that pid namespace generation would be bumped and next process to get pid 633733 would have a unique id 633733.1.

There are probably more pid namespace considerations of how that /proc API will be designed exactly.

I'm not a fan of this at all to be honest. This very much reminds me of (a weak version of) pid uuids which has been very controversial. This sounds all kinds of messy. If the pid gets recycled then you bump the generation number in all pid namespace where that pid has been recycled and not in the others and then you expose it through /proc. Then if a process from one pid namespaces looks at a process from another pid namespace through the proc file what would it see as the generation number? That can probably all be solved but the API sounds justy very unpleasant and hacky.

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/status?

Or am I completely misunderstanding the use case?

Quoted 72 lines · Amir Goldstein

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Quoted 2 lines

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Me neither. But I guess it's one more reason why any fd-generating variant of fanotify should stay priviledged.

Quoted 21 lines

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

Quoted 5 lines

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/status?

Or am I completely misunderstanding the use case?

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

But so far nothing bright has come to my mind. :-|

Quoted 124 lines · Jan Kara

Quoted 72 lines · Amir Goldstein

Quoted 61 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 9 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Quoted 2 lines

Interesting. I've never considered this interface flaw. Thanks for bringing this up!

Me neither. But I guess it's one more reason why any fd-generating variant of fanotify should stay priviledged.

Quoted 21 lines

Quoted 6 lines

I realize that existing fanotify code appears to be violating that rule already, and that you're limiting creation of fanotify file descriptors that can hit this codepath to CAP_SYS_ADMIN, but still, I think fanotify_read() probably ought to be an ioctl, or something along those lines, instead of an f_op->read handler if it messes with the caller's fd table?

Naturally, we cannot change the legacy interface. However, since fanotify has a modern FAN_REPORT_FID interface which does not mess with fd table maybe this is an opportunity not to repeat the same mistake for the FAN_REPORT_FID interface.

Matthew, can you explain what is the use case of the consumer application of pidfd. I am guessing this is for an audit user case? because if it were for permission events, event->pid would have been sufficient.

If that is the case, then I presume that the application does not really need to operate on the pidfd, it only need to avoid reporting wrong process details after pid wraparound?

Quoted 5 lines

If that is the case, then maybe a model similar to inode generation can be used to report a "pid generation" in addition to event->pid and export pid generation in /proc/<pid>/status?

Or am I completely misunderstanding the use case?

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

Also pids can be recycled so they are probably not completely reliable identifiers?

What if someone wants to process events from fanotify group by multiple processes / threads (fd can be inherited also through fork(2)...)?

I'm currently undecided whether explicit FAN_PROTECTED flag (and impact on receiving / not receiving whole event) makes this better.

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

Quoted 2 lines

What if someone wants to process events from fanotify group by multiple processes / threads (fd can be inherited also through fork(2)...)?

That's the same as passing fd between processes, no? If users want to do that, we will need to implement the ioctl or fanotify_init() flag FAN_SHARED.

Quoted 75 lines · Amir Goldstein

Quoted 68 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

Quoted 3 lines

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

I agree. So do we store thread group leader PID in fanotify group? What if thread group leader changes? I guess I have to do some reading as I don't know how all these details work internally.

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

Yes, if we hold refcount on struct pid, it should be safe against recycling. But cannot someone (even unpriviledged process in this case) mount some attack by creating a process which creates fanotify group, passes fanotify fd, and dies but pid would be still blocked because fanotify holds reference to it? I guess this is not practical as the number of fanotify groups is limited as well as number of fds.

Quoted 6 lines

Quoted 2 lines

What if someone wants to process events from fanotify group by multiple processes / threads (fd can be inherited also through fork(2)...)?

That's the same as passing fd between processes, no? If users want to do that, we will need to implement the ioctl or fanotify_init() flag FAN_SHARED.

Well, FAN_SHARED would be the current behavior so I don't think there's any point in that (we'd loose much of the security benefit gained by this excercise). I agree we'd need to implement the ioctl for such usecase but my point was that we could have a relatively sensible setup in which multiple pids may need to read events from fanotify queue and so fanotify group would need to track multiple pids allowed to read from it.

I'm sorry if I sound negative at times. I'm not settled on any particular solution. I'm just trying to brainstorm various pros and cons of possible solutions to settle on what's going to be the best :).

Quoted 112 lines · Jan Kara

Quoted 75 lines · Amir Goldstein

Quoted 68 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

Quoted 3 lines

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

I agree. So do we store thread group leader PID in fanotify group? What if thread group leader changes? I guess I have to do some reading as I don't know how all these details work internally.

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

Yes, if we hold refcount on struct pid, it should be safe against recycling. But cannot someone (even unpriviledged process in this case) mount some attack by creating a process which creates fanotify group, passes fanotify fd, and dies but pid would be still blocked because fanotify holds reference to it? I guess this is not practical as the number of fanotify groups is limited as well as number of fds.

Quoted 6 lines

Quoted 2 lines

What if someone wants to process events from fanotify group by multiple processes / threads (fd can be inherited also through fork(2)...)?

That's the same as passing fd between processes, no? If users want to do that, we will need to implement the ioctl or fanotify_init() flag FAN_SHARED.

Well, FAN_SHARED would be the current behavior so I don't think there's any point in that (we'd loose much of the security benefit gained by this excercise). I agree we'd need to implement the ioctl for such usecase but my point was that we could have a relatively sensible setup in which multiple pids may need to read events from fanotify queue and so fanotify group would need to track multiple pids allowed to read from it.

Quoted 3 lines

I'm sorry if I sound negative at times. I'm not settled on any particular solution. I'm just trying to brainstorm various pros and cons of possible solutions to settle on what's going to be the best :).

Quite honestly, I'm struggling to understand what problem we're trying to solve here... That is, whether this is an actual "security" problem, or whether it's just attempting to come up with a solution which conforms to the "general" rule of not modifying a callers fdtable upon calling functions like read().

Can someone please elaborate a little on the exact "security" implications or "threats" we're attempting to alleviate through the implementation of the additional aforementioned group initialization flags and ioctls? What is the exact scenario we're attempting to avoid which could lead to a compromise in the systems overall integrity?

Also, I can understand this as a "general" rule:

"A process should be able to receive a file descriptor from an untrusted
source and call functions like read() on it without worrying about affecting its own file descriptor table state with that."

But, in instances where event processing is offloaded to a separate dedicated "reader" process, does that actually fall under receiving a file descriptor from an "untrusted" source? I don't think so. Modification of a callers fdtable upon calling functions like read() may be considered "undesired" in general, but this is just how fanotify has always worked, is it not?

Quoted 76 lines · Amir Goldstein

Quoted 74 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

Quoted 3 lines

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

I agree. So do we store thread group leader PID in fanotify group? What if thread group leader changes? I guess I have to do some reading as I don't

know how all these details work internally.

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

Yes, if we hold refcount on struct pid, it should be safe against recycling. But cannot someone (even unpriviledged process in this case) mount some attack by creating a process which creates fanotify group, passes fanotify fd, and dies but pid would be still blocked because fanotify holds reference to it? I guess this is not practical as the number of fanotify groups is limited as well as number of fds.

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
        /* The thread which called fanotify_init() has died already. */
        return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

I hope this causes more clarity ?then confusion. :)

Quoted 139 lines · Christian Brauner

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
        /* The thread which called fanotify_init() has died already. */
        return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

Quoted 1 line

I hope this causes more clarity ?then confusion. :)

I'm afraid it's the latter :D

Sigh! We must simplify.

Thinking out loud, instead of sealing the possibility of another process reading pidfd, maybe just avoid the most obvious unintentional leak of fanotify_fd to another process by mandating FAN_CLOEXEC?

Quoted 143 lines · Amir Goldstein

Quoted 139 lines · Christian Brauner

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
        /* The thread which called fanotify_init() has died already. */
        return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

Quoted 1 line

I hope this causes more clarity ?then confusion. :)

Quoted 7 lines

I'm afraid it's the latter :D

Sigh! We must simplify.

Thinking out loud, instead of sealing the possibility of another process reading pidfd, maybe just avoid the most obvious unintentional leak of fanotify_fd to another process by mandating FAN_CLOEXEC?

Well, I don't think we need any protection from leaking fanotify_fd. It is special fd with special priviledges as any other. If you leak it, well, bad luck but that's how Unix priviledge model works.

The threat IMO is that you have a process X, that process expects to receive fd to work with from process Y. Now process Y is malicious (or taken over by an attacker) and passes to X fanotify_fd. X reads from fanotify_fd to get data to process, it performs all kinds of validity checks on untrusted input but it does not expect that the read has side effects on X's file_table and in the worst case can lead to some compromise of X or easily to DoS on X by exhausting its file_table space.

Currently this attack vector is moot because you have to have CAP_SYS_ADMIN to get to fanotify_fd and then you can certainly do worse things. But OTOH I can see why Jann was uneasy about this.

Quoted 164 lines · Jan Kara

Quoted 143 lines · Amir Goldstein

Quoted 139 lines · Christian Brauner

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
        /* The thread which called fanotify_init() has died already. */
        return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

Quoted 1 line

I hope this causes more clarity ?then confusion. :)

Quoted 7 lines

I'm afraid it's the latter :D

Sigh! We must simplify.

Thinking out loud, instead of sealing the possibility of another process reading pidfd, maybe just avoid the most obvious unintentional leak of fanotify_fd to another process by mandating FAN_CLOEXEC?

Well, I don't think we need any protection from leaking fanotify_fd. It is special fd with special priviledges as any other. If you leak it, well, bad luck but that's how Unix priviledge model works.

The threat IMO is that you have a process X, that process expects to receive fd to work with from process Y. Now process Y is malicious (or taken over by an attacker) and passes to X fanotify_fd. X reads from fanotify_fd to get data to process, it performs all kinds of validity checks on untrusted input but it does not expect that the read has side effects on X's file_table and in the worst case can lead to some compromise of X or easily to DoS on X by exhausting its file_table space.

Quoted 3 lines

Currently this attack vector is moot because you have to have CAP_SYS_ADMIN to get to fanotify_fd and then you can certainly do worse things. But OTOH I can see why Jann was uneasy about this.

As I have breifly expressed in my previous emails, the cause for concern here is flakey IMO. If there's sensible something that I'm clearly missing, then please explain.

From my perspective, the only sensible attack vector that's maybe worth worrying about here is the possibility of exhausting the fdtable of a given process, which yes, can be considered as a form of DoS. However, in any case, there are other defensive protections/measures that a programmer could employ in their application code which could prevent such from ever happening.

The whole passing of file descriptors between process Y and process X and the leaking of a file descriptor thing simply goes back to what you've mentioned above Jan. I consider it a very weak argument. When enabling FAN_REPORT_PIDFD, the process requires CAP_SYS_ADMIN. If that process ever has its execution flow hijacked by an attacker, then I'm sorry, I think there's other larger causes for concern at that point rather then worrying about the state of some other child processes fdtable.

In general cases, I get that passing a file descriptor between process Y and process X and then having process X's fdtable modified as result of calling functions like read() is considered undesired. But, for applications that makes use of fanotify is there ever a case where we pass the fanotify file descriptor to a random/unexpected process and have it process events? I don't think so. So, I suppose what I'm trying to say is that, if an application chooses to opt-in and use a flag like FAN_REPORT_PIDFD or any other future file descriptor generating variant, the expectation is that which ever process is created and event processing is passed to that process, then it should always expect to have its fdtable modified when reading events.

Quoted 165 lines · Matthew Bobrowski

Quoted 164 lines · Jan Kara

Quoted 143 lines · Amir Goldstein

Quoted 139 lines · Christian Brauner

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
        /* The thread which called fanotify_init() has died already. */
        return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

Quoted 1 line

I hope this causes more clarity ?then confusion. :)

Quoted 7 lines

I'm afraid it's the latter :D

Sigh! We must simplify.

Thinking out loud, instead of sealing the possibility of another process reading pidfd, maybe just avoid the most obvious unintentional leak of fanotify_fd to another process by mandating FAN_CLOEXEC?

Well, I don't think we need any protection from leaking fanotify_fd. It is special fd with special priviledges as any other. If you leak it, well, bad luck but that's how Unix priviledge model works.

The threat IMO is that you have a process X, that process expects to receive fd to work with from process Y. Now process Y is malicious (or taken over by an attacker) and passes to X fanotify_fd. X reads from fanotify_fd to get data to process, it performs all kinds of validity checks on untrusted input but it does not expect that the read has side effects on X's file_table and in the worst case can lead to some compromise of X or easily to DoS on X by exhausting its file_table space.

Quoted 7 lines

Quoted 3 lines

Currently this attack vector is moot because you have to have CAP_SYS_ADMIN to get to fanotify_fd and then you can certainly do worse things. But OTOH I can see why Jann was uneasy about this.

As I have breifly expressed in my previous emails, the cause for concern here is flakey IMO. If there's sensible something that I'm clearly missing, then please explain.

No, I think your understanding is correct.

Quoted 15 lines

From my perspective, the only sensible attack vector that's maybe worth worrying about here is the possibility of exhausting the fdtable of a given process, which yes, can be considered as a form of DoS. However, in any case, there are other defensive protections/measures that a programmer could employ in their application code which could prevent such from ever happening.

The whole passing of file descriptors between process Y and process X and the leaking of a file descriptor thing simply goes back to what you've mentioned above Jan. I consider it a very weak argument. When enabling FAN_REPORT_PIDFD, the process requires CAP_SYS_ADMIN. If that process ever has its execution flow hijacked by an attacker, then I'm sorry, I think there's other larger causes for concern at that point rather then worrying about the state of some other child processes fdtable.

Quoted 11 lines

In general cases, I get that passing a file descriptor between process Y and process X and then having process X's fdtable modified as result of calling functions like read() is considered undesired. But, for applications that makes use of fanotify is there ever a case where we pass the fanotify file descriptor to a random/unexpected process and have it process events? I don't think so. So, I suppose what I'm trying to say is that, if an application chooses to opt-in and use a flag like FAN_REPORT_PIDFD or any other future file descriptor generating variant, the expectation is that which ever process is created and event processing is passed to that process, then it should always expect to have its fdtable modified when reading events.

Yes, I was thinking about this some more and at this point, given the lack of convenient options for the hardening, I think the best option is to keep the interface as originally planned. Because I'm afraid the hardening options we were able to come up with would only cause confusion (and from confusion bugs easily arise) for little security gain.

Quoted 203 lines · Jan Kara

Quoted 165 lines · Matthew Bobrowski

Quoted 164 lines · Jan Kara

Quoted 143 lines · Amir Goldstein

Quoted 139 lines · Christian Brauner

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
        /* The thread which called fanotify_init() has died already. */
        return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

Quoted 1 line

I hope this causes more clarity ?then confusion. :)

Quoted 7 lines

I'm afraid it's the latter :D

Sigh! We must simplify.

Thinking out loud, instead of sealing the possibility of another process reading pidfd, maybe just avoid the most obvious unintentional leak of fanotify_fd to another process by mandating FAN_CLOEXEC?

Well, I don't think we need any protection from leaking fanotify_fd. It is special fd with special priviledges as any other. If you leak it, well, bad luck but that's how Unix priviledge model works.

The threat IMO is that you have a process X, that process expects to receive fd to work with from process Y. Now process Y is malicious (or taken over by an attacker) and passes to X fanotify_fd. X reads from fanotify_fd to get data to process, it performs all kinds of validity checks on untrusted input but it does not expect that the read has side effects on X's file_table and in the worst case can lead to some compromise of X or easily to DoS on X by exhausting its file_table space.

Quoted 7 lines

Quoted 3 lines

Currently this attack vector is moot because you have to have CAP_SYS_ADMIN to get to fanotify_fd and then you can certainly do worse things. But OTOH I can see why Jann was uneasy about this.

As I have breifly expressed in my previous emails, the cause for concern here is flakey IMO. If there's sensible something that I'm clearly missing, then please explain.

No, I think your understanding is correct.

Quoted 26 lines

From my perspective, the only sensible attack vector that's maybe worth worrying about here is the possibility of exhausting the fdtable of a given process, which yes, can be considered as a form of DoS. However, in any case, there are other defensive protections/measures that a programmer could employ in their application code which could prevent such from ever happening.

The whole passing of file descriptors between process Y and process X and the leaking of a file descriptor thing simply goes back to what you've mentioned above Jan. I consider it a very weak argument. When enabling FAN_REPORT_PIDFD, the process requires CAP_SYS_ADMIN. If that process ever has its execution flow hijacked by an attacker, then I'm sorry, I think there's other larger causes for concern at that point rather then worrying about the state of some other child processes fdtable.

In general cases, I get that passing a file descriptor between process Y and process X and then having process X's fdtable modified as result of calling functions like read() is considered undesired. But, for applications that makes use of fanotify is there ever a case where we pass the fanotify file descriptor to a random/unexpected process and have it process events? I don't think so. So, I suppose what I'm trying to say is that, if an application chooses to opt-in and use a flag like FAN_REPORT_PIDFD or any other future file descriptor generating variant, the expectation is that which ever process is created and event processing is passed to that process, then it should always expect to have its fdtable modified when reading events.

Quoted 5 lines

Yes, I was thinking about this some more and at this point, given the lack of convenient options for the hardening, I think the best option is to keep the interface as originally planned. Because I'm afraid the hardening options we were able to come up with would only cause confusion (and from confusion bugs easily arise) for little security gain.

OK, in that case are you happy for me to post hopefully the last iteration of this series with the minor nits addressed?

Quoted 80 lines · Amir Goldstein

Quoted 69 lines · Jan Kara

Quoted 60 lines · Amir Goldstein

Quoted 46 lines · Amir Goldstein

Quoted 17 lines · Jan Kara

Well, but pidfd also makes sure that /proc/<pid>/ keeps belonging to the same process while you read various data from it. And you cannot achieve that with pid+generation thing you've suggested. Plus the additional concept and its complexity is non-trivial So I tend to agree with Christian that we really want to return pidfd.

Given returning pidfd is CAP_SYS_ADMIN priviledged operation I'm undecided whether it is worth the trouble to come up with some other mechanism how to return pidfd with the event. We could return some cookie which could be then (by some ioctl or so) either transformed into real pidfd or released (so that we can release pid handle in the kernel) but it looks ugly and complicates things for everybody without bringing significant security improvement (we already can pass fd with the event). So I'm pondering whether there's some other way how we could make the interface safer - e.g. so that the process receiving the event (not the one creating the group) would also need to opt in for getting fds created in its file table.

Quoted 2 lines

But so far nothing bright has come to my mind. :-|

There is a way, it is not bright, but it is pretty simple - store an optional pid in group->fanotify_data.fd_reader.

With flag FAN_REPORT_PIDFD, both pidfd and event->fd reporting will be disabled to any process other than fd_reader. Without FAN_REPORT_PIDFD, event->fd reporting will be disabled if fd_reaader is set to a process other than the reader.

A process can call ioctl START_FD_READER to set fd_reader to itself. With FAN_REPORT_PIDFD, if reaader_fd is NULL and the reader process has CAP_SYS_ADMIN, read() sets fd_reader to itself.

Permission wise, START_FD_READER is allowed with CAP_SYS_ADMIN or if fd_reader is not owned by another process. We may consider YIELD_FD_READER ioctl if needed.

I think that this is a pretty cheap price for implementation and maybe acceptable overhead for complicating the API? Note that without passing fd, there is no need for any ioctl.

An added security benefit is that the ioctl adds is a way for the caller of fanotify_init() to make sure that even if the fanotify_fd is leaked, that event->fd will not be leaked, regardless of flag FAN_REPORT_PIDFD.

Quoted 8 lines

So the START_FD_READER ioctl feature could be implemented and documented first.
And then FAN_REPORT_PIDFD could use the feature with a very minor API difference:
- Without the flag, other processes can read fds by default and group initiator can opt-out
- With the flag, other processes cannot read fds by default and need to opt-in

Or maybe something even simpler... fanotify_init() flag FAN_PRIVATE (or FAN_PROTECTED) that limits event reading to the initiator process (not only fd reading).

Quoted 6 lines

FAN_REPORT_PIDFD requires FAN_PRIVATE.
If we do not know there is a use case for passing fanotify_fd that reports pidfds to another process why implement the ioctl. We can always implement it later if the need arises. If we contemplate this future change, though, maybe the name FAN_PROTECTED is better to start with.

Quoted 5 lines

Good ideas. I think we are fine with returning pidfd only to the process creating the fanotify group. Later we can add an ioctl which would indicate that the process is also prepared to have fds created in its file table. But I have still some open questions:
Do we want threads of the same process to still be able to receive fds?

I don't see why not.
They will be bloating the same fd table as the thread that called fanotify_init().

Quoted 6 lines

Quoted 2 lines

Also pids can be recycled so they are probably not completely reliable identifiers?

Not sure I follow. The group hold a refcount on struct pid of the process that called fanotify_init() - I think that can used to check if reader process is the same process, but not sure. Maybe there is another way (Christian?).

If the fanotify group hold's a reference to struct pid it won't get recycled. And it can be used to check if the reader thread is the same thread with some care. You also have to be specific what exactly you want to know. If you're asking if the reading process is the same as the fanotify_init() process you can be asking one of two things.

You can be asking if the reader is a thread in the same thread-group as the thread that called fanotify_init(). In that case you might need to do something like

rcu_read_lock();
struct task_struct *fanotify_init_task_struct = pid_task(stashed_struct_pid, PIDTYPE_PID);

if (!fanotify_init_task_struct) {
	/* The thread which called fanotify_init() has died already. */ 
	return -ESRCH;
}

if (same_thread_group(fanotify_init_task_struct, current)) rcu_read_unlock();

though thinking about it makes me realise that there's a corner case. If the thread that called fanotify_init() is a thread in a non-empty thread-group it can already have died and been reaped. This would mean, pid_task(..., PIDTYPE_PID) will return NULL but there are still other threads alive in the thread-group. Handling that case might be a bit complicated.

If you're asking whether the reading thread is really the same as the thread that created the fanotify instance then you might need to do sm like

rcu_read_lock();
if (pid_task(stashed_struct_pid, PIDTYPE_PID) == current) rcu_read_unlock();

Just for completeness if I remember all of this right: there's a corner case because of how de_thread() works.
During exec the thread that is execing will assume the struct pid of the old thread-group leader. (All other threads in the same thread-group will get killed.)
Assume the thread that created the fanotify instance is not the thread-group leader in its non-empty thread-group. And further assume it exec's. Then it will assume the struct pid of the old thread-group leader during de_thread().
Assume the thread inherits the fanotify fd across the exec. Now, when it tries to read a new event after the exec then pid_task() will return NULL.
However, if the thread was already the thread-group leader before the exec then pid_task() will return the same task struct as before after the exec (because no struct pid swapping needed to take place).

I hope this causes more clarity then confusion. :)

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Hey Jann,

Quoted 60 lines · Jann Horn

Quoted 28 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 initialization 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().

[...]

Quoted 30 lines

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

@@ -524,6 +562,34 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
        }
        metadata.fd = fd;
 
+       if (pidfd_mode) {
+               /*
+                * Complain if the FAN_REPORT_PIDFD and FAN_REPORT_TID mutual
+                * exclusion is ever lifted. At the time of incoporating pidfd
+                * support within fanotify, the pidfd API only supported the
+                * creation of pidfds for thread-group leaders.
+                */
+               WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+               /*
+                * 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. All other pidfd creation errors are represented as
+                * FAN_EPIDFD.
+                */
+               if (metadata.pid == 0 ||
+                   !pid_has_task(event->pid, PIDTYPE_TGID)) {
+                       pidfd = FAN_NOPIDFD;
+               } else {
+                       pidfd = pidfd_create(event->pid, 0);
+                       if (pidfd < 0)
+                               pidfd = FAN_EPIDFD;
+               }

Quoted 8 lines

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

Quoted 2 lines

(quoted diff) +2 -0

+       }
+

As a general rule, f_op->read callbacks aren't allowed to mess with the file descriptor table of the calling process. A process should be able to receive a file descriptor from an untrusted source and call functions like read() on it without worrying about affecting its own file descriptor table state with that.

Interesting, thanks for bringing this up. I never knew about this general rule. Do you mind elaborating a little on why f_op->read() callbacks aren't allowed to mess with the fdtable of the calling process? I don't quite exactly understand why this is considered to be suboptimal.

Hey Jan/Amir/Christian,

This is an updated version of the FAN_REPORT_PIDFD series which contains the addressed nits from the previous review [0]. As per request, you can also find the draft LTP tests here [1] and man-pages update for this new API change here [2].

[0] https://lore.kernel.org/linux-fsdevel/[email protected]/
[1] https://github.com/matthewbobrowski/ltp/commits/fanotify_pidfd_v2
[2] https://github.com/matthewbobrowski/man-pages/commits/fanotify_pidfd_v1

Quoted 8 lines · Matthew Bobrowski

Hey Jan/Amir/Christian,

This is an updated version of the FAN_REPORT_PIDFD series which contains the addressed nits from the previous review [0]. As per request, you can also find the draft LTP tests here [1] and man-pages update for this new API change here [2].

Quoted 3 lines

[0] https://lore.kernel.org/linux-fsdevel/[email protected]/
[1] https://github.com/matthewbobrowski/ltp/commits/fanotify_pidfd_v2
[2] https://github.com/matthewbobrowski/man-pages/commits/fanotify_pidfd_v1

FWIW, those test and man page drafts look good to me :)

Quoted 13 lines · Amir Goldstein

Quoted 11 lines · Matthew Bobrowski

Hey Jan/Amir/Christian,

This is an updated version of the FAN_REPORT_PIDFD series which contains the addressed nits from the previous review [0]. As per request, you can also find the draft LTP tests here [1] and man-pages update for this new API change here [2].

[0] https://lore.kernel.org/linux-fsdevel/[email protected]/
[1] https://github.com/matthewbobrowski/ltp/commits/fanotify_pidfd_v2
[2] https://github.com/matthewbobrowski/man-pages/commits/fanotify_pidfd_v1

Quoted 1 line

FWIW, those test and man page drafts look good to me :)

Fantastic, thanks for the review!

I will adjust the minor comments/documentation on patch 5/5 and send through an updated series.

Quoted 14 lines · Matthew Bobrowski

Quoted 13 lines · Amir Goldstein

Quoted 11 lines · Matthew Bobrowski

Hey Jan/Amir/Christian,

This is an updated version of the FAN_REPORT_PIDFD series which contains the addressed nits from the previous review [0]. As per request, you can also find the draft LTP tests here [1] and man-pages update for this new API change here [2].

[0] https://lore.kernel.org/linux-fsdevel/[email protected]/
[1] https://github.com/matthewbobrowski/ltp/commits/fanotify_pidfd_v2
[2] https://github.com/matthewbobrowski/man-pages/commits/fanotify_pidfd_v1

Quoted 6 lines

Quoted 1 line

FWIW, those test and man page drafts look good to me :)

Fantastic, thanks for the review!

I will adjust the minor comments/documentation on patch 5/5 and send through an updated series.

Alright, so I've fixed up the git commit message and comment in the source code so that it's more accurate in terms of when/why FAN_NOPIDFD is reported.

I'm going to hold with sending through v4 until I have Jan also look peek and poke at v3 as I want to avoid doing any unnecessary round trips.