> > > + 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?
>
The best option, in case this is a regression (it probably is)
is the Fixes: tag which is both a clear indication for stale
candidate patch tells the bots exactly which stable kernel the
patch should be applied to.
Otherwise, you can Cc: stable (see examples in git)
and generally any commit title with the right keywords
'fix' 'regression' 'bug' should be caught but the stable AI bots.
> > > }
> > >
> > > 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.
>
IDGI, why is the init/cleanup code not as simple as
int pidfd = FAN_NOPIDFD;
...
out_close_fd:
...
if (pidfd >= 0)
put_unused_fd(fd);
What am I missing?
Thanks,
Amir.
