On Wed 07-11-18 13:30:55, Amir Goldstein wrote:
> On Wed, Nov 7, 2018 at 1:18 PM Matthew Bobrowski
> <mbobrowski@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> Thanks,
> Amir.
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
