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

On Wed, Jun 22, 2022 at 6:52 PM Jan Kara [off-list ref] wrote:

On Mon 20-06-22 16:45:50, Amir Goldstein wrote:

quoted

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:

I looked at the code and the only two cases I found were the two cases
that you pointed out that needed to use fsnotify_ignored_events().

quoted

@@ -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...

fixed.

quoted

@@ -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'?

You are right.
This can end up clearing FAN_ONDIR and then we won't report it.
However, take a look at this:

commit 0badfa029e5fd6d5462adb767937319335637c83
Author: Amir Goldstein [off-list ref]
Date:   Thu Jul 16 11:42:09 2020 +0300

    fanotify: generalize the handling of extra event flags

    In fanotify_group_event_mask() there is logic in place to make sure we
    are not going to handle an event with no type and just FAN_ONDIR flag.
    Generalize this logic to any FANOTIFY_EVENT_FLAGS.

    There is only one more flag in this group at the moment -
    FAN_EVENT_ON_CHILD. We never report it to user, but we do pass it in to
    fanotify_alloc_event() when group is reporting fid as indication that
    event happened on child. We will have use for this indication later on.

What the hell did I mean by "We will have use for this indication later on"?
fanotify_alloc_event() does not look at the FAN_EVENT_ON_CHILD flag.
I think I had the idea that events reported in a group with FAN_REPORT_NAME
on an inode mark should not report its parent fid+name to be compatible with
inotify behavior and I think you shot this idea down, but it is only a guess.

quoted

@@ -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...

Fixed here too, but here there is no meaning to the event flags,
because test_mask masks them out.

Thanks,
Amir.