| Message ID | 20181202113826.32133-8-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: add support for more event types | expand |
On Sun 02-12-18 13:38:18, Amir Goldstein wrote: > If group requested FAN_REPORT_FID and event has file identifier, > copy that information to user reading the event after event metadata. > > fid information is formatted as struct fanotify_event_info_fid > that includes a generic header struct fanotify_event_info_header, > so that other info types could be defined in the future using the > same header. > > metadata->event_len includes the length of the fid information. > > The fid information includes the filesystem's fsid (see statfs(2)) > followed by an NFS file handle of the file that could be passed as > an argument to open_by_handle_at(2). > > Cc: <linux-api@vger.kernel.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> The patch looks mostly good, some smaller comments below. > --- > fs/notify/fanotify/fanotify.h | 5 ++ > fs/notify/fanotify/fanotify_user.c | 85 ++++++++++++++++++++++++++++-- > include/uapi/linux/fanotify.h | 20 +++++++ > 3 files changed, 105 insertions(+), 5 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index cddebea5ff67..265fbaa2d5b7 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -99,6 +99,11 @@ static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event) > !fanotify_inline_fh_len(event->fh_len); > } > > +static inline void *fanotify_event_fh(struct fanotify_event *event) > +{ > + return fanotify_fid_fh(&event->fid, event->fh_len); > +} > + > /* > * Structure for permission fanotify events. It gets allocated and freed in > * fanotify_handle_event() since we wait there for user response. When the > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 9b9e6704f9e7..032a9a225a21 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -47,6 +47,22 @@ struct kmem_cache *fanotify_mark_cache __read_mostly; > struct kmem_cache *fanotify_event_cachep __read_mostly; > struct kmem_cache *fanotify_perm_event_cachep __read_mostly; > > +static int fanotify_event_info_len(struct fanotify_event *event) > +{ > + if (!fanotify_event_has_fid(event)) > + return 0; > + > + return sizeof(struct fanotify_event_info_fid) + > + sizeof(struct file_handle) + event->fh_len; > +} > + > +#define FANOTIFY_EVENT_ALIGN (sizeof(void *)) Please no. This will be different for 32-bit vs 64-bit and that is just asking for trouble with applications that start depending on particular alignment in weird ways. Just make this 4 and be done with it. The info header is 4 bytes so any additional info will get 4-byte alignment at best anyway. > + > +static int fanotify_round_event_info_len(struct fanotify_event *event) > +{ > + return roundup(fanotify_event_info_len(event), FANOTIFY_EVENT_ALIGN); > +} > + > /* > * Get an fsnotify notification event if one exists and is small > * enough to fit in "count". Return an error pointer if the count > @@ -57,6 +73,9 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly; > static struct fsnotify_event *get_one_event(struct fsnotify_group *group, > size_t count) > { > + size_t event_size = FAN_EVENT_METADATA_LEN; > + struct fanotify_event *event; > + > assert_spin_locked(&group->notification_lock); > > pr_debug("%s: group=%p count=%zd\n", __func__, group, count); > @@ -64,11 +83,18 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group, > if (fsnotify_notify_queue_is_empty(group)) > return NULL; > > - if (FAN_EVENT_METADATA_LEN > count) > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > + event = FANOTIFY_E(fsnotify_peek_first_event(group)); > + event_size += fanotify_round_event_info_len(event); > + } > + > + if (event_size > count) > return ERR_PTR(-EINVAL); > > - /* held the notification_lock the whole time, so this is the > - * same event we peeked above */ > + /* > + * Held the notification_lock the whole time, so this is the > + * same event we peeked above > + */ > return fsnotify_remove_first_event(group); > } > > @@ -174,6 +200,47 @@ static int process_access_response(struct fsnotify_group *group, > return 0; > } > > +static int copy_fid_to_user(struct fanotify_event *event, char __user *buf) > +{ > + struct fanotify_event_info_fid info; > + struct file_handle handle; > + size_t fh_len = event->fh_len; > + size_t info_len = fanotify_event_info_len(event); > + size_t len = roundup(info_len, FANOTIFY_EVENT_ALIGN); > + > + if (!info_len) > + return 0; > + > + /* Copy event info fid header followed by vaiable sized file handle */ > + info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID; > + info.hdr.len = info_len; I'm somewhat undecided whether the info_len should include the padding or not. Including it would make code somewhat simpler (in particular userspace would not have to be aware of FANOTIFY_EVENT_ALIGN - which you currently don't propagate BTW), also fanotify_event_info_len() could just include the padding and thus reduce the possibility we forget about it in some place. OTOH not including it could save us from having to specify length of some variable length array in some future event info type (e.g. here we would not have to pass handle length if we didn't want to pass opaque file_handle). Currently I'm leaning more towards including it, what are your thoughts on this? > + info.fsid = event->fid.fsid; > + info_len = sizeof(info) + sizeof(struct file_handle); The value set here does not appear to be used anywhere? > + if (copy_to_user(buf, &info, sizeof(info))) > + return -EFAULT; You can leak 1 byte of uninitialized stack contents to userspace here (info.hdr.pad). I'd suggest you use empty initializers for info and handle to make sure we cannot leak anything and the compiler will hopefully eliminate the unnecessary zeroing. > + > + buf += sizeof(info); > + len -= sizeof(info); > + handle.handle_type = event->fh_type; > + handle.handle_bytes = event->fh_len; Use local variable fh_len here? > + if (copy_to_user(buf, &handle, sizeof(handle))) > + return -EFAULT; > + > + buf += sizeof(handle); > + len -= sizeof(handle); > + if (copy_to_user(buf, fanotify_event_fh(event), fh_len)) > + return -EFAULT; > + > + /* Pad with 0's */ > + buf += fh_len; > + len -= fh_len; > + WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > + if (len > 0 && clear_user(buf, len)) > + return -EFAULT; > + > + return 0; > +} > + Honza
On Thu, Jan 3, 2019 at 7:13 PM Jan Kara <jack@suse.cz> wrote: > > On Sun 02-12-18 13:38:18, Amir Goldstein wrote: > > If group requested FAN_REPORT_FID and event has file identifier, > > copy that information to user reading the event after event metadata. > > > > fid information is formatted as struct fanotify_event_info_fid > > that includes a generic header struct fanotify_event_info_header, > > so that other info types could be defined in the future using the > > same header. > > > > metadata->event_len includes the length of the fid information. > > > > The fid information includes the filesystem's fsid (see statfs(2)) > > followed by an NFS file handle of the file that could be passed as > > an argument to open_by_handle_at(2). > > > > Cc: <linux-api@vger.kernel.org> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > The patch looks mostly good, some smaller comments below. > > > --- > > fs/notify/fanotify/fanotify.h | 5 ++ > > fs/notify/fanotify/fanotify_user.c | 85 ++++++++++++++++++++++++++++-- > > include/uapi/linux/fanotify.h | 20 +++++++ > > 3 files changed, 105 insertions(+), 5 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > > index cddebea5ff67..265fbaa2d5b7 100644 > > --- a/fs/notify/fanotify/fanotify.h > > +++ b/fs/notify/fanotify/fanotify.h > > @@ -99,6 +99,11 @@ static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event) > > !fanotify_inline_fh_len(event->fh_len); > > } > > > > +static inline void *fanotify_event_fh(struct fanotify_event *event) > > +{ > > + return fanotify_fid_fh(&event->fid, event->fh_len); > > +} > > + > > /* > > * Structure for permission fanotify events. It gets allocated and freed in > > * fanotify_handle_event() since we wait there for user response. When the > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 9b9e6704f9e7..032a9a225a21 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -47,6 +47,22 @@ struct kmem_cache *fanotify_mark_cache __read_mostly; > > struct kmem_cache *fanotify_event_cachep __read_mostly; > > struct kmem_cache *fanotify_perm_event_cachep __read_mostly; > > > > +static int fanotify_event_info_len(struct fanotify_event *event) > > +{ > > + if (!fanotify_event_has_fid(event)) > > + return 0; > > + > > + return sizeof(struct fanotify_event_info_fid) + > > + sizeof(struct file_handle) + event->fh_len; > > +} > > + > > +#define FANOTIFY_EVENT_ALIGN (sizeof(void *)) > > Please no. This will be different for 32-bit vs 64-bit and that is just > asking for trouble with applications that start depending on particular > alignment in weird ways. Just make this 4 and be done with it. The info > header is 4 bytes so any additional info will get 4-byte alignment at best > anyway. > ok. > > + > > +static int fanotify_round_event_info_len(struct fanotify_event *event) > > +{ > > + return roundup(fanotify_event_info_len(event), FANOTIFY_EVENT_ALIGN); > > +} > > + > > /* > > * Get an fsnotify notification event if one exists and is small > > * enough to fit in "count". Return an error pointer if the count > > @@ -57,6 +73,9 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly; > > static struct fsnotify_event *get_one_event(struct fsnotify_group *group, > > size_t count) > > { > > + size_t event_size = FAN_EVENT_METADATA_LEN; > > + struct fanotify_event *event; > > + > > assert_spin_locked(&group->notification_lock); > > > > pr_debug("%s: group=%p count=%zd\n", __func__, group, count); > > @@ -64,11 +83,18 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group, > > if (fsnotify_notify_queue_is_empty(group)) > > return NULL; > > > > - if (FAN_EVENT_METADATA_LEN > count) > > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > > + event = FANOTIFY_E(fsnotify_peek_first_event(group)); > > + event_size += fanotify_round_event_info_len(event); > > + } > > + > > + if (event_size > count) > > return ERR_PTR(-EINVAL); > > > > - /* held the notification_lock the whole time, so this is the > > - * same event we peeked above */ > > + /* > > + * Held the notification_lock the whole time, so this is the > > + * same event we peeked above > > + */ > > return fsnotify_remove_first_event(group); > > } > > > > @@ -174,6 +200,47 @@ static int process_access_response(struct fsnotify_group *group, > > return 0; > > } > > > > +static int copy_fid_to_user(struct fanotify_event *event, char __user *buf) > > +{ > > + struct fanotify_event_info_fid info; > > + struct file_handle handle; > > + size_t fh_len = event->fh_len; > > + size_t info_len = fanotify_event_info_len(event); > > + size_t len = roundup(info_len, FANOTIFY_EVENT_ALIGN); > > + > > + if (!info_len) > > + return 0; > > + > > + /* Copy event info fid header followed by vaiable sized file handle */ > > + info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID; > > + info.hdr.len = info_len; > > I'm somewhat undecided whether the info_len should include the padding or > not. Including it would make code somewhat simpler (in particular userspace > would not have to be aware of FANOTIFY_EVENT_ALIGN - which you currently > don't propagate BTW), also fanotify_event_info_len() could just include the > padding and thus reduce the possibility we forget about it in some place. > OTOH not including it could save us from having to specify length of some > variable length array in some future event info type (e.g. here we would > not have to pass handle length if we didn't want to pass opaque > file_handle). > > Currently I'm leaning more towards including it, what are your thoughts on > this? > Arguments in favor of include padding in len out number and out weight the alternative, so I'll go with that. > > + info.fsid = event->fid.fsid; > > + info_len = sizeof(info) + sizeof(struct file_handle); > > The value set here does not appear to be used anywhere? > > > + if (copy_to_user(buf, &info, sizeof(info))) > > + return -EFAULT; > > You can leak 1 byte of uninitialized stack contents to userspace here > (info.hdr.pad). I'd suggest you use empty initializers for info and handle > to make sure we cannot leak anything and the compiler will hopefully > eliminate the unnecessary zeroing. > > > + > > + buf += sizeof(info); > > + len -= sizeof(info); > > + handle.handle_type = event->fh_type; > > + handle.handle_bytes = event->fh_len; > > Use local variable fh_len here? > Sure. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index cddebea5ff67..265fbaa2d5b7 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -99,6 +99,11 @@ static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event) !fanotify_inline_fh_len(event->fh_len); } +static inline void *fanotify_event_fh(struct fanotify_event *event) +{ + return fanotify_fid_fh(&event->fid, event->fh_len); +} + /* * Structure for permission fanotify events. It gets allocated and freed in * fanotify_handle_event() since we wait there for user response. When the diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 9b9e6704f9e7..032a9a225a21 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -47,6 +47,22 @@ struct kmem_cache *fanotify_mark_cache __read_mostly; struct kmem_cache *fanotify_event_cachep __read_mostly; struct kmem_cache *fanotify_perm_event_cachep __read_mostly; +static int fanotify_event_info_len(struct fanotify_event *event) +{ + if (!fanotify_event_has_fid(event)) + return 0; + + return sizeof(struct fanotify_event_info_fid) + + sizeof(struct file_handle) + event->fh_len; +} + +#define FANOTIFY_EVENT_ALIGN (sizeof(void *)) + +static int fanotify_round_event_info_len(struct fanotify_event *event) +{ + return roundup(fanotify_event_info_len(event), FANOTIFY_EVENT_ALIGN); +} + /* * Get an fsnotify notification event if one exists and is small * enough to fit in "count". Return an error pointer if the count @@ -57,6 +73,9 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly; static struct fsnotify_event *get_one_event(struct fsnotify_group *group, size_t count) { + size_t event_size = FAN_EVENT_METADATA_LEN; + struct fanotify_event *event; + assert_spin_locked(&group->notification_lock); pr_debug("%s: group=%p count=%zd\n", __func__, group, count); @@ -64,11 +83,18 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group, if (fsnotify_notify_queue_is_empty(group)) return NULL; - if (FAN_EVENT_METADATA_LEN > count) + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { + event = FANOTIFY_E(fsnotify_peek_first_event(group)); + event_size += fanotify_round_event_info_len(event); + } + + if (event_size > count) return ERR_PTR(-EINVAL); - /* held the notification_lock the whole time, so this is the - * same event we peeked above */ + /* + * Held the notification_lock the whole time, so this is the + * same event we peeked above + */ return fsnotify_remove_first_event(group); } @@ -174,6 +200,47 @@ static int process_access_response(struct fsnotify_group *group, return 0; } +static int copy_fid_to_user(struct fanotify_event *event, char __user *buf) +{ + struct fanotify_event_info_fid info; + struct file_handle handle; + size_t fh_len = event->fh_len; + size_t info_len = fanotify_event_info_len(event); + size_t len = roundup(info_len, FANOTIFY_EVENT_ALIGN); + + if (!info_len) + return 0; + + /* Copy event info fid header followed by vaiable sized file handle */ + info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID; + info.hdr.len = info_len; + info.fsid = event->fid.fsid; + info_len = sizeof(info) + sizeof(struct file_handle); + if (copy_to_user(buf, &info, sizeof(info))) + return -EFAULT; + + buf += sizeof(info); + len -= sizeof(info); + handle.handle_type = event->fh_type; + handle.handle_bytes = event->fh_len; + if (copy_to_user(buf, &handle, sizeof(handle))) + return -EFAULT; + + buf += sizeof(handle); + len -= sizeof(handle); + if (copy_to_user(buf, fanotify_event_fh(event), fh_len)) + return -EFAULT; + + /* Pad with 0's */ + buf += fh_len; + len -= fh_len; + WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); + if (len > 0 && clear_user(buf, len)) + return -EFAULT; + + return 0; +} + static ssize_t copy_event_to_user(struct fsnotify_group *group, struct fsnotify_event *fsn_event, char __user *buf) @@ -197,18 +264,26 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, fd = create_fd(group, event, &f); if (fd < 0) return fd; + } else if (fanotify_event_has_fid(event)) { + metadata.event_len += fanotify_round_event_info_len(event); } metadata.fd = fd; ret = -EFAULT; - if (copy_to_user(buf, &metadata, metadata.event_len)) + if (copy_to_user(buf, &metadata, FAN_EVENT_METADATA_LEN)) goto out_close_fd; if (fanotify_is_perm_event(event->mask)) FANOTIFY_PE(fsn_event)->fd = fd; - if (fd != FAN_NOFD) + if (fanotify_event_has_path(event)) { fd_install(fd, f); + } else if (fanotify_event_has_fid(event)) { + ret = copy_fid_to_user(event, buf + FAN_EVENT_METADATA_LEN); + if (ret < 0) + return ret; + } + return metadata.event_len; out_close_fd: diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index d07f3cbc2786..959ae2bdc7ca 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -107,6 +107,26 @@ struct fanotify_event_metadata { __s32 pid; }; +#define FAN_EVENT_INFO_TYPE_FID 1 + +/* Variable length info record following event metadata */ +struct fanotify_event_info_header { + __u8 info_type; + __u8 pad; + __u16 len; +}; + +/* Unique file identifier info record */ +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]; +}; + struct fanotify_response { __s32 fd; __u32 response;
If group requested FAN_REPORT_FID and event has file identifier, copy that information to user reading the event after event metadata. fid information is formatted as struct fanotify_event_info_fid that includes a generic header struct fanotify_event_info_header, so that other info types could be defined in the future using the same header. metadata->event_len includes the length of the fid information. The fid information includes the filesystem's fsid (see statfs(2)) followed by an NFS file handle of the file that could be passed as an argument to open_by_handle_at(2). Cc: <linux-api@vger.kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.h | 5 ++ fs/notify/fanotify/fanotify_user.c | 85 ++++++++++++++++++++++++++++-- include/uapi/linux/fanotify.h | 20 +++++++ 3 files changed, 105 insertions(+), 5 deletions(-)