Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Fri, Jan 04, 2019 at 01:42:33PM +0200, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Sun 02-12-18 13:38:26, Amir Goldstein wrote:
> > > dirent modification events (create/delete/move) do not carry the
> > > child entry name/inode information. Instead, we report FAN_ONDIR
> > > for mkdir/rmdir so user can differentiate them from creat/unlink.
> > >
> > > For backward compatibility and consistency, do not report FAN_ONDIR
> > > to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR
> > > to user in FAN_REPORT_FID mode for all event types.
> > >
> > > Unlike legacy fanotify events (open/access/close), dirent events
> > > for subdir entries (mkdir/rmdir) will be reported regardless if
> > > user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > be reported if the user asked for it.
> > >
> > > Cc: <linux-api@vger.kernel.org>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Some comments below.
> >
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index 89c19db4d45f..1aa23cefae5d 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -112,6 +112,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > >                                    int data_type)
> > >  {
> > >       __u32 marks_mask = 0, marks_ignored_mask = 0;
> > > +     __u32 test_mask, user_mask = FANOTIFY_EVENT_TYPES;
> > >       const struct path *path = data;
> > >       struct fsnotify_mark *mark;
> > >       int type;
> > > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > >               marks_ignored_mask |= mark->ignored_mask;
> > >       }
> > >
> > > +     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > > +
> > > +     /*
> > > +      * dirent modification events (create/delete/move) do not carry the
> > > +      * child entry name/inode information. Instead, we report FAN_ONDIR
> > > +      * for mkdir/rmdir so user can differentiate them from creat/unlink.
> > > +      *
> > > +      * For backward compatibility and consistency, do not report FAN_ONDIR
> > > +      * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR
> > > +      * to user in FAN_REPORT_FID mode for all event types.
> > > +      */
> > > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> > > +             /* Do not report FAN_ONDIR without an event type */
> > > +             BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS);
> > > +             if (!(test_mask & FANOTIFY_EVENT_TYPES))
> > > +                     return 0;
> > > +
> > > +             user_mask |= FAN_ONDIR;
> > > +     }
> > > +
> > > +     /*
> > > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > +      * be reported if the user asked for it.
> > > +      */
> > >       if (event_mask & FS_ISDIR &&
> > > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
> >
> > I disagree with this. It just seems inconsistent for dirent events for
> > directories to get reported without FAN_ONDIR. I understand there's not
> > great use for not reporting directory dirent events but it's not like
> > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> > makes the API more consistent. You could possibly remind the reader in the
> > manpage that FAN_ONDIR is required to get all dirent events.
> 
> I see your point.
> I have no problem with requiring FAN_ONDIR for mkdir events.
> I believe the strongest argument should be which way is easier
> to document/understand.
> 
> Matthew, if you agree that it looks easier to document Jan's proposal,
> please go a head with this and we will see how man page looks like
> before making the final decision.

To be fair, for the sake of clarity and consistency with the existing API I do
believe it would make it easier for the API consumer to comprehend what Jan has
suggested. Simple, in order to receive any events of type dirent, one must
supply FAN_ONDIR as part of their mark mask. 
 
> >
> > >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > >               return 0;
> > >
> > > -     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask &
> > > -             ~marks_ignored_mask;
> > > +     return test_mask & user_mask;
> >
> > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > I guess. E.g. when implementing recursive watching of a directory. Or what
> > is your intended usecase? It should be said explicitely in the changelog.
> 
> Recursive watch of directory tree is certainly a use case that could benefit
> from "mkdir" events. I will add that to commit message.
> 
> >
> > >  static int fanotify_encode_fid(struct fanotify_event *event,
> > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > index e9d45387089f..f5f86566c277 100644
> > > --- a/include/linux/fanotify.h
> > > +++ b/include/linux/fanotify.h
> > > @@ -61,13 +61,16 @@
> > >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > >                                FAN_OPEN_EXEC_PERM)
> > >
> > > +/* Events types that may be reported from vfs */
> > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > +                              FANOTIFY_PERM_EVENTS)
> > > +
> > >  /* Extra flags that may be reported with event or control handling of events */
> > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > >
> > >  /* Events that may be reported to user */
> > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > -                                      FANOTIFY_PERM_EVENTS | \
> > > -                                      FAN_Q_OVERFLOW)
> > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > >
> > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > >                                        FANOTIFY_EVENT_FLAGS)
> >
> > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > defines as is...
> >
> 
> Sorry. I don't understand what you mean.
> FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> Do you not like the new group definition FANOTIFY_EVENT_TYPES?
> 
> Please explain.

-- 
Matthew Bobrowski