Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark

[Jan Kara <jack@suse.cz>]

On Fri 31-08-18 18:30:32, Amir Goldstein wrote:
> On Fri, Aug 31, 2018 at 5:05 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 30-08-18 18:15:51, Amir Goldstein wrote:
> > > Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
> > > of super block mark type.
> > >
> > > A super block watch gets all events on the filesystem, regardless of
> > > the mount from which the mark was added, unless an ignore mask exists
> > > on either the inode or the mount where the event was generated.
> > >
> > > Only one of FAN_MARK_MOUNT and FAN_MARK_FILESYSTEM mark type flags
> > > may be provided to fanotify_mark() or no mark type flag for inode mark.
> > >
> > > Cc: <linux-api@vger.kernel.org>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Just one nit below, otherwise the patch look good to me.
> >
> > > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> > > index 74247917de04..7345b9a57f66 100644
> > > --- a/include/uapi/linux/fanotify.h
> > > +++ b/include/uapi/linux/fanotify.h
> > > @@ -43,6 +43,7 @@
> > >                                FAN_UNLIMITED_MARKS)
> > >
> > >  /* flags used for fanotify_modify_mark() */
> > > +#define FAN_MARK_INODE               0x00000000
> > >  #define FAN_MARK_ADD         0x00000001
> > >  #define FAN_MARK_REMOVE              0x00000002
> > >  #define FAN_MARK_DONT_FOLLOW 0x00000004
> > > @@ -51,6 +52,7 @@
> > >  #define FAN_MARK_IGNORED_MASK        0x00000020
> > >  #define FAN_MARK_IGNORED_SURV_MODIFY 0x00000040
> > >  #define FAN_MARK_FLUSH               0x00000080
> > > +#define FAN_MARK_FILESYSTEM  0x00000100
> > >
> > >  #define FAN_ALL_MARK_FLAGS   (FAN_MARK_ADD |\
> > >                                FAN_MARK_REMOVE |\
> > > @@ -59,7 +61,10 @@
> > >                                FAN_MARK_MOUNT |\
> > >                                FAN_MARK_IGNORED_MASK |\
> > >                                FAN_MARK_IGNORED_SURV_MODIFY |\
> > > -                              FAN_MARK_FLUSH)
> > > +                              FAN_MARK_FLUSH|\
> > > +                              FAN_MARK_FILESYSTEM)
> > > +
> > > +#define FAN_ALL_MARK_TYPE_FLAGS (FAN_MARK_MOUNT | FAN_MARK_FILESYSTEM)
> > >
> > >  /*
> > >   * All of the events - we build the list by hand so that we can add flags in
> >
> > So for completeness I'd add FAN_MARK_INODE to FAN_ALL_MARK_FLAGS and
> > FAN_ALL_MARK_TYPE_FLAGS. I know it doesn't change the actual value but
> > logically it belongs there when you defined it...
> >
> 
> Ok.
> 
> > Also one more thing to consider: Different mark types cannot be combined.
> > So it could save some bits in 'flags' in future if we had something like
> > FAN_MARK_TYPE_MASK and (flags & FAN_MARK_TYPE_MASK) would enumerate
> > different mark types - 0 for inode mark, FAN_MARK_MOUNT for mount mark,
> > FAN_MARK_FILESYSTEM for superblock mark etc. Again, currently there's no
> > practical difference in the values, just the names and tests for validity
> > would be slightly different.
> >
> 
> So do you prefer that I replace the test (mark_type &&
> !is_power_of_2(mark_type))
> with a switch cases statement for supported types?
> Makes sense.

Yes.

> Shall I go as far as:
> #define FAN_MARK_TYPE_BIT1      0x00000010
> #define FAN_MARK_TYPE_BIT2      0x00000100
> #define FAN_MARK_TYPE_MASK (FAN_MARK_TYPE_BIT1 | FAN_MARK_TYPE_BIT2)
> 
> /* mark type can be a combination of mark type bits */
> #define FAN_MARK_INODE          0
> #define FAN_MARK_MOUNT          FAN_MARK_TYPE_BIT1
> #define FAN_MARK_FILESYSTEM     FAN_MARK_TYPE_BIT2

Probably I would not go as far as defining FAN_MARK_TYPE_BIT?. That looks a
bit confusing and it's in userspace-visible headers. I'd just define the
mask and add it into FAN_ALL_MARK_FLAGS instead of FAN_MARK_MOUNT. That
should protect us (together with flags & supported-type checks in
do_fanotify_mark()) against messing up the definitions (at least I hope ;).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR