On Fri 15-10-21 11:10:58, Amir Goldstein wrote:
> On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> <krisman@xxxxxxxxxxxxx> wrote:
> >
> > Non-inode errors will reported with an empty file_handle. In
> > preparation for that, allow some events to print the FID record even if
> > there isn't any file_handle encoded
> >
> > Even though FILEID_ROOT is used internally, make zero-length file
> > handles be reported as FILEID_INVALID.
> >
> > Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
> > ---
> > fs/notify/fanotify/fanotify_user.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 5324890500fc..39cf8ba4a6ce 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -127,6 +127,16 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
> > FANOTIFY_EVENT_ALIGN);
> > }
> >
> > +static bool fanotify_event_allows_empty_fh(struct fanotify_event *event)
> > +{
> > + switch (event->type) {
> > + case FANOTIFY_EVENT_TYPE_FS_ERROR:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > static size_t fanotify_event_len(unsigned int info_mode,
> > struct fanotify_event *event)
> > {
> > @@ -157,7 +167,7 @@ static size_t fanotify_event_len(unsigned int info_mode,
> > if (info_mode & FAN_REPORT_PIDFD)
> > event_len += FANOTIFY_PIDFD_INFO_HDR_LEN;
> >
> > - if (fh_len)
> > + if (fh_len || fanotify_event_allows_empty_fh(event))
> > event_len += fanotify_fid_info_len(fh_len, dot_len);
> >
> > return event_len;
> > @@ -338,9 +348,6 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
> > pr_debug("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n",
> > __func__, fh_len, name_len, info_len, count);
> >
> > - if (!fh_len)
> > - return 0;
> > -
> > if (WARN_ON_ONCE(len < sizeof(info) || len > count))
> > return -EFAULT;
> >
> > @@ -375,6 +382,11 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
> >
> > handle.handle_type = fh->type;
> > handle.handle_bytes = fh_len;
> > +
> > + /* Mangle handle_type for bad file_handle */
> > + if (!fh_len)
> > + handle.handle_type = FILEID_INVALID;
> > +
> > if (copy_to_user(buf, &handle, sizeof(handle)))
> > return -EFAULT;
> >
> > @@ -467,7 +479,8 @@ static int copy_info_records_to_user(struct fanotify_event *event,
> > total_bytes += ret;
> > }
> >
> > - if (fanotify_event_object_fh_len(event)) {
> > + if (fanotify_event_object_fh_len(event) ||
> > + fanotify_event_allows_empty_fh(event)) {
> > const char *dot = NULL;
> > int dot_len = 0;
> >
>
> I don't like this fanotify_event_allows_empty_fh() implementation so much.
>
> How about this instead:
>
> static inline struct fanotify_fh *fanotify_event_object_fh(
> struct fanotify_event *event)
> {
> struct fanotify_fh *fh = NULL;
>
> /* An error event encodes (a FILEID_INVAL) fh for an empty fh */
> if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
> return &FANOTIFY_EE(event)->object_fh;
> else if (event->type == FANOTIFY_EVENT_TYPE_FID)
> fh = &FANOTIFY_FE(event)->object_fh;
> else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
> fh = fanotify_info_file_fh(&FANOTIFY_NE(event)->info);
>
> if (!fh && !fh->len)
> return NULL;
>
> return fh;
> }
>
> struct fanotify_fh *object_fh = fanotify_event_object_fh(event);
> ...
>
> - if (fanotify_event_object_fh_len(event)) {
> + if (object_fh) {
> const char *dot = NULL;
> ...
> ret = copy_fid_info_to_user(fanotify_event_fsid(event),
> - fanotify_event_object_fh(event),
> + object_fh,
> info_type, dot, dot_len,
> buf, count);
> ...
>
> And similar change to fanotify_event_len()
>
> This way, the logic of whether to report fh or not is encoded in
> fanotify_event_object_fh() and fanotify_event_object_fh_len()
> goes back to being a property of the the fh report.
I like this except that AFAICT this will be problematic for
fanotify_event_object_fh_len() because there we want to return 0 (length of
file handle buffer to copy) for error events - so "copy just header to
userspace" is going to be indistiguishable from "copy nothing".
But maybe we need helpers fanotify_event_has_object_fh() and
fanotify_event_has_dir_fh() (for symmetry) instead of directly checking
fh_len? These would then encapsulate all the magic for error events.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
