diff mbox series

[v3,07/15] fsnotify: pass arguments of fsnotify() in struct fsnotify_event_info

Message ID 20210629191035.681913-8-krisman@collabora.com (mailing list archive)
State New, archived
Headers show
Series File system wide monitoring | expand

Commit Message

Gabriel Krisman Bertazi June 29, 2021, 7:10 p.m. UTC 
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(-)

Comments

kernel test robot June 29, 2021, 8:39 p.m. UTC | #1 
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
kernel test robot June 30, 2021, 12:10 a.m. UTC | #2 
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
Amir Goldstein June 30, 2021, 3:29 a.m. UTC | #3 
()


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
>
Dan Carpenter June 30, 2021, 8:11 a.m. UTC | #4 
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
Amir Goldstein June 30, 2021, 8:35 a.m. UTC | #5 
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.
Dan Carpenter June 30, 2021, 8:45 a.m. UTC | #6 
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
Amir Goldstein June 30, 2021, 9:32 a.m. UTC | #7 
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.
Amir Goldstein June 30, 2021, 9:34 a.m. UTC | #8 
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.
Dan Carpenter June 30, 2021, 10:49 a.m. UTC | #9 
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
Amir Goldstein June 30, 2021, 12:51 p.m. UTC | #10 
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.
Jan Kara July 8, 2021, 10:43 a.m. UTC | #11 
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
Amir Goldstein July 8, 2021, 11:09 a.m. UTC | #12 
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.
Jan Kara July 8, 2021, 11:37 a.m. UTC | #13 
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 mbox series

Patch

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;
 }