On Thu, Jun 23, 2022 at 1:14 PM Jan Kara [off-list ref] wrote:

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

quoted

This flag is a new way to configure ignore mask which allows adding and
removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask.

The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on
directories and would ignore events on children depending on whether
the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask.

FAN_MARK_IGNORE can be used to ignore events on children without setting
FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on
directories unconditionally, only when FAN_ONDIR is set in ignore mask.

The new behavior is sticky.  After calling fanotify_mark() with
FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
will update the ignore mask, but will not change the event flags in
ignore mask nor how these flags are treated.

IMHO this stickyness is not very obvious. Wouldn't it be less error-prone
for users to say that once FAN_MARK_IGNORE is used for a mark, all
subsequent modifications of ignore mask have to use FAN_MARK_IGNORE? I mean
if some program bothers with FAN_MARK_IGNORE, I'd expect it to use it for
all its calls as otherwise the mixup is kind of difficult to reason
about...

I like that.

Also it follows the behavior we have picked for FAN_MARK_EVICTABLE AFAIR
but that's not really important to me.

It's kind of the opposite in the case of FAN_MARK_EVICTABLE.
FAN_MARK_EVICTABLE can be "upgraded" no non-evictable
but not the other way around.
And also with FAN_MARK_EVICTABLE we do not deprecate the
old API...

See man page draft:
https://github.com/amir73il/man-pages/commit/58851140bbc08b9ab9c7edd8830f37cf883d8d2a#diff-7a4387558a34e18ed6fb13d31933b2e4506496f8b3dd55df700f62b258e6f004R165

quoted

@@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,

      /*
       * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
-      * FAN_MARK_IGNORED_MASK.
+      * FAN_MARK_IGNORED_MASK.  They can be updated in ignore mask with
+      * FAN_MARK_IGNORE and then they do take effect.
       */
-     if (ignore)
+     switch (ignore) {
+     case 0:
+     case FAN_MARK_IGNORE:
+             break;
+     case FAN_MARK_IGNORED_MASK:
              mask &= ~FANOTIFY_EVENT_FLAGS;
+             umask = FANOTIFY_EVENT_FLAGS;
+             break;
+     default:
+             return -EINVAL;
+     }

I think this would be easier to follow as two ifs:

        /* We don't allow FAN_MARK_IGNORE & FAN_MARK_IGNORED_MASK together */
        if (ignore == FAN_MARK_IGNORE | FAN_MARK_IGNORED_MASK)
                return -EINVAL;
        /*
         * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
         * FAN_MARK_IGNORED_MASK.
         */
        if (ignore == FAN_MARK_IGNORED_MASK) {
                mask &= ~FANOTIFY_EVENT_FLAGS;
                umask = FANOTIFY_EVENT_FLAGS;
        }

Yeh that looks better.

Thanks,
Amir.