[PATCH 0/9] Fixes for fanotify name events

[PATCH 0/9] Fixes for fanotify name events

[Amir Goldstein]

Jan,

Following your feedback [1] to fanotify name events, I wrote some LTP
tests [2] to add missing test coverage:

1) dnotify/inotify: report event to both parent and child -
   catches the dnotify bug I had in v4 after unified event patch

2) fanotify10: add groups with FAN_REPORT_NAME to the setup -
   catches the bug you noticed in fanotify_group_event_mask()

3) fanotify10: add test cases with ignored mask on watching parent -
   catches the inconsistecy with ignored masks that you noticed [*]

The patches in this series apply to your fsnotify branch and are
avaiable on my fsnotify-fixes branch [3].

Patch 1 fixes issue #2 above
Patch 2 fixes another issue found by tests
Patch 3 fixes a minor issue found by code review
Patches 4-6 simplify the code based on your suggestions
Patch 7 depends on 4-6 and fixes issue #3 above [*]

Optional patches:
Patch 8 implements your suggestion of simplified handler_event()
Patch 9 is a possible fix for kernel test robot reported performance
regression. I did not get any feedback on it, but it is trivial.

Thanks,
Amir.

[*] The tests for merging ignored mask on watching parent set the
    event FAN_OPEN in both mark mask and ignored mask and set the
    flag FAN_EVENT_ON_CHILD in mark mask, because the expected behavior
    in this case is well defined.  I have patches to fix the case of
    FAN_OPEN in ignored mask and flag FAN_EVENT_ON_CHILD in mark mask,
    but decided not to post them at this time.

[1] https://lore.kernel.org/linux-fsdevel/20200716171332.GK5022@quack2.suse.cz/
[2] https://github.com/amir73il/ltp/commits/fsnotify_parent
[3] https://github.com/amir73il/linux/commits/fsnotify-fixes

Amir Goldstein (9):
  fanotify: fix reporting event to sb/mount marks
  inotify: do not set FS_EVENT_ON_CHILD in non-dir mark mask
  audit: do not set FS_EVENT_ON_CHILD in audit marks mask
  fsnotify: create helper fsnotify_inode()
  fsnotify: simplify dir argument to handle_event()
  fsnotify: pass dir and inode arguments to fsnotify()
  fsnotify: fix merge with parent mark masks
  fsnotify: create method handle_inode_event() in fsnotify_operations
  fsnotify: pass inode to fsnotify_parent()

 fs/kernfs/file.c                 |  11 ++-
 fs/nfsd/filecache.c              |  12 ++--
 fs/notify/dnotify/dnotify.c      |  38 +++-------
 fs/notify/fanotify/fanotify.c    |  17 +++--
 fs/notify/fsnotify.c             | 118 +++++++++++++++++++++++++------
 fs/notify/inotify/inotify_user.c |  14 ++--
 include/linux/fsnotify.h         |  44 ++++++------
 include/linux/fsnotify_backend.h |  33 ++++++---
 kernel/audit_fsnotify.c          |  22 +++---
 kernel/audit_tree.c              |  10 ++-
 kernel/audit_watch.c             |  19 +++--
 kernel/trace/trace.c             |   3 +-
 12 files changed, 196 insertions(+), 145 deletions(-)

-- 
2.17.1

[PATCH 1/9] fanotify: fix reporting event to sb/mount marks

[Amir Goldstein]

When reporting event with parent/name info, we should not skip sb/mount
marks mask if event has FAN_EVENT_ON_CHILD in the mask.

This check is a leftover from the time when the event on child was
reported in a separate callback than the event on parent and we did
not want to get duplicate events for sb/mount mark.

Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index a24f08a9c50f..36ea0cd6387e 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -265,13 +265,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 			continue;
 
 		/*
-		 * If the event is for a child and this mark doesn't care about
-		 * events on a child, don't send it!
-		 * The special object type "child" always cares about events on
-		 * a child, because it refers to the child inode itself.
+		 * If the event is for a child and this mark is on a parent not
+		 * watching children, don't send it!
 		 */
 		if (event_mask & FS_EVENT_ON_CHILD &&
-		    type != FSNOTIFY_OBJ_TYPE_CHILD &&
+		    type == FSNOTIFY_OBJ_TYPE_INODE &&
 		    !(mark->mask & FS_EVENT_ON_CHILD))
 			continue;
 
-- 
2.17.1

Re: [PATCH 1/9] fanotify: fix reporting event to sb/mount marks

[Jan Kara]

On Wed 22-07-20 15:58:41, Amir Goldstein wrote:
> When reporting event with parent/name info, we should not skip sb/mount
> marks mask if event has FAN_EVENT_ON_CHILD in the mask.
> 
> This check is a leftover from the time when the event on child was
> reported in a separate callback than the event on parent and we did
> not want to get duplicate events for sb/mount mark.
> 
> Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

OK, I've decided to just drop "fanotify: report both events on parent and
child with single callback" because it didn't improve anything and amend
eca4784cbb18 with this change...

								Honza

> ---
>  fs/notify/fanotify/fanotify.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index a24f08a9c50f..36ea0cd6387e 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -265,13 +265,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  			continue;
>  
>  		/*
> -		 * If the event is for a child and this mark doesn't care about
> -		 * events on a child, don't send it!
> -		 * The special object type "child" always cares about events on
> -		 * a child, because it refers to the child inode itself.
> +		 * If the event is for a child and this mark is on a parent not
> +		 * watching children, don't send it!
>  		 */
>  		if (event_mask & FS_EVENT_ON_CHILD &&
> -		    type != FSNOTIFY_OBJ_TYPE_CHILD &&
> +		    type == FSNOTIFY_OBJ_TYPE_INODE &&
>  		    !(mark->mask & FS_EVENT_ON_CHILD))
>  			continue;
>  
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/9] inotify: do not set FS_EVENT_ON_CHILD in non-dir mark mask

[Amir Goldstein]

Since commit ecf13b5f8fd6 ("fsnotify: send event with parent/name info
to sb/mount/non-dir marks") the flag FS_EVENT_ON_CHILD has a meaning in
mask of a mark on a non-dir inode.  It means that group is interested
in the name of the file with events.

Since inotify is only intereseted in names of children of a watching
parent, do not sete FS_EVENT_ON_CHILD flag for marks on non-dir.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/inotify/inotify_user.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5385d5817dd9..186722ba3894 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -75,15 +75,17 @@ struct ctl_table inotify_table[] = {
 };
 #endif /* CONFIG_SYSCTL */
 
-static inline __u32 inotify_arg_to_mask(u32 arg)
+static inline __u32 inotify_arg_to_mask(struct inode *inode, u32 arg)
 {
 	__u32 mask;
 
 	/*
-	 * everything should accept their own ignored, cares about children,
-	 * and should receive events when the inode is unmounted
+	 * Everything should accept their own ignored and should receive events
+	 * when the inode is unmounted.  All directories care about children.
 	 */
-	mask = (FS_IN_IGNORED | FS_EVENT_ON_CHILD | FS_UNMOUNT);
+	mask = (FS_IN_IGNORED | FS_UNMOUNT);
+	if (S_ISDIR(inode->i_mode))
+		mask |= FS_EVENT_ON_CHILD;
 
 	/* mask off the flags used to open the fd */
 	mask |= (arg & (IN_ALL_EVENTS | IN_ONESHOT | IN_EXCL_UNLINK));
@@ -512,7 +514,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 	int create = (arg & IN_MASK_CREATE);
 	int ret;
 
-	mask = inotify_arg_to_mask(arg);
+	mask = inotify_arg_to_mask(inode, arg);
 
 	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, group);
 	if (!fsn_mark)
@@ -565,7 +567,7 @@ static int inotify_new_watch(struct fsnotify_group *group,
 	struct idr *idr = &group->inotify_data.idr;
 	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
 
-	mask = inotify_arg_to_mask(arg);
+	mask = inotify_arg_to_mask(inode, arg);
 
 	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
 	if (unlikely(!tmp_i_mark))
-- 
2.17.1

Re: [PATCH 2/9] inotify: do not set FS_EVENT_ON_CHILD in non-dir mark mask

[Jan Kara]

On Wed 22-07-20 15:58:42, Amir Goldstein wrote:
> Since commit ecf13b5f8fd6 ("fsnotify: send event with parent/name info
> to sb/mount/non-dir marks") the flag FS_EVENT_ON_CHILD has a meaning in
> mask of a mark on a non-dir inode.  It means that group is interested
> in the name of the file with events.
> 
> Since inotify is only intereseted in names of children of a watching
> parent, do not sete FS_EVENT_ON_CHILD flag for marks on non-dir.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I've placed this commit in the series before "fsnotify: send event with
parent/name info to sb/mount/non-dir marks" and updated changelog
accordingly.

								Honza

> ---
>  fs/notify/inotify/inotify_user.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 5385d5817dd9..186722ba3894 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -75,15 +75,17 @@ struct ctl_table inotify_table[] = {
>  };
>  #endif /* CONFIG_SYSCTL */
>  
> -static inline __u32 inotify_arg_to_mask(u32 arg)
> +static inline __u32 inotify_arg_to_mask(struct inode *inode, u32 arg)
>  {
>  	__u32 mask;
>  
>  	/*
> -	 * everything should accept their own ignored, cares about children,
> -	 * and should receive events when the inode is unmounted
> +	 * Everything should accept their own ignored and should receive events
> +	 * when the inode is unmounted.  All directories care about children.
>  	 */
> -	mask = (FS_IN_IGNORED | FS_EVENT_ON_CHILD | FS_UNMOUNT);
> +	mask = (FS_IN_IGNORED | FS_UNMOUNT);
> +	if (S_ISDIR(inode->i_mode))
> +		mask |= FS_EVENT_ON_CHILD;
>  
>  	/* mask off the flags used to open the fd */
>  	mask |= (arg & (IN_ALL_EVENTS | IN_ONESHOT | IN_EXCL_UNLINK));
> @@ -512,7 +514,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
>  	int create = (arg & IN_MASK_CREATE);
>  	int ret;
>  
> -	mask = inotify_arg_to_mask(arg);
> +	mask = inotify_arg_to_mask(inode, arg);
>  
>  	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, group);
>  	if (!fsn_mark)
> @@ -565,7 +567,7 @@ static int inotify_new_watch(struct fsnotify_group *group,
>  	struct idr *idr = &group->inotify_data.idr;
>  	spinlock_t *idr_lock = &group->inotify_data.idr_lock;
>  
> -	mask = inotify_arg_to_mask(arg);
> +	mask = inotify_arg_to_mask(inode, arg);
>  
>  	tmp_i_mark = kmem_cache_alloc(inotify_inode_mark_cachep, GFP_KERNEL);
>  	if (unlikely(!tmp_i_mark))
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/9] audit: do not set FS_EVENT_ON_CHILD in audit marks mask

[Amir Goldstein]

The audit groups marks mask does not contain any events possible on
child,so setting the flag FS_EVENT_ON_CHILD in the mask is counter
productive.

It may lead to the undesired outcome of setting the dentry flag
DCACHE_FSNOTIFY_PARENT_WATCHED on a directory inode even though it is
not watching children, because the audit mark contribute the flag
FS_EVENT_ON_CHILD to the inode's fsnotify_mask and another mark could
be contributing an event that is possible on child to the inode's mask.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 kernel/audit_fsnotify.c | 2 +-
 kernel/audit_watch.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 30ca239285a3..bd3a6b79316a 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -36,7 +36,7 @@ static struct fsnotify_group *audit_fsnotify_group;
 
 /* fsnotify events we care about. */
 #define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
-			 FS_MOVE_SELF | FS_EVENT_ON_CHILD)
+			 FS_MOVE_SELF)
 
 static void audit_fsnotify_mark_free(struct audit_fsnotify_mark *audit_mark)
 {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 61fd601f1edf..e23d54bcc587 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -53,7 +53,7 @@ static struct fsnotify_group *audit_watch_group;
 
 /* fsnotify events we care about. */
 #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
-			FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
+			FS_MOVE_SELF | FS_UNMOUNT)
 
 static void audit_free_parent(struct audit_parent *parent)
 {
-- 
2.17.1

Re: [PATCH 3/9] audit: do not set FS_EVENT_ON_CHILD in audit marks mask

[Jan Kara]

On Wed 22-07-20 15:58:43, Amir Goldstein wrote:
> The audit groups marks mask does not contain any events possible on
> child,so setting the flag FS_EVENT_ON_CHILD in the mask is counter
> productive.
> 
> It may lead to the undesired outcome of setting the dentry flag
> DCACHE_FSNOTIFY_PARENT_WATCHED on a directory inode even though it is
> not watching children, because the audit mark contribute the flag
> FS_EVENT_ON_CHILD to the inode's fsnotify_mask and another mark could
> be contributing an event that is possible on child to the inode's mask.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The same as for patch 2/9...

								Honza

> ---
>  kernel/audit_fsnotify.c | 2 +-
>  kernel/audit_watch.c    | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 30ca239285a3..bd3a6b79316a 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -36,7 +36,7 @@ static struct fsnotify_group *audit_fsnotify_group;
>  
>  /* fsnotify events we care about. */
>  #define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
> -			 FS_MOVE_SELF | FS_EVENT_ON_CHILD)
> +			 FS_MOVE_SELF)
>  
>  static void audit_fsnotify_mark_free(struct audit_fsnotify_mark *audit_mark)
>  {
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 61fd601f1edf..e23d54bcc587 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -53,7 +53,7 @@ static struct fsnotify_group *audit_watch_group;
>  
>  /* fsnotify events we care about. */
>  #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\
> -			FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT)
> +			FS_MOVE_SELF | FS_UNMOUNT)
>  
>  static void audit_free_parent(struct audit_parent *parent)
>  {
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/9] fsnotify: create helper fsnotify_inode()

[Amir Goldstein]

Simple helper to consolidate biolerplate code.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/kernfs/file.c         |  6 ++----
 fs/notify/fsnotify.c     |  2 +-
 include/linux/fsnotify.h | 26 +++++++++++---------------
 kernel/trace/trace.c     |  3 +--
 4 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 5b1468bc509e..1d185bffc52f 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -910,10 +910,8 @@ static void kernfs_notify_workfn(struct work_struct *work)
 			kernfs_put(parent);
 		}
 
-		if (!p_inode) {
-			fsnotify(inode, FS_MODIFY, inode, FSNOTIFY_EVENT_INODE,
-				 NULL, 0);
-		}
+		if (!p_inode)
+			fsnotify_inode(inode, FS_MODIFY);
 
 		iput(inode);
 	}
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index efa5c1c4908a..277af3d5efce 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -74,7 +74,7 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
 			iput(iput_inode);
 
 		/* for each watch, send FS_UNMOUNT and then remove it */
-		fsnotify(inode, FS_UNMOUNT, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+		fsnotify_inode(inode, FS_UNMOUNT);
 
 		fsnotify_inode_delete(inode);
 
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index fe4f2bc5b4c2..01b71ad91339 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -38,6 +38,14 @@ static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
 	fsnotify_name(dir, mask, d_inode(dentry), &dentry->d_name, 0);
 }
 
+static inline void fsnotify_inode(struct inode *inode, __u32 mask)
+{
+	if (S_ISDIR(inode->i_mode))
+		mask |= FS_ISDIR;
+
+	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+}
+
 /* Notify this dentry's parent about a child's events. */
 static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
 				  const void *data, int data_type)
@@ -111,12 +119,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
  */
 static inline void fsnotify_link_count(struct inode *inode)
 {
-	__u32 mask = FS_ATTRIB;
-
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
-
-	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_inode(inode, FS_ATTRIB);
 }
 
 /*
@@ -131,7 +134,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	u32 fs_cookie = fsnotify_get_cookie();
 	__u32 old_dir_mask = FS_MOVED_FROM;
 	__u32 new_dir_mask = FS_MOVED_TO;
-	__u32 mask = FS_MOVE_SELF;
 	const struct qstr *new_name = &moved->d_name;
 
 	if (old_dir == new_dir)
@@ -140,7 +142,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	if (isdir) {
 		old_dir_mask |= FS_ISDIR;
 		new_dir_mask |= FS_ISDIR;
-		mask |= FS_ISDIR;
 	}
 
 	fsnotify_name(old_dir, old_dir_mask, source, old_name, fs_cookie);
@@ -149,7 +150,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	if (target)
 		fsnotify_link_count(target);
 
-	fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_inode(source, FS_MOVE_SELF);
 	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
 }
 
@@ -174,12 +175,7 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
  */
 static inline void fsnotify_inoderemove(struct inode *inode)
 {
-	__u32 mask = FS_DELETE_SELF;
-
-	if (S_ISDIR(inode->i_mode))
-		mask |= FS_ISDIR;
-
-	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_inode(inode, FS_DELETE_SELF);
 	__fsnotify_inode_delete(inode);
 }
 
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index bb62269724d5..0c655c039506 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1543,8 +1543,7 @@ static void latency_fsnotify_workfn(struct work_struct *work)
 {
 	struct trace_array *tr = container_of(work, struct trace_array,
 					      fsnotify_work);
-	fsnotify(tr->d_max_latency->d_inode, FS_MODIFY,
-		 tr->d_max_latency->d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_inode(tr->d_max_latency->d_inode, FS_MODIFY);
 }
 
 static void latency_fsnotify_workfn_irq(struct irq_work *iwork)
-- 
2.17.1

Re: [PATCH 4/9] fsnotify: create helper fsnotify_inode()

[Jan Kara]

On Wed 22-07-20 15:58:44, Amir Goldstein wrote:
> Simple helper to consolidate biolerplate code.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Applied. Thanks!

								Honza

> ---
>  fs/kernfs/file.c         |  6 ++----
>  fs/notify/fsnotify.c     |  2 +-
>  include/linux/fsnotify.h | 26 +++++++++++---------------
>  kernel/trace/trace.c     |  3 +--
>  4 files changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 5b1468bc509e..1d185bffc52f 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -910,10 +910,8 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  			kernfs_put(parent);
>  		}
>  
> -		if (!p_inode) {
> -			fsnotify(inode, FS_MODIFY, inode, FSNOTIFY_EVENT_INODE,
> -				 NULL, 0);
> -		}
> +		if (!p_inode)
> +			fsnotify_inode(inode, FS_MODIFY);
>  
>  		iput(inode);
>  	}
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index efa5c1c4908a..277af3d5efce 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -74,7 +74,7 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
>  			iput(iput_inode);
>  
>  		/* for each watch, send FS_UNMOUNT and then remove it */
> -		fsnotify(inode, FS_UNMOUNT, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +		fsnotify_inode(inode, FS_UNMOUNT);
>  
>  		fsnotify_inode_delete(inode);
>  
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index fe4f2bc5b4c2..01b71ad91339 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -38,6 +38,14 @@ static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
>  	fsnotify_name(dir, mask, d_inode(dentry), &dentry->d_name, 0);
>  }
>  
> +static inline void fsnotify_inode(struct inode *inode, __u32 mask)
> +{
> +	if (S_ISDIR(inode->i_mode))
> +		mask |= FS_ISDIR;
> +
> +	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +}
> +
>  /* Notify this dentry's parent about a child's events. */
>  static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
>  				  const void *data, int data_type)
> @@ -111,12 +119,7 @@ static inline int fsnotify_perm(struct file *file, int mask)
>   */
>  static inline void fsnotify_link_count(struct inode *inode)
>  {
> -	__u32 mask = FS_ATTRIB;
> -
> -	if (S_ISDIR(inode->i_mode))
> -		mask |= FS_ISDIR;
> -
> -	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify_inode(inode, FS_ATTRIB);
>  }
>  
>  /*
> @@ -131,7 +134,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	u32 fs_cookie = fsnotify_get_cookie();
>  	__u32 old_dir_mask = FS_MOVED_FROM;
>  	__u32 new_dir_mask = FS_MOVED_TO;
> -	__u32 mask = FS_MOVE_SELF;
>  	const struct qstr *new_name = &moved->d_name;
>  
>  	if (old_dir == new_dir)
> @@ -140,7 +142,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	if (isdir) {
>  		old_dir_mask |= FS_ISDIR;
>  		new_dir_mask |= FS_ISDIR;
> -		mask |= FS_ISDIR;
>  	}
>  
>  	fsnotify_name(old_dir, old_dir_mask, source, old_name, fs_cookie);
> @@ -149,7 +150,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	if (target)
>  		fsnotify_link_count(target);
>  
> -	fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify_inode(source, FS_MOVE_SELF);
>  	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
>  }
>  
> @@ -174,12 +175,7 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
>   */
>  static inline void fsnotify_inoderemove(struct inode *inode)
>  {
> -	__u32 mask = FS_DELETE_SELF;
> -
> -	if (S_ISDIR(inode->i_mode))
> -		mask |= FS_ISDIR;
> -
> -	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify_inode(inode, FS_DELETE_SELF);
>  	__fsnotify_inode_delete(inode);
>  }
>  
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index bb62269724d5..0c655c039506 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1543,8 +1543,7 @@ static void latency_fsnotify_workfn(struct work_struct *work)
>  {
>  	struct trace_array *tr = container_of(work, struct trace_array,
>  					      fsnotify_work);
> -	fsnotify(tr->d_max_latency->d_inode, FS_MODIFY,
> -		 tr->d_max_latency->d_inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify_inode(tr->d_max_latency->d_inode, FS_MODIFY);
>  }
>  
>  static void latency_fsnotify_workfn_irq(struct irq_work *iwork)
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 5/9] fsnotify: simplify dir argument to handle_event()

[Amir Goldstein]

The meaning of dir argument could be simplified a lot to just the base
of file_name it we let the only backends that care about it (fanotify and
dnotify) cope with the case of NULL file_name themselves, which is easy.

This will make dir argument meaning generic enough so we can use the
same argument for fsnotify() without causing confusion.

Fixes: e2c9d9039c3f ("fsnotify: pass dir argument to handle_event() callback")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/dnotify/dnotify.c      | 2 +-
 fs/notify/fanotify/fanotify.c    | 7 ++++---
 fs/notify/fsnotify.c             | 2 +-
 include/linux/fsnotify_backend.h | 4 +---
 4 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 305e5559560a..ca78d3f78da8 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -112,7 +112,7 @@ static int dnotify_handle_event(struct fsnotify_group *group, u32 mask,
 	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
 
 	/* not a dir, dnotify doesn't care */
-	if (!dir)
+	if (!dir && !(mask & FS_ISDIR))
 		return 0;
 
 	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 36ea0cd6387e..03e3dce2a97c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -245,7 +245,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)
+		if (!dir && !(event_mask & FS_ISDIR))
 			return 0;
 	}
 
@@ -525,12 +525,13 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	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);
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct inode *child = NULL;
 	bool name_event = false;
 
-	if ((fid_mode & FAN_REPORT_DIR_FID) && dir) {
+	if ((fid_mode & FAN_REPORT_DIR_FID) && dirid) {
 		/*
 		 * With both flags FAN_REPORT_DIR_FID and FAN_REPORT_FID, we
 		 * report the child fid for events reported on a non-dir child
@@ -540,7 +541,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		    (mask & FAN_EVENT_ON_CHILD) && !(mask & FAN_ONDIR))
 			child = id;
 
-		id = fanotify_dfid_inode(mask, data, data_type, dir);
+		id = dirid;
 
 		/*
 		 * We record file name only in a group with FAN_REPORT_NAME
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 277af3d5efce..834775f61f6b 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -365,7 +365,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	const struct path *path = fsnotify_data_path(data, data_type);
 	struct fsnotify_iter_info iter_info = {};
 	struct super_block *sb = to_tell->i_sb;
-	struct inode *dir = S_ISDIR(to_tell->i_mode) ? to_tell : NULL;
+	struct inode *dir = file_name ? to_tell : NULL;
 	struct mount *mnt = NULL;
 	struct inode *child = NULL;
 	int ret = 0;
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9bd75d0582b4..d94a50e0445a 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -123,9 +123,7 @@ struct mem_cgroup;
  * @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. Otherwise, @dir is the object
- *		inode if event happened on directory and NULL if event
- *		happenned on a non-directory.
+ *		@file_name is relative to
  * @file_name:	optional file name associated with event
  * @cookie:	inotify rename cookie
  * @iter_info:	array of marks from this group that are interested in the event
-- 
2.17.1

Re: [PATCH 5/9] fsnotify: simplify dir argument to handle_event()

[Jan Kara]

On Wed 22-07-20 15:58:45, Amir Goldstein wrote:
> The meaning of dir argument could be simplified a lot to just the base
> of file_name it we let the only backends that care about it (fanotify and
> dnotify) cope with the case of NULL file_name themselves, which is easy.
> 
> This will make dir argument meaning generic enough so we can use the
> same argument for fsnotify() without causing confusion.
> 
> Fixes: e2c9d9039c3f ("fsnotify: pass dir argument to handle_event() callback")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I've folded this patch into "fsnotify: pass dir argument to handle_event()
callback" and int "fanotify: add basic support for FAN_REPORT_DIR_FID".

								Honza

> ---
>  fs/notify/dnotify/dnotify.c      | 2 +-
>  fs/notify/fanotify/fanotify.c    | 7 ++++---
>  fs/notify/fsnotify.c             | 2 +-
>  include/linux/fsnotify_backend.h | 4 +---
>  4 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 305e5559560a..ca78d3f78da8 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -112,7 +112,7 @@ static int dnotify_handle_event(struct fsnotify_group *group, u32 mask,
>  	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
>  
>  	/* not a dir, dnotify doesn't care */
> -	if (!dir)
> +	if (!dir && !(mask & FS_ISDIR))
>  		return 0;
>  
>  	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 36ea0cd6387e..03e3dce2a97c 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -245,7 +245,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)
> +		if (!dir && !(event_mask & FS_ISDIR))
>  			return 0;
>  	}
>  
> @@ -525,12 +525,13 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
>  	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);
>  	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
>  	struct inode *child = NULL;
>  	bool name_event = false;
>  
> -	if ((fid_mode & FAN_REPORT_DIR_FID) && dir) {
> +	if ((fid_mode & FAN_REPORT_DIR_FID) && dirid) {
>  		/*
>  		 * With both flags FAN_REPORT_DIR_FID and FAN_REPORT_FID, we
>  		 * report the child fid for events reported on a non-dir child
> @@ -540,7 +541,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
>  		    (mask & FAN_EVENT_ON_CHILD) && !(mask & FAN_ONDIR))
>  			child = id;
>  
> -		id = fanotify_dfid_inode(mask, data, data_type, dir);
> +		id = dirid;
>  
>  		/*
>  		 * We record file name only in a group with FAN_REPORT_NAME
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 277af3d5efce..834775f61f6b 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -365,7 +365,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	const struct path *path = fsnotify_data_path(data, data_type);
>  	struct fsnotify_iter_info iter_info = {};
>  	struct super_block *sb = to_tell->i_sb;
> -	struct inode *dir = S_ISDIR(to_tell->i_mode) ? to_tell : NULL;
> +	struct inode *dir = file_name ? to_tell : NULL;
>  	struct mount *mnt = NULL;
>  	struct inode *child = NULL;
>  	int ret = 0;
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 9bd75d0582b4..d94a50e0445a 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -123,9 +123,7 @@ struct mem_cgroup;
>   * @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. Otherwise, @dir is the object
> - *		inode if event happened on directory and NULL if event
> - *		happenned on a non-directory.
> + *		@file_name is relative to
>   * @file_name:	optional file name associated with event
>   * @cookie:	inotify rename cookie
>   * @iter_info:	array of marks from this group that are interested in the event
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 6/9] fsnotify: pass dir and inode arguments to fsnotify()

[Amir Goldstein]

The arguments of fsnotify() are overloaded and mean different things
for different event types.

Replace the to_tell argument with separate arguments @dir and @inode,
because we may be sending to both dir and child.  Using the @data
argument to pass the child is not enough, because dirent events pass
this argument (for audit), but we do not report to child.

Document the new fsnotify() function argumenets.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/kernfs/file.c                 |  5 +--
 fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++----------
 include/linux/fsnotify.h         |  9 +++---
 include/linux/fsnotify_backend.h | 10 +++---
 4 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 1d185bffc52f..f277d023ebcd 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -902,8 +902,9 @@ static void kernfs_notify_workfn(struct work_struct *work)
 		if (parent) {
 			p_inode = ilookup(info->sb, kernfs_ino(parent));
 			if (p_inode) {
-				fsnotify(p_inode, FS_MODIFY | FS_EVENT_ON_CHILD,
-					 inode, FSNOTIFY_EVENT_INODE, &name, 0);
+				fsnotify(FS_MODIFY | FS_EVENT_ON_CHILD,
+					 inode, FSNOTIFY_EVENT_INODE,
+					 p_inode, &name, inode, 0);
 				iput(p_inode);
 			}
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 834775f61f6b..3b805e05c02d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -179,7 +179,7 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 	struct dentry *parent;
 	bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED;
 	__u32 p_mask;
-	struct inode *p_inode;
+	struct inode *p_inode = NULL;
 	struct name_snapshot name;
 	struct qstr *file_name = NULL;
 	int ret = 0;
@@ -213,14 +213,13 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 		WARN_ON_ONCE(inode != fsnotify_data_inode(data, data_type));
 
 		/* Notify both parent and child with child name info */
-		inode = p_inode;
 		take_dentry_name_snapshot(&name, dentry);
 		file_name = &name.name;
 		mask |= FS_EVENT_ON_CHILD;
 	}
 
 notify:
-	ret = fsnotify(inode, mask, data, data_type, file_name, 0);
+	ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0);
 
 	if (file_name)
 		release_dentry_name_snapshot(&name);
@@ -354,18 +353,31 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
 }
 
 /*
- * This is the main call to fsnotify.  The VFS calls into hook specific functions
- * in linux/fsnotify.h.  Those functions then in turn call here.  Here will call
- * out to all of the registered fsnotify_group.  Those groups can then use the
- * notification event in whatever means they feel necessary.
+ * fsnotify - This is the main call to fsnotify.
+ *
+ * The VFS calls into hook specific functions in linux/fsnotify.h.
+ * Those functions then in turn call here.  Here will call out to all of the
+ * registered fsnotify_group.  Those groups can then use the notification event
+ * in whatever means they feel necessary.
+ *
+ * @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
+ * @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(struct inode *to_tell, __u32 mask, const void *data, int data_type,
-	     const struct qstr *file_name, u32 cookie)
+int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
+	     const struct qstr *file_name, struct inode *inode, u32 cookie)
 {
 	const struct path *path = fsnotify_data_path(data, data_type);
 	struct fsnotify_iter_info iter_info = {};
-	struct super_block *sb = to_tell->i_sb;
-	struct inode *dir = file_name ? to_tell : NULL;
+	struct super_block *sb;
 	struct mount *mnt = NULL;
 	struct inode *child = NULL;
 	int ret = 0;
@@ -374,8 +386,18 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	if (path)
 		mnt = real_mount(path->mnt);
 
-	if (mask & FS_EVENT_ON_CHILD)
-		child = fsnotify_data_inode(data, data_type);
+	if (!inode) {
+		/* Dirent event - report on TYPE_INODE to dir */
+		inode = dir;
+	} else if (mask & FS_EVENT_ON_CHILD) {
+		/*
+		 * Event on child - report on TYPE_INODE to dir
+		 * and on TYPE_CHILD to child.
+		 */
+		child = inode;
+		inode = dir;
+	}
+	sb = inode->i_sb;
 
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
@@ -384,12 +406,12 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	 * SRCU because we have no references to any objects and do not
 	 * need SRCU to keep them "alive".
 	 */
-	if (!to_tell->i_fsnotify_marks && !sb->s_fsnotify_marks &&
+	if (!inode->i_fsnotify_marks && !sb->s_fsnotify_marks &&
 	    (!mnt || !mnt->mnt_fsnotify_marks) &&
 	    (!child || !child->i_fsnotify_marks))
 		return 0;
 
-	marks_mask = to_tell->i_fsnotify_mask | sb->s_fsnotify_mask;
+	marks_mask = inode->i_fsnotify_mask | sb->s_fsnotify_mask;
 	if (mnt)
 		marks_mask |= mnt->mnt_fsnotify_mask;
 	if (child)
@@ -407,7 +429,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
 
 	iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
-		fsnotify_first_mark(&to_tell->i_fsnotify_marks);
+		fsnotify_first_mark(&inode->i_fsnotify_marks);
 	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
 		fsnotify_first_mark(&sb->s_fsnotify_marks);
 	if (mnt) {
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 01b71ad91339..d9b26c6552ee 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -23,13 +23,14 @@
  * have changed (i.e. renamed over).
  *
  * Unlike fsnotify_parent(), the event will be reported regardless of the
- * FS_EVENT_ON_CHILD mask on the parent inode.
+ * FS_EVENT_ON_CHILD mask on the parent inode and will not be reported if only
+ * the child is interested and not the parent.
  */
 static inline void fsnotify_name(struct inode *dir, __u32 mask,
 				 struct inode *child,
 				 const struct qstr *name, u32 cookie)
 {
-	fsnotify(dir, mask, child, FSNOTIFY_EVENT_INODE, name, cookie);
+	fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie);
 }
 
 static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
@@ -43,7 +44,7 @@ static inline void fsnotify_inode(struct inode *inode, __u32 mask)
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
-	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify(mask, inode, FSNOTIFY_EVENT_INODE, NULL, NULL, inode, 0);
 }
 
 /* Notify this dentry's parent about a child's events. */
@@ -67,7 +68,7 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
 	return __fsnotify_parent(dentry, mask, data, data_type);
 
 notify_child:
-	return fsnotify(inode, mask, data, data_type, NULL, 0);
+	return fsnotify(mask, data, data_type, NULL, NULL, inode, 0);
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index d94a50e0445a..32104cfc27a5 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -398,8 +398,9 @@ struct fsnotify_mark {
 /* called from the vfs helpers */
 
 /* main fsnotify call to send events */
-extern int fsnotify(struct inode *to_tell, __u32 mask, const void *data,
-		    int data_type, const struct qstr *name, u32 cookie);
+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 void __fsnotify_inode_delete(struct inode *inode);
@@ -569,8 +570,9 @@ static inline void fsnotify_init_event(struct fsnotify_event *event,
 
 #else
 
-static inline int fsnotify(struct inode *to_tell, __u32 mask, const void *data,
-			   int data_type, const struct qstr *name, u32 cookie)
+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 0;
 }
-- 
2.17.1

Re: [PATCH 6/9] fsnotify: pass dir and inode arguments to fsnotify()

[Jan Kara]

On Wed 22-07-20 15:58:46, Amir Goldstein wrote:
> The arguments of fsnotify() are overloaded and mean different things
> for different event types.
> 
> Replace the to_tell argument with separate arguments @dir and @inode,
> because we may be sending to both dir and child.  Using the @data
> argument to pass the child is not enough, because dirent events pass
> this argument (for audit), but we do not report to child.
> 
> Document the new fsnotify() function argumenets.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks, applied!

								Honza

> ---
>  fs/kernfs/file.c                 |  5 +--
>  fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++----------
>  include/linux/fsnotify.h         |  9 +++---
>  include/linux/fsnotify_backend.h | 10 +++---
>  4 files changed, 52 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 1d185bffc52f..f277d023ebcd 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -902,8 +902,9 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  		if (parent) {
>  			p_inode = ilookup(info->sb, kernfs_ino(parent));
>  			if (p_inode) {
> -				fsnotify(p_inode, FS_MODIFY | FS_EVENT_ON_CHILD,
> -					 inode, FSNOTIFY_EVENT_INODE, &name, 0);
> +				fsnotify(FS_MODIFY | FS_EVENT_ON_CHILD,
> +					 inode, FSNOTIFY_EVENT_INODE,
> +					 p_inode, &name, inode, 0);
>  				iput(p_inode);
>  			}
>  
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 834775f61f6b..3b805e05c02d 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -179,7 +179,7 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
>  	struct dentry *parent;
>  	bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED;
>  	__u32 p_mask;
> -	struct inode *p_inode;
> +	struct inode *p_inode = NULL;
>  	struct name_snapshot name;
>  	struct qstr *file_name = NULL;
>  	int ret = 0;
> @@ -213,14 +213,13 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
>  		WARN_ON_ONCE(inode != fsnotify_data_inode(data, data_type));
>  
>  		/* Notify both parent and child with child name info */
> -		inode = p_inode;
>  		take_dentry_name_snapshot(&name, dentry);
>  		file_name = &name.name;
>  		mask |= FS_EVENT_ON_CHILD;
>  	}
>  
>  notify:
> -	ret = fsnotify(inode, mask, data, data_type, file_name, 0);
> +	ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0);
>  
>  	if (file_name)
>  		release_dentry_name_snapshot(&name);
> @@ -354,18 +353,31 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
>  }
>  
>  /*
> - * This is the main call to fsnotify.  The VFS calls into hook specific functions
> - * in linux/fsnotify.h.  Those functions then in turn call here.  Here will call
> - * out to all of the registered fsnotify_group.  Those groups can then use the
> - * notification event in whatever means they feel necessary.
> + * fsnotify - This is the main call to fsnotify.
> + *
> + * The VFS calls into hook specific functions in linux/fsnotify.h.
> + * Those functions then in turn call here.  Here will call out to all of the
> + * registered fsnotify_group.  Those groups can then use the notification event
> + * in whatever means they feel necessary.
> + *
> + * @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
> + * @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(struct inode *to_tell, __u32 mask, const void *data, int data_type,
> -	     const struct qstr *file_name, u32 cookie)
> +int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> +	     const struct qstr *file_name, struct inode *inode, u32 cookie)
>  {
>  	const struct path *path = fsnotify_data_path(data, data_type);
>  	struct fsnotify_iter_info iter_info = {};
> -	struct super_block *sb = to_tell->i_sb;
> -	struct inode *dir = file_name ? to_tell : NULL;
> +	struct super_block *sb;
>  	struct mount *mnt = NULL;
>  	struct inode *child = NULL;
>  	int ret = 0;
> @@ -374,8 +386,18 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	if (path)
>  		mnt = real_mount(path->mnt);
>  
> -	if (mask & FS_EVENT_ON_CHILD)
> -		child = fsnotify_data_inode(data, data_type);
> +	if (!inode) {
> +		/* Dirent event - report on TYPE_INODE to dir */
> +		inode = dir;
> +	} else if (mask & FS_EVENT_ON_CHILD) {
> +		/*
> +		 * Event on child - report on TYPE_INODE to dir
> +		 * and on TYPE_CHILD to child.
> +		 */
> +		child = inode;
> +		inode = dir;
> +	}
> +	sb = inode->i_sb;
>  
>  	/*
>  	 * Optimization: srcu_read_lock() has a memory barrier which can
> @@ -384,12 +406,12 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	 * SRCU because we have no references to any objects and do not
>  	 * need SRCU to keep them "alive".
>  	 */
> -	if (!to_tell->i_fsnotify_marks && !sb->s_fsnotify_marks &&
> +	if (!inode->i_fsnotify_marks && !sb->s_fsnotify_marks &&
>  	    (!mnt || !mnt->mnt_fsnotify_marks) &&
>  	    (!child || !child->i_fsnotify_marks))
>  		return 0;
>  
> -	marks_mask = to_tell->i_fsnotify_mask | sb->s_fsnotify_mask;
> +	marks_mask = inode->i_fsnotify_mask | sb->s_fsnotify_mask;
>  	if (mnt)
>  		marks_mask |= mnt->mnt_fsnotify_mask;
>  	if (child)
> @@ -407,7 +429,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
>  
>  	iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> -		fsnotify_first_mark(&to_tell->i_fsnotify_marks);
> +		fsnotify_first_mark(&inode->i_fsnotify_marks);
>  	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
>  		fsnotify_first_mark(&sb->s_fsnotify_marks);
>  	if (mnt) {
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 01b71ad91339..d9b26c6552ee 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -23,13 +23,14 @@
>   * have changed (i.e. renamed over).
>   *
>   * Unlike fsnotify_parent(), the event will be reported regardless of the
> - * FS_EVENT_ON_CHILD mask on the parent inode.
> + * FS_EVENT_ON_CHILD mask on the parent inode and will not be reported if only
> + * the child is interested and not the parent.
>   */
>  static inline void fsnotify_name(struct inode *dir, __u32 mask,
>  				 struct inode *child,
>  				 const struct qstr *name, u32 cookie)
>  {
> -	fsnotify(dir, mask, child, FSNOTIFY_EVENT_INODE, name, cookie);
> +	fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie);
>  }
>  
>  static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
> @@ -43,7 +44,7 @@ static inline void fsnotify_inode(struct inode *inode, __u32 mask)
>  	if (S_ISDIR(inode->i_mode))
>  		mask |= FS_ISDIR;
>  
> -	fsnotify(inode, mask, inode, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify(mask, inode, FSNOTIFY_EVENT_INODE, NULL, NULL, inode, 0);
>  }
>  
>  /* Notify this dentry's parent about a child's events. */
> @@ -67,7 +68,7 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
>  	return __fsnotify_parent(dentry, mask, data, data_type);
>  
>  notify_child:
> -	return fsnotify(inode, mask, data, data_type, NULL, 0);
> +	return fsnotify(mask, data, data_type, NULL, NULL, inode, 0);
>  }
>  
>  /*
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index d94a50e0445a..32104cfc27a5 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -398,8 +398,9 @@ struct fsnotify_mark {
>  /* called from the vfs helpers */
>  
>  /* main fsnotify call to send events */
> -extern int fsnotify(struct inode *to_tell, __u32 mask, const void *data,
> -		    int data_type, const struct qstr *name, u32 cookie);
> +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 void __fsnotify_inode_delete(struct inode *inode);
> @@ -569,8 +570,9 @@ static inline void fsnotify_init_event(struct fsnotify_event *event,
>  
>  #else
>  
> -static inline int fsnotify(struct inode *to_tell, __u32 mask, const void *data,
> -			   int data_type, const struct qstr *name, u32 cookie)
> +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 0;
>  }
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 7/9] fsnotify: fix merge with parent mark masks

[Amir Goldstein]

When reporting event with parent/name info, we should not merge
parent's mark mask and ignore mask, unless the parent has the flag
FS_EVENT_ON_CHILD in the mask.

Therefore, in fsnotify_parent(), set the FS_EVENT_ON_CHILD flag in event
mask only if parent is watching and use this flag to decide if the
parent mark masks should be merged with child/sb/mount marks.

After this change, even groups that do not subscribe to events on
children could get an event with mark iterator type TYPE_CHILD and
without mark iterator type TYPE_INODE if fanotify has marks on the same
objects.

dnotify and inotify event handlers can already cope with that situation.
audit does not subscribe to events that are possible on child, so won't
get to this situation. nfsd does not access the marks iterator from its
event handler at the moment, so it is not affected.

This is a bit too fragile, so we should prepare all groups to cope with
mark type TYPE_CHILD preferably using a generic helper.

Link: https://lore.kernel.org/linux-fsdevel/20200716223441.GA5085@quack2.suse.cz/
Fixes: ecf13b5f8fd6 ("fsnotify: send event with parent/name info to sb/mount/non-dir marks")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c |  2 +-
 fs/notify/fsnotify.c          | 20 +++++++++++++-------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 03e3dce2a97c..3336157d895d 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -538,7 +538,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		 * in addition to reporting the parent fid and maybe child name.
 		 */
 		if ((fid_mode & FAN_REPORT_FID) &&
-		    (mask & FAN_EVENT_ON_CHILD) && !(mask & FAN_ONDIR))
+		    id != dirid && !(mask & FAN_ONDIR))
 			child = id;
 
 		id = dirid;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 3b805e05c02d..494d5d70323f 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -215,7 +215,8 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 		/* Notify both parent and child with child name info */
 		take_dentry_name_snapshot(&name, dentry);
 		file_name = &name.name;
-		mask |= FS_EVENT_ON_CHILD;
+		if (parent_watched)
+			mask |= FS_EVENT_ON_CHILD;
 	}
 
 notify:
@@ -391,8 +392,8 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		inode = dir;
 	} else if (mask & FS_EVENT_ON_CHILD) {
 		/*
-		 * Event on child - report on TYPE_INODE to dir
-		 * and on TYPE_CHILD to child.
+		 * Event on child - report on TYPE_INODE to dir if it is
+		 * watching children and on TYPE_CHILD to child.
 		 */
 		child = inode;
 		inode = dir;
@@ -406,14 +407,17 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	 * SRCU because we have no references to any objects and do not
 	 * need SRCU to keep them "alive".
 	 */
-	if (!inode->i_fsnotify_marks && !sb->s_fsnotify_marks &&
+	if (!sb->s_fsnotify_marks &&
 	    (!mnt || !mnt->mnt_fsnotify_marks) &&
+	    (!inode || !inode->i_fsnotify_marks) &&
 	    (!child || !child->i_fsnotify_marks))
 		return 0;
 
-	marks_mask = inode->i_fsnotify_mask | sb->s_fsnotify_mask;
+	marks_mask = sb->s_fsnotify_mask;
 	if (mnt)
 		marks_mask |= mnt->mnt_fsnotify_mask;
+	if (inode)
+		marks_mask |= inode->i_fsnotify_mask;
 	if (child)
 		marks_mask |= child->i_fsnotify_mask;
 
@@ -428,14 +432,16 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
 
-	iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
-		fsnotify_first_mark(&inode->i_fsnotify_marks);
 	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
 		fsnotify_first_mark(&sb->s_fsnotify_marks);
 	if (mnt) {
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
 			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
 	}
+	if (inode) {
+		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
+			fsnotify_first_mark(&inode->i_fsnotify_marks);
+	}
 	if (child) {
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] =
 			fsnotify_first_mark(&child->i_fsnotify_marks);
-- 
2.17.1

Re: [PATCH 7/9] fsnotify: fix merge with parent mark masks

[Jan Kara]

On Wed 22-07-20 15:58:47, Amir Goldstein wrote:
> When reporting event with parent/name info, we should not merge
> parent's mark mask and ignore mask, unless the parent has the flag
> FS_EVENT_ON_CHILD in the mask.
> 
> Therefore, in fsnotify_parent(), set the FS_EVENT_ON_CHILD flag in event
> mask only if parent is watching and use this flag to decide if the
> parent mark masks should be merged with child/sb/mount marks.
> 
> After this change, even groups that do not subscribe to events on
> children could get an event with mark iterator type TYPE_CHILD and
> without mark iterator type TYPE_INODE if fanotify has marks on the same
> objects.
> 
> dnotify and inotify event handlers can already cope with that situation.
> audit does not subscribe to events that are possible on child, so won't
> get to this situation. nfsd does not access the marks iterator from its
> event handler at the moment, so it is not affected.
> 
> This is a bit too fragile, so we should prepare all groups to cope with
> mark type TYPE_CHILD preferably using a generic helper.
> 
> Link: https://lore.kernel.org/linux-fsdevel/20200716223441.GA5085@quack2.suse.cz/
> Fixes: ecf13b5f8fd6 ("fsnotify: send event with parent/name info to sb/mount/non-dir marks")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I've folded this into "fsnotify: send event with parent/name info to
sb/mount/non-dir marks".

									Honza

> ---
>  fs/notify/fanotify/fanotify.c |  2 +-
>  fs/notify/fsnotify.c          | 20 +++++++++++++-------
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 03e3dce2a97c..3336157d895d 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -538,7 +538,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
>  		 * in addition to reporting the parent fid and maybe child name.
>  		 */
>  		if ((fid_mode & FAN_REPORT_FID) &&
> -		    (mask & FAN_EVENT_ON_CHILD) && !(mask & FAN_ONDIR))
> +		    id != dirid && !(mask & FAN_ONDIR))
>  			child = id;
>  
>  		id = dirid;
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 3b805e05c02d..494d5d70323f 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -215,7 +215,8 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
>  		/* Notify both parent and child with child name info */
>  		take_dentry_name_snapshot(&name, dentry);
>  		file_name = &name.name;
> -		mask |= FS_EVENT_ON_CHILD;
> +		if (parent_watched)
> +			mask |= FS_EVENT_ON_CHILD;
>  	}
>  
>  notify:
> @@ -391,8 +392,8 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>  		inode = dir;
>  	} else if (mask & FS_EVENT_ON_CHILD) {
>  		/*
> -		 * Event on child - report on TYPE_INODE to dir
> -		 * and on TYPE_CHILD to child.
> +		 * Event on child - report on TYPE_INODE to dir if it is
> +		 * watching children and on TYPE_CHILD to child.
>  		 */
>  		child = inode;
>  		inode = dir;
> @@ -406,14 +407,17 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>  	 * SRCU because we have no references to any objects and do not
>  	 * need SRCU to keep them "alive".
>  	 */
> -	if (!inode->i_fsnotify_marks && !sb->s_fsnotify_marks &&
> +	if (!sb->s_fsnotify_marks &&
>  	    (!mnt || !mnt->mnt_fsnotify_marks) &&
> +	    (!inode || !inode->i_fsnotify_marks) &&
>  	    (!child || !child->i_fsnotify_marks))
>  		return 0;
>  
> -	marks_mask = inode->i_fsnotify_mask | sb->s_fsnotify_mask;
> +	marks_mask = sb->s_fsnotify_mask;
>  	if (mnt)
>  		marks_mask |= mnt->mnt_fsnotify_mask;
> +	if (inode)
> +		marks_mask |= inode->i_fsnotify_mask;
>  	if (child)
>  		marks_mask |= child->i_fsnotify_mask;
>  
> @@ -428,14 +432,16 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>  
>  	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
>  
> -	iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> -		fsnotify_first_mark(&inode->i_fsnotify_marks);
>  	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
>  		fsnotify_first_mark(&sb->s_fsnotify_marks);
>  	if (mnt) {
>  		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
>  			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
>  	}
> +	if (inode) {
> +		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> +			fsnotify_first_mark(&inode->i_fsnotify_marks);
> +	}
>  	if (child) {
>  		iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] =
>  			fsnotify_first_mark(&child->i_fsnotify_marks);
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 8/9] fsnotify: create method handle_inode_event() in fsnotify_operations

[Amir Goldstein]

The method handle_event() grew a lot of complexity due to the design of
fanotify and merging of ignore masks.

Most backends do not care about this complex functionality, so we can hide
this complexity from them.

Introduce a method handle_inode_event() that serves those backends and
passes a single inode mark and less arguments.

This change converts all backends except fanotify and inotify to use the
simplified handle_inode_event() method.  In pricipal, inotify could have
also used the new method, but that would require passing more arguments
on the simple helper (data, data_type, cookie), so we leave it with the
handle_event() method.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/nfsd/filecache.c              | 12 +++-----
 fs/notify/dnotify/dnotify.c      | 38 +++++------------------
 fs/notify/fsnotify.c             | 52 ++++++++++++++++++++++++++++++--
 include/linux/fsnotify_backend.h | 19 ++++++++++--
 kernel/audit_fsnotify.c          | 20 +++++-------
 kernel/audit_tree.c              | 10 +++---
 kernel/audit_watch.c             | 17 +++++------
 7 files changed, 97 insertions(+), 71 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index bbc7892d2928..c8b9d2667ee6 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -598,14 +598,10 @@ static struct notifier_block nfsd_file_lease_notifier = {
 };
 
 static int
-nfsd_file_fsnotify_handle_event(struct fsnotify_group *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)
+nfsd_file_fsnotify_handle_event(struct fsnotify_mark *mark, u32 mask,
+				struct inode *inode, struct inode *dir,
+				const struct qstr *name)
 {
-	struct inode *inode = fsnotify_data_inode(data, data_type);
-
 	trace_nfsd_file_fsnotify_handle_event(inode, mask);
 
 	/* Should be no marks on non-regular files */
@@ -626,7 +622,7 @@ nfsd_file_fsnotify_handle_event(struct fsnotify_group *group, u32 mask,
 
 
 static const struct fsnotify_ops nfsd_file_fsnotify_ops = {
-	.handle_event = nfsd_file_fsnotify_handle_event,
+	.handle_inode_event = nfsd_file_fsnotify_handle_event,
 	.free_mark = nfsd_file_mark_free,
 };
 
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index ca78d3f78da8..5dcda8f20c04 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -70,8 +70,9 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
  * destroy the dnotify struct if it was not registered to receive multiple
  * events.
  */
-static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
-			      struct fsnotify_mark *inode_mark)
+static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
+				struct inode *inode, struct inode *dir,
+				const struct qstr *name)
 {
 	struct dnotify_mark *dn_mark;
 	struct dnotify_struct *dn;
@@ -79,6 +80,10 @@ static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
 	struct fown_struct *fown;
 	__u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
 
+	/* not a dir, dnotify doesn't care */
+	if (!dir && !(mask & FS_ISDIR))
+		return 0;
+
 	dn_mark = container_of(inode_mark, struct dnotify_mark, fsn_mark);
 
 	spin_lock(&inode_mark->lock);
@@ -100,33 +105,6 @@ static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
 	}
 
 	spin_unlock(&inode_mark->lock);
-}
-
-static int dnotify_handle_event(struct fsnotify_group *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)
-{
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
-	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
-
-	/* not a dir, dnotify doesn't care */
-	if (!dir && !(mask & FS_ISDIR))
-		return 0;
-
-	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
-		return 0;
-
-	/*
-	 * Some events can be sent on both parent dir and subdir marks
-	 * (e.g. DN_ATTRIB).  If both parent dir and subdir are watching,
-	 * report the event once to parent dir and once to subdir.
-	 */
-	if (inode_mark)
-		dnotify_one_event(group, mask, inode_mark);
-	if (child_mark)
-		dnotify_one_event(group, mask, child_mark);
 
 	return 0;
 }
@@ -143,7 +121,7 @@ static void dnotify_free_mark(struct fsnotify_mark *fsn_mark)
 }
 
 static const struct fsnotify_ops dnotify_fsnotify_ops = {
-	.handle_event = dnotify_handle_event,
+	.handle_inode_event = dnotify_handle_event,
 	.free_mark = dnotify_free_mark,
 };
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 494d5d70323f..a960ec3a569a 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -230,6 +230,49 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 }
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 
+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)
+{
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
+	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
+	struct inode *inode = fsnotify_data_inode(data, data_type);
+	const struct fsnotify_ops *ops = group->ops;
+	int ret;
+
+	if (WARN_ON_ONCE(!ops->handle_inode_event))
+		return 0;
+
+	if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
+	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
+		return 0;
+
+	/*
+	 * An event can be sent on child mark iterator instead of inode mark
+	 * iterator because of other groups that have interest of this inode
+	 * and have marks on both parent and child.  We can simplify this case.
+	 */
+	if (!inode_mark) {
+		inode_mark = child_mark;
+		child_mark = NULL;
+		dir = NULL;
+		name = NULL;
+	}
+
+	ret = ops->handle_inode_event(inode_mark, mask, inode, dir, name);
+	if (ret || !child_mark)
+		return ret;
+
+	/*
+	 * Some events can be sent on both parent dir and child marks
+	 * (e.g. FS_ATTRIB).  If both parent dir and child are watching,
+	 * report the event once to parent dir with name and once to child
+	 * without name.
+	 */
+	return ops->handle_inode_event(child_mark, mask, inode, NULL, NULL);
+}
+
 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)
@@ -275,8 +318,13 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 	if (!(test_mask & marks_mask & ~marks_ignored_mask))
 		return 0;
 
-	return group->ops->handle_event(group, mask, data, data_type, dir,
-					file_name, cookie, iter_info);
+	if (group->ops->handle_event) {
+		return group->ops->handle_event(group, mask, data, data_type, dir,
+						file_name, cookie, iter_info);
+	}
+
+	return fsnotify_handle_event(group, mask, data, data_type, dir,
+				     file_name, cookie, iter_info);
 }
 
 static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 32104cfc27a5..f8529a3a2923 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -128,17 +128,30 @@ struct mem_cgroup;
  * @cookie:	inotify rename cookie
  * @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
+ *		have inode marks and don't have ignore mask
+ * @mark:	mark to notify
+ * @mask:	event type and flags
+ * @inode:	inode that event happened on
+ * @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
+ *
  * free_group_priv - called when a group refcnt hits 0 to clean up the private union
  * freeing_mark - called when a mark is being destroyed for some reason.  The group
- * 		MUST be holding a reference on each mark and that reference must be
- * 		dropped in this function.  inotify uses this function to send
- * 		userspace messages that marks have been removed.
+ *		MUST be holding a reference on each mark and that reference must be
+ *		dropped in this function.  inotify uses this function to send
+ *		userspace messages that marks have been removed.
  */
 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,
 			    struct fsnotify_iter_info *iter_info);
+	int (*handle_inode_event)(struct fsnotify_mark *mark, u32 mask,
+			    struct inode *inode, struct inode *dir,
+			    const struct qstr *file_name);
 	void (*free_group_priv)(struct fsnotify_group *group);
 	void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group);
 	void (*free_event)(struct fsnotify_event *event);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index bd3a6b79316a..bfcfcd61adb6 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -152,35 +152,31 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
 }
 
 /* Update mark data in audit rules based on fsnotify events. */
-static int audit_mark_handle_event(struct fsnotify_group *group, u32 mask,
-				   const void *data, int data_type,
-				   struct inode *dir,
-				   const struct qstr *dname, u32 cookie,
-				   struct fsnotify_iter_info *iter_info)
+static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
+				   struct inode *inode, struct inode *dir,
+				   const struct qstr *dname)
 {
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct audit_fsnotify_mark *audit_mark;
-	const struct inode *inode = fsnotify_data_inode(data, data_type);
 
 	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
 
-	BUG_ON(group != audit_fsnotify_group);
-
-	if (WARN_ON(!inode))
+	if (WARN_ON_ONCE(inode_mark->group != audit_fsnotify_group) ||
+	    WARN_ON_ONCE(!inode))
 		return 0;
 
 	if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
 		if (audit_compare_dname_path(dname, audit_mark->path, AUDIT_NAME_FULL))
 			return 0;
 		audit_update_mark(audit_mark, inode);
-	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
+	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF)) {
 		audit_autoremove_mark_rule(audit_mark);
+	}
 
 	return 0;
 }
 
 static const struct fsnotify_ops audit_mark_fsnotify_ops = {
-	.handle_event =	audit_mark_handle_event,
+	.handle_inode_event = audit_mark_handle_event,
 	.free_mark = audit_fsnotify_free_mark,
 };
 
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 2ce2ac1ce100..025d24abf15d 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -1037,11 +1037,9 @@ static void evict_chunk(struct audit_chunk *chunk)
 		audit_schedule_prune();
 }
 
-static int audit_tree_handle_event(struct fsnotify_group *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 audit_tree_handle_event(struct fsnotify_mark *mark, u32 mask,
+				   struct inode *inode, struct inode *dir,
+				   const struct qstr *file_name)
 {
 	return 0;
 }
@@ -1070,7 +1068,7 @@ static void audit_tree_freeing_mark(struct fsnotify_mark *mark,
 }
 
 static const struct fsnotify_ops audit_tree_ops = {
-	.handle_event = audit_tree_handle_event,
+	.handle_inode_event = audit_tree_handle_event,
 	.freeing_mark = audit_tree_freeing_mark,
 	.free_mark = audit_tree_destroy_watch,
 };
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index e23d54bcc587..246e5ba704c0 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -464,20 +464,17 @@ void audit_remove_watch_rule(struct audit_krule *krule)
 }
 
 /* Update watch data in audit rules based on fsnotify events. */
-static int audit_watch_handle_event(struct fsnotify_group *group, u32 mask,
-				    const void *data, int data_type,
-				    struct inode *dir,
-				    const struct qstr *dname, u32 cookie,
-				    struct fsnotify_iter_info *iter_info)
+static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
+				    struct inode *inode, struct inode *dir,
+				    const struct qstr *dname)
 {
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
-	const struct inode *inode = fsnotify_data_inode(data, data_type);
 	struct audit_parent *parent;
 
 	parent = container_of(inode_mark, struct audit_parent, mark);
 
-	BUG_ON(group != audit_watch_group);
-	WARN_ON(!inode);
+	if (WARN_ON_ONCE(inode_mark->group != audit_watch_group) ||
+	    WARN_ON_ONCE(!inode))
+		return 0;
 
 	if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
 		audit_update_watch(parent, dname, inode->i_sb->s_dev, inode->i_ino, 0);
@@ -490,7 +487,7 @@ static int audit_watch_handle_event(struct fsnotify_group *group, u32 mask,
 }
 
 static const struct fsnotify_ops audit_watch_fsnotify_ops = {
-	.handle_event = 	audit_watch_handle_event,
+	.handle_inode_event =	audit_watch_handle_event,
 	.free_mark =		audit_watch_free_mark,
 };
 
-- 
2.17.1

Re: [PATCH 8/9] fsnotify: create method handle_inode_event() in fsnotify_operations

[Jan Kara]

On Wed 22-07-20 15:58:48, Amir Goldstein wrote:
> The method handle_event() grew a lot of complexity due to the design of
> fanotify and merging of ignore masks.
> 
> Most backends do not care about this complex functionality, so we can hide
> this complexity from them.
> 
> Introduce a method handle_inode_event() that serves those backends and
> passes a single inode mark and less arguments.
> 
> This change converts all backends except fanotify and inotify to use the
> simplified handle_inode_event() method.  In pricipal, inotify could have
> also used the new method, but that would require passing more arguments
> on the simple helper (data, data_type, cookie), so we leave it with the
> handle_event() method.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks. Applied.

								Honza
> ---
>  fs/nfsd/filecache.c              | 12 +++-----
>  fs/notify/dnotify/dnotify.c      | 38 +++++------------------
>  fs/notify/fsnotify.c             | 52 ++++++++++++++++++++++++++++++--
>  include/linux/fsnotify_backend.h | 19 ++++++++++--
>  kernel/audit_fsnotify.c          | 20 +++++-------
>  kernel/audit_tree.c              | 10 +++---
>  kernel/audit_watch.c             | 17 +++++------
>  7 files changed, 97 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index bbc7892d2928..c8b9d2667ee6 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -598,14 +598,10 @@ static struct notifier_block nfsd_file_lease_notifier = {
>  };
>  
>  static int
> -nfsd_file_fsnotify_handle_event(struct fsnotify_group *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)
> +nfsd_file_fsnotify_handle_event(struct fsnotify_mark *mark, u32 mask,
> +				struct inode *inode, struct inode *dir,
> +				const struct qstr *name)
>  {
> -	struct inode *inode = fsnotify_data_inode(data, data_type);
> -
>  	trace_nfsd_file_fsnotify_handle_event(inode, mask);
>  
>  	/* Should be no marks on non-regular files */
> @@ -626,7 +622,7 @@ nfsd_file_fsnotify_handle_event(struct fsnotify_group *group, u32 mask,
>  
>  
>  static const struct fsnotify_ops nfsd_file_fsnotify_ops = {
> -	.handle_event = nfsd_file_fsnotify_handle_event,
> +	.handle_inode_event = nfsd_file_fsnotify_handle_event,
>  	.free_mark = nfsd_file_mark_free,
>  };
>  
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index ca78d3f78da8..5dcda8f20c04 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -70,8 +70,9 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
>   * destroy the dnotify struct if it was not registered to receive multiple
>   * events.
>   */
> -static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
> -			      struct fsnotify_mark *inode_mark)
> +static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
> +				struct inode *inode, struct inode *dir,
> +				const struct qstr *name)
>  {
>  	struct dnotify_mark *dn_mark;
>  	struct dnotify_struct *dn;
> @@ -79,6 +80,10 @@ static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
>  	struct fown_struct *fown;
>  	__u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
>  
> +	/* not a dir, dnotify doesn't care */
> +	if (!dir && !(mask & FS_ISDIR))
> +		return 0;
> +
>  	dn_mark = container_of(inode_mark, struct dnotify_mark, fsn_mark);
>  
>  	spin_lock(&inode_mark->lock);
> @@ -100,33 +105,6 @@ static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
>  	}
>  
>  	spin_unlock(&inode_mark->lock);
> -}
> -
> -static int dnotify_handle_event(struct fsnotify_group *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)
> -{
> -	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> -	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
> -
> -	/* not a dir, dnotify doesn't care */
> -	if (!dir && !(mask & FS_ISDIR))
> -		return 0;
> -
> -	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
> -		return 0;
> -
> -	/*
> -	 * Some events can be sent on both parent dir and subdir marks
> -	 * (e.g. DN_ATTRIB).  If both parent dir and subdir are watching,
> -	 * report the event once to parent dir and once to subdir.
> -	 */
> -	if (inode_mark)
> -		dnotify_one_event(group, mask, inode_mark);
> -	if (child_mark)
> -		dnotify_one_event(group, mask, child_mark);
>  
>  	return 0;
>  }
> @@ -143,7 +121,7 @@ static void dnotify_free_mark(struct fsnotify_mark *fsn_mark)
>  }
>  
>  static const struct fsnotify_ops dnotify_fsnotify_ops = {
> -	.handle_event = dnotify_handle_event,
> +	.handle_inode_event = dnotify_handle_event,
>  	.free_mark = dnotify_free_mark,
>  };
>  
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 494d5d70323f..a960ec3a569a 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -230,6 +230,49 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
>  }
>  EXPORT_SYMBOL_GPL(__fsnotify_parent);
>  
> +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)
> +{
> +	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> +	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
> +	struct inode *inode = fsnotify_data_inode(data, data_type);
> +	const struct fsnotify_ops *ops = group->ops;
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!ops->handle_inode_event))
> +		return 0;
> +
> +	if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
> +	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
> +		return 0;
> +
> +	/*
> +	 * An event can be sent on child mark iterator instead of inode mark
> +	 * iterator because of other groups that have interest of this inode
> +	 * and have marks on both parent and child.  We can simplify this case.
> +	 */
> +	if (!inode_mark) {
> +		inode_mark = child_mark;
> +		child_mark = NULL;
> +		dir = NULL;
> +		name = NULL;
> +	}
> +
> +	ret = ops->handle_inode_event(inode_mark, mask, inode, dir, name);
> +	if (ret || !child_mark)
> +		return ret;
> +
> +	/*
> +	 * Some events can be sent on both parent dir and child marks
> +	 * (e.g. FS_ATTRIB).  If both parent dir and child are watching,
> +	 * report the event once to parent dir with name and once to child
> +	 * without name.
> +	 */
> +	return ops->handle_inode_event(child_mark, mask, inode, NULL, NULL);
> +}
> +
>  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)
> @@ -275,8 +318,13 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
>  	if (!(test_mask & marks_mask & ~marks_ignored_mask))
>  		return 0;
>  
> -	return group->ops->handle_event(group, mask, data, data_type, dir,
> -					file_name, cookie, iter_info);
> +	if (group->ops->handle_event) {
> +		return group->ops->handle_event(group, mask, data, data_type, dir,
> +						file_name, cookie, iter_info);
> +	}
> +
> +	return fsnotify_handle_event(group, mask, data, data_type, dir,
> +				     file_name, cookie, iter_info);
>  }
>  
>  static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp)
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 32104cfc27a5..f8529a3a2923 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -128,17 +128,30 @@ struct mem_cgroup;
>   * @cookie:	inotify rename cookie
>   * @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
> + *		have inode marks and don't have ignore mask
> + * @mark:	mark to notify
> + * @mask:	event type and flags
> + * @inode:	inode that event happened on
> + * @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
> + *
>   * free_group_priv - called when a group refcnt hits 0 to clean up the private union
>   * freeing_mark - called when a mark is being destroyed for some reason.  The group
> - * 		MUST be holding a reference on each mark and that reference must be
> - * 		dropped in this function.  inotify uses this function to send
> - * 		userspace messages that marks have been removed.
> + *		MUST be holding a reference on each mark and that reference must be
> + *		dropped in this function.  inotify uses this function to send
> + *		userspace messages that marks have been removed.
>   */
>  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,
>  			    struct fsnotify_iter_info *iter_info);
> +	int (*handle_inode_event)(struct fsnotify_mark *mark, u32 mask,
> +			    struct inode *inode, struct inode *dir,
> +			    const struct qstr *file_name);
>  	void (*free_group_priv)(struct fsnotify_group *group);
>  	void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group);
>  	void (*free_event)(struct fsnotify_event *event);
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index bd3a6b79316a..bfcfcd61adb6 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -152,35 +152,31 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
>  }
>  
>  /* Update mark data in audit rules based on fsnotify events. */
> -static int audit_mark_handle_event(struct fsnotify_group *group, u32 mask,
> -				   const void *data, int data_type,
> -				   struct inode *dir,
> -				   const struct qstr *dname, u32 cookie,
> -				   struct fsnotify_iter_info *iter_info)
> +static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
> +				   struct inode *inode, struct inode *dir,
> +				   const struct qstr *dname)
>  {
> -	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
>  	struct audit_fsnotify_mark *audit_mark;
> -	const struct inode *inode = fsnotify_data_inode(data, data_type);
>  
>  	audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
>  
> -	BUG_ON(group != audit_fsnotify_group);
> -
> -	if (WARN_ON(!inode))
> +	if (WARN_ON_ONCE(inode_mark->group != audit_fsnotify_group) ||
> +	    WARN_ON_ONCE(!inode))
>  		return 0;
>  
>  	if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) {
>  		if (audit_compare_dname_path(dname, audit_mark->path, AUDIT_NAME_FULL))
>  			return 0;
>  		audit_update_mark(audit_mark, inode);
> -	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
> +	} else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF)) {
>  		audit_autoremove_mark_rule(audit_mark);
> +	}
>  
>  	return 0;
>  }
>  
>  static const struct fsnotify_ops audit_mark_fsnotify_ops = {
> -	.handle_event =	audit_mark_handle_event,
> +	.handle_inode_event = audit_mark_handle_event,
>  	.free_mark = audit_fsnotify_free_mark,
>  };
>  
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 2ce2ac1ce100..025d24abf15d 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -1037,11 +1037,9 @@ static void evict_chunk(struct audit_chunk *chunk)
>  		audit_schedule_prune();
>  }
>  
> -static int audit_tree_handle_event(struct fsnotify_group *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 audit_tree_handle_event(struct fsnotify_mark *mark, u32 mask,
> +				   struct inode *inode, struct inode *dir,
> +				   const struct qstr *file_name)
>  {
>  	return 0;
>  }
> @@ -1070,7 +1068,7 @@ static void audit_tree_freeing_mark(struct fsnotify_mark *mark,
>  }
>  
>  static const struct fsnotify_ops audit_tree_ops = {
> -	.handle_event = audit_tree_handle_event,
> +	.handle_inode_event = audit_tree_handle_event,
>  	.freeing_mark = audit_tree_freeing_mark,
>  	.free_mark = audit_tree_destroy_watch,
>  };
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index e23d54bcc587..246e5ba704c0 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -464,20 +464,17 @@ void audit_remove_watch_rule(struct audit_krule *krule)
>  }
>  
>  /* Update watch data in audit rules based on fsnotify events. */
> -static int audit_watch_handle_event(struct fsnotify_group *group, u32 mask,
> -				    const void *data, int data_type,
> -				    struct inode *dir,
> -				    const struct qstr *dname, u32 cookie,
> -				    struct fsnotify_iter_info *iter_info)
> +static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
> +				    struct inode *inode, struct inode *dir,
> +				    const struct qstr *dname)
>  {
> -	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> -	const struct inode *inode = fsnotify_data_inode(data, data_type);
>  	struct audit_parent *parent;
>  
>  	parent = container_of(inode_mark, struct audit_parent, mark);
>  
> -	BUG_ON(group != audit_watch_group);
> -	WARN_ON(!inode);
> +	if (WARN_ON_ONCE(inode_mark->group != audit_watch_group) ||
> +	    WARN_ON_ONCE(!inode))
> +		return 0;
>  
>  	if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
>  		audit_update_watch(parent, dname, inode->i_sb->s_dev, inode->i_ino, 0);
> @@ -490,7 +487,7 @@ static int audit_watch_handle_event(struct fsnotify_group *group, u32 mask,
>  }
>  
>  static const struct fsnotify_ops audit_watch_fsnotify_ops = {
> -	.handle_event = 	audit_watch_handle_event,
> +	.handle_inode_event =	audit_watch_handle_event,
>  	.free_mark =		audit_watch_free_mark,
>  };
>  
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 9/9] fsnotify: pass inode to fsnotify_parent()

[Amir Goldstein]

We can get inode by dereferenceing dentry->d_inode, but that may have
performance impact in the fast path of non watched file.

Kernel test robot reported a performance regression in concurrent open
workload, so maybe that can fix it.

Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: c738fbabb0ff ("fsnotify: fold fsnotify() call into fsnotify_parent()")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fsnotify.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index d9b26c6552ee..4a9b2f5b819b 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -49,10 +49,9 @@ static inline void fsnotify_inode(struct inode *inode, __u32 mask)
 
 /* Notify this dentry's parent about a child's events. */
 static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
-				  const void *data, int data_type)
+				  const void *data, int data_type,
+				  struct inode *inode)
 {
-	struct inode *inode = d_inode(dentry);
-
 	if (S_ISDIR(inode->i_mode)) {
 		mask |= FS_ISDIR;
 
@@ -77,7 +76,8 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
  */
 static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
 {
-	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE);
+	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
+			d_inode(dentry));
 }
 
 static inline int fsnotify_file(struct file *file, __u32 mask)
@@ -87,7 +87,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
 	if (file->f_mode & FMODE_NONOTIFY)
 		return 0;
 
-	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
+	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH,
+			       file_inode(file));
 }
 
 /* Simple call site for access decisions */
-- 
2.17.1

Re: [PATCH 9/9] fsnotify: pass inode to fsnotify_parent()

[Jan Kara]

On Wed 22-07-20 15:58:49, Amir Goldstein wrote:
> We can get inode by dereferenceing dentry->d_inode, but that may have
> performance impact in the fast path of non watched file.
> 
> Kernel test robot reported a performance regression in concurrent open
> workload, so maybe that can fix it.
> 
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Fixes: c738fbabb0ff ("fsnotify: fold fsnotify() call into fsnotify_parent()")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I didn't take this patch because honestly, I don't think the extra argument
is worth it unless we can prove real performance benefit...

								Honza

> ---
>  include/linux/fsnotify.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index d9b26c6552ee..4a9b2f5b819b 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -49,10 +49,9 @@ static inline void fsnotify_inode(struct inode *inode, __u32 mask)
>  
>  /* Notify this dentry's parent about a child's events. */
>  static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
> -				  const void *data, int data_type)
> +				  const void *data, int data_type,
> +				  struct inode *inode)
>  {
> -	struct inode *inode = d_inode(dentry);
> -
>  	if (S_ISDIR(inode->i_mode)) {
>  		mask |= FS_ISDIR;
>  
> @@ -77,7 +76,8 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
>   */
>  static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
>  {
> -	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE);
> +	fsnotify_parent(dentry, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> +			d_inode(dentry));
>  }
>  
>  static inline int fsnotify_file(struct file *file, __u32 mask)
> @@ -87,7 +87,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
>  	if (file->f_mode & FMODE_NONOTIFY)
>  		return 0;
>  
> -	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
> +	return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH,
> +			       file_inode(file));
>  }
>  
>  /* Simple call site for access decisions */
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/9] Fixes for fanotify name events

[Jan Kara]

On Wed 22-07-20 15:58:40, Amir Goldstein wrote:
> Jan,
> 
> Following your feedback [1] to fanotify name events, I wrote some LTP
> tests [2] to add missing test coverage:
> 
> 1) dnotify/inotify: report event to both parent and child -
>    catches the dnotify bug I had in v4 after unified event patch
> 
> 2) fanotify10: add groups with FAN_REPORT_NAME to the setup -
>    catches the bug you noticed in fanotify_group_event_mask()
> 
> 3) fanotify10: add test cases with ignored mask on watching parent -
>    catches the inconsistecy with ignored masks that you noticed [*]
> 
> The patches in this series apply to your fsnotify branch and are
> avaiable on my fsnotify-fixes branch [3].
> 
> Patch 1 fixes issue #2 above
> Patch 2 fixes another issue found by tests
> Patch 3 fixes a minor issue found by code review
> Patches 4-6 simplify the code based on your suggestions
> Patch 7 depends on 4-6 and fixes issue #3 above [*]
> 
> Optional patches:
> Patch 8 implements your suggestion of simplified handler_event()
> Patch 9 is a possible fix for kernel test robot reported performance
> regression. I did not get any feedback on it, but it is trivial.

OK, so I've added patches 1-8 to my tree. I've checked that the final
resulting source after my patch reorg is the same as after just applying
the patches. LTP tests pass so I've pushed out everything to linux-next to
give it some more beating. So everything should be ready for the merge
window.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR