Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jun 22, 2022 at 6:52 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > ignore mask is always implicitly applied to events on directories.
> >
> > Define a mark flag that replaces this legacy behavior with logic of
> > applying the ignore mask according to event flags in ignore mask.
> >
> > Implement the new logic to prepare for supporting an ignore mask that
> > ignores events on children and ignore mask that does not ignore events
> > on directories.
> >
> > To emphasize the change in terminology, also rename ignored_mask mark
> > member to ignore_mask and use accessor to get only ignored events or
> > events and flags.
> >
> > This change in terminology finally aligns with the "ignore mask"
> > language in man pages and in most of the comments.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks mostly good to me. Just one question / suggestion: You are
> introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events().
> So shouldn't we be using these helpers as much as possible throughout the
> code? Because in several places I had to check the code around whether
> using mark->ignore_mask directly is actually fine. In particular:
>
> > @@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >                       return 0;
> >       } else if (!(fid_mode & FAN_REPORT_FID)) {
> >               /* Do we have a directory inode to report? */
> > -             if (!dir && !(event_mask & FS_ISDIR))
> > +             if (!dir && !ondir)
> >                       return 0;
> >       }
> >
> >       fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> > -             /* Apply ignore mask regardless of mark's ISDIR flag */
> > -             marks_ignored_mask |= mark->ignored_mask;
> > +             /*
> > +              * Apply ignore mask depending on whether FAN_ONDIR flag in
> > +              * ignore mask should be checked to ignore events on dirs.
> > +              */
> > +             if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
> > +                     marks_ignore_mask |= mark->ignore_mask;
> >
> >               /*
> >                * If the event is on dir and this mark doesn't care about
> >                * events on dir, don't send it!
> >                */
> > -             if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
> > +             if (ondir && !(mark->mask & FAN_ONDIR))
> >                       continue;
> >
> >               marks_mask |= mark->mask;
>
> So for example here I'm wondering whether a helper should not be used...
>
> > @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >               *match_mask |= 1U << type;
> >       }
> >
> > -     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > +     test_mask = event_mask & marks_mask & ~marks_ignore_mask;
>
> Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
> marks_ignore_mask it can result in clearing this flag in the returned
> 'mask' which is likely not what we want if there are some events left
> unignored in the 'mask'?
>
> > @@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
> >       fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> >               group = mark->group;
> >               marks_mask |= mark->mask;
> > -             marks_ignored_mask |= mark->ignored_mask;
> > +             if (!(mask & FS_ISDIR) ||
> > +                 (fsnotify_ignore_mask(mark) & FS_ISDIR))
> > +                     marks_ignore_mask |= mark->ignore_mask;
> >       }
> >
> > -     pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > -              __func__, group, mask, marks_mask, marks_ignored_mask,
> > +     pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > +              __func__, group, mask, marks_mask, marks_ignore_mask,
> >                data, data_type, dir, cookie);
> >
> > -     if (!(test_mask & marks_mask & ~marks_ignored_mask))
> > +     if (!(test_mask & marks_mask & ~marks_ignore_mask))
> >               return 0;
>
> And I'm wondering about similar things here...
>

I can't remember if I left those cases on purpose.
I will check if it makes sense to use a macro here.

Amir.