Re: Ignore mask handling in fanotify_group_event_mask()

[Amir Goldstein <amir73il@gmail.com>]

On Thu, May 21, 2020 at 7:24 PM Jan Kara <jack@suse.cz> wrote:
>
> Hello Amir!
>
> I was looking into backporting of commit 55bf882c7f13dd "fanotify: fix
> merging marks masks with FAN_ONDIR" and realized one oddity in
> fanotify_group_event_mask(). The thing is: Even if the mark mask is such
> that current event shouldn't trigger on the mark, we still have to take
> mark's ignore mask into account.
>
> The most realistic example that would demonstrate the issue that comes to my
> mind is:
>
> mount mark watching for FAN_OPEN | FAN_ONDIR.
> inode mark on a directory with mask == 0 and ignore_mask == FAN_OPEN.
>
> I'd expect the group will not get any event for opening the dir but the
> code in fanotify_group_event_mask() would not prevent event generation. Now
> as I've tested the event currently actually does not get generated because
> there is a rough test in send_to_group() that actually finds out that there
> shouldn't be anything to report and so fanotify handler is actually never
> called in such case. But I don't think it's good to have an inconsistent
> test in fanotify_group_event_mask(). What do you think?
>

I agree this is not perfect.
I think that moving the marks_ignored_mask line
To the top of the foreach loop should fix the broken logic.
It will not make the code any less complicated to follow though.
Perhaps with a comment along the lines of:

             /* Ignore mask is applied regardless of ISDIR and ON_CHILD flags */
             marks_ignored_mask |= mark->ignored_mask;

Now is there a real bug here?
Probably not because send_to_group() always applied an ignore mask
that is greater or equal to that of fanotify_group_event_mask().

should fanotify_group_event_mask() re-apply the same generic logic
already applied in send_to_group()? Maybe there is no point.
After all, fanotify_group_event_mask() also does not handle
FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY, so the
assumption that send_to_group() is doing some of the logic is
already there.

Not sure if I helped answering your question.

Thanks,
Amir.