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

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Jun 24, 2022 at 5:35 PM Amir Goldstein <amir73il@gmail.com> 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 accessors to get only the effective
> ignored events or the ignored 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>
> ---

[...]

> @@ -336,7 +337,7 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
>                 fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
>                         if (!(mark->flags &
>                               FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
> -                               mark->ignored_mask = 0;
> +                               mark->ignore_mask = 0;
>                 }
>         }

Doh! I missed (again) the case of:
!FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY && !FS_EVENT_ON_CHILD

I was starting to look at a fix, but then I stopped to think about the
justification
for FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY on a directory.

The man page does say:
"... the ignore mask is cleared when a modify event occurs for the ignored file
     or directory."
But ignore mask on a parent never really worked when this man page was
written and there is no such thing as a "modify event" on the directory itself.

Furthermore, let's look at the motivation for IGNORED_SURV_MODIFY -
it is meant (I think) to suppress open/access permission events on a file
whose content was already scanned for malware until the content of that
file is modified - an important use case.

But can that use case be extended to all files in a directory?
In theory, anti-malware software could scan a directory and call it "clean"
until any of the files therein is modified. However, an infected file can also
be moved into the "clean" directory, so unless we introduce a flag
IGNORED_DOES_NOT_SURV_MOVED_TO, supporting
!IGNORED_SURV_MODIFY on a directory seems useless.

That leads me to suggest the thing I like most - deprecate.
Until someone comes up with a case to justify !IGNORED_SURV_MODIFY
on a directory, trying to set FAN_MARK_IGNORE on a directory without
IGNORED_SURV_MODIFY will return EISDIR.

We could also say that IGNORED_SURV_MODIFY is implied on
a directory, but I think the EISDIR option is cleaner and easier to
document - especially for the case of "upgrading" a directory mark
from FAN_MARK_IGNORED_MASK to new FAN_MARK_IGNORE.

We could limit that behavior to an ignore mask with EVENT_ON_CHILD
but that will just complicate things for no good reason.

Semi-related, we recently did:
ceaf69f8eadc ("fanotify: do not allow setting dirent events in mask of non-dir")
We could have also disallowed FAN_ONDIR and FAN_EVENT_ON_CHILD
on non-dir inode. Too bad I didn't see it.
Do you think that we can/should "fix" FAN_REPORT_TARGET_FID to include
those restrictions?

I would certainly like to disallow dirent events and the extra dir flags
for setting FAN_MARK_IGNORE on a non-dir inode.

I am going to be on two weeks vacation v5.19-rc5..v5.19-rc7,
so unless we have clear answers about the API questions above
early this week, FAN_MARK_IGNORE will probably have to wait
another cycle.

In any case, I am going to post v3 with my API proposal, but considering
the buggy v1 and API issue in v2, I will need to improve the test coverage
before FAN_MARK_IGNORE can be merged.

Thanks,
Amir.