Re: [PATCH 0/5] Add pidfd support to the fanotify API

[Matthew Bobrowski <repnop@google.com>]

Hey Jan!

On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote:
> On Thu 20-05-21 12:09:45, Matthew Bobrowski wrote:
> > Hey Jan/Amir/Christian,
> > 
> > This is the updated patch series for adding pidfd support to the
> > fanotify API. It incorporates all the suggestions that had come out of
> > the initial RFC patch series [0].
> > 
> > The main difference with this patch series is that FAN_REPORT_PIDFD
> > results in an additional info record object supplied alongside the
> > generic event metadata object instead of overloading metadata->pid. If
> > any of the fid flavoured init flags are specified, then the pidfd info
> > record object will follow any fid info record objects.
> > 
> > [0] https://www.spinics.net/lists/linux-fsdevel/msg193296.html
> 
> Overall the series looks fine to me - modulo the problems Christian & Amir
> found. Do you have any tests for this? Preferably for LTP so that we can
> extend the coverage there?

Cool and thanks for glancing over this series.

I've written some simple programs to verify this functionality works in
FID and non-FID modes. I definitely plan on writing LTP tests,
although it's something I'll do once we've agreed on the approach and
I've received an ACK from yourself, Amir and Christian. This series
passes all current LTP regressions. Also, I guess I'll need to write
some patches for man-pages given this is an ABI change.

There's one thing that I'd like to mention, and it's something in
regards to the overall approach we've taken that I'm not particularly
happy about and I'd like to hear all your thoughts. Basically, with
this approach the pidfd creation is done only once an event has been
queued and the notification worker wakes up and picks up the event
from the queue processes it. There's a subtle latency introduced when
taking such an approach which at times leads to pidfd creation
failures. As in, by the time pidfd_create() is called the struct pid
has already been reaped, which then results in FAN_NOPIDFD being
returned in the pidfd info record.

Having said that, I'm wondering what the thoughts are on doing pidfd
creation earlier on i.e. in the event allocation stages? This way, the
struct pid is pinned earlier on and rather than FAN_NOPIDFD being
returned in the pidfd info record because the struct pid has been
already reaped, userspace application will atleast receive a valid
pidfd which can be used to check whether the process still exists or
not. I think it'll just set the expectation better from an API
perspective.

/M