| Message ID | 20e8e7afa5f42e6cf385277159dfd90505232897.1541587830.git.mbobrowski@mbobrowski.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: introduce new event masks FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM | expand |
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. > 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. > 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.
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.
On Wed, Nov 07, 2018 at 03:15:53PM +0100, Jan Kara wrote: > 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... OK, no problem. > > 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! I will send through v7 shortly.
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; + } } else if (mask & MAY_READ) { fsnotify_mask = FS_ACCESS_PERM; } - ret = fsnotify_parent(path, NULL, fsnotify_mask); - if (ret) - return ret; - - return fsnotify(inode, fsnotify_mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); + return fsnotify_path(inode, path, fsnotify_mask); } /* @@ -182,10 +195,8 @@ static inline void fsnotify_access(struct file *file) if (S_ISDIR(inode->i_mode)) mask |= FS_ISDIR; - if (!(file->f_mode & FMODE_NONOTIFY)) { - fsnotify_parent(path, NULL, mask); - fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); - } + if (!(file->f_mode & FMODE_NONOTIFY)) + fsnotify_path(inode, path, mask); } /* @@ -200,10 +211,8 @@ static inline void fsnotify_modify(struct file *file) if (S_ISDIR(inode->i_mode)) mask |= FS_ISDIR; - if (!(file->f_mode & FMODE_NONOTIFY)) { - fsnotify_parent(path, NULL, mask); - fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); - } + if (!(file->f_mode & FMODE_NONOTIFY)) + fsnotify_path(inode, path, mask); } /* @@ -220,8 +229,7 @@ static inline void fsnotify_open(struct file *file) if (file->f_flags & __FMODE_EXEC) mask |= FS_OPEN_EXEC; - fsnotify_parent(path, NULL, mask); - fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); + fsnotify_path(inode, path, mask); } /* @@ -237,10 +245,8 @@ static inline void fsnotify_close(struct file *file) if (S_ISDIR(inode->i_mode)) mask |= FS_ISDIR; - if (!(file->f_mode & FMODE_NONOTIFY)) { - fsnotify_parent(path, NULL, mask); - fsnotify(inode, mask, path, FSNOTIFY_EVENT_PATH, NULL, 0); - } + if (!(file->f_mode & FMODE_NONOTIFY)) + fsnotify_path(inode, path, mask); } /*
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. 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. Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> --- include/linux/fsnotify.h | 48 ++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 21 deletions(-)