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...
>
I am thinking why do we need the duplicate and unaligned ignore mask logic
in send_to_group() at all?
With fanotify the only backend using the ->handle_event() multi mark
flavor, maybe we should keep it simple and let fanotify do all the specific
mark ignore logic internally?
> Overall I guess the functionality makes sense to me (in fact it is somewhat
> surprising it is not working like that from the beginning), API-wise it is
> not outright horrible, and technically it seems doable. What do you think?
>
I think that having ONDIR and ON_CHILD in ignored mask is source for
confusion.
Imagine a mount mark with FAN_ONDIR and ignored mark (on dir inode)
without FAN_ONDIR.
What should the outcome be?
Don't ignore the events on dir because ignore mask does not have ONDIR?
That is not the obvious behavior that people will expect.
ON_CHILD may be a different case, but I also prefer not to deviate it from
ONDIR.
The only thing I can think of to add clarification is FAN_MARK_PARENT.
man page already says:
"The flag has no effect when marking mounts and filesystems."
It can also say:
"The flag has no effect when set in the ignored mask..."
"The flag is implied for both mask and ignored mask when marking
directories with FAN_MARK_PARENT".
Implementation wise, this would be very simple, because we already
force set FAN_EVENT_ON_CHILD for FAN_MARK_MOUNT
and FAN_MARK_FILESYSTEM with REPORT_DIR_FID, se we can
also force set it for FAN_MARK_PARENT.
But maybe it's just me that thinks this would be more clear??
Thanks,
Amir.
