Re: [PATCH v6 4/4] fsnotify: don't merge events FS_OPEN_PERM and FS_OPEN_EXEC_PERM

[Jan Kara <jack@suse.cz>]

On Wed 07-11-18 13:30:55, Amir Goldstein wrote:
> On Wed, Nov 7, 2018 at 1:18 PM Matthew Bobrowski
> <mbobrowski@mbobrowski.org> wrote:
> >
> > Permission events are not to be consolidated into a single event mask.
> > In order for this to not happen, we require additional calls to
> > fsnotify_parent() and fsnotify() within the fsnotify_perm() when the
> > conditon to set FS_OPEN_EXEC_PERM is evaluated to true.
> >
> 
> That shouldn't be a separate patch. it should be squashed into the patch
> introducing FS_OPEN_EXEC_PERM there is no reason to have an
> interim commit where events are merged.

Agreed.

> > To simplify the code that provides this functionality a simple wrapper
> > fsnotify_path() has been defined to keep things nice and clean. Other
> > functions that used the same fsnotify_parent()/fsnotify() call
> > combination have been updated to use the simplified fsnotify_path()
> > wrapper.
> >
> 
> And this should be a separate re-factoring patch.

And agreed too. You can put this refactoring commit before the one
introducing FS_OPEN_EXEC_PERM to make your life simpler...

								Honza
> 
> > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> > ---
> >  include/linux/fsnotify.h | 48 ++++++++++++++++++++++------------------
> >  1 file changed, 27 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > index 9c7b594bf540..660ffc751352 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -26,6 +26,21 @@ static inline int fsnotify_parent(const struct path *path, struct dentry *dentry
> >         return __fsnotify_parent(path, dentry, mask);
> >  }
> >
> > +/*
> > + * Simple wrapper to consolidate calls fsnotify_parent()/fsnotify() when
> > + * an event is on a path.
> > + */
> > +static inline int fsnotify_path(struct inode *inode, const struct path *path,
> > +                               __u32 mask)
> > +{
> > +       int ret;
> > +
> > +       ret = fsnotify_parent(path, NULL, mask);
> > +       if (ret) return ret;
> > +
> > +       return fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0);
> > +}
> > +
> >  /* Simple call site for access decisions */
> >  static inline int fsnotify_perm(struct file *file, int mask)
> >  {
> > @@ -41,17 +56,15 @@ static inline int fsnotify_perm(struct file *file, int mask)
> >         if (mask & MAY_OPEN) {
> >                 fsnotify_mask = FS_OPEN_PERM;
> >
> > -               if (file->f_flags & __FMODE_EXEC)
> > -                       fsnotify_mask |= FS_OPEN_EXEC_PERM;
> > +               if (file->f_flags & __FMODE_EXEC) {
> > +                       ret = fsnotify_path(inode, path, FS_OPEN_EXEC_PERM);
> > +                       if (ret) return ret;
> 
> return should be in newline - just was just me hand writing a patch in email...
> 
> After making these small fixes, you may add to patches:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> Thanks,
> Amir.
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR