On Fri, 6 Dec 2024 at 19:29, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> Because with fanotify the event mask is used both as a filter for subscribe
> and as a filter to the reported event->mask, so with your current patch
> a user watching only FAN_MNT_DETACH, will get a FAN_MNT_DETACH
> event on mount move. Is that the intention?
I imagine there's a case for watching a single mount and seeing if it
goes away. In that case it's irrelevant whether the mount got moved
away or it was destroyed.
> Is there even a use case for watching only attach or only detach?
I'm not sure, there could well be.
> Are we ever likely to add more mount events besides attach/detach?
Yes, modification (i.e. flag/propagation/etc changes). And that one
could really make sense on a per-mount basis instead of per-ns.
> If the answers are no and no, then I think we should consider forcing
> to set and clear the mount events together.
>
> There are more simplifications that follow if we make that decision...
To me it looks like this would be a very minor simplification and the
main purpose would be to avoid confusing the user, right?
In that case maybe documenting the behavior would be preferable to
adding constraints.
> > +#ifdef CONFIG_FSNOTIFY
> > + __u32 n_fsnotify_mask;
>
> There is no point in this "optimization" mask if all the mntns
> marks are interested in all the two possible mount events.
> The "optimization" would not have been needed even if we would allow watching
> only attach or detach, but I guess this helps keeping the code generic...
I just did a mindless copy of other watchable objects. Let's keep
this for now, then we'll see later if removing it is a simplification
or not.
> > @@ -303,17 +305,19 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
> > __func__, iter_info->report_mask, event_mask, data, data_type);
> >
> > - if (!fid_mode) {
> > - /* Do we have path to open a file descriptor? */
> > - if (!path)
> > - return 0;
> > - /* Path type events are only relevant for files and dirs */
> > - if (!d_is_reg(path->dentry) && !d_can_lookup(path->dentry))
> > - return 0;
> > - } else if (!(fid_mode & FAN_REPORT_FID)) {
> > - /* Do we have a directory inode to report? */
> > - if (!dir && !ondir)
> > - return 0;
> > + if (data_type != FSNOTIFY_EVENT_MNT) {
>
> Until we allow mixing other mark type (e.g. ignore mount mark for
> specific mount)
> and if we mandate watching both mount events, then all the logic below
> is irrelevant
> and if (data_type == FSNOTIFY_EVENT_MNT) can always
> return FANOTIFY_MOUNT_EVENTS;
Hmm, but there's no hurt in keeping the logic, right?
> > + /* FIXME: is this the proper way to check if fsnotify_init() ran? */
> > + if (!fsnotify_mark_connector_cachep)
> > + return;
>
> checking if (ns->n_fsnotify_marks) is easier.
> marks cannot be added before boot completed and user requested to add marks.
Yeah, okay.
> mount events are not reported with event->fd.
> The condition that uses FANOTIFY_FD_EVENTS needs to be fixed
> to accommodate the case of mount events.
>
>
> if (mask &
> ~(FANOTIFY_FD_EVENTS|FANOTIFY_MOUNT_EVENTS|FANOTIFY_EVENT_FLAGS) &&
Okay.
Thanks,
Miklos
