fanotify: Adding pidfd support to the fanotify API

With the idea to have fanotify(7) return pidfds within a struct fanotify_event_metadata, pidfd_create()'s scope is to increased so that it can be called from other subsystems within the Linux kernel. The current static qualifier from its definition is to be removed and a new function declaration for pidfd_create() is to be added to the linux/pid.h header file.

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

---
 include/linux/pid.h | 1 +
 kernel/pid.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

include/linux/pid.h +1 -0

index fa10acb8d6a4..af308e15f174 100644
@@ -78,6 +78,7 @@ struct file;
 
 extern struct pid *pidfd_pid(const struct file *file);
 struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
+int pidfd_create(struct pid *pid, unsigned int flags);
 
 static inline struct pid *get_pid(struct pid *pid)
 {

kernel/pid.c +1 -1

index ebdf9c60cd0b..91c4b6891c15 100644
@@ -553,7 +553,7 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
  * Return: On success, a cloexec pidfd is returned.
  *         On error, a negative errno number will be returned.
  */
-static int pidfd_create(struct pid *pid, unsigned int flags)
+int pidfd_create(struct pid *pid, unsigned int flags)
 {
 	int fd;
 

Signature · 3 lines

2.31.1.368.gbe11c130af-goog

/M

With the idea to have fanotify(7) return pidfds within a struct fanotify_event_metadata, pidfd_create()'s scope is to increased so that it can be called from other subsystems within the Linux kernel. The current static qualifier from its definition is to be removed and a new function declaration for pidfd_create() is to be added to the linux/pid.h header file.

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

---
 include/linux/pid.h | 1 +
 kernel/pid.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

include/linux/pid.h +1 -0

index fa10acb8d6a4..af308e15f174 100644
@@ -78,6 +78,7 @@ struct file;
 
 extern struct pid *pidfd_pid(const struct file *file);
 struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
+int pidfd_create(struct pid *pid, unsigned int flags);
 
 static inline struct pid *get_pid(struct pid *pid)
 {

kernel/pid.c +0 -0

index ebdf9c60cd0b..91c4b6891c15 100644

Quoted 34 lines · Matthew Bobrowski

With the idea to have fanotify(7) return pidfds within a struct fanotify_event_metadata, pidfd_create()'s scope is to increased so that it can be called from other subsystems within the Linux kernel. The current static qualifier from its definition is to be removed and a new function declaration for pidfd_create() is to be added to the linux/pid.h header file.

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

---
 include/linux/pid.h | 1 +
 kernel/pid.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

include/linux/pid.h +1 -0

index fa10acb8d6a4..af308e15f174 100644
@@ -78,6 +78,7 @@ struct file;
 
 extern struct pid *pidfd_pid(const struct file *file);
 struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
+int pidfd_create(struct pid *pid, unsigned int flags);
 
 static inline struct pid *get_pid(struct pid *pid)
 {

kernel/pid.c +0 -1

index ebdf9c60cd0b..91c4b6891c15 100644
@@ -553,7 +553,7 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
  * Return: On success, a cloexec pidfd is returned.
  *         On error, a negative errno number will be returned.
  */
-static int pidfd_create(struct pid *pid, unsigned int flags)

kernel/pid.c struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) · line 556

Quoted 1 line

kernel/pid.c +1 -0

+int pidfd_create(struct pid *pid, unsigned int flags)

Can you please add a comment to the kernel doc mentioning that this helper is _not_ intended to be exported to modules? I don't want drivers to get the idea that it's ok to start returning pidfds from everywhere just yet.

And I think we should add sm like

	if (flags & ~(O_NONBLOCK | O_CLOEXEC | O_RDWR))
		return -EINVAL;

in pidfd_open() so future callers don't accidently create pidfds with random flags we don't support.

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

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

If pidfd creation fails, the pid in struct fanotify_event_metadata is set to FAN_NOPIDFD(-1).

Falling back and providing a pid instead of a
pidfd on pidfd creation failures was considered, although this could possibly lead to confusion and unpredictability within userspace applications as distinguishing between whether an actual pidfd or pid was returned could be difficult, so it's best to be explicit.

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

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

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

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

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

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

        if (path && path->mnt && path->dentry) {
                fd = create_fd(group, path, &f);

@@ -941,6 +959,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 #endif
                return -EINVAL;
 
+       /*
+        * A pidfd can only be returned for a thread-group leader; thus
+        * FAN_REPORT_TID and FAN_REPORT_PIDFD need to be mutually
+        * exclusive. Once the pidfd API supports the creation of pidfds on
+        * individual threads, then we can look at removing this conditional.
+        */
+       if ((flags & FAN_REPORT_PIDFD) && (flags & FAN_REPORT_TID))
+               return -EINVAL;
+
        if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_BITS)
                return -EINVAL;
 

@@ -1312,7 +1339,7 @@ SYSCALL32_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 10);
+       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 11);
        BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
        fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,

include/linux/fanotify.h +0 -0

index 3e9c56ee651f..894740a6f4e0 100644

Quoted 12 lines · Matthew Bobrowski

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

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

Quoted 2 lines

If pidfd creation fails, the pid in struct fanotify_event_metadata is set to FAN_NOPIDFD(-1).

Hi Matthew,

All in all looks good, just a few small nits.

Quoted 5 lines

Falling back and providing a pid instead of a
pidfd on pidfd creation failures was considered, although this could possibly lead to confusion and unpredictability within userspace applications as distinguishing between whether an actual pidfd or pid was returned could be difficult, so it's best to be explicit.

I don't think this should have been even "considered" so I see little value in this paragraph in commit message.

Quoted 30 lines

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

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

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

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

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

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

Quoted 5 lines

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

+                * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+                * blindly lifted without pidfds for threads actually being
+                * supported.
+                */
+               WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));

Better WARN_ON_ONCE() the outcome of this error is not terrible. Also in the comment above I would not refer to this warning as "panic".

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

Quoted 9 lines

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

+
+               pidfd = pidfd_create(event->pid, 0);
+               if (unlikely(pidfd < 0))
+                       metadata.pid = FAN_NOPIDFD;
+               else
+                       metadata.pid = pidfd;
+       } else {
+               metadata.pid = pid_vnr(event->pid);
+       }

You should rebase your work on:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify and resolve conflicts with "unprivileged listener" code.

Need to make sure that pidfd is not reported to an unprivileged listener even if group was initialized by a privileged process. This is a conscious conservative choice that we made for reporting pid info to unprivileged listener that can be revisited in the future.

Quoted 35 lines

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

        if (path && path->mnt && path->dentry) {
                fd = create_fd(group, path, &f);

@@ -941,6 +959,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 #endif
                return -EINVAL;
 
+       /*
+        * A pidfd can only be returned for a thread-group leader; thus
+        * FAN_REPORT_TID and FAN_REPORT_PIDFD need to be mutually
+        * exclusive. Once the pidfd API supports the creation of pidfds on
+        * individual threads, then we can look at removing this conditional.
+        */
+       if ((flags & FAN_REPORT_PIDFD) && (flags & FAN_REPORT_TID))
+               return -EINVAL;
+
        if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_BITS)
                return -EINVAL;
 

@@ -1312,7 +1339,7 @@ SYSCALL32_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 10);
+       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 11);
        BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
        fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,

include/linux/fanotify.h +0 -0

index 3e9c56ee651f..894740a6f4e0 100644
@@ -21,7 +21,7 @@
 #define FANOTIFY_FID_BITS      (FAN_REPORT_FID | FAN_REPORT_DFID_NAME)
 

include/linux/fanotify.h line 27

Quoted 6 lines

include/linux/fanotify.h +1 -1

 #define FANOTIFY_INIT_FLAGS    (FANOTIFY_CLASS_BITS | FANOTIFY_FID_BITS | \
-                                FAN_REPORT_TID | \
+                                FAN_REPORT_TID | FAN_REPORT_PIDFD | \
                                 FAN_CLOEXEC | FAN_NONBLOCK | \
                                 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 

FAN_REPORT_PIDFD should be added to FANOTIFY_ADMIN_INIT_FLAGS from fsnotify branch.

Quoted 13 lines · Amir Goldstein

Quoted 12 lines · Matthew Bobrowski

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

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

Quoted 6 lines

Quoted 2 lines

If pidfd creation fails, the pid in struct fanotify_event_metadata is set to FAN_NOPIDFD(-1).

Hi Matthew,

All in all looks good, just a few small nits.

Thanks for feedback Amir! :)

Quoted 8 lines

Quoted 5 lines

Falling back and providing a pid instead of a
pidfd on pidfd creation failures was considered, although this could possibly lead to confusion and unpredictability within userspace applications as distinguishing between whether an actual pidfd or pid was returned could be difficult, so it's best to be explicit.

I don't think this should have been even "considered" so I see little value in this paragraph in commit message.

Fair point. I will discard this sentence for all subsequent iterations of this patch series. I guess the idea was that this patch series was meant to be labeled as being "RFC", so some extra thoughts had been noted.

Quoted 30 lines

Quoted 30 lines

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

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

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

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

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

Quoted 8 lines

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

Quoted 5 lines

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

+                * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+                * blindly lifted without pidfds for threads actually being
+                * supported.
+                */
+               WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));

Better WARN_ON_ONCE() the outcome of this error is not terrible. Also in the comment above I would not refer to this warning as "panic".

ACK.

Quoted 10 lines

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

Quoted 9 lines

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

+
+               pidfd = pidfd_create(event->pid, 0);
+               if (unlikely(pidfd < 0))
+                       metadata.pid = FAN_NOPIDFD;
+               else
+                       metadata.pid = pidfd;
+       } else {
+               metadata.pid = pid_vnr(event->pid);
+       }

Quoted 3 lines

You should rebase your work on:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify and resolve conflicts with "unprivileged listener" code.

ACK.

Quoted 4 lines

Need to make sure that pidfd is not reported to an unprivileged listener even if group was initialized by a privileged process. This is a conscious conservative choice that we made for reporting pid info to unprivileged listener that can be revisited in the future.

OK, I see. In that case, I guess I can add the FAN_REPORT_PIDFD check above the current conditional [0]:

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

        metadata.pid = 0;

...

That way, AFAIK even if it is an unprivileged listener the pid info will be overwritten as intended.

Quoted 35 lines

Quoted 35 lines

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

        if (path && path->mnt && path->dentry) {
                fd = create_fd(group, path, &f);

@@ -941,6 +959,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 #endif
                return -EINVAL;
 
+       /*
+        * A pidfd can only be returned for a thread-group leader; thus
+        * FAN_REPORT_TID and FAN_REPORT_PIDFD need to be mutually
+        * exclusive. Once the pidfd API supports the creation of pidfds on
+        * individual threads, then we can look at removing this conditional.
+        */
+       if ((flags & FAN_REPORT_PIDFD) && (flags & FAN_REPORT_TID))
+               return -EINVAL;
+
        if (event_f_flags & ~FANOTIFY_INIT_ALL_EVENT_F_BITS)
                return -EINVAL;
 

@@ -1312,7 +1339,7 @@ SYSCALL32_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 10);
+       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 11);
        BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
        fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,

include/linux/fanotify.h +0 -0

index 3e9c56ee651f..894740a6f4e0 100644
@@ -21,7 +21,7 @@
 #define FANOTIFY_FID_BITS      (FAN_REPORT_FID | FAN_REPORT_DFID_NAME)
 

Quoted 9 lines

include/linux/fanotify.h line 27

Quoted 6 lines

include/linux/fanotify.h +1 -1

 #define FANOTIFY_INIT_FLAGS    (FANOTIFY_CLASS_BITS | FANOTIFY_FID_BITS | \
-                                FAN_REPORT_TID | \
+                                FAN_REPORT_TID | FAN_REPORT_PIDFD | \
                                 FAN_CLOEXEC | FAN_NONBLOCK | \
                                 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 

FAN_REPORT_PIDFD should be added to FANOTIFY_ADMIN_INIT_FLAGS from fsnotify branch.

ACK.

Before sending any other version of this patch series through I will see what Jan and Christian have to say.

Quoted 28 lines · Matthew Bobrowski

Quoted 9 lines

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

Quoted 8 lines

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

+               pidfd = pidfd_create(event->pid, 0);
+               if (unlikely(pidfd < 0))
+                       metadata.pid = FAN_NOPIDFD;
+               else
+                       metadata.pid = pidfd;
+       } else {
+               metadata.pid = pid_vnr(event->pid);
+       }

Quoted 3 lines

You should rebase your work on:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify and resolve conflicts with "unprivileged listener" code.

ACK.

Quoted 4 lines

Need to make sure that pidfd is not reported to an unprivileged listener even if group was initialized by a privileged process. This is a conscious conservative choice that we made for reporting pid info to unprivileged listener that can be revisited in the future.

OK, I see. In that case, I guess I can add the FAN_REPORT_PIDFD check above the current conditional [0]:

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

        metadata.pid = 0;

...

Quoted 3 lines

That way, AFAIK even if it is an unprivileged listener the pid info will be overwritten as intended.

Situation is a bit more subtle than that.
If you override event->pid with zero and zero is interpreted as pidfd that would not be consistent with uapi documentation.

You need to make sure that event->pid is FAN_NOPIDFD in case (!capable(CAP_SYS_ADMIN) &&
FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD))
Hopefully, you can do that while keeping the special cases to minimum...

Quoted 12 lines · Matthew Bobrowski

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

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

Quoted 2 lines

If pidfd creation fails, the pid in struct fanotify_event_metadata is set to FAN_NOPIDFD(-1).

Hi Matthew,

All in all looks good, just a few small nits.

Quoted 5 lines

Falling back and providing a pid instead of a
pidfd on pidfd creation failures was considered, although this could possibly lead to confusion and unpredictability within userspace applications as distinguishing between whether an actual pidfd or pid was returned could be difficult, so it's best to be explicit.

I don't think this should have been even "considered" so I see little value in this paragraph in commit message.

Quoted 26 lines

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

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

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

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

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

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

Quoted 10 lines

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

+
+       if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+               pid_has_task(event->pid, PIDTYPE_TGID)) {
+               /*
+                * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+                * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+                * blindly lifted without pidfds for threads actually being
+                * supported.
+                */
+               WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));

Better WARN_ON_ONCE() the outcome of this error is not terrible. Also in the comment above I would not refer to this warning as "panic".

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

Quoted 9 lines

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

+
+               pidfd = pidfd_create(event->pid, 0);
+               if (unlikely(pidfd < 0))
+                       metadata.pid = FAN_NOPIDFD;
+               else
+                       metadata.pid = pidfd;
+       } else {
+               metadata.pid = pid_vnr(event->pid);
+       }

You should rebase your work on:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify and resolve conflicts with "unprivileged listener" code.

Need to make sure that pidfd is not reported to an unprivileged listener even if group was initialized by a privileged process.

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

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

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

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

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

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

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

Quoted 18 lines · Matthew Bobrowski

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

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

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

Quoted 1 line

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

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

Quoted 20 lines

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

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

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

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

Quoted 7 lines

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

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

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

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

BTW, why is the pid_has_task() check here?

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

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

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

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

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

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

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

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

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata.pid = pid_vnr(event->pid);
+
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+		pid_has_task(event->pid, PIDTYPE_TGID)) {
+		/*
+		 * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+		 * FAN_REPORT_TID, panic here if the mutual exclusion is ever

Quoted 57 lines · Matthew Bobrowski

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

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

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

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

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

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

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

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
      metadata.vers = FANOTIFY_METADATA_VERSION;
      metadata.reserved = 0;
      metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-     metadata.pid = pid_vnr(event->pid);
+
+     if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+             pid_has_task(event->pid, PIDTYPE_TGID)) {
+             /*
+              * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+              * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+              * blindly lifted without pidfds for threads actually being
+              * supported.
+              */
+             WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+             pidfd = pidfd_create(event->pid, 0);
+             if (unlikely(pidfd < 0))
+                     metadata.pid = FAN_NOPIDFD;

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

Quoted 2 lines

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

+             else
+                     metadata.pid = pidfd;

I'm not a fan of overloading fields (Yes, we did this for the _legacy_ clone() syscall for CLONE_PIDFD/CLONE_PARENT_SETTID but in general it's never a good idea if there are other options, imho.). Could/should we consider the possibility of adding a new pidfd field to struct fanotify_event_metadata?

Quoted 59 lines · Christian Brauner

Quoted 57 lines · Matthew Bobrowski

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

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

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

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

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

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

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

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
      metadata.vers = FANOTIFY_METADATA_VERSION;
      metadata.reserved = 0;
      metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-     metadata.pid = pid_vnr(event->pid);
+
+     if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+             pid_has_task(event->pid, PIDTYPE_TGID)) {
+             /*
+              * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+              * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+              * blindly lifted without pidfds for threads actually being
+              * supported.
+              */
+             WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+             pidfd = pidfd_create(event->pid, 0);
+             if (unlikely(pidfd < 0))
+                     metadata.pid = FAN_NOPIDFD;

Quoted 8 lines

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

Quoted 2 lines

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

+             else
+                     metadata.pid = pidfd;

I'm not a fan of overloading fields (Yes, we did this for the _legacy_ clone() syscall for CLONE_PIDFD/CLONE_PARENT_SETTID but in general it's never a good idea if there are other options, imho.). Could/should we consider the possibility of adding a new pidfd field to struct fanotify_event_metadata?

struct fanotify_event_metadata is fully booked.
We could use a variable length record, but IMO that's an overkill for pidfd.

If you are concerned about users wrongly interpreting pidfd as pid and getting the the wrong process we could use invalid negative values for pidfd, e.g. metadata.pid = ~pidfd.
This has the quality that 0 can mean both FAN_NOPIDFD and no pid. UAPI to this would be abstracted with
#define FAN_EVENT_PIDFD(event) (~((event)->pid))

I am not convinced that this helps more than it hurts users, but abstracting the accessor to pidfd could be a good idea anyway.

Quoted 57 lines · Matthew Bobrowski

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

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

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

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

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

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

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

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata.pid = pid_vnr(event->pid);
+
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+		pid_has_task(event->pid, PIDTYPE_TGID)) {
+		/*
+		 * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+		 * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+		 * blindly lifted without pidfds for threads actually being
+		 * supported.
+		 */
+		WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+		pidfd = pidfd_create(event->pid, 0);
+		if (unlikely(pidfd < 0))
+			metadata.pid = FAN_NOPIDFD;

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

Quoted 2 lines

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

+		else
+			metadata.pid = pidfd;

I'm not a fan of overloading fields (Yes, we did this for the _legacy_ clone() syscall for CLONE_PIDFD/CLONE_PARENT_SETTID but in general it's never a good idea if there are other options, imho.). Could/should we consider the possibility of adding a new pidfd field to struct fanotify_event_metadata?

Quoted 58 lines · Christian Brauner

Quoted 57 lines · Matthew Bobrowski

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

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

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

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

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

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

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

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata.pid = pid_vnr(event->pid);
+
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+		pid_has_task(event->pid, PIDTYPE_TGID)) {
+		/*
+		 * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+		 * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+		 * blindly lifted without pidfds for threads actually being
+		 * supported.
+		 */
+		WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+		pidfd = pidfd_create(event->pid, 0);
+		if (unlikely(pidfd < 0))
+			metadata.pid = FAN_NOPIDFD;

Quoted 8 lines

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

Quoted 2 lines

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

+		else
+			metadata.pid = pidfd;

I'm not a fan of overloading fields (Yes, we did this for the _legacy_ clone() syscall for CLONE_PIDFD/CLONE_PARENT_SETTID but in general it's never a good idea if there are other options, imho.). Could/should we consider the possibility of adding a new pidfd field to struct fanotify_event_metadata?

I'm not a huge fan of overloading fields either but in this particular case I'm fine with that because:

a) storage size & type matches
b) it describes exactly the same information, just in a different way

It is not possible to store the pidfd elsewhere in fanotify_event_metadata. But it is certainly possible to use extended event info to return pidfd instead - similarly to how we return e.g. handle + fsid for some notification groups. It just means somewhat longer events and more complicated parsing of structured events in userspace. But as I write above, in this case I don't think it is worth it - only if we think that returning both pid and pidfd could ever be useful.

Quoted 74 lines · Jan Kara

Quoted 58 lines · Christian Brauner

Quoted 57 lines · Matthew Bobrowski

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

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

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

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

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

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

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

@@ -340,7 +340,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata.pid = pid_vnr(event->pid);
+
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_PIDFD) &&
+		pid_has_task(event->pid, PIDTYPE_TGID)) {
+		/*
+		 * Given FAN_REPORT_PIDFD is to be mutually exclusive with
+		 * FAN_REPORT_TID, panic here if the mutual exclusion is ever
+		 * blindly lifted without pidfds for threads actually being
+		 * supported.
+		 */
+		WARN_ON(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
+
+		pidfd = pidfd_create(event->pid, 0);
+		if (unlikely(pidfd < 0))
+			metadata.pid = FAN_NOPIDFD;

Quoted 8 lines

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

Quoted 2 lines

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

+		else
+			metadata.pid = pidfd;

I'm not a fan of overloading fields (Yes, we did this for the _legacy_ clone() syscall for CLONE_PIDFD/CLONE_PARENT_SETTID but in general it's never a good idea if there are other options, imho.). Could/should we consider the possibility of adding a new pidfd field to struct fanotify_event_metadata?

I'm not a huge fan of overloading fields either but in this particular case I'm fine with that because:

a) storage size & type matches
b) it describes exactly the same information, just in a different way

Quoted 7 lines

It is not possible to store the pidfd elsewhere in fanotify_event_metadata. But it is certainly possible to use extended event info to return pidfd instead - similarly to how we return e.g. handle + fsid for some notification groups. It just means somewhat longer events and more complicated parsing of structured events in userspace. But as I write above, in this case I don't think it is worth it - only if we think that returning both pid and pidfd could ever be useful.

Yeah, I don't hink users need to do that. After all they can parse the PID out of the /proc/self/fdinfo/<pidfd> file.

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

Quoted 48 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

Quoted 11 lines

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

But there is a middle path option.
The event metadata can be self described without extending it:

 struct fanotify_event_metadata {
        __u32 event_len;
        __u8 vers;

Quoted 67 lines · Amir Goldstein

Quoted 48 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

Quoted 11 lines

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

But there is a middle path option.
The event metadata can be self described without extending it:

 struct fanotify_event_metadata {
        __u32 event_len;
        __u8 vers;

Review

Quoted 6 lines

(quoted diff) +2 -1

-       __u8 reserved;
+#define FANOTIFY_METADATA_FLAG_PIDFD   1
+       __u8 flags;
        __u16 metadata_len;
        __aligned_u64 mask;
        __s32 fd;

Well, yes, but do we want another way to describe what fanotify_event_metadata actually contains? I don't think parsing extended event information is that bad to make changes like this worth it...

Quoted 76 lines · Jan Kara

Quoted 73 lines · Amir Goldstein

Quoted 48 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

Quoted 11 lines

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

But there is a middle path option.
The event metadata can be self described without extending it:

 struct fanotify_event_metadata {
        __u32 event_len;
        __u8 vers;

(quoted diff) +2 -1

-       __u8 reserved;
+#define FANOTIFY_METADATA_FLAG_PIDFD   1
+       __u8 flags;
        __u16 metadata_len;
        __aligned_u64 mask;
        __s32 fd;

Quoted 3 lines

Well, yes, but do we want another way to describe what fanotify_event_metadata actually contains? I don't think parsing extended event information is that bad to make changes like this worth it...

Fine by me.
But in that case, we should report pidfd in addition to pid.

There is an advantage in doing that -
For the use case of filtering out events generated by self_pid of the listener, there is no need to follow pidfd in order to check if event->pid == self_pid.

Quoted 77 lines · Amir Goldstein

Quoted 76 lines · Jan Kara

Quoted 73 lines · Amir Goldstein

Quoted 48 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

Quoted 11 lines

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

But there is a middle path option.
The event metadata can be self described without extending it:

 struct fanotify_event_metadata {
        __u32 event_len;
        __u8 vers;

(quoted diff) +2 -1

-       __u8 reserved;
+#define FANOTIFY_METADATA_FLAG_PIDFD   1
+       __u8 flags;
        __u16 metadata_len;
        __aligned_u64 mask;
        __s32 fd;

Quoted 6 lines

Quoted 3 lines

Well, yes, but do we want another way to describe what fanotify_event_metadata actually contains? I don't think parsing extended event information is that bad to make changes like this worth it...

Fine by me.
But in that case, we should report pidfd in addition to pid.

Agreed.

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

Quoted 47 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

Quoted 10 lines

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

This approach makes sense to me.

Jan/Amir, just to be clear, we've agreed to go ahead with the extended struct approach whereby specifying the FAN_REPORT_PIDFD flag will result in an event which includes an additional struct (i.e. fanotify_event_info_pid) alongside the generic existing

fanotify_event_metadata (also ensuring that pid has been provided). Events will be provided to userspace applications just like when specifying FAN_REPORT_FID, correct?

Quoted 60 lines · Matthew Bobrowski

Quoted 57 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

Quoted 6 lines

This approach makes sense to me.

Jan/Amir, just to be clear, we've agreed to go ahead with the extended struct approach whereby specifying the FAN_REPORT_PIDFD flag will result in an event which includes an additional struct (i.e. fanotify_event_info_pid) alongside the generic existing

struct fanotify_event_info_pidfd?

Quoted 61 lines · Amir Goldstein

Quoted 60 lines · Matthew Bobrowski

Quoted 57 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

Quoted 8 lines

Quoted 6 lines

This approach makes sense to me.

Jan/Amir, just to be clear, we've agreed to go ahead with the extended struct approach whereby specifying the FAN_REPORT_PIDFD flag will result in an event which includes an additional struct (i.e. fanotify_event_info_pid) alongside the generic existing

struct fanotify_event_info_pidfd?

Well, yeah? I mean, my line of thought was that we'd also need to include struct fanotify_event_info_header alongside the event to provide more meta-information about the additional event you'd expect to receive when FAN_REPORT_PIDFD is provided, so we'd end up with something like:

struct fanotify_event_info_pidfd {
       struct fanotify_event_info_header hdr;
       __s32 pidfd;
}

Unless this of course is overbaking it and there's no need to do this?

Quoted 83 lines · Matthew Bobrowski

Quoted 61 lines · Amir Goldstein

Quoted 60 lines · Matthew Bobrowski

Quoted 57 lines · Jan Kara

Quoted 27 lines · Matthew Bobrowski

Quoted 23 lines · Christian Brauner

A general question about struct fanotify_event_metadata and its extensibility model:
looking through the code it seems that this struct is read via fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_ straightforward, right? It would essentially the complement to copy_struct_from_user() which Aleksa and I added (1 or 2 years ago) which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct size -> reject.
- If the user supplied a buffer < smaller than what the current kernel supports -> copy only what userspace knows about, and return the size userspace knows about.
- If the user supplied a buffer that is larger than what the current kernel knows about -> copy only what the kernel knows about, zero the rest, and return the kernel size.

Quoted 2 lines

Extension should then be fairly straightforward (64bit aligned increments)?

Quoted 8 lines

You'd think that it's fairly straightforward, but I have a feeling that the whole fanotify_event_metadata extensibility discussion and the current limitation to do so revolves around whether it can be achieved in a way which can guarantee that no userspace applications would break. I think the answer to this is that there's no guarantee because of <<reasons>>, so the decision to extend fanotify's feature set was done via other means i.e. introduction of additional structures.

There's no real problem extending fanotify_event_metadata. We already have multiple extended version of that structure in use (see e.g. FAN_REPORT_FID flag and its effect, extended versions of the structure in include/uapi/linux/fanotify.h). The key for backward compatibility is to create extended struct only when explicitely requested by a flag when creating notification group - and that would be the case here - FAN_REPORT_PIDFD or how you called it. It is just that extending the structure means adding 8 bytes to each event and parsing extended structure is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell the meaning of all the fields just from the event you receive) while reusing 'pid' field means that you have to know how the notification group was created (whether FAN_REPORT_PIDFD was used or not) to be able to interpret the contents of the event. Actually I think the self-describing feature of fanotify event stream is useful (e.g. when application manages multiple fanotify groups or when fanotify group descriptors are passed among processes) so now I'm more leaning towards using the extended structure instead of reusing 'pid' as Christian suggests. I'm sorry for the confusion.

Quoted 8 lines

Quoted 6 lines

This approach makes sense to me.

Jan/Amir, just to be clear, we've agreed to go ahead with the extended struct approach whereby specifying the FAN_REPORT_PIDFD flag will result in an event which includes an additional struct (i.e. fanotify_event_info_pid) alongside the generic existing

struct fanotify_event_info_pidfd?

Well, yeah? I mean, my line of thought was that we'd also need to include struct fanotify_event_info_header alongside the event to provide more meta-information about the additional event you'd expect to receive when FAN_REPORT_PIDFD is provided, so we'd end up with something like:

struct fanotify_event_info_pidfd {
       struct fanotify_event_info_header hdr;
       __s32 pidfd;
}

Quoted 2 lines

Unless this of course is overbaking it and there's no need to do this?

We need this. I was just pointing out that you wrote fanotify_event_info_pid must have been a typo.

Amir, I was just thinking about this a little over the weekend and I don't think we discussed how to handle the FAN_REPORT_PIDFD | FAN_REPORT_FID and friends case? My immediate thought is to make FAN_REPORT_PIDFD mutually exclusive with FAN_REPORT_FID and friends, but then again receiving a pidfd along with FID events may be also useful for some? What are your thoughts on this? If we don't go ahead with mutual exclusion, then this multiple event types alongside struct fanotify_event_metadata starts getting a little clunky, don't you think?

Quoted 10 lines

Amir, I was just thinking about this a little over the weekend and I don't think we discussed how to handle the FAN_REPORT_PIDFD | FAN_REPORT_FID and friends case? My immediate thought is to make FAN_REPORT_PIDFD mutually exclusive with FAN_REPORT_FID and friends, but then again receiving a pidfd along with FID events may be also useful for some? What are your thoughts on this? If we don't go ahead with mutual exclusion, then this multiple event types alongside struct fanotify_event_metadata starts getting a little clunky, don't you think?

The current format of an fanotify event already supports multiple info records:

[fanotify_event_metadata] [[fanotify_event_info_header][event record #1]] [[fanotify_event_info_header][event record #2]]...

(meta)->event_len is the total event length including all info records.

For example, FAN_REPORT_FID | FAN_REPORT_DFID_MAME produces (for some events) two info records, one FAN_EVENT_INFO_TYPE_FID record and one FAN_EVENT_INFO_TYPE_DFID_NAME record.

So I see no problem with combination of FAN_REPORT_FID and FAN_REPORT_PIDFD.

Quoted 17 lines · Amir Goldstein

Quoted 10 lines

Amir, I was just thinking about this a little over the weekend and I don't think we discussed how to handle the FAN_REPORT_PIDFD | FAN_REPORT_FID and friends case? My immediate thought is to make FAN_REPORT_PIDFD mutually exclusive with FAN_REPORT_FID and friends, but then again receiving a pidfd along with FID events may be also useful for some? What are your thoughts on this? If we don't go ahead with mutual exclusion, then this multiple event types alongside struct fanotify_event_metadata starts getting a little clunky, don't you think?

The current format of an fanotify event already supports multiple info records:

[fanotify_event_metadata] [[fanotify_event_info_header][event record #1]] [[fanotify_event_info_header][event record #2]]...

Quoted 5 lines

(meta)->event_len is the total event length including all info records.

For example, FAN_REPORT_FID | FAN_REPORT_DFID_MAME produces (for some events) two info records, one FAN_EVENT_INFO_TYPE_FID record and one FAN_EVENT_INFO_TYPE_DFID_NAME record.

Ah, that's right! I now remember reviewing some patches associated with the FID change series which mentioned the possibility of receiving multiple FID info records. As the implementation currently stands, AFAIK there's not possibility for fanotify to ever return more than two info records, right?

Quoted 2 lines

So I see no problem with combination of FAN_REPORT_FID and FAN_REPORT_PIDFD.

OK.

Is there any preference in terms of whether the new FAN_REPORT_PIDFD info records precede or come after FAN_REPORT_FID/FAN_REPORT_DFID_NAME info records in FAN_REPORT_FID or FAN_REPORT_FID | FAN_REPORT_DFID_NAME configurations?

Quoted 31 lines · Matthew Bobrowski

Quoted 17 lines · Amir Goldstein

Quoted 10 lines

Amir, I was just thinking about this a little over the weekend and I don't think we discussed how to handle the FAN_REPORT_PIDFD | FAN_REPORT_FID and friends case? My immediate thought is to make FAN_REPORT_PIDFD mutually exclusive with FAN_REPORT_FID and friends, but then again receiving a pidfd along with FID events may be also useful for some? What are your thoughts on this? If we don't go ahead with mutual exclusion, then this multiple event types alongside struct fanotify_event_metadata starts getting a little clunky, don't you think?

The current format of an fanotify event already supports multiple info records:

[fanotify_event_metadata] [[fanotify_event_info_header][event record #1]] [[fanotify_event_info_header][event record #2]]...

Quoted 5 lines

(meta)->event_len is the total event length including all info records.

For example, FAN_REPORT_FID | FAN_REPORT_DFID_MAME produces (for some events) two info records, one FAN_EVENT_INFO_TYPE_FID record and one FAN_EVENT_INFO_TYPE_DFID_NAME record.

Ah, that's right! I now remember reviewing some patches associated with the FID change series which mentioned the possibility of receiving multiple FID info records. As the implementation currently stands, AFAIK there's not possibility for fanotify to ever return more than two info records, right?

Quoted 4 lines

Is there any preference in terms of whether the new FAN_REPORT_PIDFD info records precede or come after FAN_REPORT_FID/FAN_REPORT_DFID_NAME info records in FAN_REPORT_FID or FAN_REPORT_FID | FAN_REPORT_DFID_NAME configurations?

Doesn't matter.

Your typical application would first filter by pid/pidfd and only if process matches the filters would it care to examine the event fid info, correct?

Hey Jan/Amir/Christian,

This is the RFC patch series for adding pidfd support to the fanotify API.

tl;dr rather than returning a pid within struct fanotify_event_metadata, a pidfd is returned instead when fanotify has been explicitly told to do so via the new FAN_REPORT_PIDFD flag.

The main driver behind returning a pidfd within an event instead of a pid is that it permits userspace applications to better detect if they've possibly lost a TOCTOU race. A common paradigm among userspace applications that make use of the fanotify API is to crawl /proc/<pid> upon receiving an event. Userspace applications do this in order to further ascertain contextual meta-information about the process that was responsible for generating the filesystem event. On high pressure systems, where pid recycling isn't uncommon, it's no longer considered as a reliable approach to directly sift through /proc/<pid> and have userspace applications use the information contained within /proc/<pid> as it could, and does, lead to program execution inaccuracies.

Now when a pidfd is returned in an event, a userspace application can:

a) Obtain the pid responsible for generating the filesystem event from the pidfds fdinfo.

b) Detect whether the userspace application lost the procfs access race by sending a 0 signal on the pidfd and checking the return value. A -ESRCH is indicative of the userspace application losing the race, meaning that the pid has been recycled.