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

On Thu, Jun 10, 2021 at 10:11:51AM +0300, Amir Goldstein wrote:

quoted

quoted

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.

quoted

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.

Ah, OK, noted.

quoted

quoted

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.

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);

You're missing nothing, it's me that's missing a few brain cells. Sorry,
the context switching on my end is real and I had overlooked what you
meant. But yes, this will most definitely work.

/M