[PATCH v5 2/3] fanotify: notify on mount attach and detach

[Amir Goldstein]

On Thu, Feb 13, 2025 at 1:00 PM Miklos Szeredi <miklos at szeredi.hu> wrote:
>
> On Tue, 11 Feb 2025 at 16:50, Jan Kara <jack at suse.cz> wrote:
> >
> > On Wed 29-01-25 17:58:00, Miklos Szeredi wrote:
>
> > >       fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
> > > -     if (mask & ~(FANOTIFY_FD_EVENTS|FANOTIFY_EVENT_FLAGS) &&
> > > +     if (mask & ~(FANOTIFY_FD_EVENTS|FANOTIFY_MOUNT_EVENTS|FANOTIFY_EVENT_FLAGS) &&
> >
> > I understand why you need this but the condition is really hard to
> > understand now and the comment above it becomes out of date. Perhaps I'd
> > move this and the following two checks for FAN_RENAME and
> > FANOTIFY_PRE_CONTENT_EVENTS into !FAN_GROUP_FLAG(group, FAN_REPORT_MNT)
> > branch to make things more obvious?
>
> Okay.  git diff -w below.
>
> Thanks,
> Miklos
>
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1936,6 +1936,8 @@ static int do_fanotify_mark(int fanotify_fd,
> unsigned int flags, __u64 mask,
>              mark_type != FAN_MARK_INODE)
>                 return -EINVAL;
>
> +       /* The following checks are not relevant to mount events */
> +       if (!FAN_GROUP_FLAG(group, FAN_REPORT_MNT)) {

Sorry for nit picking, but you already have this !FAN_REPORT_MNT
branch above:

+       /* Only report mount events on mnt namespace */
+       if (FAN_GROUP_FLAG(group, FAN_REPORT_MNT)) {
+               if (mask & ~FANOTIFY_MOUNT_EVENTS)
+                       return -EINVAL;
...
+       } else {
+               if (mask & FANOTIFY_MOUNT_EVENTS)

Which can be easily moved down here and then we get in one place:

if (FAN_REPORT_MNT) {
    /* event rules for FAN_REPORT_MNT */
} else {
    /* event rules for !FAN_REPORT_MNT */
}

TBH, with the check for (mask & ~FANOTIFY_MOUNT_EVENTS)
I personally wouldn't mind leaving checks for FAN_RENAME and
 FANOTIFY_PRE_CONTENT_EVENTS outside of the else branch,
but I don't have a strong objection to including them in the else branch.

Thanks,
Amir.