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

On Wed 21-04-21 13:12:23, Amir Goldstein wrote:

On Wed, Apr 21, 2021 at 1:00 PM Jan Kara [off-list ref] wrote:

quoted

On Wed 21-04-21 12:29:14, Amir Goldstein wrote:

quoted

On Wed, Apr 21, 2021 at 11:04 AM Jan Kara [off-list ref] wrote:

quoted

On Tue 20-04-21 12:36:59, Matthew Bobrowski wrote:

quoted

On Mon, Apr 19, 2021 at 05:02:33PM +0200, Christian Brauner wrote:

quoted

A general question about struct fanotify_event_metadata and its
extensibility model:
looking through the code it seems that this struct is read via
fanotify_rad(). So the user is expected to supply a buffer with at least

#define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata))

bytes. In addition you can return the info to the user about how many
bytes the kernel has written from fanotify_read().

So afaict extending fanotify_event_metadata should be _fairly_
straightforward, right? It would essentially the complement to
copy_struct_from_user() which Aleksa and I added (1 or 2 years ago)
which deals with user->kernel and you're dealing with kernel->user:
- If the user supplied a buffer smaller than the minimum known struct
  size -> reject.
- If the user supplied a buffer < smaller than what the current kernel
  supports -> copy only what userspace knows about, and return the size
  userspace knows about.
- If the user supplied a buffer that is larger than what the current
  kernel knows about -> copy only what the kernel knows about, zero the
  rest, and return the kernel size.

Extension should then be fairly straightforward (64bit aligned
increments)?

You'd think that it's fairly straightforward, but I have a feeling
that the whole fanotify_event_metadata extensibility discussion and
the current limitation to do so revolves around whether it can be
achieved in a way which can guarantee that no userspace applications
would break. I think the answer to this is that there's no guarantee
because of <<reasons>>, so the decision to extend fanotify's feature
set was done via other means i.e. introduction of additional
structures.

There's no real problem extending fanotify_event_metadata. We already have
multiple extended version of that structure in use (see e.g. FAN_REPORT_FID
flag and its effect, extended versions of the structure in
include/uapi/linux/fanotify.h). The key for backward compatibility is to
create extended struct only when explicitely requested by a flag when
creating notification group - and that would be the case here -
FAN_REPORT_PIDFD or how you called it. It is just that extending the
structure means adding 8 bytes to each event and parsing extended structure
is more cumbersome than just fetching s32 from a well known location.

On the other hand extended structure is self-describing (i.e., you can tell
the meaning of all the fields just from the event you receive) while
reusing 'pid' field means that you have to know how the notification group
was created (whether FAN_REPORT_PIDFD was used or not) to be able to
interpret the contents of the event. Actually I think the self-describing
feature of fanotify event stream is useful (e.g. when application manages
multiple fanotify groups or when fanotify group descriptors are passed
among processes) so now I'm more leaning towards using the extended
structure instead of reusing 'pid' as Christian suggests. I'm sorry for the
confusion.

But there is a middle path option.
The event metadata can be self described without extending it:

 struct fanotify_event_metadata {
        __u32 event_len;
        __u8 vers;
-       __u8 reserved;
+#define FANOTIFY_METADATA_FLAG_PIDFD   1
+       __u8 flags;
        __u16 metadata_len;
        __aligned_u64 mask;
        __s32 fd;

Well, yes, but do we want another way to describe what fanotify_event_metadata
actually contains? I don't think parsing extended event information is that
bad to make changes like this worth it...

Fine by me.
But in that case, we should report pidfd in addition to pid.

Agreed.

There is an advantage in doing that -
For the use case of filtering out events generated by self_pid
of the listener, there is no need to follow pidfd in order to check
if event->pid == self_pid.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR