Re: Ignore mask handling in fanotify_group_event_mask()

[Amir Goldstein <amir73il@gmail.com>]

On Mon, May 25, 2020 at 10:23 AM Jan Kara <jack@suse.cz> wrote:
>
> On Sat 23-05-20 20:14:58, Amir Goldstein wrote:
> > On Thu, May 21, 2020 at 9:10 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > 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().
> > >
> >
> > That's a wrong statement of course.
> > We do need to re-apply the ignore mask when narrowing the event mask.
> >
> > Exposing the bug requires a "compound" event.
> >
> > The only case of compound event I could think of is this:
> >
> > mount mark with mask == 0 and ignore_mask == FAN_OPEN. inode mark
> > on a directory with mask == FAN_EXEC | FAN_EVENT_ON_CHILD.
> >
> > The event: FAN_OPEN | FAN_EXEC | FAN_EVENT_ON_CHILD
> > would be reported to group with the FAN_OPEN flag despite the
> > fact that FAN_OPEN is in ignore mask of mount mark because
> > the mark iteration loop skips over non-inode marks for events
> > on child.
> >
> > I'll try to work that case into the relevant LTP test to prove it and
> > post a fix.
>
> Ha, that's clever. But FAN_EXEC does not exist in current fanotify. We only
> have FAN_OPEN_EXEC... And I don't think we have any compound events.
>

Typo. I meant FAN_OPEN_EXEC and you can see from LTP test
we do have at least this one compound event.

We could also split it if we wanted to, but no reason to do it now.

Thanks,
Amir.