| Message ID | 20181125134352.21499-5-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: add support for more event types | expand |
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? > @@ -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)? Honza
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.
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
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.
On Wed 28-11-18 20:34:47, Amir Goldstein wrote: > 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. Aha, I see. Thanks for your patience with me :). So then how about just folding fill_event_metadata() into copy_event_to_user() (as a preparatory patch). It is relatively small function, has a single call site and with FID changes the distinction isn't so clear anymore... Honza
> > 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. > > Aha, I see. Thanks for your patience with me :). So then how about just > folding fill_event_metadata() into copy_event_to_user() (as a preparatory > patch). It is relatively small function, has a single call site and with > FID changes the distinction isn't so clear anymore... > Sure. While you are here, before I start reworking, wanted to run by you a few minor suggestions: struct fanotify_event_info_fid { struct fanotify_event_info_header hdr; .... Seems more appropriate name than the shorter fanotify_event_fid name that you suggested. It bothers me a bit not to use struct file_handle in the definition, but I don't with to start moving struct file_handle into uapi. I am a bit lost trying to understand which uapi include file I need to include in fanotify.h if I want to use it. fcntl.h? I am going to add a separate internal struct for storing fid in event (either embedded of allocated) because I am going to make it more compact (similar to struct ovl_fh) struct fanotify_fid { __kernel_fsid_t fsid; u8 handle_bytes; u8 handle_type; u16 pad; unsigned char f_handle[0]; }; Will let you know when I have something ready. AFAICT, this rework should not affect the rest of the patches in the series (i.e. cached fsid and dirent events), so if you have time, you don't need to wait on my rework to continue review of v3. Thanks, Amir.
On Thu 29-11-18 10:16:27, Amir Goldstein wrote: > > > 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. > > > > Aha, I see. Thanks for your patience with me :). So then how about just > > folding fill_event_metadata() into copy_event_to_user() (as a preparatory > > patch). It is relatively small function, has a single call site and with > > FID changes the distinction isn't so clear anymore... > > > > Sure. While you are here, before I start reworking, wanted to run by you > a few minor suggestions: > > struct fanotify_event_info_fid { > struct fanotify_event_info_header hdr; > .... > > Seems more appropriate name than the shorter fanotify_event_fid name > that you suggested. Fine by me. > It bothers me a bit not to use struct file_handle in the definition, > but I don't with to start moving struct file_handle into uapi. > I am a bit lost trying to understand which uapi include file I need > to include in fanotify.h if I want to use it. fcntl.h? Hum, so far we got away with not defining file_handle to userspace and userspace headers don't define it either (it's just anonymouns pointer). The manpage for name_to_handle_at() and open_by_handle_at() does show it's internal structure but I'm not sure that really counts. But userspace is actually expected to fill in handle_bytes when passing handle to name_to_handle_at() so people using this syscall must have the definition somehow. Probably their private one... So I think moving the struct file_handle definition into uapi is the right thing (with the justification above). That's a cleanup on its own. I would probably move the definition into include/uapi/linux/fs.h as that's where other similar structures are defined and from kernel POV it gets included as a part of include/linux/fs.h as previously. > I am going to add a separate internal struct for storing fid in event > (either embedded of allocated) because I am going to make it more compact > (similar to struct ovl_fh) > > struct fanotify_fid { > __kernel_fsid_t fsid; > u8 handle_bytes; > u8 handle_type; > u16 pad; > unsigned char f_handle[0]; > }; > > Will let you know when I have something ready. OK. > AFAICT, this rework should not affect the rest of the patches in the > series (i.e. cached fsid > and dirent events), so if you have time, you don't need to wait on my > rework to continue > review of v3. OK, thanks for info. I'm slowly crunching through the patches but it takes time and I have also other things to do... Honza
On Thu, Nov 29, 2018 at 12:16 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 29-11-18 10:16:27, Amir Goldstein wrote: > > > > 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. > > > > > > Aha, I see. Thanks for your patience with me :). So then how about just > > > folding fill_event_metadata() into copy_event_to_user() (as a preparatory > > > patch). It is relatively small function, has a single call site and with > > > FID changes the distinction isn't so clear anymore... > > > > > > > Sure. While you are here, before I start reworking, wanted to run by you > > a few minor suggestions: > > > > struct fanotify_event_info_fid { > > struct fanotify_event_info_header hdr; > > .... > > > > Seems more appropriate name than the shorter fanotify_event_fid name > > that you suggested. > > Fine by me. > > > It bothers me a bit not to use struct file_handle in the definition, > > but I don't with to start moving struct file_handle into uapi. > > I am a bit lost trying to understand which uapi include file I need > > to include in fanotify.h if I want to use it. fcntl.h? > > Hum, so far we got away with not defining file_handle to userspace and > userspace headers don't define it either (it's just anonymouns pointer). > The manpage for name_to_handle_at() and open_by_handle_at() does show it's > internal structure but I'm not sure that really counts. But userspace is > actually expected to fill in handle_bytes when passing handle to > name_to_handle_at() so people using this syscall must have the definition > somehow. Probably their private one... > > So I think moving the struct file_handle definition into uapi is the right > thing (with the justification above). That's a cleanup on its own. I would > probably move the definition into include/uapi/linux/fs.h as that's where > other similar structures are defined and from kernel POV it gets included > as a part of include/linux/fs.h as previously. > That makes perfectly sense. I couldn't figure out what it means for uapi headers that struct file_handle is defined in /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h under ifdef __USE_GNU, but I see that SYNC_FILE_RANGE_* are also defined at the same place and they are already in include/uapi/linux/fs.h so that should be safe for struct file_handle as well. Thanks, Amir.
> > struct fanotify_event_info_fid { > > struct fanotify_event_info_header hdr; > > .... > > > > Seems more appropriate name than the shorter fanotify_event_fid name > > that you suggested. > > Fine by me. FYI, at the moment the uapi struct looks like this: struct fanotify_event_info_header { __u8 info_type; __u8 pad; __u16 len; }; struct fanotify_event_info_fid { struct fanotify_event_info_header hdr; __kernel_fsid_t fsid; struct file_handle fh; }; But for my global watch prototype [1], I also defined this struct internally: struct fanotify_event_fid { __kernel_fsid_t fsid; struct file_handle fh; }; To be used as key to hash the object in userspace. This key is often generated by open_by_handle_at() and statfs() from application and not received from fanotify. I wonder if it would be useful to include this definition in fanotify uapi headers and then: struct fanotify_event_info_fid { struct fanotify_event_info_header hdr; struct fanotify_event_fid fid; }; [1] https://github.com/amir73il/inotify-tools/commits/fanotify_fid-v4 > > > It bothers me a bit not to use struct file_handle in the definition, > > but I don't with to start moving struct file_handle into uapi. > > I am a bit lost trying to understand which uapi include file I need > > to include in fanotify.h if I want to use it. fcntl.h? > ... > > struct fanotify_fid { > > __kernel_fsid_t fsid; > > u8 handle_bytes; > > u8 handle_type; > > u16 pad; > > unsigned char f_handle[0]; > > }; > > > > Will let you know when I have something ready. I ended up putting fh_type;fh_len in a 64bit alignment gap in found in fanotify_event struct and now I use fh_type to determine if the event info is path (0), fid (>0) or none (FILEID_INVALID). I pushed the reworked fanotify_dirent [2] branch and I am quite content with the result. For comparison, also pushed fanotify_fid-v3 [3] and fanotify_fid-v4 [4], which correspond to the last patch you posted review on (cached fsid). [2] https://github.com/amir73il/inotify-tools/commits/fanotify_dirent [3] https://github.com/amir73il/inotify-tools/commits/fanotify_fid-v3 [4] https://github.com/amir73il/inotify-tools/commits/fanotify_fid-v4 > > > AFAICT, this rework should not affect the rest of the patches in the > > series (i.e. cached fsid > > and dirent events), so if you have time, you don't need to wait on my > > rework to continue > > review of v3. > > OK, thanks for info. I'm slowly crunching through the patches but it > takes time and I have also other things to do... > I'm well aware of that and thanks for taking the time to crunch this far. I hope you will like the changes in v4. The remaining 4 fanotify_dirent patches, did have some rebase conflicts over fanotify_fid-v4, but the logic remained mostly unchanged. I will post the entire v4 patch set next week, so you may continue the review when you have time. Thanks! Amir.
On Fri, Nov 30, 2018 at 5:32 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > struct fanotify_event_info_fid { > > > struct fanotify_event_info_header hdr; > > > .... > > > > > > Seems more appropriate name than the shorter fanotify_event_fid name > > > that you suggested. > > > > Fine by me. > > FYI, at the moment the uapi struct looks like this: > > struct fanotify_event_info_header { > __u8 info_type; > __u8 pad; > __u16 len; > }; > > struct fanotify_event_info_fid { > struct fanotify_event_info_header hdr; > __kernel_fsid_t fsid; > struct file_handle fh; > }; > I changed my mind about using struct file_handle. The current v4 struct is defined: struct fanotify_event_info_fid { struct fanotify_event_info_header hdr; __kernel_fsid_t fsid; /* * Following is an opaque struct file_handle that can be passed as * an argument to open_by_handle_at(2). */ unsigned char handle[0]; }; It's true that name_to_handle_at(2) users need to know about the layout of struct file_handle to pass the buffer size. But the users of fanotify FAN_REPORT_FID and the users of open_by_handle_at(2) can treat the handle as an opaque blob. > > > > > It bothers me a bit not to use struct file_handle in the definition, > > > but I don't with to start moving struct file_handle into uapi. > > > I am a bit lost trying to understand which uapi include file I need > > > to include in fanotify.h if I want to use it. fcntl.h? > > I gave up trying to move struct file_handle to uapi headers, because it is already defined in glibc fcntl-linux.h and I couldn't find a way out of this mess, nor did I find a good reason to pursue this for the current patch set. ... > > I pushed the reworked fanotify_dirent [2] branch and I am quite content > with the result. > For comparison, also pushed fanotify_fid-v3 [3] and fanotify_fid-v4 [4], > which correspond to the last patch you posted review on (cached fsid). > > [2] https://github.com/amir73il/inotify-tools/commits/fanotify_dirent > [3] https://github.com/amir73il/inotify-tools/commits/fanotify_fid-v3 > [4] https://github.com/amir73il/inotify-tools/commits/fanotify_fid-v4 > Re-pushed those branches without changes to uapi/linux/fs.h. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index ecd5f4aec624..59d093923c97 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -178,7 +178,7 @@ init: __maybe_unused event->pid = get_pid(task_pid(current)); else event->pid = get_pid(task_tgid(current)); - if (path) { + if (path && !FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { event->path = *path; path_get(&event->path); } else { 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; +}; + /* * Structure for normal fanotify events. It gets allocated in * fanotify_handle_event() and freed when the information is retrieved by @@ -14,11 +22,19 @@ extern struct kmem_cache *fanotify_perm_event_cachep; */ struct fanotify_event { struct fsnotify_event fse; - /* - * We hold ref to this path so it may be dereferenced at any point - * during this object's lifetime - */ - struct path path; + union { + /* + * We hold ref to this path so it may be dereferenced at any + * point during this object's lifetime + */ + struct path path; + /* + * With FAN_REPORT_FID, we do not hold any reference on the + * victim object. Instead we store its NFS file handle and its + * filesystem's fsid as a unique identifier. + */ + struct fanotify_info info; + }; struct pid *pid; }; 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 { metadata->fd = create_fd(group, event, file); if (metadata->fd < 0) ret = metadata->fd; diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index 909c98fcace2..0fd8736269c4 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -44,6 +44,7 @@ /* Flags to determine fanotify event format */ #define FAN_REPORT_TID 0x00000100 /* event->pid is thread id */ +#define FAN_REPORT_FID 0x00000200 /* Report unique file id */ /* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_INIT_FLAGS (FAN_CLOEXEC | FAN_NONBLOCK | \ @@ -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]; +}; + struct fanotify_response { __s32 fd; __u32 response; @@ -122,12 +141,19 @@ struct fanotify_response { /* Helper functions to deal with fanotify_event_metadata buffers */ #define FAN_EVENT_METADATA_LEN (sizeof(struct fanotify_event_metadata)) -#define FAN_EVENT_NEXT(meta, len) ((len) -= (meta)->event_len, \ - (struct fanotify_event_metadata*)(((char *)(meta)) + \ - (meta)->event_len)) +#define FAN_EVENT_NEXT(meta, len) \ + ((len) -= (meta)->event_len, \ + (struct fanotify_event_metadata *)(((char *)(meta)) + \ + (meta)->event_len)) + +#define FAN_EVENT_OK(meta, len) \ + ((long)(len) >= (long)FAN_EVENT_METADATA_LEN && \ + (long)(meta)->event_len >= (long)FAN_EVENT_METADATA_LEN && \ + (long)(meta)->event_len <= (long)(len)) -#define FAN_EVENT_OK(meta, len) ((long)(len) >= (long)FAN_EVENT_METADATA_LEN && \ - (long)(meta)->event_len >= (long)FAN_EVENT_METADATA_LEN && \ - (long)(meta)->event_len <= (long)(len)) +/* Get the first event info record if one exists */ +#define FAN_EVENT_INFO(meta) \ + ((long)(meta)->event_len > (long)FAN_EVENT_METADATA_LEN ? \ + (struct fanotify_event_info *)((meta) + 1) : NULL) #endif /* _UAPI_LINUX_FANOTIFY_H */
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> --- fs/notify/fanotify/fanotify.c | 2 +- fs/notify/fanotify/fanotify.h | 26 ++++++++++++++++---- fs/notify/fanotify/fanotify_user.c | 5 ++-- include/uapi/linux/fanotify.h | 38 +++++++++++++++++++++++++----- 4 files changed, 57 insertions(+), 14 deletions(-)