On Thu, Jun 10, 2021 at 08:18:01AM +0300, Amir Goldstein wrote:
> On Thu, Jun 10, 2021 at 3:22 AM Matthew Bobrowski <repnop@xxxxxxxxxx> wrote:
> > @@ -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.
> >
> > 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.
> > + }
> > + }
> > +
> > 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.
> > + 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?
> > }
> >
> > 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
