Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Nov 28, 2018 at 5:27 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 25-11-18 15:43:43, Amir Goldstein wrote:
> > When user requests the flag FAN_REPORT_FID in fanotify_init(),
> > a unique file indetifier of the event target object will be reported
> > with the event.
> >
> > This commit only defines the internal and user visible structures used
> > to store and report the unique file identifier.
> >
> > The file identifier includes the filesystem's fsid (i.e. from statfs(2))
> > and an NFS file handle of the file (i.e. from name_to_handle_at(2)).
> >
> > The file identifier makes holding the path reference and passing a file
> > descriptor to user redundant, so those are disabled in a group with
> > FAN_REPORT_FID.
> >
> > Cc: <linux-api@vger.kernel.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> On a general note I'd fold patches 4-6 into one patch as separating them
> looks weird to me and does not really simplify the review (I had to jump
> among these three patches frequently to understand what's going on).
>
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index fb84dd3289f8..2e4fca30afda 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -7,6 +7,14 @@ extern struct kmem_cache *fanotify_mark_cache;
> >  extern struct kmem_cache *fanotify_event_cachep;
> >  extern struct kmem_cache *fanotify_perm_event_cachep;
> >
> > +/* The size of the variable length buffer storing fsid and file handle */
> > +#define FANOTIFY_FID_LEN(handle_bytes)       \
> > +     (sizeof(struct fanotify_event_fid) + (handle_bytes))
> > +
> > +struct fanotify_info {
> > +     struct fanotify_event_fid *fid;
> > +};
> > +
>
> Hum, lot of file handles actually fit in 24 bytes and separate allocation
> for such small things is really an overkill. And the rate at which we
> produce events can be relatively large. So I'd prefer if could embed such
> small handles inside the struct fanotify_event. Probably as a separate
> optimization patch.
>
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 2dbb2662a92f..93e1aa2a389f 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -133,9 +133,10 @@ static int fill_event_metadata(struct fsnotify_group *group,
> >       metadata->reserved = 0;
> >       metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS;
> >       metadata->pid = pid_vnr(event->pid);
> > -     if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
> > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID) ||
> > +         unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) {
> >               metadata->fd = FAN_NOFD;
> > -     else {
> > +     } else {
>
> Use FANOTIFY_HAS_FID() helper here?

Not here. FAN_REPORT_FID implies that event->fd will never be used.
But I need to check FANOTIFY_HAS_FID() before copy_fid_to_user(),
because we could fail to decode fid and still report the event without fid.

>
> > @@ -106,6 +107,24 @@ struct fanotify_event_metadata {
> >       __s32 pid;
> >  };
> >
> > +#define FAN_EVENT_INFO_TYPE_FID              1
> > +
> > +/* Variable length info record header following event metadata */
> > +struct fanotify_event_info {
> > +     __u8 info_type;
> > +     __u8 reserved;
> > +     __u16 info_len;
> > +     unsigned char info[0];
> > +};
> > +
> > +/* Unique file identifier info record */
> > +struct fanotify_event_fid {
> > +     __kernel_fsid_t fsid;
> > +     __u32 handle_bytes;
> > +     __s32 handle_type;
> > +     unsigned char f_handle[0];
> > +};
> > +
>
> Hum, I find another generic embedded fanotify_event_info an
> overengineering. fanotify_event_metadata with embedded versioning and
> length was supposed to be extensible enough without further generic headers
> being embedded... I know you had ideas for further extension of reported
> information so probably that is the reason but at least from my POV why not
> just bump the 'vers' field to 4 for groups with FAN_REPORT_FID and create
> struct fanotify_event_metadata_v4 which would have all necessary fields for
> passing the filehandle (and probably it does not have to have 'fd' field)?
>

Two reasons mainly:
1. Considering further extensions, this design looks like a better fit to me.
2. Related to #1, I don't like the way uapi gets bloated with all definition of
past format versions, so IMO format bump should be last resort

I'm perfectly fine with getting rid of fanotify_event_info record header.
I agree that it is overengineering.

How about:
struct fanotify_event_info_fid {
          struct fanotify_event_metadata hdr;
          struct fanotify_event_fid fid;
};

Then fanotify_example program from man fanotify(7) is legit even when adding
FAN_REPORT_FID to fanotify_init().

Programs that want to access fid can cast to (struct fanotify_event_info_fid *)
and access fid info.

This will make the process of adapting existing programs to FAN_REPORT_FID
smoother IMO. And the only cost we need to pay is carry event->fd = FAN_NOFD
in wasted event space.

Thoughts?

Amir.