Re: Ignore mask handling in fanotify_group_event_mask()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat 23-05-20 20:14:58, Amir Goldstein wrote:
> On Thu, May 21, 2020 at 9:10 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Thu, May 21, 2020 at 7:24 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxx>
SUSE Labs, CR



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux