On Mon 14-03-22 21:17:11, 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...
> >
>
> I am thinking why do we need the duplicate and unaligned ignore mask logic
> in send_to_group() at all?
I agree the duplication is not good.
> 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?
Well, if we leave handling of ignore marks completely to ->handle_event()
then we pay the cost of indirect call even for ignored events. Not sure how
much will it be visible in the overall costs, it's probably worth
benchmarking and we'll see.
> > 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??
Yeah, I'm not sure if adding another flag that iteracts with ON_CHILD or
ONDIR adds any clarity to this mess. In my opinion defining that ON_CHILD
flag in the ignore mask means "apply this ignore mask to events from
immediate children" has an intuitive meaning as it is exactly matching the
semantics of ON_CHILD in the normal mark mask.
With ONDIR I agree things are not as obvious. Historically we have applied
ignore mask even for events coming from directories regardless of ONDIR
flag in the ignore mask. So ignore mask without any special flag has the
implicit meaning of "apply to all events regardless of type of originating
inode". I don't think we can change that meaning at this point. We could
define meaning of ONDIR in ignore mask to either "ignore only events from
directories" or to "ignore only events from ordinary files". But neither
seems particularly natural or useful.
Another concern is that currently fanotify_mark() ignores both ONDIR and
ON_CHILD flags in the ignore mask. So there is a chance someone happens to
set them by accident. For the ON_CHILD flag I would be willing to take a
chance and assign the meaning to this flag because I think chances for
real world breakage are really low. But for ONDIR, given it is unclear
whether ignore masks should apply for directory events without that flag, I
think the chances someone sets it "just to be sure" are high enough that I
would be reluctant to take that chance. So for ONDIR I think we are more or
less stuck with keeping it unused in the ignore mask.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
