Ignore mask handling in fanotify_group_event_mask()

Ignore mask handling in fanotify_group_event_mask()

[Jan Kara]

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?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Ignore mask handling in fanotify_group_event_mask()

[Amir Goldstein]

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.

Re: Ignore mask handling in fanotify_group_event_mask()

[Amir Goldstein]

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.

Thanks,
Amir.

Re: Ignore mask handling in fanotify_group_event_mask()

[Jan Kara]

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. 

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Ignore mask handling in fanotify_group_event_mask()

[Amir Goldstein]

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.

Re: Ignore mask handling in fanotify_group_event_mask()

[Jan Kara]

On Mon 25-05-20 11:52:54, Amir Goldstein wrote:
> 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.

Yeah, I understood what you meant once I saw the changelog of your patch.
Thanks for it.

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

Agreed.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR