Re: [PATCH 1/2] fanotify: store fsid in mark instead of in connector

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Nov 30, 2023 at 4:25 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sat 18-11-23 20:30:17, Amir Goldstein wrote:
> > Some filesystems like fuse and nfs have zero or non-unique fsid.
> > We would like to avoid reporting ambiguous fsid in events, so we need
> > to avoid marking objects with same fsid and different sb.
> >
> > To make this easier to enforce, store the fsid in the marks of the group
> > instead of in the shared conenctor.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Very nice! I like the result. Just a few nits below.
>
> > +static inline __kernel_fsid_t *fanotify_mark_fsid(struct fsnotify_mark *mark)
> > +{
> > +     return &FANOTIFY_MARK(mark)->fsid;
> > +}
>
> I guess, there's no big win in using this helper compared to using
> FANOTIFY_MARK(mark)->fsid so I'd just drop this helper.

ok.

>
> > @@ -530,6 +528,7 @@ struct fsnotify_mark {
> >  #define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY       0x0100
> >  #define FSNOTIFY_MARK_FLAG_NO_IREF           0x0200
> >  #define FSNOTIFY_MARK_FLAG_HAS_IGNORE_FLAGS  0x0400
> > +#define FSNOTIFY_MARK_FLAG_HAS_FSID          0x0800
> >       unsigned int flags;             /* flags [mark->lock] */
> >  };
>
> So this flag is in fact private to fanotify notification framework. Either
> we could just drop this flag and use
>
>   FANOTIFY_MARK(mark)->fsid[0] != 0 || FANOTIFY_MARK(mark)->fsid[1] != 0

Cannot.
Zero fsid is now a valid fsid in an inode mark (e.g. fuse).
The next patch also adds the flag FSNOTIFY_MARK_FLAG_WEAK_FSID

>
> instead or we could at least add a comment that this flags is in fact
> private to fanotify?

There is already a comment, because all the flags above are fanotify flags:

        /* fanotify mark flags */
#define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY  0x0100
#define FSNOTIFY_MARK_FLAG_NO_IREF              0x0200
#define FSNOTIFY_MARK_FLAG_HAS_IGNORE_FLAGS     0x0400

Thanks,
Amir.