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 7:43 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 28-11-18 18:24:11, Amir Goldstein wrote:
> > On Wed, Nov 28, 2018 at 5:27 PM Jan Kara <jack@suse.cz> wrote:
> > > > 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.
>
> OK. So maybe something like would be more obvious?
>
>         if (fanotify_event_has_path(event)) {
>                 create and store fd
>         } else if (fanotify_event_has_fid(event))
>                 store fid
>         } else {
>                 clear fd & fid
>         }

The issue is that fill_event_metadata() function fills a fixed size header
and later copy_event_to_user() copies that header to user and then
copies the variable sized fid to user, so this is not the place to "store"
fid, but I will work on readability of this code.

>
> > > > @@ -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.
>
> OK, what I'm uneasy about is the fact that the event format is defined by
> group flags and not determined within the event itself. To demonstrate
> what I mean: Group with FAN_REPORT_FID has fanotify_event_fid appended at
> the end. If we have flag FAN_REPORT_ELSE, then group with FAN_REPORT_ELSE
> would have fanotify_event_else appended at the end. Now if you have group
> with FAN_REPORT_FID | FAN_REPORT_ELSE, then what happens?
>
> So when thinking about this more I actually think your idea with some
> header is not bad. I'd just implement it like:
>
> struct fanotify_event_info_header {
>         __u8 info_type;
>         __u8 pad;
>         __u16 len;
> }
>
> and then
>
> struct fanotify_event_fid {
>         struct fanotify_event_info_header hdr;
>         __kernel_fsid_t fsid;
>         ...
> }
>
> We have to be careful with padding but it should work here since everything
> is at 32-bit granularity. Thoughts?
>

Sure. That's the easiest for me. Not that different from what I have.

Thanks,
Amir.