On Wed 13-04-22 14:09:12, Amir Goldstein wrote:
> On Mon, Mar 14, 2022 at 1:33 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Mon 14-03-22 11:28:23, Amir Goldstein wrote:
> > > On Mon, Mar 14, 2022 at 10:47 AM Jan Kara <jack@xxxxxxx> wrote:
> > > >
> > > > On Sat 12-03-22 11:22:29, Srinivas wrote:
> > > > > If a process calls fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> > > > > FAN_OPEN_PERM, 0, "/mountpoint") no other directory exclusions can be
> > > > > applied.
> > > > >
> > > > > However a path (file) exclusion can still be applied using
> > > > >
> > > > > fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_IGNORED_MASK |
> > > > > FAN_MARK_IGNORED_SURV_MODIFY, FAN_OPEN_PERM | FAN_CLOSE_WRITE, AT_FDCWD,
> > > > > "/tmp/fio/abc"); ===> path exclusion that works.
> > > > >
> > > > > I think the directory exclusion not working is a bug as otherwise AV
> > > > > solutions cant exclude directories when using FAN_MARK_MOUNT.
> > > > >
> > > > > I believe the change should be simple since we are already supporting
> > > > > path exclusions. So we should be able to add the same for the directory
> > > > > inode.
> > > > >
> > > > > 215676 – fanotify Ignoring/Excluding a Directory not working with
> > > > > FAN_MARK_MOUNT (kernel.org)
> > > >
> > > > Thanks for report! So I believe this should be fixed by commit 4f0b903ded
> > > > ("fsnotify: fix merge with parent's ignored mask") which is currently
> > > > sitting in my tree and will go to Linus during the merge (opening in a
> > > > week).
> > >
> > > Actually, in a closer look, that fix alone is not enough.
> > >
> > > With the current upstream kernel this should work to exclude events
> > > in a directory:
> > >
> > > fanotify_mark(fd, FAN_MARK_ADD, FAN_EVENT_ON_CHILD |
> > > FAN_OPEN_PERM | FAN_CLOSE_WRITE,
> > > AT_FDCWD, "/tmp/fio/");
> > > fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_IGNORED_MASK |
> > > FAN_MARK_IGNORED_SURV_MODIFY,
> > > FAN_OPEN_PERM | FAN_CLOSE_WRITE,
> > > AT_FDCWD, "/tmp/fio/");
> > >
> > > The first call tells fanotify that the inode mark on "/tmp/foo" is
> > > interested in events on children (and not only on self).
> > > The second call sets the ignored mark for open/close events.
> > >
> > > The fix only removed the need to include the events in the
> > > first call.
> > >
> > > Should we also interpret FAN_EVENT_ON_CHILD correctly
> > > in a call to fanotify_mark() to set an ignored mask?
> > > Possibly. But that has not been done yet.
> > > I can look into that if there is interest.
> >
> > Oh, right. I forgot about the need for FAN_EVENT_ON_CHILD in the
> > mark->mask. It seems we can set FAN_EVENT_ON_CHILD in the ignored_mask as
> > well but it just gets ignored currently. So we would need to propagate it
> > even from ignore_mask to inode->i_fsnotify_mask. But send_to_group() would
> > also need to be more careful now with ignore masks and apply them from
> > parent only if the particular mark has FAN_EVENT_ON_CHILD in the ignore
> > mask. Interestingly fanotify_group_event_mask() does explicitely apply
> > ignore_mask from the parent regardless of FAN_EVENT_ON_CHILD flags. So
> > there is some inconsistency there and it would need some tweaking...
> >
>
> Jan,
>
> Just a heads up - you were right about this inconsistency and I have both
> patches to fix it [1] and LTP test to reproduce the issue [2] and started work
> on the new FAN_MARK_IGNORE API.
> The new API has no tests yet, but it has a man page draft [3].
>
> The description of the bugs as I wrote them in the fix commit message:
>
> This results in several subtle changes of behavior, hopefully all
> desired changes of behavior, for example:
>
> - Group A has a mount mark with FS_MODIFY in mask
> - Group A has a mark with ignored mask that does not survive FS_MODIFY
> and does not watch children on directory D.
> - Group B has a mark with FS_MODIFY in mask that does watch children
> on directory D.
> - FS_MODIFY event on file D/foo should not clear the ignored mask of
> group A, but before this change it does
>
> And if group A ignored mask was set to survive FS_MODIFY:
> - FS_MODIFY event on file D/foo should be reported to group A on account
> of the mount mark, but before this change it is wrongly ignored
>
> Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events
> on child and on dir")
Thanks for looking into this! Yeah, the change in behavior looks OK to me.
Honza
> [1] https://github.com/amir73il/linux/commits/fan_mark_ignore
> [2] https://github.com/amir73il/ltp/commits/fan_mark_ignore
> [3] https://github.com/amir73il/man-pages/commits/fan_mark_ignore
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
