Re: [PATCH v7 05/18] fsnotify: introduce pre-content permission events

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Nov 12, 2024 at 9:12 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@toxicpanda.com> wrote:
> >
> >  #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> > +static inline int fsnotify_pre_content(struct file *file)
> > +{
> > +       struct inode *inode = file_inode(file);
> > +
> > +       /*
> > +        * Pre-content events are only reported for regular files and dirs
> > +        * if there are any pre-content event watchers on this sb.
> > +        */
> > +       if ((!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode)) ||
> > +           !(inode->i_sb->s_iflags & SB_I_ALLOW_HSM) ||
> > +           !fsnotify_sb_has_priority_watchers(inode->i_sb,
> > +                                              FSNOTIFY_PRIO_PRE_CONTENT))
> > +               return 0;
> > +
> > +       return fsnotify_file(file, FS_PRE_ACCESS);
> > +}
>
> Yeah, no.
>
> None of this should check inode->i_sb->s_iflags at any point.
>
> The "is there a pre-content" thing should check one thing, and one
> thing only: that "is this file watched" flag.
> The whole indecipherable mess of inline functions that do random
> things in <linux/fsnotify.h> needs to be cleaned up, not made even
> more indecipherable.
>
> I'm NAKing this whole series until this is all sane and cleaned up,
> and I don't want to see a new hacky version being sent out tomorrow
> with just another layer of new hacks, with random new inline functions
> that call other inline functions and have complex odd conditionals
> that make no sense.
>
> Really. If the new hooks don't have that *SINGLE* bit test, they will
> not get merged.
>
> And that *SINGLE* bit test had better not be hidden under multiple
> layers of odd inline functions.
>
> You DO NOT get to use the same old broken complex function for the new
> hooks that then mix these odd helpers.
>
> This whole "add another crazy inline function using another crazy
> helper needs to STOP. Later on in the patch series you do
>
> +/*
> + * fsnotify_truncate_perm - permission hook before file truncate
> + */
> +static inline int fsnotify_truncate_perm(const struct path *path,
> loff_t length)
> +{
> +       return fsnotify_pre_content(path, &length, 0);
> +}
>
> or things like this:
>
> +static inline bool fsnotify_file_has_pre_content_watches(struct file *file)
> +{
> +       if (!(file->f_mode & FMODE_NOTIFY_PERM))
> +               return false;
> +
> +       if (!(file_inode(file)->i_sb->s_iflags & SB_I_ALLOW_HSM))
> +               return false;
> +
> +       return fsnotify_file_object_watched(file, FSNOTIFY_PRE_CONTENT_EVENTS);
> +}
>
> and no, NONE of that should be tested at runtime.
>
> I repeat: you should have *ONE* inline function that basically does
>
>  static inline bool fsnotify_file_watched(struct file *file)
>  {
>         return file && unlikely(file->f_mode & FMODE_NOTIFY_PERM);
>  }
>
> and absolutely nothing else. If that file is set, the file has
> notification events, and you go to an out-of-line slow case. You don't
> inline the unlikely cases after that.
>
> And you make sure that you only set that special bit on files and
> filesystems that support it. You most definitely don't check for
> SB_I_ALLOW_HSM kind of flags at runtime in critical code.

I understand your point. It makes sense.
But it requires using another FMODE_HSM flag,
because FMODE_NOTIFY_PERM covers also the legacy
FS_ACCESS_PERM event, which has different semantics
that I consider broken, but it is what it is.

I am fine not optimizing out the legacy FS_ACCESS_PERM event
and just making sure not to add new bad code, if that is what you prefer
and I also am fine with using two FMODE_ flags if that is prefered.

Thanks,
Amir.