Message ID | 20210629191035.681913-8-krisman@collabora.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | File system wide monitoring | expand |
Hi Gabriel, I love your patch! Yet something to improve: [auto build test ERROR on ext3/fsnotify] [also build test ERROR on ext4/dev linus/master v5.13 next-20210629] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210630-031347 base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify config: i386-tinyconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/dca640915c7b840656b052e138effd501bd5d5e1 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210630-031347 git checkout dca640915c7b840656b052e138effd501bd5d5e1 # save the attached .config to linux build tree make W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from fs/open.c:12: include/linux/fsnotify.h: In function 'fsnotify_name': >> include/linux/fsnotify.h:33:2: error: implicit declaration of function '__fsnotify'; did you mean 'fsnotify'? [-Werror=implicit-function-declaration] 33 | __fsnotify(mask, &(struct fsnotify_event_info) { | ^~~~~~~~~~ | fsnotify cc1: some warnings being treated as errors vim +33 include/linux/fsnotify.h 19 20 /* 21 * Notify this @dir inode about a change in a child directory entry. 22 * The directory entry may have turned positive or negative or its inode may 23 * have changed (i.e. renamed over). 24 * 25 * Unlike fsnotify_parent(), the event will be reported regardless of the 26 * FS_EVENT_ON_CHILD mask on the parent inode and will not be reported if only 27 * the child is interested and not the parent. 28 */ 29 static inline void fsnotify_name(struct inode *dir, __u32 mask, 30 struct inode *child, 31 const struct qstr *name, u32 cookie) 32 { > 33 __fsnotify(mask, &(struct fsnotify_event_info) { 34 .data = child, .data_type = FSNOTIFY_EVENT_INODE, 35 .dir = dir, .name = name, .cookie = cookie, 36 }); 37 } 38 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Gabriel, I love your patch! Yet something to improve: [auto build test ERROR on ext3/fsnotify] [also build test ERROR on ext4/dev linus/master v5.13 next-20210629] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210630-031347 base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify config: nios2-defconfig (attached as .config) compiler: nios2-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/dca640915c7b840656b052e138effd501bd5d5e1 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210630-031347 git checkout dca640915c7b840656b052e138effd501bd5d5e1 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from fs/kernfs/file.c:16: include/linux/fsnotify.h: In function 'fsnotify_name': include/linux/fsnotify.h:33:2: error: implicit declaration of function '__fsnotify'; did you mean 'fsnotify'? [-Werror=implicit-function-declaration] 33 | __fsnotify(mask, &(struct fsnotify_event_info) { | ^~~~~~~~~~ | fsnotify fs/kernfs/file.c: In function 'kernfs_notify_workfn': >> fs/kernfs/file.c:887:7: error: passing argument 2 of 'fsnotify' from incompatible pointer type [-Werror=incompatible-pointer-types] 887 | inode, FSNOTIFY_EVENT_INODE, | ^~~~~ | | | struct inode * In file included from include/linux/fsnotify.h:15, from fs/kernfs/file.c:16: include/linux/fsnotify_backend.h:636:41: note: expected 'const struct fsnotify_event_info *' but argument is of type 'struct inode *' 636 | const struct fsnotify_event_info *event_info) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~ >> fs/kernfs/file.c:886:5: error: too many arguments to function 'fsnotify' 886 | fsnotify(FS_MODIFY | FS_EVENT_ON_CHILD, | ^~~~~~~~ In file included from include/linux/fsnotify.h:15, from fs/kernfs/file.c:16: include/linux/fsnotify_backend.h:635:19: note: declared here 635 | static inline int fsnotify(__u32 mask, | ^~~~~~~~ cc1: some warnings being treated as errors vim +/fsnotify +887 fs/kernfs/file.c 414985ae23c031 Tejun Heo 2013-11-28 845 ecca47ce829484 Tejun Heo 2014-07-01 846 static void kernfs_notify_workfn(struct work_struct *work) 414985ae23c031 Tejun Heo 2013-11-28 847 { ecca47ce829484 Tejun Heo 2014-07-01 848 struct kernfs_node *kn; d911d98748018f Tejun Heo 2014-04-09 849 struct kernfs_super_info *info; ecca47ce829484 Tejun Heo 2014-07-01 850 repeat: ecca47ce829484 Tejun Heo 2014-07-01 851 /* pop one off the notify_list */ ecca47ce829484 Tejun Heo 2014-07-01 852 spin_lock_irq(&kernfs_notify_lock); ecca47ce829484 Tejun Heo 2014-07-01 853 kn = kernfs_notify_list; ecca47ce829484 Tejun Heo 2014-07-01 854 if (kn == KERNFS_NOTIFY_EOL) { ecca47ce829484 Tejun Heo 2014-07-01 855 spin_unlock_irq(&kernfs_notify_lock); d911d98748018f Tejun Heo 2014-04-09 856 return; ecca47ce829484 Tejun Heo 2014-07-01 857 } ecca47ce829484 Tejun Heo 2014-07-01 858 kernfs_notify_list = kn->attr.notify_next; ecca47ce829484 Tejun Heo 2014-07-01 859 kn->attr.notify_next = NULL; ecca47ce829484 Tejun Heo 2014-07-01 860 spin_unlock_irq(&kernfs_notify_lock); d911d98748018f Tejun Heo 2014-04-09 861 d911d98748018f Tejun Heo 2014-04-09 862 /* kick fsnotify */ d911d98748018f Tejun Heo 2014-04-09 863 mutex_lock(&kernfs_mutex); d911d98748018f Tejun Heo 2014-04-09 864 ecca47ce829484 Tejun Heo 2014-07-01 865 list_for_each_entry(info, &kernfs_root(kn)->supers, node) { df6a58c5c5aa8e Tejun Heo 2016-06-17 866 struct kernfs_node *parent; 497b0c5a7c0688 Amir Goldstein 2020-07-16 867 struct inode *p_inode = NULL; d911d98748018f Tejun Heo 2014-04-09 868 struct inode *inode; 25b229dff4ffff Al Viro 2019-04-26 869 struct qstr name; d911d98748018f Tejun Heo 2014-04-09 870 df6a58c5c5aa8e Tejun Heo 2016-06-17 871 /* df6a58c5c5aa8e Tejun Heo 2016-06-17 872 * We want fsnotify_modify() on @kn but as the df6a58c5c5aa8e Tejun Heo 2016-06-17 873 * modifications aren't originating from userland don't df6a58c5c5aa8e Tejun Heo 2016-06-17 874 * have the matching @file available. Look up the inodes df6a58c5c5aa8e Tejun Heo 2016-06-17 875 * and generate the events manually. df6a58c5c5aa8e Tejun Heo 2016-06-17 876 */ 67c0496e87d193 Tejun Heo 2019-11-04 877 inode = ilookup(info->sb, kernfs_ino(kn)); d911d98748018f Tejun Heo 2014-04-09 878 if (!inode) d911d98748018f Tejun Heo 2014-04-09 879 continue; d911d98748018f Tejun Heo 2014-04-09 880 25b229dff4ffff Al Viro 2019-04-26 881 name = (struct qstr)QSTR_INIT(kn->name, strlen(kn->name)); df6a58c5c5aa8e Tejun Heo 2016-06-17 882 parent = kernfs_get_parent(kn); df6a58c5c5aa8e Tejun Heo 2016-06-17 883 if (parent) { 67c0496e87d193 Tejun Heo 2019-11-04 884 p_inode = ilookup(info->sb, kernfs_ino(parent)); df6a58c5c5aa8e Tejun Heo 2016-06-17 885 if (p_inode) { 40a100d3adc1ad Amir Goldstein 2020-07-22 @886 fsnotify(FS_MODIFY | FS_EVENT_ON_CHILD, 40a100d3adc1ad Amir Goldstein 2020-07-22 @887 inode, FSNOTIFY_EVENT_INODE, 40a100d3adc1ad Amir Goldstein 2020-07-22 888 p_inode, &name, inode, 0); df6a58c5c5aa8e Tejun Heo 2016-06-17 889 iput(p_inode); d911d98748018f Tejun Heo 2014-04-09 890 } d911d98748018f Tejun Heo 2014-04-09 891 df6a58c5c5aa8e Tejun Heo 2016-06-17 892 kernfs_put(parent); df6a58c5c5aa8e Tejun Heo 2016-06-17 893 } df6a58c5c5aa8e Tejun Heo 2016-06-17 894 82ace1efb3cb1d Amir Goldstein 2020-07-22 895 if (!p_inode) 82ace1efb3cb1d Amir Goldstein 2020-07-22 896 fsnotify_inode(inode, FS_MODIFY); 497b0c5a7c0688 Amir Goldstein 2020-07-16 897 d911d98748018f Tejun Heo 2014-04-09 898 iput(inode); d911d98748018f Tejun Heo 2014-04-09 899 } d911d98748018f Tejun Heo 2014-04-09 900 d911d98748018f Tejun Heo 2014-04-09 901 mutex_unlock(&kernfs_mutex); ecca47ce829484 Tejun Heo 2014-07-01 902 kernfs_put(kn); ecca47ce829484 Tejun Heo 2014-07-01 903 goto repeat; ecca47ce829484 Tejun Heo 2014-07-01 904 } ecca47ce829484 Tejun Heo 2014-07-01 905 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
() On Tue, Jun 29, 2021 at 10:12 PM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > From: Amir Goldstein <amir73il@gmail.com> > > There are a lot of arguments to fsnotify() and the handle_event() method. > Pass them in a const struct instead of on the argument list. > > Apart from being more tidy, this helps with passing error reports to the > backend. __fsnotify_parent() argument list was intentionally left > untouched, because its argument list is still short enough and because > most of the event info arguments are initialized inside > __fsnotify_parent(). > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify.c | 59 +++++++++++------------ > fs/notify/fsnotify.c | 83 +++++++++++++++++--------------- > include/linux/fsnotify.h | 15 ++++-- > include/linux/fsnotify_backend.h | 73 +++++++++++++++++++++------- > 4 files changed, 140 insertions(+), 90 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 0f760770d4c9..4f2febb15e94 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -253,21 +253,22 @@ static int fanotify_get_response(struct fsnotify_group *group, > * been included within the event mask, but have not been explicitly > * requested by the user, will not be present in the returned mask. > */ > -static u32 fanotify_group_event_mask(struct fsnotify_group *group, > - struct fsnotify_iter_info *iter_info, > - u32 event_mask, const void *data, > - int data_type, struct inode *dir) > +static u32 fanotify_group_event_mask( > + struct fsnotify_group *group, u32 event_mask, > + const struct fsnotify_event_info *event_info, > + struct fsnotify_iter_info *iter_info) > { > __u32 marks_mask = 0, marks_ignored_mask = 0; > __u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS | > FANOTIFY_EVENT_FLAGS; > - const struct path *path = fsnotify_data_path(data, data_type); > + const struct path *path = fsnotify_event_info_path(event_info); > unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS); > struct fsnotify_mark *mark; > int type; > > pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n", > - __func__, iter_info->report_mask, event_mask, data, data_type); > + __func__, iter_info->report_mask, event_mask, > + event_info->data, event_info->data_type); > > if (!fid_mode) { > /* Do we have path to open a file descriptor? */ > @@ -278,7 +279,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, > return 0; > } else if (!(fid_mode & FAN_REPORT_FID)) { > /* Do we have a directory inode to report? */ > - if (!dir && !(event_mask & FS_ISDIR)) > + if (!event_info->dir && !(event_mask & FS_ISDIR)) > return 0; > } > > @@ -427,13 +428,13 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > * FS_ATTRIB reports the child inode even if reported on a watched parent. > * FS_CREATE reports the modified dir inode and not the created inode. > */ > -static struct inode *fanotify_fid_inode(u32 event_mask, const void *data, > - int data_type, struct inode *dir) > +static struct inode *fanotify_fid_inode(u32 event_mask, > + const struct fsnotify_event_info *event_info) > { > if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) > - return dir; > + return event_info->dir; > > - return fsnotify_data_inode(data, data_type); > + return fsnotify_event_info_inode(event_info); > } > > /* > @@ -444,18 +445,18 @@ static struct inode *fanotify_fid_inode(u32 event_mask, const void *data, > * reported to parent. > * Otherwise, do not report dir fid. > */ > -static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data, > - int data_type, struct inode *dir) > +static struct inode *fanotify_dfid_inode(u32 event_mask, > + const struct fsnotify_event_info *event_info) > { > - struct inode *inode = fsnotify_data_inode(data, data_type); > + struct inode *inode = fsnotify_event_info_inode(event_info); > > if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) > - return dir; > + return event_info->dir; > > if (S_ISDIR(inode->i_mode)) > return inode; > > - return dir; > + return event_info->dir; > } > > static struct fanotify_event *fanotify_alloc_path_event(const struct path *path, > @@ -563,17 +564,17 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > return &fne->fae; > } > > -static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, > - u32 mask, const void *data, > - int data_type, struct inode *dir, > - const struct qstr *file_name, > - __kernel_fsid_t *fsid) > +static struct fanotify_event *fanotify_alloc_event( > + struct fsnotify_group *group, u32 mask, > + const struct fsnotify_event_info *event_info, > + __kernel_fsid_t *fsid) > { > struct fanotify_event *event = NULL; > gfp_t gfp = GFP_KERNEL_ACCOUNT; > - struct inode *id = fanotify_fid_inode(mask, data, data_type, dir); > - struct inode *dirid = fanotify_dfid_inode(mask, data, data_type, dir); > - const struct path *path = fsnotify_data_path(data, data_type); > + struct inode *id = fanotify_fid_inode(mask, event_info); > + struct inode *dirid = fanotify_dfid_inode(mask, event_info); > + const struct path *path = fsnotify_event_info_path(event_info); > + const struct qstr *file_name = event_info->name; > unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS); > struct mem_cgroup *old_memcg; > struct inode *child = NULL; > @@ -712,9 +713,7 @@ static void fanotify_insert_event(struct fsnotify_group *group, > } > > static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, > - const void *data, int data_type, > - struct inode *dir, > - const struct qstr *file_name, u32 cookie, > + const struct fsnotify_event_info *event_info, > struct fsnotify_iter_info *iter_info) > { > int ret = 0; > @@ -744,8 +743,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, > > BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19); > > - mask = fanotify_group_event_mask(group, iter_info, mask, data, > - data_type, dir); > + mask = fanotify_group_event_mask(group, mask, event_info, iter_info); > if (!mask) > return 0; > > @@ -767,8 +765,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, > return 0; > } > > - event = fanotify_alloc_event(group, mask, data, data_type, dir, > - file_name, &fsid); > + event = fanotify_alloc_event(group, mask, event_info, &fsid); > ret = -ENOMEM; > if (unlikely(!event)) { > /* > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 30d422b8c0fc..36205a769dde 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -177,8 +177,8 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt, > * Notify only the child without name info if parent is not watching and > * inode/sb/mount are not interested in events with parent and name info. > */ > -int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, > - int data_type) > +int __fsnotify_parent(struct dentry *dentry, __u32 mask, > + const void *data, int data_type) > { > const struct path *path = fsnotify_data_path(data, data_type); > struct mount *mnt = path ? real_mount(path->mnt) : NULL; > @@ -229,7 +229,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, > } > > notify: > - ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0); > + ret = __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = data, .data_type = data_type, > + .dir = p_inode, .name = file_name, > + .inode = inode, > + }); > > if (file_name) > release_dentry_name_snapshot(&name); > @@ -240,13 +244,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, > EXPORT_SYMBOL_GPL(__fsnotify_parent); > > static int fsnotify_handle_inode_event(struct fsnotify_group *group, > - struct fsnotify_mark *inode_mark, > - u32 mask, const void *data, int data_type, > - struct inode *dir, const struct qstr *name, > - u32 cookie) > + struct fsnotify_mark *inode_mark, u32 mask, > + const struct fsnotify_event_info *event_info) > { > - const struct path *path = fsnotify_data_path(data, data_type); > - struct inode *inode = fsnotify_data_inode(data, data_type); > + const struct path *path = fsnotify_event_info_path(event_info); > + struct inode *inode = fsnotify_event_info_inode(event_info); > const struct fsnotify_ops *ops = group->ops; > > if (WARN_ON_ONCE(!ops->handle_inode_event)) > @@ -260,16 +262,17 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, > if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS)) > return 0; > > - return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); > + return ops->handle_inode_event(inode_mark, mask, inode, event_info->dir, > + event_info->name, event_info->cookie); > } > > static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > - const void *data, int data_type, > - struct inode *dir, const struct qstr *name, > - u32 cookie, struct fsnotify_iter_info *iter_info) > + const struct fsnotify_event_info *event_info, > + struct fsnotify_iter_info *iter_info) > { > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); > struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); > + struct fsnotify_event_info child_event_info = { }; > int ret; > > if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || > @@ -284,8 +287,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > * interested in this event? > */ > if (parent_mark->mask & FS_EVENT_ON_CHILD) { > - ret = fsnotify_handle_inode_event(group, parent_mark, mask, > - data, data_type, dir, name, 0); > + ret = fsnotify_handle_inode_event(group, parent_mark, > + mask, event_info); > if (ret) > return ret; > } > @@ -302,18 +305,22 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > * The child watcher is expecting an event without a file name > * and without the FS_EVENT_ON_CHILD flag. > */ > + if (WARN_ON_ONCE(!event_info->inode)) > + return 0; > + > mask &= ~FS_EVENT_ON_CHILD; > - dir = NULL; > - name = NULL; > + child_event_info = *event_info; > + child_event_info.dir = NULL; > + child_event_info.name = NULL; > + event_info = &child_event_info; > } > > - return fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type, > - dir, name, cookie); > + return fsnotify_handle_inode_event(group, inode_mark, mask, event_info); > } > > -static int send_to_group(__u32 mask, const void *data, int data_type, > - struct inode *dir, const struct qstr *file_name, > - u32 cookie, struct fsnotify_iter_info *iter_info) > +static int send_to_group(__u32 mask, > + const struct fsnotify_event_info *event_info, > + struct fsnotify_iter_info *iter_info) > { > struct fsnotify_group *group = NULL; > __u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS); > @@ -351,18 +358,18 @@ static int send_to_group(__u32 mask, const void *data, int data_type, > > pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n", > __func__, group, mask, marks_mask, marks_ignored_mask, > - data, data_type, dir, cookie); > + event_info->data, event_info->data_type, event_info->dir, > + event_info->cookie); > > if (!(test_mask & marks_mask & ~marks_ignored_mask)) > return 0; > > if (group->ops->handle_event) { > - return group->ops->handle_event(group, mask, data, data_type, dir, > - file_name, cookie, iter_info); > + return group->ops->handle_event(group, mask, event_info, > + iter_info); > } > > - return fsnotify_handle_event(group, mask, data, data_type, dir, > - file_name, cookie, iter_info); > + return fsnotify_handle_event(group, mask, event_info, iter_info); > } > > static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp) > @@ -448,21 +455,22 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) > * in whatever means they feel necessary. > * > * @mask: event type and flags > + * Input args in struct fsnotify_event_info: > * @data: object that event happened on > * @data_type: type of object for fanotify_data_XXX() accessors > * @dir: optional directory associated with event - > - * if @file_name is not NULL, this is the directory that > - * @file_name is relative to > - * @file_name: optional file name associated with event > + * if @name is not NULL, this is the directory that > + * @name is relative to > + * @name: optional file name associated with event > * @inode: optional inode associated with event - > * either @dir or @inode must be non-NULL. > * if both are non-NULL event may be reported to both. > * @cookie: inotify rename cookie > */ > -int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, > - const struct qstr *file_name, struct inode *inode, u32 cookie) > +int __fsnotify(__u32 mask, const struct fsnotify_event_info *event_info) > { > - const struct path *path = fsnotify_data_path(data, data_type); > + const struct path *path = fsnotify_event_info_path(event_info); > + struct inode *inode = event_info->inode; > struct fsnotify_iter_info iter_info = {}; > struct super_block *sb; > struct mount *mnt = NULL; > @@ -475,13 +483,13 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, > > if (!inode) { > /* Dirent event - report on TYPE_INODE to dir */ > - inode = dir; > + inode = event_info->dir; > } else if (mask & FS_EVENT_ON_CHILD) { > /* > * Event on child - report on TYPE_PARENT to dir if it is > * watching children and on TYPE_INODE to child. > */ > - parent = dir; > + parent = event_info->dir; > } > sb = inode->i_sb; > > @@ -538,8 +546,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, > * That's why this traversal is so complicated... > */ > while (fsnotify_iter_select_report_types(&iter_info)) { > - ret = send_to_group(mask, data, data_type, dir, file_name, > - cookie, &iter_info); > + ret = send_to_group(mask, event_info, &iter_info); > > if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS)) > goto out; > @@ -552,7 +559,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, > > return ret; > } > -EXPORT_SYMBOL_GPL(fsnotify); > +EXPORT_SYMBOL_GPL(__fsnotify); > > static __init int fsnotify_init(void) > { > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index f8acddcf54fb..8c2c681b4495 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -30,7 +30,10 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask, > struct inode *child, > const struct qstr *name, u32 cookie) > { > - fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie); > + __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = child, .data_type = FSNOTIFY_EVENT_INODE, > + .dir = dir, .name = name, .cookie = cookie, > + }); > } > > static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry, > @@ -44,7 +47,10 @@ static inline void fsnotify_inode(struct inode *inode, __u32 mask) > if (S_ISDIR(inode->i_mode)) > mask |= FS_ISDIR; > > - fsnotify(mask, inode, FSNOTIFY_EVENT_INODE, NULL, NULL, inode, 0); > + __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = inode, .data_type = FSNOTIFY_EVENT_INODE, > + .inode = inode, > + }); > } > > /* Notify this dentry's parent about a child's events. */ > @@ -68,7 +74,10 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask, > return __fsnotify_parent(dentry, mask, data, data_type); > > notify_child: > - return fsnotify(mask, data, data_type, NULL, NULL, inode, 0); > + return __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = data, .data_type = data_type, > + .inode = inode, > + }); > } > > /* > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index f9e2c6cd0f7d..b1590f654ade 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h > @@ -112,6 +112,28 @@ struct fsnotify_iter_info; > > struct mem_cgroup; > > +/* > + * Event info args passed to fsnotify() and to backends on handle_event(): > + * @data: object that event happened on > + * @data_type: type of object for fanotify_data_XXX() accessors > + * @dir: optional directory associated with event - > + * if @name is not NULL, this is the directory that > + * @name is relative to > + * @name: optional file name associated with event > + * @inode: optional inode associated with event - > + * either @dir or @inode must be non-NULL. > + * if both are non-NULL event may be reported to both. > + * @cookie: inotify rename cookie > + */ > +struct fsnotify_event_info { > + const void *data; > + int data_type; > + struct inode *dir; > + const struct qstr *name; > + struct inode *inode; > + u32 cookie; > +}; > + > /* > * Each group much define these ops. The fsnotify infrastructure will call > * these operations for each relevant group. > @@ -119,13 +141,7 @@ struct mem_cgroup; > * handle_event - main call for a group to handle an fs event > * @group: group to notify > * @mask: event type and flags > - * @data: object that event happened on > - * @data_type: type of object for fanotify_data_XXX() accessors > - * @dir: optional directory associated with event - > - * if @file_name is not NULL, this is the directory that > - * @file_name is relative to > - * @file_name: optional file name associated with event > - * @cookie: inotify rename cookie > + * @event_info: information attached to the event > * @iter_info: array of marks from this group that are interested in the event > * > * handle_inode_event - simple variant of handle_event() for groups that only > @@ -147,8 +163,7 @@ struct mem_cgroup; > */ > struct fsnotify_ops { > int (*handle_event)(struct fsnotify_group *group, u32 mask, > - const void *data, int data_type, struct inode *dir, > - const struct qstr *file_name, u32 cookie, > + const struct fsnotify_event_info *event_info, > struct fsnotify_iter_info *iter_info); > int (*handle_inode_event)(struct fsnotify_mark *mark, u32 mask, > struct inode *inode, struct inode *dir, > @@ -262,6 +277,12 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type) > } > } > > +static inline struct inode *fsnotify_event_info_inode( > + const struct fsnotify_event_info *event_info) > +{ > + return fsnotify_data_inode(event_info->data, event_info->data_type); > +} > + > static inline const struct path *fsnotify_data_path(const void *data, > int data_type) > { > @@ -273,6 +294,12 @@ static inline const struct path *fsnotify_data_path(const void *data, > } > } > > +static inline const struct path *fsnotify_event_info_path( > + const struct fsnotify_event_info *event_info) > +{ > + return fsnotify_data_path(event_info->data, event_info->data_type); > +} > + > enum fsnotify_obj_type { > FSNOTIFY_OBJ_TYPE_INODE, > FSNOTIFY_OBJ_TYPE_PARENT, > @@ -410,11 +437,22 @@ struct fsnotify_mark { > /* called from the vfs helpers */ > > /* main fsnotify call to send events */ > -extern int fsnotify(__u32 mask, const void *data, int data_type, > - struct inode *dir, const struct qstr *name, > - struct inode *inode, u32 cookie); > -extern int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, > - int data_type); > +extern int __fsnotify(__u32 mask, > + const struct fsnotify_event_info *event_info); > +extern int __fsnotify_parent(struct dentry *dentry, __u32 mask, > + const void *data, int data_type); > + > +static inline int fsnotify(__u32 mask, const void *data, int data_type, > + struct inode *dir, const struct qstr *name, > + struct inode *inode, u32 cookie) > +{ > + return __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = data, .data_type = data_type, > + .dir = dir, .name = name, .inode = inode, > + .cookie = cookie, > + }); > +} > + > extern void __fsnotify_inode_delete(struct inode *inode); > extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt); > extern void fsnotify_sb_delete(struct super_block *sb); > @@ -594,15 +632,14 @@ static inline void fsnotify_init_event(struct fsnotify_event *event) > > #else > > -static inline int fsnotify(__u32 mask, const void *data, int data_type, > - struct inode *dir, const struct qstr *name, > - struct inode *inode, u32 cookie) > +static inline int fsnotify(__u32 mask, > + const struct fsnotify_event_info *event_info) > { > return 0; > } > Did you miss the kernel test robot messages on v2? These noop implementations in my patch are wrong. noop implementation of fsnotify() should not change signature and noop implementation of __fsnotify() should be added. Thanks, Amir. > static inline int __fsnotify_parent(struct dentry *dentry, __u32 mask, > - const void *data, int data_type) > + const void *data, int data_type) > { > return 0; > } > -- > 2.32.0 >
Hi Gabriel, url: https://github.com/0day-ci/linux/commits/Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210630-031347 base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify config: x86_64-randconfig-m001-20210628 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: fs/notify/fsnotify.c:505 __fsnotify() warn: variable dereferenced before check 'inode' (see line 494) vim +/inode +505 fs/notify/fsnotify.c dca640915c7b84 Amir Goldstein 2021-06-29 470 int __fsnotify(__u32 mask, const struct fsnotify_event_info *event_info) 90586523eb4b34 Eric Paris 2009-05-21 471 { dca640915c7b84 Amir Goldstein 2021-06-29 472 const struct path *path = fsnotify_event_info_path(event_info); dca640915c7b84 Amir Goldstein 2021-06-29 473 struct inode *inode = event_info->inode; 3427ce71554123 Miklos Szeredi 2017-10-30 474 struct fsnotify_iter_info iter_info = {}; 40a100d3adc1ad Amir Goldstein 2020-07-22 475 struct super_block *sb; 60f7ed8c7c4d06 Amir Goldstein 2018-09-01 476 struct mount *mnt = NULL; fecc4559780d52 Amir Goldstein 2020-12-02 477 struct inode *parent = NULL; 9385a84d7e1f65 Jan Kara 2016-11-10 478 int ret = 0; 71d734103edfa2 Mel Gorman 2020-07-08 479 __u32 test_mask, marks_mask; 90586523eb4b34 Eric Paris 2009-05-21 480 71d734103edfa2 Mel Gorman 2020-07-08 481 if (path) aa93bdc5500cc9 Amir Goldstein 2020-03-19 482 mnt = real_mount(path->mnt); 3a9fb89f4cd04c Eric Paris 2009-12-17 483 40a100d3adc1ad Amir Goldstein 2020-07-22 484 if (!inode) { 40a100d3adc1ad Amir Goldstein 2020-07-22 485 /* Dirent event - report on TYPE_INODE to dir */ dca640915c7b84 Amir Goldstein 2021-06-29 486 inode = event_info->dir; ^^^^^^^^^^^^^^^^^^^^^^^ Presumably this is non-NULL 40a100d3adc1ad Amir Goldstein 2020-07-22 487 } else if (mask & FS_EVENT_ON_CHILD) { 40a100d3adc1ad Amir Goldstein 2020-07-22 488 /* fecc4559780d52 Amir Goldstein 2020-12-02 489 * Event on child - report on TYPE_PARENT to dir if it is fecc4559780d52 Amir Goldstein 2020-12-02 490 * watching children and on TYPE_INODE to child. 40a100d3adc1ad Amir Goldstein 2020-07-22 491 */ dca640915c7b84 Amir Goldstein 2021-06-29 492 parent = event_info->dir; 40a100d3adc1ad Amir Goldstein 2020-07-22 493 } 40a100d3adc1ad Amir Goldstein 2020-07-22 @494 sb = inode->i_sb; ^^^^^^^^^^^^ Dereference 497b0c5a7c0688 Amir Goldstein 2020-07-16 495 7c49b8616460eb Dave Hansen 2015-09-04 496 /* 7c49b8616460eb Dave Hansen 2015-09-04 497 * Optimization: srcu_read_lock() has a memory barrier which can 7c49b8616460eb Dave Hansen 2015-09-04 498 * be expensive. It protects walking the *_fsnotify_marks lists. 7c49b8616460eb Dave Hansen 2015-09-04 499 * However, if we do not walk the lists, we do not have to do 7c49b8616460eb Dave Hansen 2015-09-04 500 * SRCU because we have no references to any objects and do not 7c49b8616460eb Dave Hansen 2015-09-04 501 * need SRCU to keep them "alive". 7c49b8616460eb Dave Hansen 2015-09-04 502 */ 9b93f33105f5f9 Amir Goldstein 2020-07-16 503 if (!sb->s_fsnotify_marks && 497b0c5a7c0688 Amir Goldstein 2020-07-16 504 (!mnt || !mnt->mnt_fsnotify_marks) && 9b93f33105f5f9 Amir Goldstein 2020-07-16 @505 (!inode || !inode->i_fsnotify_marks) && ^^^^^^ Unnecessary check for NULL fecc4559780d52 Amir Goldstein 2020-12-02 506 (!parent || !parent->i_fsnotify_marks)) 7c49b8616460eb Dave Hansen 2015-09-04 507 return 0; 71d734103edfa2 Mel Gorman 2020-07-08 508 9b93f33105f5f9 Amir Goldstein 2020-07-16 509 marks_mask = sb->s_fsnotify_mask; 71d734103edfa2 Mel Gorman 2020-07-08 510 if (mnt) 71d734103edfa2 Mel Gorman 2020-07-08 511 marks_mask |= mnt->mnt_fsnotify_mask; 9b93f33105f5f9 Amir Goldstein 2020-07-16 512 if (inode) ^^^^^^ 9b93f33105f5f9 Amir Goldstein 2020-07-16 513 marks_mask |= inode->i_fsnotify_mask; fecc4559780d52 Amir Goldstein 2020-12-02 514 if (parent) fecc4559780d52 Amir Goldstein 2020-12-02 515 marks_mask |= parent->i_fsnotify_mask; 497b0c5a7c0688 Amir Goldstein 2020-07-16 516 71d734103edfa2 Mel Gorman 2020-07-08 517 613a807fe7c793 Eric Paris 2010-07-28 518 /* 613a807fe7c793 Eric Paris 2010-07-28 519 * if this is a modify event we may need to clear the ignored masks 497b0c5a7c0688 Amir Goldstein 2020-07-16 520 * otherwise return if none of the marks care about this type of event. 613a807fe7c793 Eric Paris 2010-07-28 521 */ 71d734103edfa2 Mel Gorman 2020-07-08 522 test_mask = (mask & ALL_FSNOTIFY_EVENTS); 71d734103edfa2 Mel Gorman 2020-07-08 523 if (!(mask & FS_MODIFY) && !(test_mask & marks_mask)) 613a807fe7c793 Eric Paris 2010-07-28 524 return 0; 75c1be487a690d Eric Paris 2010-07-28 525 9385a84d7e1f65 Jan Kara 2016-11-10 526 iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu); 75c1be487a690d Eric Paris 2010-07-28 527 45a9fb3725d886 Amir Goldstein 2019-01-10 528 iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] = 45a9fb3725d886 Amir Goldstein 2019-01-10 529 fsnotify_first_mark(&sb->s_fsnotify_marks); 9bdda4e9cf2dce Amir Goldstein 2018-09-01 530 if (mnt) { 47d9c7cc457adc Amir Goldstein 2018-04-20 531 iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] = 3427ce71554123 Miklos Szeredi 2017-10-30 532 fsnotify_first_mark(&mnt->mnt_fsnotify_marks); 7131485a93679f Eric Paris 2009-12-17 533 } 9b93f33105f5f9 Amir Goldstein 2020-07-16 534 if (inode) { ^^^^^ Lots of checking... Maybe this is really NULL? 9b93f33105f5f9 Amir Goldstein 2020-07-16 535 iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] = 9b93f33105f5f9 Amir Goldstein 2020-07-16 536 fsnotify_first_mark(&inode->i_fsnotify_marks); 9b93f33105f5f9 Amir Goldstein 2020-07-16 537 } fecc4559780d52 Amir Goldstein 2020-12-02 538 if (parent) { fecc4559780d52 Amir Goldstein 2020-12-02 539 iter_info.marks[FSNOTIFY_OBJ_TYPE_PARENT] = fecc4559780d52 Amir Goldstein 2020-12-02 540 fsnotify_first_mark(&parent->i_fsnotify_marks); 497b0c5a7c0688 Amir Goldstein 2020-07-16 541 } 75c1be487a690d Eric Paris 2010-07-28 542 8edc6e1688fc8f Jan Kara 2014-11-13 543 /* 60f7ed8c7c4d06 Amir Goldstein 2018-09-01 544 * We need to merge inode/vfsmount/sb mark lists so that e.g. inode mark 60f7ed8c7c4d06 Amir Goldstein 2018-09-01 545 * ignore masks are properly reflected for mount/sb mark notifications. 8edc6e1688fc8f Jan Kara 2014-11-13 546 * That's why this traversal is so complicated... 8edc6e1688fc8f Jan Kara 2014-11-13 547 */ d9a6f30bb89309 Amir Goldstein 2018-04-20 548 while (fsnotify_iter_select_report_types(&iter_info)) { dca640915c7b84 Amir Goldstein 2021-06-29 549 ret = send_to_group(mask, event_info, &iter_info); 613a807fe7c793 Eric Paris 2010-07-28 550 ff8bcbd03da881 Eric Paris 2010-10-28 551 if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS)) ff8bcbd03da881 Eric Paris 2010-10-28 552 goto out; ff8bcbd03da881 Eric Paris 2010-10-28 553 d9a6f30bb89309 Amir Goldstein 2018-04-20 554 fsnotify_iter_next(&iter_info); 90586523eb4b34 Eric Paris 2009-05-21 555 } ff8bcbd03da881 Eric Paris 2010-10-28 556 ret = 0; ff8bcbd03da881 Eric Paris 2010-10-28 557 out: 9385a84d7e1f65 Jan Kara 2016-11-10 558 srcu_read_unlock(&fsnotify_mark_srcu, iter_info.srcu_idx); c4ec54b40d33f8 Eric Paris 2009-12-17 559 98b5c10d320adf Jean-Christophe Dubois 2010-03-23 560 return ret; 90586523eb4b34 Eric Paris 2009-05-21 561 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Jun 30, 2021 at 11:12 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > Hi Gabriel, > > url: https://github.com/0day-ci/linux/commits/Gabriel-Krisman-Bertazi/File-system-wide-monitoring/20210630-031347 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify > config: x86_64-randconfig-m001-20210628 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > smatch warnings: > fs/notify/fsnotify.c:505 __fsnotify() warn: variable dereferenced before check 'inode' (see line 494) > > vim +/inode +505 fs/notify/fsnotify.c > > dca640915c7b84 Amir Goldstein 2021-06-29 470 int __fsnotify(__u32 mask, const struct fsnotify_event_info *event_info) > 90586523eb4b34 Eric Paris 2009-05-21 471 { > dca640915c7b84 Amir Goldstein 2021-06-29 472 const struct path *path = fsnotify_event_info_path(event_info); > dca640915c7b84 Amir Goldstein 2021-06-29 473 struct inode *inode = event_info->inode; > 3427ce71554123 Miklos Szeredi 2017-10-30 474 struct fsnotify_iter_info iter_info = {}; > 40a100d3adc1ad Amir Goldstein 2020-07-22 475 struct super_block *sb; > 60f7ed8c7c4d06 Amir Goldstein 2018-09-01 476 struct mount *mnt = NULL; > fecc4559780d52 Amir Goldstein 2020-12-02 477 struct inode *parent = NULL; > 9385a84d7e1f65 Jan Kara 2016-11-10 478 int ret = 0; > 71d734103edfa2 Mel Gorman 2020-07-08 479 __u32 test_mask, marks_mask; > 90586523eb4b34 Eric Paris 2009-05-21 480 > 71d734103edfa2 Mel Gorman 2020-07-08 481 if (path) > aa93bdc5500cc9 Amir Goldstein 2020-03-19 482 mnt = real_mount(path->mnt); > 3a9fb89f4cd04c Eric Paris 2009-12-17 483 > 40a100d3adc1ad Amir Goldstein 2020-07-22 484 if (!inode) { > 40a100d3adc1ad Amir Goldstein 2020-07-22 485 /* Dirent event - report on TYPE_INODE to dir */ > dca640915c7b84 Amir Goldstein 2021-06-29 486 inode = event_info->dir; > ^^^^^^^^^^^^^^^^^^^^^^^ > Presumably this is non-NULL > > 40a100d3adc1ad Amir Goldstein 2020-07-22 487 } else if (mask & FS_EVENT_ON_CHILD) { > 40a100d3adc1ad Amir Goldstein 2020-07-22 488 /* > fecc4559780d52 Amir Goldstein 2020-12-02 489 * Event on child - report on TYPE_PARENT to dir if it is > fecc4559780d52 Amir Goldstein 2020-12-02 490 * watching children and on TYPE_INODE to child. > 40a100d3adc1ad Amir Goldstein 2020-07-22 491 */ > dca640915c7b84 Amir Goldstein 2021-06-29 492 parent = event_info->dir; > 40a100d3adc1ad Amir Goldstein 2020-07-22 493 } > 40a100d3adc1ad Amir Goldstein 2020-07-22 @494 sb = inode->i_sb; > ^^^^^^^^^^^^ > Dereference > > 497b0c5a7c0688 Amir Goldstein 2020-07-16 495 > 7c49b8616460eb Dave Hansen 2015-09-04 496 /* > 7c49b8616460eb Dave Hansen 2015-09-04 497 * Optimization: srcu_read_lock() has a memory barrier which can > 7c49b8616460eb Dave Hansen 2015-09-04 498 * be expensive. It protects walking the *_fsnotify_marks lists. > 7c49b8616460eb Dave Hansen 2015-09-04 499 * However, if we do not walk the lists, we do not have to do > 7c49b8616460eb Dave Hansen 2015-09-04 500 * SRCU because we have no references to any objects and do not > 7c49b8616460eb Dave Hansen 2015-09-04 501 * need SRCU to keep them "alive". > 7c49b8616460eb Dave Hansen 2015-09-04 502 */ > 9b93f33105f5f9 Amir Goldstein 2020-07-16 503 if (!sb->s_fsnotify_marks && > 497b0c5a7c0688 Amir Goldstein 2020-07-16 504 (!mnt || !mnt->mnt_fsnotify_marks) && > 9b93f33105f5f9 Amir Goldstein 2020-07-16 @505 (!inode || !inode->i_fsnotify_marks) && > ^^^^^^ > Unnecessary check for NULL > > fecc4559780d52 Amir Goldstein 2020-12-02 506 (!parent || !parent->i_fsnotify_marks)) > 7c49b8616460eb Dave Hansen 2015-09-04 507 return 0; > 71d734103edfa2 Mel Gorman 2020-07-08 508 > 9b93f33105f5f9 Amir Goldstein 2020-07-16 509 marks_mask = sb->s_fsnotify_mask; > 71d734103edfa2 Mel Gorman 2020-07-08 510 if (mnt) > 71d734103edfa2 Mel Gorman 2020-07-08 511 marks_mask |= mnt->mnt_fsnotify_mask; > 9b93f33105f5f9 Amir Goldstein 2020-07-16 512 if (inode) > ^^^^^^ > > 9b93f33105f5f9 Amir Goldstein 2020-07-16 513 marks_mask |= inode->i_fsnotify_mask; > fecc4559780d52 Amir Goldstein 2020-12-02 514 if (parent) > fecc4559780d52 Amir Goldstein 2020-12-02 515 marks_mask |= parent->i_fsnotify_mask; > 497b0c5a7c0688 Amir Goldstein 2020-07-16 516 > 71d734103edfa2 Mel Gorman 2020-07-08 517 > 613a807fe7c793 Eric Paris 2010-07-28 518 /* > 613a807fe7c793 Eric Paris 2010-07-28 519 * if this is a modify event we may need to clear the ignored masks > 497b0c5a7c0688 Amir Goldstein 2020-07-16 520 * otherwise return if none of the marks care about this type of event. > 613a807fe7c793 Eric Paris 2010-07-28 521 */ > 71d734103edfa2 Mel Gorman 2020-07-08 522 test_mask = (mask & ALL_FSNOTIFY_EVENTS); > 71d734103edfa2 Mel Gorman 2020-07-08 523 if (!(mask & FS_MODIFY) && !(test_mask & marks_mask)) > 613a807fe7c793 Eric Paris 2010-07-28 524 return 0; > 75c1be487a690d Eric Paris 2010-07-28 525 > 9385a84d7e1f65 Jan Kara 2016-11-10 526 iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu); > 75c1be487a690d Eric Paris 2010-07-28 527 > 45a9fb3725d886 Amir Goldstein 2019-01-10 528 iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] = > 45a9fb3725d886 Amir Goldstein 2019-01-10 529 fsnotify_first_mark(&sb->s_fsnotify_marks); > 9bdda4e9cf2dce Amir Goldstein 2018-09-01 530 if (mnt) { > 47d9c7cc457adc Amir Goldstein 2018-04-20 531 iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] = > 3427ce71554123 Miklos Szeredi 2017-10-30 532 fsnotify_first_mark(&mnt->mnt_fsnotify_marks); > 7131485a93679f Eric Paris 2009-12-17 533 } > 9b93f33105f5f9 Amir Goldstein 2020-07-16 534 if (inode) { > ^^^^^ > Lots of checking... Maybe this is really NULL? Do you have feeling of dejavu? ;-) https://lore.kernel.org/linux-fsdevel/20200730192537.GB13525@quack2.suse.cz/ We've been through this. Maybe you silenced the smach warning on fsnotify() and the rename to __fsnotifty() caused this warning to refloat? Thanks, Amir.
On Wed, Jun 30, 2021 at 11:35:32AM +0300, Amir Goldstein wrote: > > Do you have feeling of dejavu? ;-) > https://lore.kernel.org/linux-fsdevel/20200730192537.GB13525@quack2.suse.cz/ That was a year ago. I have trouble remembering emails I sent yesterday. > > We've been through this. > Maybe you silenced the smach warning on fsnotify() and the rename to > __fsnotifty() > caused this warning to refloat? Yes. Renaming the function will make it show up as a new warning. Also this is an email from the kbuild-bot and last years email was from me, so it's a different tool and a different record of sent messages. (IMO, you should really just remove the bogus NULL checks because everyone looking at the warning will think the code is buggy). regards, dan carpenter
On Wed, Jun 30, 2021 at 11:46 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Jun 30, 2021 at 11:35:32AM +0300, Amir Goldstein wrote: > > > > Do you have feeling of dejavu? ;-) > > https://lore.kernel.org/linux-fsdevel/20200730192537.GB13525@quack2.suse.cz/ > > That was a year ago. I have trouble remembering emails I sent > yesterday. > > > > > We've been through this. > > Maybe you silenced the smach warning on fsnotify() and the rename to > > __fsnotifty() > > caused this warning to refloat? > > Yes. Renaming the function will make it show up as a new warning. Also > this is an email from the kbuild-bot and last years email was from me, > so it's a different tool and a different record of sent messages. > > (IMO, you should really just remove the bogus NULL checks because > everyone looking at the warning will think the code is buggy). > I think the warning is really incorrect. Why does it presume that event_info->dir is non-NULL? Did smach check all the callers to fsnotify() or something? What about future callers that will pass NULL, just like this one: https://lore.kernel.org/linux-fsdevel/20210629191035.681913-12-krisman@collabora.com/ Please fix the tool. Thanks, Amir.
On Wed, Jun 30, 2021 at 12:32 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, Jun 30, 2021 at 11:46 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Wed, Jun 30, 2021 at 11:35:32AM +0300, Amir Goldstein wrote: > > > > > > Do you have feeling of dejavu? ;-) > > > https://lore.kernel.org/linux-fsdevel/20200730192537.GB13525@quack2.suse.cz/ > > > > That was a year ago. I have trouble remembering emails I sent > > yesterday. > > > > > > > > We've been through this. > > > Maybe you silenced the smach warning on fsnotify() and the rename to > > > __fsnotifty() > > > caused this warning to refloat? > > > > Yes. Renaming the function will make it show up as a new warning. Also > > this is an email from the kbuild-bot and last years email was from me, > > so it's a different tool and a different record of sent messages. > > > > (IMO, you should really just remove the bogus NULL checks because > > everyone looking at the warning will think the code is buggy). > > > > I think the warning is really incorrect. > Why does it presume that event_info->dir is non-NULL? > Did smach check all the callers to fsnotify() or something? > What about future callers that will pass NULL, just like this one: > > https://lore.kernel.org/linux-fsdevel/20210629191035.681913-12-krisman@collabora.com/ > FWIW, the caller of this new helper passes NULL as inode: https://lore.kernel.org/linux-fsdevel/20210629191035.681913-14-krisman@collabora.com/ Thanks, Amir.
I think my bug report was not clear... :/ The code looks like this: sb = inode->i_sb; if (inode) ... The NULL check cannot be false because if "inode" is NULL we would have already crashed when we dereference it on the line before. In this case, based on last years discussion, the "inode" pointer can't be NULL. The debate is only whether the unnecessary NULL checks help readability or hurt readability. > Why does it presume that event_info->dir is non-NULL? That was my commentary, just from reading the code. Smatch says that "event->dir" is unknown. > Did smach check all the callers to fsnotify() or something? The kbuild-bot doesn't build the cross function database but if you did use the cross function database then, yes, it does track all the callers. There are two pointers that we care about, the "inode" and the parent inode (dir). Smatch can figure out when "inode" is NULL vs non-NULL but where it gets stuck is on the some of the parent inodes like this call from fsnotify_dirent(): fsnotify_name(dir, mask, d_inode(dentry), &dentry->d_name, 0); ^^^^^^^^^^^^^^^ Smatch doesn't know that d_inode() is always non-NULL at this point. regards, dan carpenter
On Wed, Jun 30, 2021 at 1:49 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > I think my bug report was not clear... :/ The code looks like this: > > sb = inode->i_sb; > > if (inode) ... > > The NULL check cannot be false because if "inode" is NULL we would have > already crashed when we dereference it on the line before. > > In this case, based on last years discussion, the "inode" pointer can't > be NULL. The debate is only whether the unnecessary NULL checks help > readability or hurt readability. > Right. Sorry, I forgot. Anyway, patch 11/15 of the same series changes this code to: sb = event_info->sb ?: inode->i_sb; So inode can and will be NULL coming from the caller of fsnotify_sb_error(sb, NULL). I think that should make smach happy? You can try to run it after patch 11/15 of this series. Thanks, Amir.
On Tue 29-06-21 15:10:27, Gabriel Krisman Bertazi wrote: > From: Amir Goldstein <amir73il@gmail.com> > > There are a lot of arguments to fsnotify() and the handle_event() method. > Pass them in a const struct instead of on the argument list. > > Apart from being more tidy, this helps with passing error reports to the > backend. __fsnotify_parent() argument list was intentionally left > untouched, because its argument list is still short enough and because > most of the event info arguments are initialized inside > __fsnotify_parent(). > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify.c | 59 +++++++++++------------ > fs/notify/fsnotify.c | 83 +++++++++++++++++--------------- > include/linux/fsnotify.h | 15 ++++-- > include/linux/fsnotify_backend.h | 73 +++++++++++++++++++++------- > 4 files changed, 140 insertions(+), 90 deletions(-) Besides the noop function issue Amir has already pointed out I have just a few nits: > @@ -229,7 +229,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, > } > > notify: > - ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0); > + ret = __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = data, .data_type = data_type, > + .dir = p_inode, .name = file_name, > + .inode = inode, > + }); What's the advantage of using __fsnotify() here instead of fsnotify()? In terms of readability the fewer places with these initializers the better I'd say... > static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > - const void *data, int data_type, > - struct inode *dir, const struct qstr *name, > - u32 cookie, struct fsnotify_iter_info *iter_info) > + const struct fsnotify_event_info *event_info, > + struct fsnotify_iter_info *iter_info) > { > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); > struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); > + struct fsnotify_event_info child_event_info = { }; > int ret; No need to init child_event_info. It is fully rewritten if it gets used... > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > index f8acddcf54fb..8c2c681b4495 100644 > --- a/include/linux/fsnotify.h > +++ b/include/linux/fsnotify.h > @@ -30,7 +30,10 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask, > struct inode *child, > const struct qstr *name, u32 cookie) > { > - fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie); > + __fsnotify(mask, &(struct fsnotify_event_info) { > + .data = child, .data_type = FSNOTIFY_EVENT_INODE, > + .dir = dir, .name = name, .cookie = cookie, > + }); > } Hmm, maybe we could have a macro initializer like: #define FSNOTIFY_EVENT_INFO(data, data_type, dir, name, inode, cookie) \ (struct fsnotify_event_info) { \ .data = (data), .data_type = (data_type), .dir = (dir), \ .name = (name), .inode = (inode), .cookie = (cookie)} Then we'd have: __fsnotify(mask, &FSNOTIFY_EVENT_INFO(child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie)); Which looks a bit nicer to me. What do you think guys? Honza
On Thu, Jul 8, 2021 at 1:43 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 29-06-21 15:10:27, Gabriel Krisman Bertazi wrote: > > From: Amir Goldstein <amir73il@gmail.com> > > > > There are a lot of arguments to fsnotify() and the handle_event() method. > > Pass them in a const struct instead of on the argument list. > > > > Apart from being more tidy, this helps with passing error reports to the > > backend. __fsnotify_parent() argument list was intentionally left > > untouched, because its argument list is still short enough and because > > most of the event info arguments are initialized inside > > __fsnotify_parent(). > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > > --- > > fs/notify/fanotify/fanotify.c | 59 +++++++++++------------ > > fs/notify/fsnotify.c | 83 +++++++++++++++++--------------- > > include/linux/fsnotify.h | 15 ++++-- > > include/linux/fsnotify_backend.h | 73 +++++++++++++++++++++------- > > 4 files changed, 140 insertions(+), 90 deletions(-) > > Besides the noop function issue Amir has already pointed out I have just a > few nits: > > > @@ -229,7 +229,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, > > } > > > > notify: > > - ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0); > > + ret = __fsnotify(mask, &(struct fsnotify_event_info) { > > + .data = data, .data_type = data_type, > > + .dir = p_inode, .name = file_name, > > + .inode = inode, > > + }); > > What's the advantage of using __fsnotify() here instead of fsnotify()? In > terms of readability the fewer places with these initializers the better > I'd say... > > > static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, > > - const void *data, int data_type, > > - struct inode *dir, const struct qstr *name, > > - u32 cookie, struct fsnotify_iter_info *iter_info) > > + const struct fsnotify_event_info *event_info, > > + struct fsnotify_iter_info *iter_info) > > { > > struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); > > struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); > > + struct fsnotify_event_info child_event_info = { }; > > int ret; > > No need to init child_event_info. It is fully rewritten if it gets used... > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > index f8acddcf54fb..8c2c681b4495 100644 > > --- a/include/linux/fsnotify.h > > +++ b/include/linux/fsnotify.h > > @@ -30,7 +30,10 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask, > > struct inode *child, > > const struct qstr *name, u32 cookie) > > { > > - fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie); > > + __fsnotify(mask, &(struct fsnotify_event_info) { > > + .data = child, .data_type = FSNOTIFY_EVENT_INODE, > > + .dir = dir, .name = name, .cookie = cookie, > > + }); > > } > > Hmm, maybe we could have a macro initializer like: > > #define FSNOTIFY_EVENT_INFO(data, data_type, dir, name, inode, cookie) \ > (struct fsnotify_event_info) { \ > .data = (data), .data_type = (data_type), .dir = (dir), \ > .name = (name), .inode = (inode), .cookie = (cookie)} > > Then we'd have: > __fsnotify(mask, &FSNOTIFY_EVENT_INFO(child, FSNOTIFY_EVENT_INODE, > dir, name, NULL, cookie)); > > Which looks a bit nicer to me. What do you think guys? > Sure, looks good. But I think it would be even better to have different "wrapper defines" like FSNOTIFY_NAME_EVENT_INFO() will less irrelevant arguments. Thanks, Amir.
On Thu 08-07-21 14:09:43, Amir Goldstein wrote: > On Thu, Jul 8, 2021 at 1:43 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 29-06-21 15:10:27, Gabriel Krisman Bertazi wrote: > > > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h > > > index f8acddcf54fb..8c2c681b4495 100644 > > > --- a/include/linux/fsnotify.h > > > +++ b/include/linux/fsnotify.h > > > @@ -30,7 +30,10 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask, > > > struct inode *child, > > > const struct qstr *name, u32 cookie) > > > { > > > - fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie); > > > + __fsnotify(mask, &(struct fsnotify_event_info) { > > > + .data = child, .data_type = FSNOTIFY_EVENT_INODE, > > > + .dir = dir, .name = name, .cookie = cookie, > > > + }); > > > } > > > > Hmm, maybe we could have a macro initializer like: > > > > #define FSNOTIFY_EVENT_INFO(data, data_type, dir, name, inode, cookie) \ > > (struct fsnotify_event_info) { \ > > .data = (data), .data_type = (data_type), .dir = (dir), \ > > .name = (name), .inode = (inode), .cookie = (cookie)} > > > > Then we'd have: > > __fsnotify(mask, &FSNOTIFY_EVENT_INFO(child, FSNOTIFY_EVENT_INODE, > > dir, name, NULL, cookie)); > > > > Which looks a bit nicer to me. What do you think guys? > > > > Sure, looks good. > But I think it would be even better to have different "wrapper defines" like > FSNOTIFY_NAME_EVENT_INFO() will less irrelevant arguments. If we don't overdo it, I agree :) I mean if we end up with a different helper for each site creating this structure, I'm not sure it helps much... Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 0f760770d4c9..4f2febb15e94 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -253,21 +253,22 @@ static int fanotify_get_response(struct fsnotify_group *group, * been included within the event mask, but have not been explicitly * requested by the user, will not be present in the returned mask. */ -static u32 fanotify_group_event_mask(struct fsnotify_group *group, - struct fsnotify_iter_info *iter_info, - u32 event_mask, const void *data, - int data_type, struct inode *dir) +static u32 fanotify_group_event_mask( + struct fsnotify_group *group, u32 event_mask, + const struct fsnotify_event_info *event_info, + struct fsnotify_iter_info *iter_info) { __u32 marks_mask = 0, marks_ignored_mask = 0; __u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS | FANOTIFY_EVENT_FLAGS; - const struct path *path = fsnotify_data_path(data, data_type); + const struct path *path = fsnotify_event_info_path(event_info); unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS); struct fsnotify_mark *mark; int type; pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n", - __func__, iter_info->report_mask, event_mask, data, data_type); + __func__, iter_info->report_mask, event_mask, + event_info->data, event_info->data_type); if (!fid_mode) { /* Do we have path to open a file descriptor? */ @@ -278,7 +279,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, return 0; } else if (!(fid_mode & FAN_REPORT_FID)) { /* Do we have a directory inode to report? */ - if (!dir && !(event_mask & FS_ISDIR)) + if (!event_info->dir && !(event_mask & FS_ISDIR)) return 0; } @@ -427,13 +428,13 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, * FS_ATTRIB reports the child inode even if reported on a watched parent. * FS_CREATE reports the modified dir inode and not the created inode. */ -static struct inode *fanotify_fid_inode(u32 event_mask, const void *data, - int data_type, struct inode *dir) +static struct inode *fanotify_fid_inode(u32 event_mask, + const struct fsnotify_event_info *event_info) { if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) - return dir; + return event_info->dir; - return fsnotify_data_inode(data, data_type); + return fsnotify_event_info_inode(event_info); } /* @@ -444,18 +445,18 @@ static struct inode *fanotify_fid_inode(u32 event_mask, const void *data, * reported to parent. * Otherwise, do not report dir fid. */ -static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data, - int data_type, struct inode *dir) +static struct inode *fanotify_dfid_inode(u32 event_mask, + const struct fsnotify_event_info *event_info) { - struct inode *inode = fsnotify_data_inode(data, data_type); + struct inode *inode = fsnotify_event_info_inode(event_info); if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) - return dir; + return event_info->dir; if (S_ISDIR(inode->i_mode)) return inode; - return dir; + return event_info->dir; } static struct fanotify_event *fanotify_alloc_path_event(const struct path *path, @@ -563,17 +564,17 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, return &fne->fae; } -static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, - u32 mask, const void *data, - int data_type, struct inode *dir, - const struct qstr *file_name, - __kernel_fsid_t *fsid) +static struct fanotify_event *fanotify_alloc_event( + struct fsnotify_group *group, u32 mask, + const struct fsnotify_event_info *event_info, + __kernel_fsid_t *fsid) { struct fanotify_event *event = NULL; gfp_t gfp = GFP_KERNEL_ACCOUNT; - struct inode *id = fanotify_fid_inode(mask, data, data_type, dir); - struct inode *dirid = fanotify_dfid_inode(mask, data, data_type, dir); - const struct path *path = fsnotify_data_path(data, data_type); + struct inode *id = fanotify_fid_inode(mask, event_info); + struct inode *dirid = fanotify_dfid_inode(mask, event_info); + const struct path *path = fsnotify_event_info_path(event_info); + const struct qstr *file_name = event_info->name; unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS); struct mem_cgroup *old_memcg; struct inode *child = NULL; @@ -712,9 +713,7 @@ static void fanotify_insert_event(struct fsnotify_group *group, } static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, - const void *data, int data_type, - struct inode *dir, - const struct qstr *file_name, u32 cookie, + const struct fsnotify_event_info *event_info, struct fsnotify_iter_info *iter_info) { int ret = 0; @@ -744,8 +743,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19); - mask = fanotify_group_event_mask(group, iter_info, mask, data, - data_type, dir); + mask = fanotify_group_event_mask(group, mask, event_info, iter_info); if (!mask) return 0; @@ -767,8 +765,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask, return 0; } - event = fanotify_alloc_event(group, mask, data, data_type, dir, - file_name, &fsid); + event = fanotify_alloc_event(group, mask, event_info, &fsid); ret = -ENOMEM; if (unlikely(!event)) { /* diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 30d422b8c0fc..36205a769dde 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -177,8 +177,8 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt, * Notify only the child without name info if parent is not watching and * inode/sb/mount are not interested in events with parent and name info. */ -int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, - int data_type) +int __fsnotify_parent(struct dentry *dentry, __u32 mask, + const void *data, int data_type) { const struct path *path = fsnotify_data_path(data, data_type); struct mount *mnt = path ? real_mount(path->mnt) : NULL; @@ -229,7 +229,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, } notify: - ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0); + ret = __fsnotify(mask, &(struct fsnotify_event_info) { + .data = data, .data_type = data_type, + .dir = p_inode, .name = file_name, + .inode = inode, + }); if (file_name) release_dentry_name_snapshot(&name); @@ -240,13 +244,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, EXPORT_SYMBOL_GPL(__fsnotify_parent); static int fsnotify_handle_inode_event(struct fsnotify_group *group, - struct fsnotify_mark *inode_mark, - u32 mask, const void *data, int data_type, - struct inode *dir, const struct qstr *name, - u32 cookie) + struct fsnotify_mark *inode_mark, u32 mask, + const struct fsnotify_event_info *event_info) { - const struct path *path = fsnotify_data_path(data, data_type); - struct inode *inode = fsnotify_data_inode(data, data_type); + const struct path *path = fsnotify_event_info_path(event_info); + struct inode *inode = fsnotify_event_info_inode(event_info); const struct fsnotify_ops *ops = group->ops; if (WARN_ON_ONCE(!ops->handle_inode_event)) @@ -260,16 +262,17 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group, if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS)) return 0; - return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie); + return ops->handle_inode_event(inode_mark, mask, inode, event_info->dir, + event_info->name, event_info->cookie); } static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, - const void *data, int data_type, - struct inode *dir, const struct qstr *name, - u32 cookie, struct fsnotify_iter_info *iter_info) + const struct fsnotify_event_info *event_info, + struct fsnotify_iter_info *iter_info) { struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info); + struct fsnotify_event_info child_event_info = { }; int ret; if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) || @@ -284,8 +287,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, * interested in this event? */ if (parent_mark->mask & FS_EVENT_ON_CHILD) { - ret = fsnotify_handle_inode_event(group, parent_mark, mask, - data, data_type, dir, name, 0); + ret = fsnotify_handle_inode_event(group, parent_mark, + mask, event_info); if (ret) return ret; } @@ -302,18 +305,22 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask, * The child watcher is expecting an event without a file name * and without the FS_EVENT_ON_CHILD flag. */ + if (WARN_ON_ONCE(!event_info->inode)) + return 0; + mask &= ~FS_EVENT_ON_CHILD; - dir = NULL; - name = NULL; + child_event_info = *event_info; + child_event_info.dir = NULL; + child_event_info.name = NULL; + event_info = &child_event_info; } - return fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type, - dir, name, cookie); + return fsnotify_handle_inode_event(group, inode_mark, mask, event_info); } -static int send_to_group(__u32 mask, const void *data, int data_type, - struct inode *dir, const struct qstr *file_name, - u32 cookie, struct fsnotify_iter_info *iter_info) +static int send_to_group(__u32 mask, + const struct fsnotify_event_info *event_info, + struct fsnotify_iter_info *iter_info) { struct fsnotify_group *group = NULL; __u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS); @@ -351,18 +358,18 @@ static int send_to_group(__u32 mask, const void *data, int data_type, pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n", __func__, group, mask, marks_mask, marks_ignored_mask, - data, data_type, dir, cookie); + event_info->data, event_info->data_type, event_info->dir, + event_info->cookie); if (!(test_mask & marks_mask & ~marks_ignored_mask)) return 0; if (group->ops->handle_event) { - return group->ops->handle_event(group, mask, data, data_type, dir, - file_name, cookie, iter_info); + return group->ops->handle_event(group, mask, event_info, + iter_info); } - return fsnotify_handle_event(group, mask, data, data_type, dir, - file_name, cookie, iter_info); + return fsnotify_handle_event(group, mask, event_info, iter_info); } static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp) @@ -448,21 +455,22 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info) * in whatever means they feel necessary. * * @mask: event type and flags + * Input args in struct fsnotify_event_info: * @data: object that event happened on * @data_type: type of object for fanotify_data_XXX() accessors * @dir: optional directory associated with event - - * if @file_name is not NULL, this is the directory that - * @file_name is relative to - * @file_name: optional file name associated with event + * if @name is not NULL, this is the directory that + * @name is relative to + * @name: optional file name associated with event * @inode: optional inode associated with event - * either @dir or @inode must be non-NULL. * if both are non-NULL event may be reported to both. * @cookie: inotify rename cookie */ -int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, - const struct qstr *file_name, struct inode *inode, u32 cookie) +int __fsnotify(__u32 mask, const struct fsnotify_event_info *event_info) { - const struct path *path = fsnotify_data_path(data, data_type); + const struct path *path = fsnotify_event_info_path(event_info); + struct inode *inode = event_info->inode; struct fsnotify_iter_info iter_info = {}; struct super_block *sb; struct mount *mnt = NULL; @@ -475,13 +483,13 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, if (!inode) { /* Dirent event - report on TYPE_INODE to dir */ - inode = dir; + inode = event_info->dir; } else if (mask & FS_EVENT_ON_CHILD) { /* * Event on child - report on TYPE_PARENT to dir if it is * watching children and on TYPE_INODE to child. */ - parent = dir; + parent = event_info->dir; } sb = inode->i_sb; @@ -538,8 +546,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, * That's why this traversal is so complicated... */ while (fsnotify_iter_select_report_types(&iter_info)) { - ret = send_to_group(mask, data, data_type, dir, file_name, - cookie, &iter_info); + ret = send_to_group(mask, event_info, &iter_info); if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS)) goto out; @@ -552,7 +559,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, return ret; } -EXPORT_SYMBOL_GPL(fsnotify); +EXPORT_SYMBOL_GPL(__fsnotify); static __init int fsnotify_init(void) { diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index f8acddcf54fb..8c2c681b4495 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h @@ -30,7 +30,10 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask, struct inode *child, const struct qstr *name, u32 cookie) { - fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie); + __fsnotify(mask, &(struct fsnotify_event_info) { + .data = child, .data_type = FSNOTIFY_EVENT_INODE, + .dir = dir, .name = name, .cookie = cookie, + }); } static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry, @@ -44,7 +47,10 @@ static inline void fsnotify_inode(struct inode *inode, __u32 mask) if (S_ISDIR(inode->i_mode)) mask |= FS_ISDIR; - fsnotify(mask, inode, FSNOTIFY_EVENT_INODE, NULL, NULL, inode, 0); + __fsnotify(mask, &(struct fsnotify_event_info) { + .data = inode, .data_type = FSNOTIFY_EVENT_INODE, + .inode = inode, + }); } /* Notify this dentry's parent about a child's events. */ @@ -68,7 +74,10 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask, return __fsnotify_parent(dentry, mask, data, data_type); notify_child: - return fsnotify(mask, data, data_type, NULL, NULL, inode, 0); + return __fsnotify(mask, &(struct fsnotify_event_info) { + .data = data, .data_type = data_type, + .inode = inode, + }); } /* diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index f9e2c6cd0f7d..b1590f654ade 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -112,6 +112,28 @@ struct fsnotify_iter_info; struct mem_cgroup; +/* + * Event info args passed to fsnotify() and to backends on handle_event(): + * @data: object that event happened on + * @data_type: type of object for fanotify_data_XXX() accessors + * @dir: optional directory associated with event - + * if @name is not NULL, this is the directory that + * @name is relative to + * @name: optional file name associated with event + * @inode: optional inode associated with event - + * either @dir or @inode must be non-NULL. + * if both are non-NULL event may be reported to both. + * @cookie: inotify rename cookie + */ +struct fsnotify_event_info { + const void *data; + int data_type; + struct inode *dir; + const struct qstr *name; + struct inode *inode; + u32 cookie; +}; + /* * Each group much define these ops. The fsnotify infrastructure will call * these operations for each relevant group. @@ -119,13 +141,7 @@ struct mem_cgroup; * handle_event - main call for a group to handle an fs event * @group: group to notify * @mask: event type and flags - * @data: object that event happened on - * @data_type: type of object for fanotify_data_XXX() accessors - * @dir: optional directory associated with event - - * if @file_name is not NULL, this is the directory that - * @file_name is relative to - * @file_name: optional file name associated with event - * @cookie: inotify rename cookie + * @event_info: information attached to the event * @iter_info: array of marks from this group that are interested in the event * * handle_inode_event - simple variant of handle_event() for groups that only @@ -147,8 +163,7 @@ struct mem_cgroup; */ struct fsnotify_ops { int (*handle_event)(struct fsnotify_group *group, u32 mask, - const void *data, int data_type, struct inode *dir, - const struct qstr *file_name, u32 cookie, + const struct fsnotify_event_info *event_info, struct fsnotify_iter_info *iter_info); int (*handle_inode_event)(struct fsnotify_mark *mark, u32 mask, struct inode *inode, struct inode *dir, @@ -262,6 +277,12 @@ static inline struct inode *fsnotify_data_inode(const void *data, int data_type) } } +static inline struct inode *fsnotify_event_info_inode( + const struct fsnotify_event_info *event_info) +{ + return fsnotify_data_inode(event_info->data, event_info->data_type); +} + static inline const struct path *fsnotify_data_path(const void *data, int data_type) { @@ -273,6 +294,12 @@ static inline const struct path *fsnotify_data_path(const void *data, } } +static inline const struct path *fsnotify_event_info_path( + const struct fsnotify_event_info *event_info) +{ + return fsnotify_data_path(event_info->data, event_info->data_type); +} + enum fsnotify_obj_type { FSNOTIFY_OBJ_TYPE_INODE, FSNOTIFY_OBJ_TYPE_PARENT, @@ -410,11 +437,22 @@ struct fsnotify_mark { /* called from the vfs helpers */ /* main fsnotify call to send events */ -extern int fsnotify(__u32 mask, const void *data, int data_type, - struct inode *dir, const struct qstr *name, - struct inode *inode, u32 cookie); -extern int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, - int data_type); +extern int __fsnotify(__u32 mask, + const struct fsnotify_event_info *event_info); +extern int __fsnotify_parent(struct dentry *dentry, __u32 mask, + const void *data, int data_type); + +static inline int fsnotify(__u32 mask, const void *data, int data_type, + struct inode *dir, const struct qstr *name, + struct inode *inode, u32 cookie) +{ + return __fsnotify(mask, &(struct fsnotify_event_info) { + .data = data, .data_type = data_type, + .dir = dir, .name = name, .inode = inode, + .cookie = cookie, + }); +} + extern void __fsnotify_inode_delete(struct inode *inode); extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt); extern void fsnotify_sb_delete(struct super_block *sb); @@ -594,15 +632,14 @@ static inline void fsnotify_init_event(struct fsnotify_event *event) #else -static inline int fsnotify(__u32 mask, const void *data, int data_type, - struct inode *dir, const struct qstr *name, - struct inode *inode, u32 cookie) +static inline int fsnotify(__u32 mask, + const struct fsnotify_event_info *event_info) { return 0; } static inline int __fsnotify_parent(struct dentry *dentry, __u32 mask, - const void *data, int data_type) + const void *data, int data_type) { return 0; }