Re: [PATCH v4 07/15] fanotify: copy event fid info to user

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux