On Thu, Jan 3, 2019 at 7:13 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> 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.
