Re: [PATCH v2 5/5] fanotify: add pidfd support to the fanotify API

On Thu, Jun 10, 2021 at 08:18:01AM +0300, Amir Goldstein wrote:

On Thu, Jun 10, 2021 at 3:22 AM Matthew Bobrowski [off-list ref] wrote:

quoted

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

It feels like this should be pidfd = FAN_NOPIDFD?

I had considered this, but in all honesty I wasn't sure what the behavior
is when put_unused_fd() is provided a negative value, nor whether it is
accepted. The way I saw it was that if fid info record copying had errored
out for whatever reason and we jumped to the out_close_fd label we'd also,
perhaps unnecessarily, take the pidfd clean up route, which IMO wouldn't be
required.

quoted

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

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

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

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

Ah, right, I didn't know that this was considered as an anti-pattern. But
then again, I can totally understand why it would be. No objections with
merging this comment with the one that precedes the parent if statement.

quoted

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

@@ -545,10 +610,19 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
                fd_install(fd, f);

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

This WARN_ON, if needed at all, would be better places inside if (pidfd_mode &&
code block above where you would only need to
     WARN_ON_ONCE(FAN_GROUP_FLAG(group, FAN_REPORT_TID));
as close as possible to PIDTYPE_TGID line.

Agree, there's no reason why it can't be moved to the above pidfd_mode
check.

quoted

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

This looks like a bug in upstream.

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

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

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

quoted

        }

        return metadata.event_len;

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

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

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

/M