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

[Jan Kara <jack@suse.cz>]

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
	}

> > > @@ -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?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR