[PATCH v2 00/20] fanotify: super block mark

[PATCH v2 00/20] fanotify: super block mark

[Amir Goldstein]

Hi Jan,

It's been over a year since I posted my last patch revision [1].
This series, also available on my fanoitfy_sb branch [2], addresses
your feedback to attach marks to a super block instead of the root inode.

Patch 1 fixes a bug with ignore mask, which I had already posted.
Most of the patches thereafter make the code handling inode and vfsmount
marks generic for N object types (where N=2).
Patches 17-18 add the super block mark object type and the bits of
non generic code needed to support it.
Patches 19-20 add the API and implementation for FAN_MARK_FILESSYTEM
to fanotify backend.

I tested this series with existing fsnotify LTP tests and have written
another four LTP tests [3] - one regression test for patch [1/20] and
three tests to verify functionality of a super block mark.

For context, here are my TODO items for a followup series:
- Add support for fanotify_init() flags FAN_EVENT_INFO_NOFD
  and FAN_EVENT_INFO_FH to report file handle on event instead
  of file descriptor.
- Support events with data type FSNOTIFY_EVENT_INODE on an
  inode or super block mark to a group that was initialized
  with flag FAN_EVENT_INFO_FH.
- Add support for fanotify_init() flags FAN_EVENT_INFO_FILENAME
  and FAN_EVENT_INFO_COOKIE to achieve the full functionality
  available with inotify.

Please let me know what you think of this series as well as
future plans.

Thanks,
Amir.

[1] https://lwn.net/Articles/716973/
[2] https://github.com/amir73il/linux/commits/fanotify_sb
[3] https://github.com/amir73il/ltp/commits/fanotify_sb

Amir Goldstein (20):
  fanotify: fix logic of events on child
  fsnotify: fix ignore mask logic in send_to_group()
  fsnotify: fix typo in a comment about mark->g_list
  MAINTAINERS: add an entry for FSNOTIFY infrastructure
  fsnotify: use type id to identify connector object type
  fsnotify: remove redundant arguments to handle_event()
  fsnotify: introduce marks iteration helpers
  fsnotify: generalize iteration of marks by object type
  fsnotify: generalize send_to_group()
  fanotify: generalize fanotify_should_send_event()
  fsnotify: add fsnotify_add_inode_mark() wrappers
  fsnotify: introduce prototype struct fsnotify_obj
  fsnotify: pass fsnotify_obj instead of **connp argument
  fsnotify: pass object and object type to fsnotify_add_mark()
  fsnotify: make fsnotify_recalc_mask() unaware of object type
  fsnotify: generalize fsnotify_detach_connector_from_object()
  fsnotify: add super block object type
  fsnotify: send path type events to group with super block marks
  fanotify: factor out helpers to add/remove mark
  fanotify: add API to attach/detach super block mark

 MAINTAINERS                          |   8 ++
 fs/inode.c                           |   2 +-
 fs/mount.h                           |   4 +-
 fs/notify/dnotify/dnotify.c          |  27 ++---
 fs/notify/fanotify/fanotify.c        |  45 ++++-----
 fs/notify/fanotify/fanotify_user.c   | 135 ++++++++++++-------------
 fs/notify/fdinfo.c                   |   6 +-
 fs/notify/fsnotify.c                 | 189 ++++++++++++++++++++---------------
 fs/notify/fsnotify.h                 |  34 +++++--
 fs/notify/group.c                    |   2 +-
 fs/notify/inotify/inotify.h          |   2 -
 fs/notify/inotify/inotify_fsnotify.c |   6 +-
 fs/notify/inotify/inotify_user.c     |  14 +--
 fs/notify/mark.c                     | 166 +++++++++++++++---------------
 fs/super.c                           |   2 +-
 include/linux/fs.h                   |  10 +-
 include/linux/fsnotify.h             |   8 ++
 include/linux/fsnotify_backend.h     | 134 ++++++++++++++++++++-----
 include/linux/fsnotify_obj.h         |  14 +++
 include/uapi/linux/fanotify.h        |   7 +-
 kernel/audit_fsnotify.c              |   5 +-
 kernel/audit_tree.c                  |  15 ++-
 kernel/audit_watch.c                 |   7 +-
 kernel/auditsc.c                     |   4 +-
 24 files changed, 507 insertions(+), 339 deletions(-)
 create mode 100644 include/linux/fsnotify_obj.h

-- 
2.7.4

[PATCH v2 01/20] fanotify: fix logic of events on child

[Amir Goldstein]

When event on child inodes are sent to the parent inode mark and
parent inode mark was not marked with FAN_EVENT_ON_CHILD, the event
will not be delivered to the listener process. However, if the same
process also has a mount mark, the event to the parent inode will be
delivered regadless of the mount mark mask.

This behavior is incorrect in the case where the mount mark mask does
not contain the specific event type. For example, the process adds
a mark on a directory with mask FAN_MODIFY (without FAN_EVENT_ON_CHILD)
and a mount mark with mask FAN_CLOSE_NOWRITE (without FAN_ONDIR).

A modify event on a file inside that directory (and inside that mount)
should not create a FAN_MODIFY event, because neither of the marks
requested to get that event on the file.

Fixes: 1968f5eed54c ("fanotify: use both marks when possible")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 6702a6a0bbb5..e0e6a9d627df 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -92,7 +92,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 				       u32 event_mask,
 				       const void *data, int data_type)
 {
-	__u32 marks_mask, marks_ignored_mask;
+	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	const struct path *path = data;
 
 	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
@@ -108,24 +108,20 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	    !d_can_lookup(path->dentry))
 		return false;
 
-	if (inode_mark && vfsmnt_mark) {
-		marks_mask = (vfsmnt_mark->mask | inode_mark->mask);
-		marks_ignored_mask = (vfsmnt_mark->ignored_mask | inode_mark->ignored_mask);
-	} else if (inode_mark) {
-		/*
-		 * if the event is for a child and this inode doesn't care about
-		 * events on the child, don't send it!
-		 */
-		if ((event_mask & FS_EVENT_ON_CHILD) &&
-		    !(inode_mark->mask & FS_EVENT_ON_CHILD))
-			return false;
-		marks_mask = inode_mark->mask;
-		marks_ignored_mask = inode_mark->ignored_mask;
-	} else if (vfsmnt_mark) {
-		marks_mask = vfsmnt_mark->mask;
-		marks_ignored_mask = vfsmnt_mark->ignored_mask;
-	} else {
-		BUG();
+	/*
+	 * if the event is for a child and this inode doesn't care about
+	 * events on the child, don't send it!
+	 */
+	if (inode_mark &&
+	    (!(event_mask & FS_EVENT_ON_CHILD) ||
+	     (inode_mark->mask & FS_EVENT_ON_CHILD))) {
+		marks_mask |= inode_mark->mask;
+		marks_ignored_mask |= inode_mark->ignored_mask;
+	}
+
+	if (vfsmnt_mark) {
+		marks_mask |= vfsmnt_mark->mask;
+		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
 	}
 
 	if (d_is_dir(path->dentry) &&
-- 
2.7.4

Re: [PATCH v2 01/20] fanotify: fix logic of events on child

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 1968f5eed54c fanotify: use both marks when possible.

The bot has also determined it's probably a bug fixing patch. (score: 70.2871)

The bot has tested the following trees: v4.16, v4.15.15, v4.14.32, v4.9.92, v4.4.126.

v4.16: Build OK!
v4.15.15: Build OK!
v4.14.32: Build OK!
v4.9.92: Failed to apply! Possible dependencies:
    3cd5eca8d7a2 ("fsnotify: constify 'data' passed to ->handle_event()")

v4.4.126: Failed to apply! Possible dependencies:
    3cd5eca8d7a2 ("fsnotify: constify 'data' passed to ->handle_event()")


--
Thanks,
Sasha

Re: [PATCH v2 01/20] fanotify: fix logic of events on child

[Jan Kara]

On Thu 05-04-18 16:18:02, Amir Goldstein wrote:
> When event on child inodes are sent to the parent inode mark and
> parent inode mark was not marked with FAN_EVENT_ON_CHILD, the event
> will not be delivered to the listener process. However, if the same
> process also has a mount mark, the event to the parent inode will be
> delivered regadless of the mount mark mask.
> 
> This behavior is incorrect in the case where the mount mark mask does
> not contain the specific event type. For example, the process adds
> a mark on a directory with mask FAN_MODIFY (without FAN_EVENT_ON_CHILD)
> and a mount mark with mask FAN_CLOSE_NOWRITE (without FAN_ONDIR).
> 
> A modify event on a file inside that directory (and inside that mount)
> should not create a FAN_MODIFY event, because neither of the marks
> requested to get that event on the file.
> 
> Fixes: 1968f5eed54c ("fanotify: use both marks when possible")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

This is already in my tree and I'll push it to Linus next week. JFYI.

								Honza

> ---
>  fs/notify/fanotify/fanotify.c | 34 +++++++++++++++-------------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 6702a6a0bbb5..e0e6a9d627df 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -92,7 +92,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  				       u32 event_mask,
>  				       const void *data, int data_type)
>  {
> -	__u32 marks_mask, marks_ignored_mask;
> +	__u32 marks_mask = 0, marks_ignored_mask = 0;
>  	const struct path *path = data;
>  
>  	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
> @@ -108,24 +108,20 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>  	    !d_can_lookup(path->dentry))
>  		return false;
>  
> -	if (inode_mark && vfsmnt_mark) {
> -		marks_mask = (vfsmnt_mark->mask | inode_mark->mask);
> -		marks_ignored_mask = (vfsmnt_mark->ignored_mask | inode_mark->ignored_mask);
> -	} else if (inode_mark) {
> -		/*
> -		 * if the event is for a child and this inode doesn't care about
> -		 * events on the child, don't send it!
> -		 */
> -		if ((event_mask & FS_EVENT_ON_CHILD) &&
> -		    !(inode_mark->mask & FS_EVENT_ON_CHILD))
> -			return false;
> -		marks_mask = inode_mark->mask;
> -		marks_ignored_mask = inode_mark->ignored_mask;
> -	} else if (vfsmnt_mark) {
> -		marks_mask = vfsmnt_mark->mask;
> -		marks_ignored_mask = vfsmnt_mark->ignored_mask;
> -	} else {
> -		BUG();
> +	/*
> +	 * if the event is for a child and this inode doesn't care about
> +	 * events on the child, don't send it!
> +	 */
> +	if (inode_mark &&
> +	    (!(event_mask & FS_EVENT_ON_CHILD) ||
> +	     (inode_mark->mask & FS_EVENT_ON_CHILD))) {
> +		marks_mask |= inode_mark->mask;
> +		marks_ignored_mask |= inode_mark->ignored_mask;
> +	}
> +
> +	if (vfsmnt_mark) {
> +		marks_mask |= vfsmnt_mark->mask;
> +		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
>  	}
>  
>  	if (d_is_dir(path->dentry) &&
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 02/20] fsnotify: fix ignore mask logic in send_to_group()

[Amir Goldstein]

The ignore mask logic in send_to_group() does not match the logic
in fanotify_should_send_event(). In the latter, a vfsmount mark ignore
mask precedes an inode mark mask and in the former, it does not.

That difference may cause events to be sent to fanotify backend for no
reason. Fix the logic in send_to_group() to match that of
fanotify_should_send_event().

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

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 219b269c737e..613ec7e5a465 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -192,8 +192,9 @@ static int send_to_group(struct inode *to_tell,
 			 struct fsnotify_iter_info *iter_info)
 {
 	struct fsnotify_group *group = NULL;
-	__u32 inode_test_mask = 0;
-	__u32 vfsmount_test_mask = 0;
+	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
+	__u32 marks_mask = 0;
+	__u32 marks_ignored_mask = 0;
 
 	if (unlikely(!inode_mark && !vfsmount_mark)) {
 		BUG();
@@ -213,29 +214,25 @@ static int send_to_group(struct inode *to_tell,
 	/* does the inode mark tell us to do something? */
 	if (inode_mark) {
 		group = inode_mark->group;
-		inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
-		inode_test_mask &= inode_mark->mask;
-		inode_test_mask &= ~inode_mark->ignored_mask;
+		marks_mask |= inode_mark->mask;
+		marks_ignored_mask |= inode_mark->ignored_mask;
 	}
 
 	/* does the vfsmount_mark tell us to do something? */
 	if (vfsmount_mark) {
-		vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 		group = vfsmount_mark->group;
-		vfsmount_test_mask &= vfsmount_mark->mask;
-		vfsmount_test_mask &= ~vfsmount_mark->ignored_mask;
-		if (inode_mark)
-			vfsmount_test_mask &= ~inode_mark->ignored_mask;
+		marks_mask |= vfsmount_mark->mask;
+		marks_ignored_mask |= vfsmount_mark->ignored_mask;
 	}
 
 	pr_debug("%s: group=%p to_tell=%p mask=%x inode_mark=%p"
-		 " inode_test_mask=%x vfsmount_mark=%p vfsmount_test_mask=%x"
+		 " vfsmount_mark=%p marks_mask=%x marks_ignored_mask=%x"
 		 " data=%p data_is=%d cookie=%d\n",
-		 __func__, group, to_tell, mask, inode_mark,
-		 inode_test_mask, vfsmount_mark, vfsmount_test_mask, data,
+		 __func__, group, to_tell, mask, inode_mark, vfsmount_mark,
+		 marks_mask, marks_ignored_mask, data,
 		 data_is, cookie);
 
-	if (!inode_test_mask && !vfsmount_test_mask)
+	if (!(test_mask & marks_mask & ~marks_ignored_mask))
 		return 0;
 
 	return group->ops->handle_event(group, to_tell, inode_mark,
-- 
2.7.4

Re: [PATCH v2 02/20] fsnotify: fix ignore mask logic in send_to_group()

[Jan Kara]

On Thu 05-04-18 16:18:03, Amir Goldstein wrote:
> The ignore mask logic in send_to_group() does not match the logic
> in fanotify_should_send_event(). In the latter, a vfsmount mark ignore
> mask precedes an inode mark mask and in the former, it does not.
> 
> That difference may cause events to be sent to fanotify backend for no
> reason. Fix the logic in send_to_group() to match that of
> fanotify_should_send_event().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks. The patch looks good and I've merged it to my tree.

								Honza

> ---
>  fs/notify/fsnotify.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 219b269c737e..613ec7e5a465 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -192,8 +192,9 @@ static int send_to_group(struct inode *to_tell,
>  			 struct fsnotify_iter_info *iter_info)
>  {
>  	struct fsnotify_group *group = NULL;
> -	__u32 inode_test_mask = 0;
> -	__u32 vfsmount_test_mask = 0;
> +	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
> +	__u32 marks_mask = 0;
> +	__u32 marks_ignored_mask = 0;
>  
>  	if (unlikely(!inode_mark && !vfsmount_mark)) {
>  		BUG();
> @@ -213,29 +214,25 @@ static int send_to_group(struct inode *to_tell,
>  	/* does the inode mark tell us to do something? */
>  	if (inode_mark) {
>  		group = inode_mark->group;
> -		inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
> -		inode_test_mask &= inode_mark->mask;
> -		inode_test_mask &= ~inode_mark->ignored_mask;
> +		marks_mask |= inode_mark->mask;
> +		marks_ignored_mask |= inode_mark->ignored_mask;
>  	}
>  
>  	/* does the vfsmount_mark tell us to do something? */
>  	if (vfsmount_mark) {
> -		vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
>  		group = vfsmount_mark->group;
> -		vfsmount_test_mask &= vfsmount_mark->mask;
> -		vfsmount_test_mask &= ~vfsmount_mark->ignored_mask;
> -		if (inode_mark)
> -			vfsmount_test_mask &= ~inode_mark->ignored_mask;
> +		marks_mask |= vfsmount_mark->mask;
> +		marks_ignored_mask |= vfsmount_mark->ignored_mask;
>  	}
>  
>  	pr_debug("%s: group=%p to_tell=%p mask=%x inode_mark=%p"
> -		 " inode_test_mask=%x vfsmount_mark=%p vfsmount_test_mask=%x"
> +		 " vfsmount_mark=%p marks_mask=%x marks_ignored_mask=%x"
>  		 " data=%p data_is=%d cookie=%d\n",
> -		 __func__, group, to_tell, mask, inode_mark,
> -		 inode_test_mask, vfsmount_mark, vfsmount_test_mask, data,
> +		 __func__, group, to_tell, mask, inode_mark, vfsmount_mark,
> +		 marks_mask, marks_ignored_mask, data,
>  		 data_is, cookie);
>  
> -	if (!inode_test_mask && !vfsmount_test_mask)
> +	if (!(test_mask & marks_mask & ~marks_ignored_mask))
>  		return 0;
>  
>  	return group->ops->handle_event(group, to_tell, inode_mark,
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 03/20] fsnotify: fix typo in a comment about mark->g_list

[Amir Goldstein]

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fsnotify_backend.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 067d52e95f02..759ba9113ec4 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -248,7 +248,7 @@ struct fsnotify_mark {
 	/* Group this mark is for. Set on mark creation, stable until last ref
 	 * is dropped */
 	struct fsnotify_group *group;
-	/* List of marks by group->i_fsnotify_marks. Also reused for queueing
+	/* List of marks by group->marks_list. Also reused for queueing
 	 * mark into destroy_list when it's waiting for the end of SRCU period
 	 * before it can be freed. [group->mark_mutex] */
 	struct list_head g_list;
-- 
2.7.4

Re: [PATCH v2 03/20] fsnotify: fix typo in a comment about mark->g_list

[Jan Kara]

On Thu 05-04-18 16:18:04, Amir Goldstein wrote:
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks. Applied.

								Honza

> ---
>  include/linux/fsnotify_backend.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 067d52e95f02..759ba9113ec4 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -248,7 +248,7 @@ struct fsnotify_mark {
>  	/* Group this mark is for. Set on mark creation, stable until last ref
>  	 * is dropped */
>  	struct fsnotify_group *group;
> -	/* List of marks by group->i_fsnotify_marks. Also reused for queueing
> +	/* List of marks by group->marks_list. Also reused for queueing
>  	 * mark into destroy_list when it's waiting for the end of SRCU period
>  	 * before it can be freed. [group->mark_mutex] */
>  	struct list_head g_list;
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 04/20] MAINTAINERS: add an entry for FSNOTIFY infrastructure

[Amir Goldstein]

There is alreay an entry for all the backends, but those entries do
not cover all the fsnotify files.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 73c0cdabf755..719a4e11ab59 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5799,6 +5799,14 @@ F:	fs/crypto/
 F:	include/linux/fscrypt*.h
 F:	Documentation/filesystems/fscrypt.rst
 
+FSNOTIFY: FILESYSTEM NOTIFICATION INFRASTRUCTURE
+M:	Jan Kara <jack@suse.cz>
+R:	Amir Goldstein <amir73il@gmail.com>
+L:	linux-fsdevel@vger.kernel.org
+S:	Maintained
+F:	fs/notify/
+F:	include/linux/fsnotify*.h
+
 FUJITSU FR-V (FRV) PORT
 S:	Orphan
 F:	arch/frv/
-- 
2.7.4

Re: [PATCH v2 04/20] MAINTAINERS: add an entry for FSNOTIFY infrastructure

[Jan Kara]

On Thu 05-04-18 16:18:05, Amir Goldstein wrote:
> There is alreay an entry for all the backends, but those entries do
> not cover all the fsnotify files.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Thanks. Applied.

								Honza

> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 73c0cdabf755..719a4e11ab59 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5799,6 +5799,14 @@ F:	fs/crypto/
>  F:	include/linux/fscrypt*.h
>  F:	Documentation/filesystems/fscrypt.rst
>  
> +FSNOTIFY: FILESYSTEM NOTIFICATION INFRASTRUCTURE
> +M:	Jan Kara <jack@suse.cz>
> +R:	Amir Goldstein <amir73il@gmail.com>
> +L:	linux-fsdevel@vger.kernel.org
> +S:	Maintained
> +F:	fs/notify/
> +F:	include/linux/fsnotify*.h
> +
>  FUJITSU FR-V (FRV) PORT
>  S:	Orphan
>  F:	arch/frv/
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 05/20] fsnotify: use type id to identify connector object type

[Amir Goldstein]

An fsnotify_mark_connector is referencing a single type of object
(either inode or vfsmount). Instead of storing a type mask in
connector->flags, store a single type id in connector->type to
identify the type of object.

When a connector object is detached from the object, its type is set
to FSNOTIFY_OBJ_TYPE_DETACHED and this object is not going to be
reused.

The function fsnotify_clear_marks_by_group() is the only place where
type mask was used, so use type flags instead of type id to this
function.

This change is going to be more convenient when adding a new object
type (super block).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fdinfo.c               |  6 +++---
 fs/notify/group.c                |  2 +-
 fs/notify/mark.c                 | 29 ++++++++++++++---------------
 include/linux/fsnotify_backend.h | 21 ++++++++++++++-------
 4 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index d478629c728b..10aac1942c9f 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -77,7 +77,7 @@ static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 	struct inotify_inode_mark *inode_mark;
 	struct inode *inode;
 
-	if (!(mark->connector->flags & FSNOTIFY_OBJ_TYPE_INODE))
+	if (mark->connector->type != FSNOTIFY_OBJ_TYPE_INODE)
 		return;
 
 	inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark);
@@ -116,7 +116,7 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 	if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
 		mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
 
-	if (mark->connector->flags & FSNOTIFY_OBJ_TYPE_INODE) {
+	if (mark->connector->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = igrab(mark->connector->inode);
 		if (!inode)
 			return;
@@ -126,7 +126,7 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		show_mark_fhandle(m, inode);
 		seq_putc(m, '\n');
 		iput(inode);
-	} else if (mark->connector->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
+	} else if (mark->connector->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
 		struct mount *mnt = real_mount(mark->connector->mnt);
 
 		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
diff --git a/fs/notify/group.c b/fs/notify/group.c
index b7a4b6a69efa..aa5468f23e45 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -67,7 +67,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
 	fsnotify_group_stop_queueing(group);
 
 	/* Clear all marks for this group and queue them for destruction */
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_ALL_TYPES);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_ALL_TYPES_MASK);
 
 	/*
 	 * Some marks can still be pinned when waiting for response from
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index e9191b416434..ef44808b28ca 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -119,9 +119,9 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
 			new_mask |= mark->mask;
 	}
-	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		conn->inode->i_fsnotify_mask = new_mask;
-	else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 		real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask;
 }
 
@@ -139,7 +139,7 @@ void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	spin_lock(&conn->lock);
 	__fsnotify_recalc_mask(conn);
 	spin_unlock(&conn->lock);
-	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		__fsnotify_update_child_dentry_flags(conn->inode);
 }
 
@@ -166,18 +166,18 @@ static struct inode *fsnotify_detach_connector_from_object(
 {
 	struct inode *inode = NULL;
 
-	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) {
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = conn->inode;
 		rcu_assign_pointer(inode->i_fsnotify_marks, NULL);
 		inode->i_fsnotify_mask = 0;
 		conn->inode = NULL;
-		conn->flags &= ~FSNOTIFY_OBJ_TYPE_INODE;
-	} else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
+		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
+	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
 		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify_marks,
 				   NULL);
 		real_mount(conn->mnt)->mnt_fsnotify_mask = 0;
 		conn->mnt = NULL;
-		conn->flags &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
 	}
 
 	return inode;
@@ -442,10 +442,10 @@ static int fsnotify_attach_connector_to_object(
 	spin_lock_init(&conn->lock);
 	INIT_HLIST_HEAD(&conn->list);
 	if (inode) {
-		conn->flags = FSNOTIFY_OBJ_TYPE_INODE;
+		conn->type = FSNOTIFY_OBJ_TYPE_INODE;
 		conn->inode = igrab(inode);
 	} else {
-		conn->flags = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+		conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
 		conn->mnt = mnt;
 	}
 	/*
@@ -479,8 +479,7 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector(
 	if (!conn)
 		goto out;
 	spin_lock(&conn->lock);
-	if (!(conn->flags & (FSNOTIFY_OBJ_TYPE_INODE |
-			     FSNOTIFY_OBJ_TYPE_VFSMOUNT))) {
+	if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) {
 		spin_unlock(&conn->lock);
 		srcu_read_unlock(&fsnotify_mark_srcu, idx);
 		return NULL;
@@ -646,16 +645,16 @@ struct fsnotify_mark *fsnotify_find_mark(
 	return NULL;
 }
 
-/* Clear any marks in a group with given type */
+/* Clear any marks in a group with given type mask */
 void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
-				   unsigned int type)
+				   unsigned int type_mask)
 {
 	struct fsnotify_mark *lmark, *mark;
 	LIST_HEAD(to_free);
 	struct list_head *head = &to_free;
 
 	/* Skip selection step if we want to clear all marks. */
-	if (type == FSNOTIFY_OBJ_ALL_TYPES) {
+	if (type_mask == FSNOTIFY_OBJ_ALL_TYPES_MASK) {
 		head = &group->marks_list;
 		goto clear;
 	}
@@ -670,7 +669,7 @@ void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
 	 */
 	mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
 	list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
-		if (mark->connector->flags & type)
+		if ((1U << mark->connector->type) & type_mask)
 			list_move(&mark->g_list, &to_free);
 	}
 	mutex_unlock(&group->mark_mutex);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 759ba9113ec4..9c690eb692a2 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -201,6 +201,17 @@ struct fsnotify_group {
 #define FSNOTIFY_EVENT_PATH	1
 #define FSNOTIFY_EVENT_INODE	2
 
+enum fsnotify_obj_type {
+	FSNOTIFY_OBJ_TYPE_INODE,
+	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
+	FSNOTIFY_OBJ_TYPE_MAX,
+	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_MAX
+};
+
+#define FSNOTIFY_OBJ_TYPE_INODE_FL	(1U << FSNOTIFY_OBJ_TYPE_INODE)
+#define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+#define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
+
 /*
  * Inode / vfsmount point to this structure which tracks all marks attached to
  * the inode / vfsmount. The reference to inode / vfsmount is held by this
@@ -209,11 +220,7 @@ struct fsnotify_group {
  */
 struct fsnotify_mark_connector {
 	spinlock_t lock;
-#define FSNOTIFY_OBJ_TYPE_INODE		0x01
-#define FSNOTIFY_OBJ_TYPE_VFSMOUNT	0x02
-#define FSNOTIFY_OBJ_ALL_TYPES		(FSNOTIFY_OBJ_TYPE_INODE | \
-					 FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-	unsigned int flags;	/* Type of object [lock] */
+	unsigned int type;	/* Type of object [lock] */
 	union {	/* Object pointer [lock] */
 		struct inode *inode;
 		struct vfsmount *mnt;
@@ -365,12 +372,12 @@ extern void fsnotify_clear_marks_by_group(struct fsnotify_group *group, unsigned
 /* run all the marks in a group, and clear all of the vfsmount marks */
 static inline void fsnotify_clear_vfsmount_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL);
 }
 /* run all the marks in a group, and clear all of the inode marks */
 static inline void fsnotify_clear_inode_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE_FL);
 }
 extern void fsnotify_get_mark(struct fsnotify_mark *mark);
 extern void fsnotify_put_mark(struct fsnotify_mark *mark);
-- 
2.7.4

Re: [PATCH v2 05/20] fsnotify: use type id to identify connector object type

[Jan Kara]

On Thu 05-04-18 16:18:06, Amir Goldstein wrote:
> An fsnotify_mark_connector is referencing a single type of object
> (either inode or vfsmount). Instead of storing a type mask in
> connector->flags, store a single type id in connector->type to
> identify the type of object.
> 
> When a connector object is detached from the object, its type is set
> to FSNOTIFY_OBJ_TYPE_DETACHED and this object is not going to be
> reused.
> 
> The function fsnotify_clear_marks_by_group() is the only place where
> type mask was used, so use type flags instead of type id to this
> function.
> 
> This change is going to be more convenient when adding a new object
> type (super block).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Just one nit below:

> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 759ba9113ec4..9c690eb692a2 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -201,6 +201,17 @@ struct fsnotify_group {
>  #define FSNOTIFY_EVENT_PATH	1
>  #define FSNOTIFY_EVENT_INODE	2
>  
> +enum fsnotify_obj_type {
> +	FSNOTIFY_OBJ_TYPE_INODE,
> +	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
> +	FSNOTIFY_OBJ_TYPE_MAX,

This would be better named FSNOTIFY_OBJ_TYPE_COUNT. _MAX suffix suggests
this is still a valid type. Or do you indeed want to treat TYPE_DETACHED as
a regular type?

								Honza

> +	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_MAX
> +};
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 05/20] fsnotify: use type id to identify connector object type

[Amir Goldstein]

On Tue, Apr 17, 2018 at 7:26 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:06, Amir Goldstein wrote:
>> An fsnotify_mark_connector is referencing a single type of object
>> (either inode or vfsmount). Instead of storing a type mask in
>> connector->flags, store a single type id in connector->type to
>> identify the type of object.
>>
>> When a connector object is detached from the object, its type is set
>> to FSNOTIFY_OBJ_TYPE_DETACHED and this object is not going to be
>> reused.
>>
>> The function fsnotify_clear_marks_by_group() is the only place where
>> type mask was used, so use type flags instead of type id to this
>> function.
>>
>> This change is going to be more convenient when adding a new object
>> type (super block).
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Just one nit below:
>
>> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
>> index 759ba9113ec4..9c690eb692a2 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -201,6 +201,17 @@ struct fsnotify_group {
>>  #define FSNOTIFY_EVENT_PATH  1
>>  #define FSNOTIFY_EVENT_INODE 2
>>
>> +enum fsnotify_obj_type {
>> +     FSNOTIFY_OBJ_TYPE_INODE,
>> +     FSNOTIFY_OBJ_TYPE_VFSMOUNT,
>> +     FSNOTIFY_OBJ_TYPE_MAX,
>
> This would be better named FSNOTIFY_OBJ_TYPE_COUNT. _MAX suffix suggests
> this is still a valid type. Or do you indeed want to treat TYPE_DETACHED as
> a regular type?
>

No. _COUNT is good. _DETACHED is never iterated when iterating by type.

Thanks,
Amir.

[PATCH v2 06/20] fsnotify: remove redundant arguments to handle_event()

[Amir Goldstein]

inode_mark and vfsmount_mark arguments are passed to handle_event()
operation as function arguments as well as on iter_info struct.
The difference is that iter_info struct may contain marks that should
not be handled and are represented as NULL arguments to inode_mark or
vfsmount_mark.

Instead of passing the inode_mark and vfsmount_mark arguments, add
a type_mask member to iter_info struct to indicate which marks should
be handled, versus marks that should only be kept alive during user
wait.

This change is going to be used for passing more mark types
with handle_event() (i.e. super block marks).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/dnotify/dnotify.c          |  6 ++---
 fs/notify/fanotify/fanotify.c        | 14 +++++-----
 fs/notify/fsnotify.c                 | 51 ++++++++++++++++++------------------
 fs/notify/fsnotify.h                 |  6 -----
 fs/notify/inotify/inotify.h          |  2 --
 fs/notify/inotify/inotify_fsnotify.c |  6 ++---
 fs/notify/inotify/inotify_user.c     |  6 +++--
 include/linux/fsnotify_backend.h     | 31 ++++++++++++++++++++--
 kernel/audit_fsnotify.c              |  3 +--
 kernel/audit_tree.c                  |  2 --
 kernel/audit_watch.c                 |  3 +--
 11 files changed, 73 insertions(+), 57 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 63a1ca4b9dee..b4e685b63f33 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -79,12 +79,11 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
  */
 static int dnotify_handle_event(struct fsnotify_group *group,
 				struct inode *inode,
-				struct fsnotify_mark *inode_mark,
-				struct fsnotify_mark *vfsmount_mark,
 				u32 mask, const void *data, int data_type,
 				const unsigned char *file_name, u32 cookie,
 				struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct dnotify_mark *dn_mark;
 	struct dnotify_struct *dn;
 	struct dnotify_struct **prev;
@@ -95,7 +94,8 @@ static int dnotify_handle_event(struct fsnotify_group *group,
 	if (!S_ISDIR(inode->i_mode))
 		return 0;
 
-	BUG_ON(vfsmount_mark);
+	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
+		return 0;
 
 	dn_mark = container_of(inode_mark, struct dnotify_mark, fsn_mark);
 
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e0e6a9d627df..86ccf79c0671 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -87,11 +87,12 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	return ret;
 }
 
-static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
-				       struct fsnotify_mark *vfsmnt_mark,
-				       u32 event_mask,
-				       const void *data, int data_type)
+static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
+				       u32 event_mask, const void *data,
+				       int data_type)
 {
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
+	struct fsnotify_mark *vfsmnt_mark = fsnotify_iter_vfsmount_mark(iter_info);
 	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	const struct path *path = data;
 
@@ -169,8 +170,6 @@ init: __maybe_unused
 
 static int fanotify_handle_event(struct fsnotify_group *group,
 				 struct inode *inode,
-				 struct fsnotify_mark *inode_mark,
-				 struct fsnotify_mark *fanotify_mark,
 				 u32 mask, const void *data, int data_type,
 				 const unsigned char *file_name, u32 cookie,
 				 struct fsnotify_iter_info *iter_info)
@@ -190,8 +189,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	BUILD_BUG_ON(FAN_ACCESS_PERM != FS_ACCESS_PERM);
 	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
 
-	if (!fanotify_should_send_event(inode_mark, fanotify_mark, mask, data,
-					data_type))
+	if (!fanotify_should_send_event(iter_info, mask, data, data_type))
 		return 0;
 
 	pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 613ec7e5a465..cb0afa664057 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -184,22 +184,20 @@ int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 
 static int send_to_group(struct inode *to_tell,
-			 struct fsnotify_mark *inode_mark,
-			 struct fsnotify_mark *vfsmount_mark,
 			 __u32 mask, const void *data,
 			 int data_is, u32 cookie,
 			 const unsigned char *file_name,
 			 struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
+	struct fsnotify_mark *vfsmount_mark = fsnotify_iter_vfsmount_mark(iter_info);
 	struct fsnotify_group *group = NULL;
 	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
 	__u32 marks_mask = 0;
 	__u32 marks_ignored_mask = 0;
 
-	if (unlikely(!inode_mark && !vfsmount_mark)) {
-		BUG();
+	if (WARN_ON(!iter_info->type_mask))
 		return 0;
-	}
 
 	/* clear ignored on inode modification */
 	if (mask & FS_MODIFY) {
@@ -235,8 +233,7 @@ static int send_to_group(struct inode *to_tell,
 	if (!(test_mask & marks_mask & ~marks_ignored_mask))
 		return 0;
 
-	return group->ops->handle_event(group, to_tell, inode_mark,
-					vfsmount_mark, mask, data, data_is,
+	return group->ops->handle_event(group, to_tell, mask, data, data_is,
 					file_name, cookie, iter_info);
 }
 
@@ -307,16 +304,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 
 	if ((mask & FS_MODIFY) ||
 	    (test_mask & to_tell->i_fsnotify_mask)) {
-		iter_info.inode_mark =
-			fsnotify_first_mark(&to_tell->i_fsnotify_marks);
+		fsnotify_iter_set_inode_mark(&iter_info,
+			fsnotify_first_mark(&to_tell->i_fsnotify_marks));
 	}
 
 	if (mnt && ((mask & FS_MODIFY) ||
 		    (test_mask & mnt->mnt_fsnotify_mask))) {
-		iter_info.inode_mark =
-			fsnotify_first_mark(&to_tell->i_fsnotify_marks);
-		iter_info.vfsmount_mark =
-			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
+		fsnotify_iter_set_inode_mark(&iter_info,
+			fsnotify_first_mark(&to_tell->i_fsnotify_marks));
+		fsnotify_iter_set_vfsmount_mark(&iter_info,
+			fsnotify_first_mark(&mnt->mnt_fsnotify_marks));
 	}
 
 	/*
@@ -324,7 +321,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 * ignore masks are properly reflected for mount mark notifications.
 	 * That's why this traversal is so complicated...
 	 */
-	while (iter_info.inode_mark || iter_info.vfsmount_mark) {
+	while (iter_info.type_mask) {
 		struct fsnotify_mark *inode_mark = iter_info.inode_mark;
 		struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark;
 
@@ -332,24 +329,28 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 			int cmp = fsnotify_compare_groups(inode_mark->group,
 							  vfsmount_mark->group);
 			if (cmp > 0)
-				inode_mark = NULL;
+				iter_info.type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
 			else if (cmp < 0)
-				vfsmount_mark = NULL;
+				iter_info.type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
 		}
 
-		ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask,
-				    data, data_is, cookie, file_name,
-				    &iter_info);
+		ret = send_to_group(to_tell, mask, data, data_is, cookie,
+				    file_name, &iter_info);
 
 		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
 			goto out;
 
-		if (inode_mark)
-			iter_info.inode_mark =
-				fsnotify_next_mark(iter_info.inode_mark);
-		if (vfsmount_mark)
-			iter_info.vfsmount_mark =
-				fsnotify_next_mark(iter_info.vfsmount_mark);
+		if (iter_info.type_mask & FSNOTIFY_OBJ_TYPE_INODE_FL)
+			fsnotify_iter_set_inode_mark(&iter_info,
+				fsnotify_next_mark(iter_info.inode_mark));
+		else if (iter_info.inode_mark)
+			iter_info.type_mask |= FSNOTIFY_OBJ_TYPE_INODE_FL;
+
+		if (iter_info.type_mask & FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL)
+			fsnotify_iter_set_vfsmount_mark(&iter_info,
+				fsnotify_next_mark(iter_info.vfsmount_mark));
+		else if (iter_info.vfsmount_mark)
+			iter_info.type_mask |= FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
 	}
 	ret = 0;
 out:
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 60f365dc1408..34515d2c4ba3 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -9,12 +9,6 @@
 
 #include "../mount.h"
 
-struct fsnotify_iter_info {
-	struct fsnotify_mark *inode_mark;
-	struct fsnotify_mark *vfsmount_mark;
-	int srcu_idx;
-};
-
 /* destroy all events sitting in this groups notification queue */
 extern void fsnotify_flush_notify(struct fsnotify_group *group);
 
diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index c00d2caca894..7e4578d35b61 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -25,8 +25,6 @@ extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 					   struct fsnotify_group *group);
 extern int inotify_handle_event(struct fsnotify_group *group,
 				struct inode *inode,
-				struct fsnotify_mark *inode_mark,
-				struct fsnotify_mark *vfsmount_mark,
 				u32 mask, const void *data, int data_type,
 				const unsigned char *file_name, u32 cookie,
 				struct fsnotify_iter_info *iter_info);
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 8b73332735ba..3193ef6c584d 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -65,12 +65,11 @@ static int inotify_merge(struct list_head *list,
 
 int inotify_handle_event(struct fsnotify_group *group,
 			 struct inode *inode,
-			 struct fsnotify_mark *inode_mark,
-			 struct fsnotify_mark *vfsmount_mark,
 			 u32 mask, const void *data, int data_type,
 			 const unsigned char *file_name, u32 cookie,
 			 struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct inotify_inode_mark *i_mark;
 	struct inotify_event_info *event;
 	struct fsnotify_event *fsn_event;
@@ -78,7 +77,8 @@ int inotify_handle_event(struct fsnotify_group *group,
 	int len = 0;
 	int alloc_len = sizeof(struct inotify_event_info);
 
-	BUG_ON(vfsmount_mark);
+	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
+		return 0;
 
 	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
 	    (data_type == FSNOTIFY_EVENT_PATH)) {
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 2c908b31d6c9..0e44bf585a17 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -471,10 +471,12 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 				    struct fsnotify_group *group)
 {
 	struct inotify_inode_mark *i_mark;
+	struct fsnotify_iter_info iter_info = {};
 
 	/* Queue ignore event for the watch */
-	inotify_handle_event(group, NULL, fsn_mark, NULL, FS_IN_IGNORED,
-			     NULL, FSNOTIFY_EVENT_NONE, NULL, 0, NULL);
+	fsnotify_iter_set_inode_mark(&iter_info, fsn_mark);
+	inotify_handle_event(group, NULL, FS_IN_IGNORED, NULL,
+			     FSNOTIFY_EVENT_NONE, NULL, 0, &iter_info);
 
 	i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
 	/* remove this mark from the idr */
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9c690eb692a2..62e05b8a9bc7 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -98,8 +98,6 @@ struct fsnotify_iter_info;
 struct fsnotify_ops {
 	int (*handle_event)(struct fsnotify_group *group,
 			    struct inode *inode,
-			    struct fsnotify_mark *inode_mark,
-			    struct fsnotify_mark *vfsmount_mark,
 			    u32 mask, const void *data, int data_type,
 			    const unsigned char *file_name, u32 cookie,
 			    struct fsnotify_iter_info *iter_info);
@@ -212,6 +210,35 @@ enum fsnotify_obj_type {
 #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
 
+struct fsnotify_iter_info {
+	struct fsnotify_mark *inode_mark;
+	struct fsnotify_mark *vfsmount_mark;
+	unsigned int type_mask;
+	int srcu_idx;
+};
+
+#define FSNOTIFY_ITER_FUNCS(name, NAME) \
+static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
+		struct fsnotify_iter_info *iter_info) \
+{ \
+	return (iter_info->type_mask & FSNOTIFY_OBJ_TYPE_##NAME##_FL) ? \
+		iter_info->name##_mark : NULL; \
+} \
+\
+static inline void fsnotify_iter_set_##name##_mark( \
+		struct fsnotify_iter_info *iter_info, \
+		struct fsnotify_mark *mark) \
+{ \
+	iter_info->name##_mark = mark; \
+	if (mark) \
+		iter_info->type_mask |= FSNOTIFY_OBJ_TYPE_##NAME##_FL; \
+	else \
+		iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_##NAME##_FL; \
+}
+
+FSNOTIFY_ITER_FUNCS(inode, INODE)
+FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
+
 /*
  * Inode / vfsmount point to this structure which tracks all marks attached to
  * the inode / vfsmount. The reference to inode / vfsmount is held by this
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 52f368b6561e..1b80ff8d6632 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -165,12 +165,11 @@ 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,
 				    struct inode *to_tell,
-				    struct fsnotify_mark *inode_mark,
-				    struct fsnotify_mark *vfsmount_mark,
 				    u32 mask, const void *data, int data_type,
 				    const unsigned char *dname, u32 cookie,
 				    struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct audit_fsnotify_mark *audit_mark;
 	const struct inode *inode = NULL;
 
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index fd353120e0d9..f5b85c33f0cb 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -989,8 +989,6 @@ static void evict_chunk(struct audit_chunk *chunk)
 
 static int audit_tree_handle_event(struct fsnotify_group *group,
 				   struct inode *to_tell,
-				   struct fsnotify_mark *inode_mark,
-				   struct fsnotify_mark *vfsmount_mark,
 				   u32 mask, const void *data, int data_type,
 				   const unsigned char *file_name, u32 cookie,
 				   struct fsnotify_iter_info *iter_info)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 9eb8b3511636..43fcae4b0500 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -472,12 +472,11 @@ 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,
 				    struct inode *to_tell,
-				    struct fsnotify_mark *inode_mark,
-				    struct fsnotify_mark *vfsmount_mark,
 				    u32 mask, const void *data, int data_type,
 				    const unsigned char *dname, u32 cookie,
 				    struct fsnotify_iter_info *iter_info)
 {
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	const struct inode *inode;
 	struct audit_parent *parent;
 
-- 
2.7.4

Re: [PATCH v2 06/20] fsnotify: remove redundant arguments to handle_event()

[Jan Kara]

On Thu 05-04-18 16:18:07, Amir Goldstein wrote:
> inode_mark and vfsmount_mark arguments are passed to handle_event()
> operation as function arguments as well as on iter_info struct.
> The difference is that iter_info struct may contain marks that should
> not be handled and are represented as NULL arguments to inode_mark or
> vfsmount_mark.
> 
> Instead of passing the inode_mark and vfsmount_mark arguments, add
> a type_mask member to iter_info struct to indicate which marks should
> be handled, versus marks that should only be kept alive during user
> wait.
> 
> This change is going to be used for passing more mark types
> with handle_event() (i.e. super block marks).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Again only nit here:

> @@ -212,6 +210,35 @@ enum fsnotify_obj_type {
>  #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
>  #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
>  
> +struct fsnotify_iter_info {
> +	struct fsnotify_mark *inode_mark;
> +	struct fsnotify_mark *vfsmount_mark;
> +	unsigned int type_mask;
> +	int srcu_idx;
> +};

I'd rename 'type_mask' to something more expressing the fact that these are
marks we currently report to - 'report_mask'?

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

Re: [PATCH v2 06/20] fsnotify: remove redundant arguments to handle_event()

[Amir Goldstein]

On Tue, Apr 17, 2018 at 7:33 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:07, Amir Goldstein wrote:
>> inode_mark and vfsmount_mark arguments are passed to handle_event()
>> operation as function arguments as well as on iter_info struct.
>> The difference is that iter_info struct may contain marks that should
>> not be handled and are represented as NULL arguments to inode_mark or
>> vfsmount_mark.
>>
>> Instead of passing the inode_mark and vfsmount_mark arguments, add
>> a type_mask member to iter_info struct to indicate which marks should
>> be handled, versus marks that should only be kept alive during user
>> wait.
>>
>> This change is going to be used for passing more mark types
>> with handle_event() (i.e. super block marks).
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Again only nit here:
>
>> @@ -212,6 +210,35 @@ enum fsnotify_obj_type {
>>  #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL        (1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
>>  #define FSNOTIFY_OBJ_ALL_TYPES_MASK  ((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
>>
>> +struct fsnotify_iter_info {
>> +     struct fsnotify_mark *inode_mark;
>> +     struct fsnotify_mark *vfsmount_mark;
>> +     unsigned int type_mask;
>> +     int srcu_idx;
>> +};
>
> I'd rename 'type_mask' to something more expressing the fact that these are
> marks we currently report to - 'report_mask'?
>

OK. For lack of a better name.

Thanks,
Amir.

[PATCH v2 07/20] fsnotify: introduce marks iteration helpers

[Amir Goldstein]

Introduce helpers fsnotify_iter_first() and fsnotify_iter_next()
to abstract the inode / vfsmount marks merged list iteration.

This is a preparation patch before generalizing mark list
iteration to more mark object types (i.e. super block marks).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c | 68 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 24 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index cb0afa664057..665f681dd913 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -261,6 +261,48 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
 }
 
 /*
+ * iter_info is a multi head priority queue of marks.
+ * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
+ * same priority and clears the type_mask for other marks.
+ * Returns the type_mask of the chosen subset.
+ */
+static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
+{
+	struct fsnotify_mark *inode_mark = iter_info->inode_mark;
+	struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
+
+	if (inode_mark && vfsmount_mark) {
+		int cmp = fsnotify_compare_groups(inode_mark->group,
+						  vfsmount_mark->group);
+		if (cmp > 0)
+			iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
+		else if (cmp < 0)
+			iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
+	}
+
+	return iter_info->type_mask;
+}
+
+/*
+ * Pop from iter_info multi head queue, the marks that were iterated in the
+ * current iteration step and set type_mask for all non NULL marks.
+ */
+static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
+{
+	if (iter_info->type_mask & FSNOTIFY_OBJ_TYPE_INODE_FL)
+		fsnotify_iter_set_inode_mark(iter_info,
+			fsnotify_next_mark(iter_info->inode_mark));
+	else if (iter_info->inode_mark)
+		iter_info->type_mask |= FSNOTIFY_OBJ_TYPE_INODE_FL;
+
+	if (iter_info->type_mask & FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL)
+		fsnotify_iter_set_vfsmount_mark(iter_info,
+			fsnotify_next_mark(iter_info->vfsmount_mark));
+	else if (iter_info->vfsmount_mark)
+		iter_info->type_mask |= FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
+}
+
+/*
  * 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
@@ -321,36 +363,14 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 * ignore masks are properly reflected for mount mark notifications.
 	 * That's why this traversal is so complicated...
 	 */
-	while (iter_info.type_mask) {
-		struct fsnotify_mark *inode_mark = iter_info.inode_mark;
-		struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark;
-
-		if (inode_mark && vfsmount_mark) {
-			int cmp = fsnotify_compare_groups(inode_mark->group,
-							  vfsmount_mark->group);
-			if (cmp > 0)
-				iter_info.type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
-			else if (cmp < 0)
-				iter_info.type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
-		}
-
+	while (fsnotify_iter_first(&iter_info)) {
 		ret = send_to_group(to_tell, mask, data, data_is, cookie,
 				    file_name, &iter_info);
 
 		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
 			goto out;
 
-		if (iter_info.type_mask & FSNOTIFY_OBJ_TYPE_INODE_FL)
-			fsnotify_iter_set_inode_mark(&iter_info,
-				fsnotify_next_mark(iter_info.inode_mark));
-		else if (iter_info.inode_mark)
-			iter_info.type_mask |= FSNOTIFY_OBJ_TYPE_INODE_FL;
-
-		if (iter_info.type_mask & FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL)
-			fsnotify_iter_set_vfsmount_mark(&iter_info,
-				fsnotify_next_mark(iter_info.vfsmount_mark));
-		else if (iter_info.vfsmount_mark)
-			iter_info.type_mask |= FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
+		fsnotify_iter_next(&iter_info);
 	}
 	ret = 0;
 out:
-- 
2.7.4

Re: [PATCH v2 07/20] fsnotify: introduce marks iteration helpers

[Jan Kara]

On Thu 05-04-18 16:18:08, Amir Goldstein wrote:
> Introduce helpers fsnotify_iter_first() and fsnotify_iter_next()
> to abstract the inode / vfsmount marks merged list iteration.
> 
> This is a preparation patch before generalizing mark list
> iteration to more mark object types (i.e. super block marks).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

And another naming suggestion :)


>  /*
> + * iter_info is a multi head priority queue of marks.
> + * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
> + * same priority and clears the type_mask for other marks.
> + * Returns the type_mask of the chosen subset.
> + */
> +static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)

fsnotify_iter_select_report_types()?

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

Re: [PATCH v2 07/20] fsnotify: introduce marks iteration helpers

[Amir Goldstein]

On Tue, Apr 17, 2018 at 7:38 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:08, Amir Goldstein wrote:
>> Introduce helpers fsnotify_iter_first() and fsnotify_iter_next()
>> to abstract the inode / vfsmount marks merged list iteration.
>>
>> This is a preparation patch before generalizing mark list
>> iteration to more mark object types (i.e. super block marks).
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> And another naming suggestion :)
>

Yeh, too many dimensions in this iteration makes naming a challange...

>
>>  /*
>> + * iter_info is a multi head priority queue of marks.
>> + * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
>> + * same priority and clears the type_mask for other marks.
>> + * Returns the type_mask of the chosen subset.
>> + */
>> +static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
>
> fsnotify_iter_select_report_types()?
>

Fine.

Thanks,
Amir.

[PATCH v2 08/20] fsnotify: generalize iteration of marks by object type

[Amir Goldstein]

Make some code that handles marks of object types inode and vfsmount
generic, so it can handle other object types.

Introduce foreach_obj_type macros to iterate marks by object type
with or without consulting a type mask.

This is going to be used for adding mark of another object type
(super block mark).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++++----------------
 fs/notify/mark.c                 | 23 +++++++++++------
 include/linux/fsnotify_backend.h | 38 +++++++++++++++++++++-------
 3 files changed, 76 insertions(+), 39 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 665f681dd913..1759121db50d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -263,21 +263,31 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
 /*
  * iter_info is a multi head priority queue of marks.
  * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
- * same priority and clears the type_mask for other marks.
+ * same group and clears the type_mask for other marks.
  * Returns the type_mask of the chosen subset.
  */
 static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
 {
-	struct fsnotify_mark *inode_mark = iter_info->inode_mark;
-	struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
-
-	if (inode_mark && vfsmount_mark) {
-		int cmp = fsnotify_compare_groups(inode_mark->group,
-						  vfsmount_mark->group);
-		if (cmp > 0)
-			iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
-		else if (cmp < 0)
-			iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
+	struct fsnotify_group *max_prio_group = NULL;
+	unsigned int type_mask = iter_info->type_mask;
+	struct fsnotify_mark *mark;
+	int type;
+
+	if (!type_mask || is_power_of_2(type_mask))
+		return type_mask;
+
+	/* Choose max prio group among groups of all queue heads */
+	fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
+		if (mark &&
+		    fsnotify_compare_groups(max_prio_group, mark->group) > 0)
+			max_prio_group = mark->group;
+	}
+
+	/* Clear the type mask for marks with other groups */
+	fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
+		if (mark &&
+		    fsnotify_compare_groups(max_prio_group, mark->group) != 0)
+			iter_info->type_mask &= ~(1U << type);
 	}
 
 	return iter_info->type_mask;
@@ -289,17 +299,17 @@ static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
  */
 static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
 {
-	if (iter_info->type_mask & FSNOTIFY_OBJ_TYPE_INODE_FL)
-		fsnotify_iter_set_inode_mark(iter_info,
-			fsnotify_next_mark(iter_info->inode_mark));
-	else if (iter_info->inode_mark)
-		iter_info->type_mask |= FSNOTIFY_OBJ_TYPE_INODE_FL;
-
-	if (iter_info->type_mask & FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL)
-		fsnotify_iter_set_vfsmount_mark(iter_info,
-			fsnotify_next_mark(iter_info->vfsmount_mark));
-	else if (iter_info->vfsmount_mark)
-		iter_info->type_mask |= FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
+	int type;
+
+	fsnotify_foreach_obj_type(type) {
+		unsigned int flag = 1U << type;
+
+		if (iter_info->type_mask & flag)
+			fsnotify_iter_set_type_mark(iter_info, type,
+				fsnotify_next_mark(iter_info->marks[type]));
+		else if (iter_info->marks[type])
+			iter_info->type_mask |= flag;
+	}
 }
 
 /*
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index ef44808b28ca..3df2d5ecf0b7 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -294,12 +294,12 @@ static void fsnotify_put_mark_wake(struct fsnotify_mark *mark)
 
 bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
 {
-	/* This can fail if mark is being removed */
-	if (!fsnotify_get_mark_safe(iter_info->inode_mark))
-		return false;
-	if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) {
-		fsnotify_put_mark_wake(iter_info->inode_mark);
-		return false;
+	int type;
+
+	fsnotify_foreach_obj_type(type) {
+		/* This can fail if mark is being removed */
+		if (!fsnotify_get_mark_safe(iter_info->marks[type]))
+			goto fail;
 	}
 
 	/*
@@ -310,13 +310,20 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
 	srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
 
 	return true;
+
+fail:
+	while (type-- > 0)
+		fsnotify_put_mark_wake(iter_info->marks[type]);
+	return false;
 }
 
 void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
 {
+	int type;
+
 	iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
-	fsnotify_put_mark_wake(iter_info->inode_mark);
-	fsnotify_put_mark_wake(iter_info->vfsmount_mark);
+	fsnotify_foreach_obj_type(type)
+		fsnotify_put_mark_wake(iter_info->marks[type]);
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 62e05b8a9bc7..2f8c4255e679 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -211,34 +211,54 @@ enum fsnotify_obj_type {
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
 
 struct fsnotify_iter_info {
-	struct fsnotify_mark *inode_mark;
-	struct fsnotify_mark *vfsmount_mark;
+	struct fsnotify_mark *marks[FSNOTIFY_OBJ_TYPE_MAX];
 	unsigned int type_mask;
 	int srcu_idx;
 };
 
+static inline struct fsnotify_mark *fsnotify_iter_type_mark(
+		struct fsnotify_iter_info *iter_info, int type)
+{
+	return (iter_info->type_mask & (1U << type)) ?
+		iter_info->marks[type] : NULL;
+}
+
+static inline void fsnotify_iter_set_type_mark(
+		struct fsnotify_iter_info *iter_info,
+		int type, struct fsnotify_mark *mark)
+{
+	iter_info->marks[type] = mark;
+	if (mark)
+		iter_info->type_mask |= (1U << type);
+	else
+		iter_info->type_mask &= ~(1U << type);
+}
+
 #define FSNOTIFY_ITER_FUNCS(name, NAME) \
 static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 		struct fsnotify_iter_info *iter_info) \
 { \
-	return (iter_info->type_mask & FSNOTIFY_OBJ_TYPE_##NAME##_FL) ? \
-		iter_info->name##_mark : NULL; \
+	return fsnotify_iter_type_mark(iter_info, FSNOTIFY_OBJ_TYPE_##NAME); \
 } \
 \
 static inline void fsnotify_iter_set_##name##_mark( \
 		struct fsnotify_iter_info *iter_info, \
 		struct fsnotify_mark *mark) \
 { \
-	iter_info->name##_mark = mark; \
-	if (mark) \
-		iter_info->type_mask |= FSNOTIFY_OBJ_TYPE_##NAME##_FL; \
-	else \
-		iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_##NAME##_FL; \
+	fsnotify_iter_set_type_mark(iter_info, FSNOTIFY_OBJ_TYPE_##NAME, mark); \
 }
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 
+#define fsnotify_foreach_obj_type(type) \
+	for (type = 0; type < FSNOTIFY_OBJ_TYPE_MAX; type++)
+
+#define fsnotify_iter_foreach_obj_type_mark(iter, type, mark) \
+	for (type = 0, mark = fsnotify_iter_type_mark(iter, type); \
+	     type < FSNOTIFY_OBJ_TYPE_MAX; \
+	     type++, mark = fsnotify_iter_type_mark(iter, type)) \
+
 /*
  * Inode / vfsmount point to this structure which tracks all marks attached to
  * the inode / vfsmount. The reference to inode / vfsmount is held by this
-- 
2.7.4

Re: [PATCH v2 08/20] fsnotify: generalize iteration of marks by object type

[Jan Kara]

On Thu 05-04-18 16:18:09, Amir Goldstein wrote:
> Make some code that handles marks of object types inode and vfsmount
> generic, so it can handle other object types.
> 
> Introduce foreach_obj_type macros to iterate marks by object type
> with or without consulting a type mask.
> 
> This is going to be used for adding mark of another object type
> (super block mark).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++++----------------
>  fs/notify/mark.c                 | 23 +++++++++++------
>  include/linux/fsnotify_backend.h | 38 +++++++++++++++++++++-------
>  3 files changed, 76 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 665f681dd913..1759121db50d 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -263,21 +263,31 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
>  /*
>   * iter_info is a multi head priority queue of marks.
>   * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
> - * same priority and clears the type_mask for other marks.
> + * same group and clears the type_mask for other marks.
>   * Returns the type_mask of the chosen subset.
>   */
>  static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
>  {
> -	struct fsnotify_mark *inode_mark = iter_info->inode_mark;
> -	struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
> -
> -	if (inode_mark && vfsmount_mark) {
> -		int cmp = fsnotify_compare_groups(inode_mark->group,
> -						  vfsmount_mark->group);
> -		if (cmp > 0)
> -			iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
> -		else if (cmp < 0)
> -			iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
> +	struct fsnotify_group *max_prio_group = NULL;
> +	unsigned int type_mask = iter_info->type_mask;
> +	struct fsnotify_mark *mark;
> +	int type;
> +
> +	if (!type_mask || is_power_of_2(type_mask))
> +		return type_mask;
> +
> +	/* Choose max prio group among groups of all queue heads */
> +	fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {

Does this macro really bring any benefit over:

	fsnotify_foreach_obj_type(type) {
		mark = iter_info->marks[type];

Frankly the second makes it clearer what's going on and you don't have to
wonder whether unset types are also included or not... What might be worth it
is something like fsnotify_iter_foreach_mark() which would iterate over all
*set* marks. But then there's the question whether you are iterating over
all types set in the mask or all types set in the ->marks array so probably
that will be error prone.

> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index ef44808b28ca..3df2d5ecf0b7 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -294,12 +294,12 @@ static void fsnotify_put_mark_wake(struct fsnotify_mark *mark)
>  
>  bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>  {
> -	/* This can fail if mark is being removed */
> -	if (!fsnotify_get_mark_safe(iter_info->inode_mark))
> -		return false;
> -	if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) {
> -		fsnotify_put_mark_wake(iter_info->inode_mark);
> -		return false;
> +	int type;
> +
> +	fsnotify_foreach_obj_type(type) {
> +		/* This can fail if mark is being removed */
> +		if (!fsnotify_get_mark_safe(iter_info->marks[type]))
> +			goto fail;
>  	}
>  
>  	/*
> @@ -310,13 +310,20 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>  	srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
>  
>  	return true;
> +
> +fail:
> +	while (type-- > 0)

Can you please rewrite it as:

	for (type--; type >= 0; type--)

I know I'm lame but your version made me think for a while whether index 0
and the last index will be actually treated correctly. And we had bugs like
this in the past...

> +		fsnotify_put_mark_wake(iter_info->marks[type]);
> +	return false;
>  }
>  

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

Re: [PATCH v2 08/20] fsnotify: generalize iteration of marks by object type

[Amir Goldstein]

On Tue, Apr 17, 2018 at 8:01 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:09, Amir Goldstein wrote:
>> Make some code that handles marks of object types inode and vfsmount
>> generic, so it can handle other object types.
>>
>> Introduce foreach_obj_type macros to iterate marks by object type
>> with or without consulting a type mask.
>>
>> This is going to be used for adding mark of another object type
>> (super block mark).
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++++----------------
>>  fs/notify/mark.c                 | 23 +++++++++++------
>>  include/linux/fsnotify_backend.h | 38 +++++++++++++++++++++-------
>>  3 files changed, 76 insertions(+), 39 deletions(-)
>>
>> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
>> index 665f681dd913..1759121db50d 100644
>> --- a/fs/notify/fsnotify.c
>> +++ b/fs/notify/fsnotify.c
>> @@ -263,21 +263,31 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
>>  /*
>>   * iter_info is a multi head priority queue of marks.
>>   * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
>> - * same priority and clears the type_mask for other marks.
>> + * same group and clears the type_mask for other marks.
>>   * Returns the type_mask of the chosen subset.
>>   */
>>  static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
>>  {
>> -     struct fsnotify_mark *inode_mark = iter_info->inode_mark;
>> -     struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
>> -
>> -     if (inode_mark && vfsmount_mark) {
>> -             int cmp = fsnotify_compare_groups(inode_mark->group,
>> -                                               vfsmount_mark->group);
>> -             if (cmp > 0)
>> -                     iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
>> -             else if (cmp < 0)
>> -                     iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
>> +     struct fsnotify_group *max_prio_group = NULL;
>> +     unsigned int type_mask = iter_info->type_mask;
>> +     struct fsnotify_mark *mark;
>> +     int type;
>> +
>> +     if (!type_mask || is_power_of_2(type_mask))
>> +             return type_mask;
>> +
>> +     /* Choose max prio group among groups of all queue heads */
>> +     fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
>
> Does this macro really bring any benefit over:
>
>         fsnotify_foreach_obj_type(type) {
>                 mark = iter_info->marks[type];

You mean:
       mark = fsnotify_iter_type_mark(info, type, mark);

Which is non NULL only if type was selected.
Needs a better name??
How about fsnotify_iter_test_type_mark() to counter
fsnotify_iter_set_type_mark()?

>
> Frankly the second makes it clearer what's going on and you don't have to
> wonder whether unset types are also included or not... What might be worth it
> is something like fsnotify_iter_foreach_mark() which would iterate over all
> *set* marks. But then there's the question whether you are iterating over
> all types set in the mask or all types set in the ->marks array so probably
> that will be error prone.

I had such a macro in an earlier version, but it was quite ugly so I let it go.
I'll get rid of fsnotify_iter_foreach_obj_type_mark() and see if it looks nicer.

>
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index ef44808b28ca..3df2d5ecf0b7 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -294,12 +294,12 @@ static void fsnotify_put_mark_wake(struct fsnotify_mark *mark)
>>
>>  bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>>  {
>> -     /* This can fail if mark is being removed */
>> -     if (!fsnotify_get_mark_safe(iter_info->inode_mark))
>> -             return false;
>> -     if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) {
>> -             fsnotify_put_mark_wake(iter_info->inode_mark);
>> -             return false;
>> +     int type;
>> +
>> +     fsnotify_foreach_obj_type(type) {
>> +             /* This can fail if mark is being removed */
>> +             if (!fsnotify_get_mark_safe(iter_info->marks[type]))
>> +                     goto fail;
>>       }
>>
>>       /*
>> @@ -310,13 +310,20 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>>       srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
>>
>>       return true;
>> +
>> +fail:
>> +     while (type-- > 0)
>
> Can you please rewrite it as:
>
>         for (type--; type >= 0; type--)
>
> I know I'm lame but your version made me think for a while whether index 0
> and the last index will be actually treated correctly. And we had bugs like
> this in the past...
>

Sure.

Thanks,
Amir.

Re: [PATCH v2 08/20] fsnotify: generalize iteration of marks by object type

[Jan Kara]

On Tue 17-04-18 22:11:04, Amir Goldstein wrote:
> On Tue, Apr 17, 2018 at 8:01 PM, Jan Kara <jack@suse.cz> wrote:
> > On Thu 05-04-18 16:18:09, Amir Goldstein wrote:
> >> Make some code that handles marks of object types inode and vfsmount
> >> generic, so it can handle other object types.
> >>
> >> Introduce foreach_obj_type macros to iterate marks by object type
> >> with or without consulting a type mask.
> >>
> >> This is going to be used for adding mark of another object type
> >> (super block mark).
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >> ---
> >>  fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++++----------------
> >>  fs/notify/mark.c                 | 23 +++++++++++------
> >>  include/linux/fsnotify_backend.h | 38 +++++++++++++++++++++-------
> >>  3 files changed, 76 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> >> index 665f681dd913..1759121db50d 100644
> >> --- a/fs/notify/fsnotify.c
> >> +++ b/fs/notify/fsnotify.c
> >> @@ -263,21 +263,31 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
> >>  /*
> >>   * iter_info is a multi head priority queue of marks.
> >>   * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
> >> - * same priority and clears the type_mask for other marks.
> >> + * same group and clears the type_mask for other marks.
> >>   * Returns the type_mask of the chosen subset.
> >>   */
> >>  static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
> >>  {
> >> -     struct fsnotify_mark *inode_mark = iter_info->inode_mark;
> >> -     struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
> >> -
> >> -     if (inode_mark && vfsmount_mark) {
> >> -             int cmp = fsnotify_compare_groups(inode_mark->group,
> >> -                                               vfsmount_mark->group);
> >> -             if (cmp > 0)
> >> -                     iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
> >> -             else if (cmp < 0)
> >> -                     iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
> >> +     struct fsnotify_group *max_prio_group = NULL;
> >> +     unsigned int type_mask = iter_info->type_mask;
> >> +     struct fsnotify_mark *mark;
> >> +     int type;
> >> +
> >> +     if (!type_mask || is_power_of_2(type_mask))
> >> +             return type_mask;
> >> +
> >> +     /* Choose max prio group among groups of all queue heads */
> >> +     fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
> >
> > Does this macro really bring any benefit over:
> >
> >         fsnotify_foreach_obj_type(type) {
> >                 mark = iter_info->marks[type];
> 
> You mean:
>        mark = fsnotify_iter_type_mark(info, type, mark);
> 
> Which is non NULL only if type was selected.

Hum, probably but it seems a bit confusing (and thus error prone in the
long term). So how about making fsnotify_iter_next() just advancing mark
pointers based on report_mask (to move over marks that were already
reported) and then clearing report_mask.
fsnotify_iter_select_report_types() would the walk over iter_info->marks
array, consider all non-NULL entries for reporting, select among them
those with the highest priority, and set report_mask appropriately.
That way we won't even need fsnotify_iter_set_{inode|vfsmount}_mark()
macros.

> Needs a better name??
> How about fsnotify_iter_test_type_mark() to counter
> fsnotify_iter_set_type_mark()?

If there are not many places, that need fsnotify_iter_type_mark() (which
currently it seems there are not), then I'd just make it
fsnotify_iter_should_report_type(iter, type) which returns bool - whether
that type should be reported to or not. And then just explicitely use
iter->marks[type] if type should be reported. IMO fsnotify_iter_type_mark()
does not offer much advantage over this as you have to test for mark being
NULL anyway.

If there are many places that need fsnotify_iter_type_mark(), then I guess
we could provide iteration only over types that should be reported. Does
that sound good?

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

Re: [PATCH v2 08/20] fsnotify: generalize iteration of marks by object type

[Amir Goldstein]

On Wed, Apr 18, 2018 at 11:34 AM, Jan Kara <jack@suse.cz> wrote:
> On Tue 17-04-18 22:11:04, Amir Goldstein wrote:
>> On Tue, Apr 17, 2018 at 8:01 PM, Jan Kara <jack@suse.cz> wrote:
>> > On Thu 05-04-18 16:18:09, Amir Goldstein wrote:
>> >> Make some code that handles marks of object types inode and vfsmount
>> >> generic, so it can handle other object types.
>> >>
>> >> Introduce foreach_obj_type macros to iterate marks by object type
>> >> with or without consulting a type mask.
>> >>
>> >> This is going to be used for adding mark of another object type
>> >> (super block mark).
>> >>
>> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> >> ---
>> >>  fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++++----------------
>> >>  fs/notify/mark.c                 | 23 +++++++++++------
>> >>  include/linux/fsnotify_backend.h | 38 +++++++++++++++++++++-------
>> >>  3 files changed, 76 insertions(+), 39 deletions(-)
>> >>
>> >> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
>> >> index 665f681dd913..1759121db50d 100644
>> >> --- a/fs/notify/fsnotify.c
>> >> +++ b/fs/notify/fsnotify.c
>> >> @@ -263,21 +263,31 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
>> >>  /*
>> >>   * iter_info is a multi head priority queue of marks.
>> >>   * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
>> >> - * same priority and clears the type_mask for other marks.
>> >> + * same group and clears the type_mask for other marks.
>> >>   * Returns the type_mask of the chosen subset.
>> >>   */
>> >>  static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
>> >>  {
>> >> -     struct fsnotify_mark *inode_mark = iter_info->inode_mark;
>> >> -     struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
>> >> -
>> >> -     if (inode_mark && vfsmount_mark) {
>> >> -             int cmp = fsnotify_compare_groups(inode_mark->group,
>> >> -                                               vfsmount_mark->group);
>> >> -             if (cmp > 0)
>> >> -                     iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
>> >> -             else if (cmp < 0)
>> >> -                     iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
>> >> +     struct fsnotify_group *max_prio_group = NULL;
>> >> +     unsigned int type_mask = iter_info->type_mask;
>> >> +     struct fsnotify_mark *mark;
>> >> +     int type;
>> >> +
>> >> +     if (!type_mask || is_power_of_2(type_mask))
>> >> +             return type_mask;
>> >> +
>> >> +     /* Choose max prio group among groups of all queue heads */
>> >> +     fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
>> >
>> > Does this macro really bring any benefit over:
>> >
>> >         fsnotify_foreach_obj_type(type) {
>> >                 mark = iter_info->marks[type];
>>
>> You mean:
>>        mark = fsnotify_iter_type_mark(info, type, mark);
>>
>> Which is non NULL only if type was selected.
>
> Hum, probably but it seems a bit confusing (and thus error prone in the
> long term). So how about making fsnotify_iter_next() just advancing mark
> pointers based on report_mask (to move over marks that were already
> reported) and then clearing report_mask.

OK, but no need to clear report_mask in fsnotify_iter_next(), because
select_report_types will recalculate the report mask.

> fsnotify_iter_select_report_types() would the walk over iter_info->marks
> array, consider all non-NULL entries for reporting, select among them
> those with the highest priority, and set report_mask appropriately.
> That way we won't even need fsnotify_iter_set_{inode|vfsmount}_mark()
> macros.

Right.

>
>> Needs a better name??
>> How about fsnotify_iter_test_type_mark() to counter
>> fsnotify_iter_set_type_mark()?
>
> If there are not many places, that need fsnotify_iter_type_mark() (which
> currently it seems there are not), then I'd just make it
> fsnotify_iter_should_report_type(iter, type) which returns bool - whether
> that type should be reported to or not. And then just explicitely use
> iter->marks[type] if type should be reported. IMO fsnotify_iter_type_mark()
> does not offer much advantage over this as you have to test for mark being
> NULL anyway.
>

I left convenience macros:
fsnotify_iter_should_report_type(iter, type)
fsnotify_iter_set_report_type(iter, type)
fsnotify_iter_set_report_type_mark(iter, type, mark)

> If there are many places that need fsnotify_iter_type_mark(), then I guess
> we could provide iteration only over types that should be reported. Does
> that sound good?
>

Yes, result looks much better IMO.
Rabased on your for_test branch and push to
https://github.com/amir73il/linux/commits/fanotify_sb

Thanks!
Amir.

[PATCH v2 09/20] fsnotify: generalize send_to_group()

[Amir Goldstein]

Use foreach_obj_type macros to generalize the code that filters
events by marks mask and ignored_mask.

This is going to be used for adding mark of super block object type.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 1759121db50d..d6e38aac9463 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -189,46 +189,38 @@ static int send_to_group(struct inode *to_tell,
 			 const unsigned char *file_name,
 			 struct fsnotify_iter_info *iter_info)
 {
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
-	struct fsnotify_mark *vfsmount_mark = fsnotify_iter_vfsmount_mark(iter_info);
 	struct fsnotify_group *group = NULL;
 	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
 	__u32 marks_mask = 0;
 	__u32 marks_ignored_mask = 0;
+	struct fsnotify_mark *mark;
+	int type;
 
 	if (WARN_ON(!iter_info->type_mask))
 		return 0;
 
 	/* clear ignored on inode modification */
 	if (mask & FS_MODIFY) {
-		if (inode_mark &&
-		    !(inode_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-			inode_mark->ignored_mask = 0;
-		if (vfsmount_mark &&
-		    !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-			vfsmount_mark->ignored_mask = 0;
-	}
-
-	/* does the inode mark tell us to do something? */
-	if (inode_mark) {
-		group = inode_mark->group;
-		marks_mask |= inode_mark->mask;
-		marks_ignored_mask |= inode_mark->ignored_mask;
+		fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
+			if (mark &&
+			    !(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
+				mark->ignored_mask = 0;
+		}
 	}
 
-	/* does the vfsmount_mark tell us to do something? */
-	if (vfsmount_mark) {
-		group = vfsmount_mark->group;
-		marks_mask |= vfsmount_mark->mask;
-		marks_ignored_mask |= vfsmount_mark->ignored_mask;
+	fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
+		/* does the object mark tell us to do something? */
+		if (mark) {
+			group = mark->group;
+			marks_mask |= mark->mask;
+			marks_ignored_mask |= mark->ignored_mask;
+		}
 	}
 
-	pr_debug("%s: group=%p to_tell=%p mask=%x inode_mark=%p"
-		 " vfsmount_mark=%p marks_mask=%x marks_ignored_mask=%x"
+	pr_debug("%s: group=%p to_tell=%p mask=%x marks_mask=%x marks_ignored_mask=%x"
 		 " data=%p data_is=%d cookie=%d\n",
-		 __func__, group, to_tell, mask, inode_mark, vfsmount_mark,
-		 marks_mask, marks_ignored_mask, data,
-		 data_is, cookie);
+		 __func__, group, to_tell, mask, marks_mask, marks_ignored_mask,
+		 data, data_is, cookie);
 
 	if (!(test_mask & marks_mask & ~marks_ignored_mask))
 		return 0;
-- 
2.7.4

[PATCH v2 10/20] fanotify: generalize fanotify_should_send_event()

[Amir Goldstein]

Use foreach_obj_type macros to generalize the code that filters
events by marks mask and ignored_mask.

This is going to be used for adding mark of super block object type.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 86ccf79c0671..c770dd36c3fe 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -91,14 +91,13 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 				       u32 event_mask, const void *data,
 				       int data_type)
 {
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
-	struct fsnotify_mark *vfsmnt_mark = fsnotify_iter_vfsmount_mark(iter_info);
 	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	const struct path *path = data;
+	struct fsnotify_mark *mark;
+	int type;
 
-	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
-		 " data_type=%d\n", __func__, inode_mark, vfsmnt_mark,
-		 event_mask, data, data_type);
+	pr_debug("%s: type_mask=%x mask=%x data=%p data_type=%d\n",
+		 __func__, iter_info->type_mask, event_mask, data, data_type);
 
 	/* if we don't have enough info to send an event to userspace say no */
 	if (data_type != FSNOTIFY_EVENT_PATH)
@@ -109,20 +108,20 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 	    !d_can_lookup(path->dentry))
 		return false;
 
-	/*
-	 * if the event is for a child and this inode doesn't care about
-	 * events on the child, don't send it!
-	 */
-	if (inode_mark &&
-	    (!(event_mask & FS_EVENT_ON_CHILD) ||
-	     (inode_mark->mask & FS_EVENT_ON_CHILD))) {
-		marks_mask |= inode_mark->mask;
-		marks_ignored_mask |= inode_mark->ignored_mask;
-	}
-
-	if (vfsmnt_mark) {
-		marks_mask |= vfsmnt_mark->mask;
-		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
+	fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
+		/*
+		 * if the event is for a child and this inode doesn't care about
+		 * events on the child, don't send it!
+		 */
+		if (mark && type == FSNOTIFY_OBJ_TYPE_INODE &&
+		    (event_mask & FS_EVENT_ON_CHILD) &&
+		    !(mark->mask & FS_EVENT_ON_CHILD))
+			continue;
+
+		if (mark) {
+			marks_mask |= mark->mask;
+			marks_ignored_mask |= mark->ignored_mask;
+		}
 	}
 
 	if (d_is_dir(path->dentry) &&
-- 
2.7.4

[PATCH v2 11/20] fsnotify: add fsnotify_add_inode_mark() wrappers

[Amir Goldstein]

Before changing the arguments of the functions fsnotify_add_mark()
and fsnotify_add_mark_locked(), convert most callers to use a wrapper.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/dnotify/dnotify.c      |  2 +-
 fs/notify/inotify/inotify_user.c |  2 +-
 include/linux/fsnotify_backend.h | 16 +++++++++++++++-
 kernel/audit_fsnotify.c          |  2 +-
 kernel/audit_tree.c              | 10 +++++-----
 kernel/audit_watch.c             |  2 +-
 6 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index b4e685b63f33..e2bea2ac5dfb 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -319,7 +319,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 		dn_mark = container_of(fsn_mark, struct dnotify_mark, fsn_mark);
 		spin_lock(&fsn_mark->lock);
 	} else {
-		error = fsnotify_add_mark_locked(new_fsn_mark, inode, NULL, 0);
+		error = fsnotify_add_inode_mark_locked(new_fsn_mark, inode, 0);
 		if (error) {
 			mutex_unlock(&dnotify_group->mark_mutex);
 			goto out_err;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 0e44bf585a17..af3efe78b599 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -566,7 +566,7 @@ static int inotify_new_watch(struct fsnotify_group *group,
 	}
 
 	/* we are on the idr, now get on the inode */
-	ret = fsnotify_add_mark_locked(&tmp_i_mark->fsn_mark, inode, NULL, 0);
+	ret = fsnotify_add_inode_mark_locked(&tmp_i_mark->fsn_mark, inode, 0);
 	if (ret) {
 		/* we failed to get on the inode, get off the idr */
 		inotify_remove_from_idr(group, tmp_i_mark);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 2f8c4255e679..d96bbc6e77ce 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -406,7 +406,21 @@ extern struct fsnotify_mark *fsnotify_find_mark(
 extern int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode,
 			     struct vfsmount *mnt, int allow_dups);
 extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-				    struct inode *inode, struct vfsmount *mnt, int allow_dups);
+				    struct inode *inode, struct vfsmount *mnt,
+				    int allow_dups);
+/* attach the mark to the inode */
+static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark,
+					  struct inode *inode,
+					  int allow_dups)
+{
+	return fsnotify_add_mark(mark, inode, NULL, allow_dups);
+}
+static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark,
+						 struct inode *inode,
+						 int allow_dups)
+{
+	return fsnotify_add_mark_locked(mark, inode, NULL, allow_dups);
+}
 /* given a group and a mark, flag mark to be freed when all references are dropped */
 extern void fsnotify_destroy_mark(struct fsnotify_mark *mark,
 				  struct fsnotify_group *group);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 1b80ff8d6632..fba78047fb37 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -109,7 +109,7 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa
 	audit_update_mark(audit_mark, dentry->d_inode);
 	audit_mark->rule = krule;
 
-	ret = fsnotify_add_mark(&audit_mark->mark, inode, NULL, true);
+	ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, true);
 	if (ret < 0) {
 		fsnotify_put_mark(&audit_mark->mark);
 		audit_mark = ERR_PTR(ret);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index f5b85c33f0cb..9d50821f8ad8 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -288,8 +288,8 @@ static void untag_chunk(struct node *p)
 	if (!new)
 		goto Fallback;
 
-	if (fsnotify_add_mark_locked(&new->mark, entry->connector->inode,
-				     NULL, 1)) {
+	if (fsnotify_add_inode_mark_locked(&new->mark, entry->connector->inode,
+					   1)) {
 		fsnotify_put_mark(&new->mark);
 		goto Fallback;
 	}
@@ -354,7 +354,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree)
 		return -ENOMEM;
 
 	entry = &chunk->mark;
-	if (fsnotify_add_mark(entry, inode, NULL, 0)) {
+	if (fsnotify_add_inode_mark(entry, inode, 0)) {
 		fsnotify_put_mark(entry);
 		return -ENOSPC;
 	}
@@ -434,8 +434,8 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 		return -ENOENT;
 	}
 
-	if (fsnotify_add_mark_locked(chunk_entry,
-			     old_entry->connector->inode, NULL, 1)) {
+	if (fsnotify_add_inode_mark_locked(chunk_entry,
+			     old_entry->connector->inode, 1)) {
 		spin_unlock(&old_entry->lock);
 		mutex_unlock(&old_entry->group->mark_mutex);
 		fsnotify_put_mark(chunk_entry);
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 43fcae4b0500..439a3a01368c 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -160,7 +160,7 @@ static struct audit_parent *audit_init_parent(struct path *path)
 
 	fsnotify_init_mark(&parent->mark, audit_watch_group);
 	parent->mark.mask = AUDIT_FS_WATCH;
-	ret = fsnotify_add_mark(&parent->mark, inode, NULL, 0);
+	ret = fsnotify_add_inode_mark(&parent->mark, inode, 0);
 	if (ret < 0) {
 		audit_free_parent(parent);
 		return ERR_PTR(ret);
-- 
2.7.4

[PATCH v2 12/20] fsnotify: introduce prototype struct fsnotify_obj

[Amir Goldstein]

struct inode and struct vfsmount are both types of objects, which marks
can be attached to. Let them "inherit" from a prototype struct, so that
marks manipulation code can be made more generic.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/inode.c                         |  2 +-
 fs/mount.h                         |  4 ++--
 fs/notify/dnotify/dnotify.c        |  8 ++++----
 fs/notify/fanotify/fanotify_user.c | 24 ++++++++++++------------
 fs/notify/fsnotify.c               | 22 +++++++++++-----------
 fs/notify/fsnotify.h               |  4 ++--
 fs/notify/inotify/inotify_user.c   |  6 +++---
 fs/notify/mark.c                   | 16 ++++++++--------
 include/linux/fs.h                 |  6 ++----
 include/linux/fsnotify_backend.h   |  5 +++--
 include/linux/fsnotify_obj.h       | 14 ++++++++++++++
 kernel/audit_tree.c                |  2 +-
 kernel/audit_watch.c               |  2 +-
 kernel/auditsc.c                   |  4 ++--
 14 files changed, 66 insertions(+), 53 deletions(-)
 create mode 100644 include/linux/fsnotify_obj.h

diff --git a/fs/inode.c b/fs/inode.c
index ef362364d396..121baae0453d 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -190,7 +190,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 #endif
 
 #ifdef CONFIG_FSNOTIFY
-	inode->i_fsnotify_mask = 0;
+	inode->i_fsnotify.mask = 0;
 #endif
 	inode->i_flctx = NULL;
 	this_cpu_inc(nr_inodes);
diff --git a/fs/mount.h b/fs/mount.h
index f39bc9da4d73..54de165ea032 100644
--- a/fs/mount.h
+++ b/fs/mount.h
@@ -4,6 +4,7 @@
 #include <linux/poll.h>
 #include <linux/ns_common.h>
 #include <linux/fs_pin.h>
+#include <linux/fsnotify_obj.h>
 
 struct mnt_namespace {
 	atomic_t		count;
@@ -61,8 +62,7 @@ struct mount {
 	struct hlist_node mnt_mp_list;	/* list mounts with the same mountpoint */
 	struct list_head mnt_umounting; /* list entry for umount propagation */
 #ifdef CONFIG_FSNOTIFY
-	struct fsnotify_mark_connector __rcu *mnt_fsnotify_marks;
-	__u32 mnt_fsnotify_mask;
+	struct fsnotify_obj mnt_fsnotify;
 #endif
 	int mnt_id;			/* mount identifier */
 	int mnt_group_id;		/* peer group identifier */
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index e2bea2ac5dfb..d46167c69c99 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -33,7 +33,7 @@ static struct kmem_cache *dnotify_mark_cache __read_mostly;
 static struct fsnotify_group *dnotify_group __read_mostly;
 
 /*
- * dnotify will attach one of these to each inode (i_fsnotify_marks) which
+ * dnotify will attach one of these to each inode (i_fsnotify.marks) which
  * is being watched by dnotify.  If multiple userspace applications are watching
  * the same directory with dnotify their information is chained in dn
  */
@@ -158,7 +158,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 	if (!S_ISDIR(inode->i_mode))
 		return;
 
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, dnotify_group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, dnotify_group);
 	if (!fsn_mark)
 		return;
 	dn_mark = container_of(fsn_mark, struct dnotify_mark, fsn_mark);
@@ -218,7 +218,7 @@ static __u32 convert_arg(unsigned long arg)
 
 /*
  * If multiple processes watch the same inode with dnotify there is only one
- * dnotify mark in inode->i_fsnotify_marks but we chain a dnotify_struct
+ * dnotify mark in inode->i_fsnotify.marks but we chain a dnotify_struct
  * onto that mark.  This function either attaches the new dnotify_struct onto
  * that list, or it |= the mask onto an existing dnofiy_struct.
  */
@@ -314,7 +314,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 	mutex_lock(&dnotify_group->mark_mutex);
 
 	/* add the new_fsn_mark or find an old one. */
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, dnotify_group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, dnotify_group);
 	if (fsn_mark) {
 		dn_mark = container_of(fsn_mark, struct dnotify_mark, fsn_mark);
 		spin_lock(&fsn_mark->lock);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c07eb3d655ea..54ab6aafbc76 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -533,7 +533,7 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 	int destroy_mark;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify_marks,
+	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify.marks,
 				      group);
 	if (!fsn_mark) {
 		mutex_unlock(&group->mark_mutex);
@@ -542,8 +542,8 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 &destroy_mark);
-	if (removed & real_mount(mnt)->mnt_fsnotify_mask)
-		fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify_marks);
+	if (removed & real_mount(mnt)->mnt_fsnotify.mask)
+		fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify.marks);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
@@ -563,7 +563,7 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 	int destroy_mark;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, group);
 	if (!fsn_mark) {
 		mutex_unlock(&group->mark_mutex);
 		return -ENOENT;
@@ -571,8 +571,8 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 &destroy_mark);
-	if (removed & inode->i_fsnotify_mask)
-		fsnotify_recalc_mask(inode->i_fsnotify_marks);
+	if (removed & inode->i_fsnotify.mask)
+		fsnotify_recalc_mask(inode->i_fsnotify.marks);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
@@ -647,7 +647,7 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify_marks,
+	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify.marks,
 				      group);
 	if (!fsn_mark) {
 		fsn_mark = fanotify_add_new_mark(group, NULL, mnt);
@@ -657,8 +657,8 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 		}
 	}
 	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
-	if (added & ~real_mount(mnt)->mnt_fsnotify_mask)
-		fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify_marks);
+	if (added & ~real_mount(mnt)->mnt_fsnotify.mask)
+		fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify.marks);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
@@ -685,7 +685,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 		return 0;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, group);
 	if (!fsn_mark) {
 		fsn_mark = fanotify_add_new_mark(group, inode, NULL);
 		if (IS_ERR(fsn_mark)) {
@@ -694,8 +694,8 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 		}
 	}
 	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
-	if (added & ~inode->i_fsnotify_mask)
-		fsnotify_recalc_mask(inode->i_fsnotify_marks);
+	if (added & ~inode->i_fsnotify.mask)
+		fsnotify_recalc_mask(inode->i_fsnotify.marks);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index d6e38aac9463..6a235091d2e4 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -160,7 +160,7 @@ int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask
 
 	if (unlikely(!fsnotify_inode_watches_children(p_inode)))
 		__fsnotify_update_child_dentry_flags(p_inode);
-	else if (p_inode->i_fsnotify_mask & mask) {
+	else if (p_inode->i_fsnotify.mask & mask) {
 		struct name_snapshot name;
 
 		/* we are notifying a parent so come up with the new mask which
@@ -326,13 +326,13 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
-	 * be expensive.  It protects walking the *_fsnotify_marks lists.
+	 * be expensive.  It protects walking the *_fsnotify.marks lists.
 	 * However, if we do not walk the lists, we do not have to do
 	 * SRCU because we have no references to any objects and do not
 	 * need SRCU to keep them "alive".
 	 */
-	if (!to_tell->i_fsnotify_marks &&
-	    (!mnt || !mnt->mnt_fsnotify_marks))
+	if (!to_tell->i_fsnotify.marks &&
+	    (!mnt || !mnt->mnt_fsnotify.marks))
 		return 0;
 	/*
 	 * if this is a modify event we may need to clear the ignored masks
@@ -340,24 +340,24 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 * this type of event.
 	 */
 	if (!(mask & FS_MODIFY) &&
-	    !(test_mask & to_tell->i_fsnotify_mask) &&
-	    !(mnt && test_mask & mnt->mnt_fsnotify_mask))
+	    !(test_mask & to_tell->i_fsnotify.mask) &&
+	    !(mnt && test_mask & mnt->mnt_fsnotify.mask))
 		return 0;
 
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
 
 	if ((mask & FS_MODIFY) ||
-	    (test_mask & to_tell->i_fsnotify_mask)) {
+	    (test_mask & to_tell->i_fsnotify.mask)) {
 		fsnotify_iter_set_inode_mark(&iter_info,
-			fsnotify_first_mark(&to_tell->i_fsnotify_marks));
+			fsnotify_first_mark(&to_tell->i_fsnotify.marks));
 	}
 
 	if (mnt && ((mask & FS_MODIFY) ||
-		    (test_mask & mnt->mnt_fsnotify_mask))) {
+		    (test_mask & mnt->mnt_fsnotify.mask))) {
 		fsnotify_iter_set_inode_mark(&iter_info,
-			fsnotify_first_mark(&to_tell->i_fsnotify_marks));
+			fsnotify_first_mark(&to_tell->i_fsnotify.marks));
 		fsnotify_iter_set_vfsmount_mark(&iter_info,
-			fsnotify_first_mark(&mnt->mnt_fsnotify_marks));
+			fsnotify_first_mark(&mnt->mnt_fsnotify.marks));
 	}
 
 	/*
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 34515d2c4ba3..8f4d3e43b5b5 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -24,12 +24,12 @@ extern void fsnotify_destroy_marks(struct fsnotify_mark_connector __rcu **connp)
 /* run the list of all marks associated with inode and destroy them */
 static inline void fsnotify_clear_marks_by_inode(struct inode *inode)
 {
-	fsnotify_destroy_marks(&inode->i_fsnotify_marks);
+	fsnotify_destroy_marks(&inode->i_fsnotify.marks);
 }
 /* run the list of all marks associated with vfsmount and destroy them */
 static inline void fsnotify_clear_marks_by_mount(struct vfsmount *mnt)
 {
-	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify_marks);
+	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify.marks);
 }
 /* Wait until all marks queued for destruction are destroyed */
 extern void fsnotify_wait_marks_destroyed(void);
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index af3efe78b599..2d6c6f2fed04 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -498,7 +498,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 
 	mask = inotify_arg_to_mask(arg);
 
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify_marks, group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, group);
 	if (!fsn_mark)
 		return -ENOENT;
 
@@ -517,11 +517,11 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 		/* more bits in old than in new? */
 		int dropped = (old_mask & ~new_mask);
 		/* more bits in this fsn_mark than the inode's mask? */
-		int do_inode = (new_mask & ~inode->i_fsnotify_mask);
+		int do_inode = (new_mask & ~inode->i_fsnotify.mask);
 
 		/* update the inode with this new fsn_mark */
 		if (dropped || do_inode)
-			fsnotify_recalc_mask(inode->i_fsnotify_marks);
+			fsnotify_recalc_mask(inode->i_fsnotify.marks);
 
 	}
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 3df2d5ecf0b7..97bf4d0fae7a 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -120,9 +120,9 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 			new_mask |= mark->mask;
 	}
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		conn->inode->i_fsnotify_mask = new_mask;
+		conn->inode->i_fsnotify.mask = new_mask;
 	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-		real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask;
+		real_mount(conn->mnt)->mnt_fsnotify.mask = new_mask;
 }
 
 /*
@@ -168,14 +168,14 @@ static struct inode *fsnotify_detach_connector_from_object(
 
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = conn->inode;
-		rcu_assign_pointer(inode->i_fsnotify_marks, NULL);
-		inode->i_fsnotify_mask = 0;
+		rcu_assign_pointer(inode->i_fsnotify.marks, NULL);
+		inode->i_fsnotify.mask = 0;
 		conn->inode = NULL;
 		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
 	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
-		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify_marks,
+		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify.marks,
 				   NULL);
-		real_mount(conn->mnt)->mnt_fsnotify_mask = 0;
+		real_mount(conn->mnt)->mnt_fsnotify.mask = 0;
 		conn->mnt = NULL;
 		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
 	}
@@ -515,9 +515,9 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 	if (WARN_ON(!inode && !mnt))
 		return -EINVAL;
 	if (inode)
-		connp = &inode->i_fsnotify_marks;
+		connp = &inode->i_fsnotify.marks;
 	else
-		connp = &real_mount(mnt)->mnt_fsnotify_marks;
+		connp = &real_mount(mnt)->mnt_fsnotify.marks;
 restart:
 	spin_lock(&mark->lock);
 	conn = fsnotify_grab_connector(connp);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c6baf767619e..144a488fb74b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -35,6 +35,7 @@
 #include <linux/delayed_call.h>
 #include <linux/uuid.h>
 #include <linux/errseq.h>
+#include <linux/fsnotify_obj.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -560,8 +561,6 @@ is_uncached_acl(struct posix_acl *acl)
 #define IOP_XATTR	0x0008
 #define IOP_DEFAULT_READLINK	0x0010
 
-struct fsnotify_mark_connector;
-
 /*
  * Keep mostly read-only and often accessed (especially for
  * the RCU path lookup and 'stat' data) fields at the beginning
@@ -661,8 +660,7 @@ struct inode {
 	__u32			i_generation;
 
 #ifdef CONFIG_FSNOTIFY
-	__u32			i_fsnotify_mask; /* all events this inode cares about */
-	struct fsnotify_mark_connector __rcu	*i_fsnotify_marks;
+	struct fsnotify_obj	i_fsnotify;
 #endif
 
 #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index d96bbc6e77ce..0940b78df4b6 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -19,6 +19,7 @@
 #include <linux/atomic.h>
 #include <linux/user_namespace.h>
 #include <linux/refcount.h>
+#include <linux/fsnotify_obj.h>
 
 /*
  * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily
@@ -335,11 +336,11 @@ extern u32 fsnotify_get_cookie(void);
 static inline int fsnotify_inode_watches_children(struct inode *inode)
 {
 	/* FS_EVENT_ON_CHILD is set if the inode may care */
-	if (!(inode->i_fsnotify_mask & FS_EVENT_ON_CHILD))
+	if (!(inode->i_fsnotify.mask & FS_EVENT_ON_CHILD))
 		return 0;
 	/* this inode might care about child events, does it care about the
 	 * specific set of events that can happen on a child? */
-	return inode->i_fsnotify_mask & FS_EVENTS_POSS_ON_CHILD;
+	return inode->i_fsnotify.mask & FS_EVENTS_POSS_ON_CHILD;
 }
 
 /*
diff --git a/include/linux/fsnotify_obj.h b/include/linux/fsnotify_obj.h
new file mode 100644
index 000000000000..437e78e60f82
--- /dev/null
+++ b/include/linux/fsnotify_obj.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FSNOTIFY_OBJ_H
+#define _LINUX_FSNOTIFY_OBJ_H
+
+struct fsnotify_mark_connector;
+
+/* struct to embed in objects, which marks can be attached to */
+struct fsnotify_obj {
+	struct fsnotify_mark_connector __rcu *marks;
+	/* all events this object cares about */
+	__u32 mask;
+};
+
+#endif
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 9d50821f8ad8..9f83676aec64 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -393,7 +393,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	struct node *p;
 	int n;
 
-	old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks,
+	old_entry = fsnotify_find_mark(&inode->i_fsnotify.marks,
 				       audit_tree_group);
 	if (!old_entry)
 		return create_chunk(inode, tree);
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 439a3a01368c..1b017875b8d2 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -103,7 +103,7 @@ static inline struct audit_parent *audit_find_parent(struct inode *inode)
 	struct audit_parent *parent = NULL;
 	struct fsnotify_mark *entry;
 
-	entry = fsnotify_find_mark(&inode->i_fsnotify_marks, audit_watch_group);
+	entry = fsnotify_find_mark(&inode->i_fsnotify.marks, audit_watch_group);
 	if (entry)
 		parent = container_of(entry, struct audit_parent, mark);
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e80459f7e132..6c3f9e0ea2ed 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1600,7 +1600,7 @@ static inline void handle_one(const struct inode *inode)
 	struct audit_tree_refs *p;
 	struct audit_chunk *chunk;
 	int count;
-	if (likely(!inode->i_fsnotify_marks))
+	if (likely(!inode->i_fsnotify.marks))
 		return;
 	context = current->audit_context;
 	p = context->trees;
@@ -1643,7 +1643,7 @@ static void handle_path(const struct dentry *dentry)
 	seq = read_seqbegin(&rename_lock);
 	for(;;) {
 		struct inode *inode = d_backing_inode(d);
-		if (inode && unlikely(inode->i_fsnotify_marks)) {
+		if (inode && unlikely(inode->i_fsnotify.marks)) {
 			struct audit_chunk *chunk;
 			chunk = audit_tree_lookup(inode);
 			if (chunk) {
-- 
2.7.4

Re: [PATCH v2 12/20] fsnotify: introduce prototype struct fsnotify_obj

[Jan Kara]

On Thu 05-04-18 16:18:13, Amir Goldstein wrote:
> struct inode and struct vfsmount are both types of objects, which marks
> can be attached to. Let them "inherit" from a prototype struct, so that
> marks manipulation code can be made more generic.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I like this!

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

[PATCH v2 13/20] fsnotify: pass fsnotify_obj instead of **connp argument

[Amir Goldstein]

The object marks manipulation functions fsnotify_destroy_marks()
fsnotify_find_mark() and their helpers take a **connp argument to
dereference the connector pointer. Pass the container fsnotify_obj pointer
argument instead to prepare for more cleanups.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/dnotify/dnotify.c        |  4 ++--
 fs/notify/fanotify/fanotify_user.c | 10 ++++-----
 fs/notify/fsnotify.h               |  8 ++++----
 fs/notify/inotify/inotify_user.c   |  2 +-
 fs/notify/mark.c                   | 42 ++++++++++++++++++--------------------
 include/linux/fsnotify_backend.h   |  7 +++----
 kernel/audit_tree.c                |  3 +--
 kernel/audit_watch.c               |  2 +-
 8 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index d46167c69c99..c7a74ee4b7a3 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -158,7 +158,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 	if (!S_ISDIR(inode->i_mode))
 		return;
 
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, dnotify_group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, dnotify_group);
 	if (!fsn_mark)
 		return;
 	dn_mark = container_of(fsn_mark, struct dnotify_mark, fsn_mark);
@@ -314,7 +314,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 	mutex_lock(&dnotify_group->mark_mutex);
 
 	/* add the new_fsn_mark or find an old one. */
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, dnotify_group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, dnotify_group);
 	if (fsn_mark) {
 		dn_mark = container_of(fsn_mark, struct dnotify_mark, fsn_mark);
 		spin_lock(&fsn_mark->lock);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 54ab6aafbc76..8da861cb3f5c 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -533,8 +533,7 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 	int destroy_mark;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify.marks,
-				      group);
+	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify, group);
 	if (!fsn_mark) {
 		mutex_unlock(&group->mark_mutex);
 		return -ENOENT;
@@ -563,7 +562,7 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 	int destroy_mark;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, group);
 	if (!fsn_mark) {
 		mutex_unlock(&group->mark_mutex);
 		return -ENOENT;
@@ -647,8 +646,7 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify.marks,
-				      group);
+	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify, group);
 	if (!fsn_mark) {
 		fsn_mark = fanotify_add_new_mark(group, NULL, mnt);
 		if (IS_ERR(fsn_mark)) {
@@ -685,7 +683,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 		return 0;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, group);
 	if (!fsn_mark) {
 		fsn_mark = fanotify_add_new_mark(group, inode, NULL);
 		if (IS_ERR(fsn_mark)) {
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 8f4d3e43b5b5..6459481c964f 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -19,17 +19,17 @@ extern struct srcu_struct fsnotify_mark_srcu;
 extern int fsnotify_compare_groups(struct fsnotify_group *a,
 				   struct fsnotify_group *b);
 
-/* Destroy all marks connected via given connector */
-extern void fsnotify_destroy_marks(struct fsnotify_mark_connector __rcu **connp);
+/* Destroy all marks connected to a given object */
+extern void fsnotify_destroy_marks(struct fsnotify_obj *obj);
 /* run the list of all marks associated with inode and destroy them */
 static inline void fsnotify_clear_marks_by_inode(struct inode *inode)
 {
-	fsnotify_destroy_marks(&inode->i_fsnotify.marks);
+	fsnotify_destroy_marks(&inode->i_fsnotify);
 }
 /* run the list of all marks associated with vfsmount and destroy them */
 static inline void fsnotify_clear_marks_by_mount(struct vfsmount *mnt)
 {
-	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify.marks);
+	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify);
 }
 /* Wait until all marks queued for destruction are destroyed */
 extern void fsnotify_wait_marks_destroyed(void);
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 2d6c6f2fed04..a563d053cc79 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -498,7 +498,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 
 	mask = inotify_arg_to_mask(arg);
 
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify.marks, group);
+	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, group);
 	if (!fsn_mark)
 		return -ENOENT;
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 97bf4d0fae7a..112f54dceeef 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -436,10 +436,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
 	return -1;
 }
 
-static int fsnotify_attach_connector_to_object(
-				struct fsnotify_mark_connector __rcu **connp,
-				struct inode *inode,
-				struct vfsmount *mnt)
+static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
+					       struct inode *inode,
+					       struct vfsmount *mnt)
 {
 	struct fsnotify_mark_connector *conn;
 
@@ -456,10 +455,10 @@ static int fsnotify_attach_connector_to_object(
 		conn->mnt = mnt;
 	}
 	/*
-	 * cmpxchg() provides the barrier so that readers of *connp can see
+	 * cmpxchg() provides the barrier so that readers of obj->marks can see
 	 * only initialized structure
 	 */
-	if (cmpxchg(connp, NULL, conn)) {
+	if (cmpxchg(&obj->marks, NULL, conn)) {
 		/* Someone else created list structure for us */
 		if (inode)
 			iput(inode);
@@ -476,13 +475,13 @@ static int fsnotify_attach_connector_to_object(
  * they are sure list cannot go away under them.
  */
 static struct fsnotify_mark_connector *fsnotify_grab_connector(
-				struct fsnotify_mark_connector __rcu **connp)
+				struct fsnotify_obj *obj)
 {
 	struct fsnotify_mark_connector *conn;
 	int idx;
 
 	idx = srcu_read_lock(&fsnotify_mark_srcu);
-	conn = srcu_dereference(*connp, &fsnotify_mark_srcu);
+	conn = srcu_dereference(obj->marks, &fsnotify_mark_srcu);
 	if (!conn)
 		goto out;
 	spin_lock(&conn->lock);
@@ -508,22 +507,22 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 {
 	struct fsnotify_mark *lmark, *last = NULL;
 	struct fsnotify_mark_connector *conn;
-	struct fsnotify_mark_connector __rcu **connp;
+	struct fsnotify_obj *obj;
 	int cmp;
 	int err = 0;
 
 	if (WARN_ON(!inode && !mnt))
 		return -EINVAL;
 	if (inode)
-		connp = &inode->i_fsnotify.marks;
+		obj = &inode->i_fsnotify;
 	else
-		connp = &real_mount(mnt)->mnt_fsnotify.marks;
+		obj = &real_mount(mnt)->mnt_fsnotify;
 restart:
 	spin_lock(&mark->lock);
-	conn = fsnotify_grab_connector(connp);
+	conn = fsnotify_grab_connector(obj);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(connp, inode, mnt);
+		err = fsnotify_attach_connector_to_object(obj, inode, mnt);
 		if (err)
 			return err;
 		goto restart;
@@ -626,17 +625,16 @@ int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode,
 }
 
 /*
- * Given a list of marks, find the mark associated with given group. If found
- * take a reference to that mark and return it, else return NULL.
+ * Given a list of object marks, find the mark associated with given group.
+ * If found take a reference to that mark and return it, else return NULL.
  */
-struct fsnotify_mark *fsnotify_find_mark(
-				struct fsnotify_mark_connector __rcu **connp,
-				struct fsnotify_group *group)
+struct fsnotify_mark *fsnotify_find_mark(struct fsnotify_obj *obj,
+					 struct fsnotify_group *group)
 {
 	struct fsnotify_mark_connector *conn;
 	struct fsnotify_mark *mark;
 
-	conn = fsnotify_grab_connector(connp);
+	conn = fsnotify_grab_connector(obj);
 	if (!conn)
 		return NULL;
 
@@ -697,14 +695,14 @@ void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
 	}
 }
 
-/* Destroy all marks attached to inode / vfsmount */
-void fsnotify_destroy_marks(struct fsnotify_mark_connector __rcu **connp)
+/* Destroy all marks attached to an object */
+void fsnotify_destroy_marks(struct fsnotify_obj *obj)
 {
 	struct fsnotify_mark_connector *conn;
 	struct fsnotify_mark *mark, *old_mark = NULL;
 	struct inode *inode;
 
-	conn = fsnotify_grab_connector(connp);
+	conn = fsnotify_grab_connector(obj);
 	if (!conn)
 		return;
 	/*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 0940b78df4b6..86ea950561a8 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -399,10 +399,9 @@ extern struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group
 extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn);
 extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 			       struct fsnotify_group *group);
-/* Find mark belonging to given group in the list of marks */
-extern struct fsnotify_mark *fsnotify_find_mark(
-				struct fsnotify_mark_connector __rcu **connp,
-				struct fsnotify_group *group);
+/* Find mark belonging to given group in the list of object marks */
+extern struct fsnotify_mark *fsnotify_find_mark(struct fsnotify_obj *obj,
+						struct fsnotify_group *group);
 /* attach the mark to the inode or vfsmount */
 extern int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode,
 			     struct vfsmount *mnt, int allow_dups);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 9f83676aec64..0b09de9982ea 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -393,8 +393,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	struct node *p;
 	int n;
 
-	old_entry = fsnotify_find_mark(&inode->i_fsnotify.marks,
-				       audit_tree_group);
+	old_entry = fsnotify_find_mark(&inode->i_fsnotify, audit_tree_group);
 	if (!old_entry)
 		return create_chunk(inode, tree);
 
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 1b017875b8d2..cafec7b3985f 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -103,7 +103,7 @@ static inline struct audit_parent *audit_find_parent(struct inode *inode)
 	struct audit_parent *parent = NULL;
 	struct fsnotify_mark *entry;
 
-	entry = fsnotify_find_mark(&inode->i_fsnotify.marks, audit_watch_group);
+	entry = fsnotify_find_mark(&inode->i_fsnotify, audit_watch_group);
 	if (entry)
 		parent = container_of(entry, struct audit_parent, mark);
 
-- 
2.7.4

[PATCH v2 14/20] fsnotify: pass object and object type to fsnotify_add_mark()

[Amir Goldstein]

Instead of passing inode and vfsmount arguments to fsnotify_add_mark()
and its _locked variant, pass an abstract fsnotify_obj and the object
type.

This is going to be used for adding a mark to a new type of object
(super block object).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 16 ++++++++-------
 fs/notify/fsnotify.h               | 11 ++++++++++
 fs/notify/mark.c                   | 42 +++++++++++++++-----------------------
 include/linux/fsnotify_backend.h   | 18 +++++++++++-----
 4 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8da861cb3f5c..ecd3539587cc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -614,8 +614,8 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 }
 
 static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
-						   struct inode *inode,
-						   struct vfsmount *mnt)
+						   struct fsnotify_obj *obj,
+						   unsigned int type)
 {
 	struct fsnotify_mark *mark;
 	int ret;
@@ -628,7 +628,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 		return ERR_PTR(-ENOMEM);
 
 	fsnotify_init_mark(mark, group);
-	ret = fsnotify_add_mark_locked(mark, inode, mnt, 0);
+	ret = fsnotify_add_mark_locked(mark, obj, type, 0);
 	if (ret) {
 		fsnotify_put_mark(mark);
 		return ERR_PTR(ret);
@@ -642,13 +642,14 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 				      struct vfsmount *mnt, __u32 mask,
 				      unsigned int flags)
 {
+	struct fsnotify_obj *obj = &real_mount(mnt)->mnt_fsnotify;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify, group);
+	fsn_mark = fsnotify_find_mark(obj, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, NULL, mnt);
+		fsn_mark = fanotify_add_new_mark(group, obj, FSNOTIFY_OBJ_TYPE_VFSMOUNT);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
@@ -667,6 +668,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
 {
+	struct fsnotify_obj *obj = &inode->i_fsnotify;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
@@ -683,9 +685,9 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 		return 0;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, group);
+	fsn_mark = fsnotify_find_mark(obj, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, inode, NULL);
+		fsn_mark = fanotify_add_new_mark(group, obj, FSNOTIFY_OBJ_TYPE_INODE);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 6459481c964f..2a3fe62e2bf6 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -44,4 +44,15 @@ extern void __fsnotify_update_child_dentry_flags(struct inode *inode);
 extern struct fsnotify_event_holder *fsnotify_alloc_event_holder(void);
 extern void fsnotify_destroy_event_holder(struct fsnotify_event_holder *holder);
 
+static inline struct inode *fsnotify_inode(struct fsnotify_obj *obj)
+{
+	return container_of(obj, struct inode, i_fsnotify);
+}
+
+static inline struct vfsmount *fsnotify_vfsmount(struct fsnotify_obj *obj)
+{
+	return &(container_of(obj, struct mount, mnt_fsnotify))->mnt;
+}
+
+
 #endif	/* __FS_NOTIFY_FSNOTIFY_H_ */
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 112f54dceeef..ea6d97f5fc3b 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -437,9 +437,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
 }
 
 static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
-					       struct inode *inode,
-					       struct vfsmount *mnt)
+					       unsigned int type)
 {
+	struct inode *inode = NULL;
 	struct fsnotify_mark_connector *conn;
 
 	conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
@@ -447,13 +447,11 @@ static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
 		return -ENOMEM;
 	spin_lock_init(&conn->lock);
 	INIT_HLIST_HEAD(&conn->list);
-	if (inode) {
-		conn->type = FSNOTIFY_OBJ_TYPE_INODE;
-		conn->inode = igrab(inode);
-	} else {
-		conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
-		conn->mnt = mnt;
-	}
+	conn->type = type;
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
+		inode = conn->inode = igrab(fsnotify_inode(obj));
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+		conn->mnt = fsnotify_vfsmount(obj);
 	/*
 	 * cmpxchg() provides the barrier so that readers of obj->marks can see
 	 * only initialized structure
@@ -502,27 +500,22 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector(
  * priority, highest number first, and then by the group's location in memory.
  */
 static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
-				  struct inode *inode, struct vfsmount *mnt,
+				  struct fsnotify_obj *obj, unsigned int type,
 				  int allow_dups)
 {
 	struct fsnotify_mark *lmark, *last = NULL;
 	struct fsnotify_mark_connector *conn;
-	struct fsnotify_obj *obj;
 	int cmp;
 	int err = 0;
 
-	if (WARN_ON(!inode && !mnt))
+	if (WARN_ON(!fsnotify_valid_obj_type(type)))
 		return -EINVAL;
-	if (inode)
-		obj = &inode->i_fsnotify;
-	else
-		obj = &real_mount(mnt)->mnt_fsnotify;
 restart:
 	spin_lock(&mark->lock);
 	conn = fsnotify_grab_connector(obj);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(obj, inode, mnt);
+		err = fsnotify_attach_connector_to_object(obj, type);
 		if (err)
 			return err;
 		goto restart;
@@ -568,14 +561,13 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
  * These marks may be used for the fsnotify backend to determine which
  * event types should be delivered to which group.
  */
-int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
-			     struct vfsmount *mnt, int allow_dups)
+int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
+			     struct fsnotify_obj *obj, unsigned int type,
+			     int allow_dups)
 {
 	struct fsnotify_group *group = mark->group;
 	int ret = 0;
 
-	BUG_ON(inode && mnt);
-	BUG_ON(!inode && !mnt);
 	BUG_ON(!mutex_is_locked(&group->mark_mutex));
 
 	/*
@@ -592,7 +584,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
 	fsnotify_get_mark(mark); /* for g_list */
 	spin_unlock(&mark->lock);
 
-	ret = fsnotify_add_mark_list(mark, inode, mnt, allow_dups);
+	ret = fsnotify_add_mark_list(mark, obj, type, allow_dups);
 	if (ret)
 		goto err;
 
@@ -612,14 +604,14 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
 	return ret;
 }
 
-int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode,
-		      struct vfsmount *mnt, int allow_dups)
+int fsnotify_add_mark(struct fsnotify_mark *mark, struct fsnotify_obj *obj,
+		      unsigned int type, int allow_dups)
 {
 	int ret;
 	struct fsnotify_group *group = mark->group;
 
 	mutex_lock(&group->mark_mutex);
-	ret = fsnotify_add_mark_locked(mark, inode, mnt, allow_dups);
+	ret = fsnotify_add_mark_locked(mark, obj, type, allow_dups);
 	mutex_unlock(&group->mark_mutex);
 	return ret;
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 86ea950561a8..4a034e6832ea 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -211,6 +211,11 @@ enum fsnotify_obj_type {
 #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
 
+static inline bool fsnotify_valid_obj_type(unsigned int type)
+{
+	return (type < FSNOTIFY_OBJ_TYPE_MAX);
+}
+
 struct fsnotify_iter_info {
 	struct fsnotify_mark *marks[FSNOTIFY_OBJ_TYPE_MAX];
 	unsigned int type_mask;
@@ -403,23 +408,26 @@ extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 extern struct fsnotify_mark *fsnotify_find_mark(struct fsnotify_obj *obj,
 						struct fsnotify_group *group);
 /* attach the mark to the inode or vfsmount */
-extern int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode,
-			     struct vfsmount *mnt, int allow_dups);
+extern int fsnotify_add_mark(struct fsnotify_mark *mark,
+			     struct fsnotify_obj *obj, unsigned int type,
+			     int allow_dups);
 extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-				    struct inode *inode, struct vfsmount *mnt,
+				    struct fsnotify_obj *obj, unsigned int type,
 				    int allow_dups);
 /* attach the mark to the inode */
 static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark,
 					  struct inode *inode,
 					  int allow_dups)
 {
-	return fsnotify_add_mark(mark, inode, NULL, allow_dups);
+	return fsnotify_add_mark(mark, &inode->i_fsnotify,
+				 FSNOTIFY_OBJ_TYPE_INODE, allow_dups);
 }
 static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark,
 						 struct inode *inode,
 						 int allow_dups)
 {
-	return fsnotify_add_mark_locked(mark, inode, NULL, allow_dups);
+	return fsnotify_add_mark_locked(mark, &inode->i_fsnotify,
+					FSNOTIFY_OBJ_TYPE_INODE, allow_dups);
 }
 /* given a group and a mark, flag mark to be freed when all references are dropped */
 extern void fsnotify_destroy_mark(struct fsnotify_mark *mark,
-- 
2.7.4

Re: [PATCH v2 14/20] fsnotify: pass object and object type to fsnotify_add_mark()

[Jan Kara]

On Thu 05-04-18 16:18:15, Amir Goldstein wrote:
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 112f54dceeef..ea6d97f5fc3b 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -437,9 +437,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
>  }
>  
>  static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
> -					       struct inode *inode,
> -					       struct vfsmount *mnt)
> +					       unsigned int type)
>  {
> +	struct inode *inode = NULL;
>  	struct fsnotify_mark_connector *conn;
>  
>  	conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
> @@ -447,13 +447,11 @@ static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
>  		return -ENOMEM;
>  	spin_lock_init(&conn->lock);
>  	INIT_HLIST_HEAD(&conn->list);
> -	if (inode) {
> -		conn->type = FSNOTIFY_OBJ_TYPE_INODE;
> -		conn->inode = igrab(inode);
> -	} else {
> -		conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> -		conn->mnt = mnt;
> -	}
> +	conn->type = type;
> +	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
> +		inode = conn->inode = igrab(fsnotify_inode(obj));
> +	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
> +		conn->mnt = fsnotify_vfsmount(obj);


How about making connector point to fsnotify_obj and then get real inode /
mount pointers only in those few places which need them (basically only for
grabbing inode references and dentry flag propagation AFAIR)? That should
make the code even less aware of different object types.

Also fsnotify_inode() and fsnotify_vfsmount() seem like too generic names.
I'd call them fsnotify_obj_inode() and fsnotify_obj_vfsmount().

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

Re: [PATCH v2 14/20] fsnotify: pass object and object type to fsnotify_add_mark()

[Amir Goldstein]

On Thu, Apr 19, 2018 at 3:23 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:15, Amir Goldstein wrote:
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index 112f54dceeef..ea6d97f5fc3b 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -437,9 +437,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
>>  }
>>
>>  static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
>> -                                            struct inode *inode,
>> -                                            struct vfsmount *mnt)
>> +                                            unsigned int type)
>>  {
>> +     struct inode *inode = NULL;
>>       struct fsnotify_mark_connector *conn;
>>
>>       conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
>> @@ -447,13 +447,11 @@ static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
>>               return -ENOMEM;
>>       spin_lock_init(&conn->lock);
>>       INIT_HLIST_HEAD(&conn->list);
>> -     if (inode) {
>> -             conn->type = FSNOTIFY_OBJ_TYPE_INODE;
>> -             conn->inode = igrab(inode);
>> -     } else {
>> -             conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
>> -             conn->mnt = mnt;
>> -     }
>> +     conn->type = type;
>> +     if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
>> +             inode = conn->inode = igrab(fsnotify_inode(obj));
>> +     else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
>> +             conn->mnt = fsnotify_vfsmount(obj);
>
>
> How about making connector point to fsnotify_obj and then get real inode /
> mount pointers only in those few places which need them (basically only for
> grabbing inode references and dentry flag propagation AFAIR)? That should
> make the code even less aware of different object types.
>

Yes, I thought of doing that. I will add it in another patch, to limit
the changes of
this one.

> Also fsnotify_inode() and fsnotify_vfsmount() seem like too generic names.
> I'd call them fsnotify_obj_inode() and fsnotify_obj_vfsmount().
>

OK.

Thanks,
Amir.

[PATCH v2 15/20] fsnotify: make fsnotify_recalc_mask() unaware of object type

[Amir Goldstein]

Pass an abstract fsnotify_obj to fsnotify_recalc_mask() and factor out
the object type dependent code to a helper fsnotify_connector_obj(),
which is called only from fsnotify_put_mark().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/dnotify/dnotify.c        | 11 ++++++-----
 fs/notify/fanotify/fanotify_user.c |  8 ++++----
 fs/notify/fsnotify.h               |  1 -
 fs/notify/inotify/inotify_user.c   |  2 +-
 fs/notify/mark.c                   | 27 ++++++++++++++++++++-------
 include/linux/fsnotify_backend.h   |  4 ++--
 6 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index c7a74ee4b7a3..bdffdd29e182 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -50,7 +50,8 @@ struct dnotify_mark {
  * it calls the fsnotify function so it can update the set of all events relevant
  * to this inode.
  */
-static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
+static void dnotify_recalc_inode_mask(struct inode *inode,
+				      struct fsnotify_mark *fsn_mark)
 {
 	__u32 new_mask = 0;
 	struct dnotify_struct *dn;
@@ -66,7 +67,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 		return;
 	fsn_mark->mask = new_mask;
 
-	fsnotify_recalc_mask(fsn_mark->connector);
+	fsnotify_recalc_mask(&inode->i_fsnotify);
 }
 
 /*
@@ -113,7 +114,7 @@ static int dnotify_handle_event(struct fsnotify_group *group,
 		else {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
-			dnotify_recalc_inode_mask(inode_mark);
+			dnotify_recalc_inode_mask(inode, inode_mark);
 		}
 	}
 
@@ -171,7 +172,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 		if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
-			dnotify_recalc_inode_mask(fsn_mark);
+			dnotify_recalc_inode_mask(inode, fsn_mark);
 			break;
 		}
 		prev = &dn->dn_next;
@@ -364,7 +365,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 	else if (error == -EEXIST)
 		error = 0;
 
-	dnotify_recalc_inode_mask(fsn_mark);
+	dnotify_recalc_inode_mask(inode, fsn_mark);
 out:
 	spin_unlock(&fsn_mark->lock);
 
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index ecd3539587cc..9f2e7e2c33b3 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -542,7 +542,7 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 &destroy_mark);
 	if (removed & real_mount(mnt)->mnt_fsnotify.mask)
-		fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify.marks);
+		fsnotify_recalc_mask(&real_mount(mnt)->mnt_fsnotify);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
@@ -571,7 +571,7 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 &destroy_mark);
 	if (removed & inode->i_fsnotify.mask)
-		fsnotify_recalc_mask(inode->i_fsnotify.marks);
+		fsnotify_recalc_mask(&inode->i_fsnotify);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
@@ -657,7 +657,7 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 	}
 	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
 	if (added & ~real_mount(mnt)->mnt_fsnotify.mask)
-		fsnotify_recalc_mask(real_mount(mnt)->mnt_fsnotify.marks);
+		fsnotify_recalc_mask(&real_mount(mnt)->mnt_fsnotify);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
@@ -695,7 +695,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 	}
 	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
 	if (added & ~inode->i_fsnotify.mask)
-		fsnotify_recalc_mask(inode->i_fsnotify.marks);
+		fsnotify_recalc_mask(&inode->i_fsnotify);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 2a3fe62e2bf6..e70d39aed9d8 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -54,5 +54,4 @@ static inline struct vfsmount *fsnotify_vfsmount(struct fsnotify_obj *obj)
 	return &(container_of(obj, struct mount, mnt_fsnotify))->mnt;
 }
 
-
 #endif	/* __FS_NOTIFY_FSNOTIFY_H_ */
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index a563d053cc79..49be6fb9432b 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -521,7 +521,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 
 		/* update the inode with this new fsn_mark */
 		if (dropped || do_inode)
-			fsnotify_recalc_mask(inode->i_fsnotify.marks);
+			fsnotify_recalc_mask(&inode->i_fsnotify);
 
 	}
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index ea6d97f5fc3b..53bacbce1145 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -109,8 +109,9 @@ void fsnotify_get_mark(struct fsnotify_mark *mark)
 	refcount_inc(&mark->refcnt);
 }
 
-static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+static void __fsnotify_recalc_mask(struct fsnotify_obj *obj)
 {
+	struct fsnotify_mark_connector *conn = obj->marks;
 	u32 new_mask = 0;
 	struct fsnotify_mark *mark;
 
@@ -119,10 +120,20 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
 			new_mask |= mark->mask;
 	}
+	obj->mask = new_mask;
+}
+
+static struct fsnotify_obj *fsnotify_connector_obj(
+			    struct fsnotify_mark_connector *conn)
+{
+	assert_spin_locked(&conn->lock);
+
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		conn->inode->i_fsnotify.mask = new_mask;
+		return &conn->inode->i_fsnotify;
 	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-		real_mount(conn->mnt)->mnt_fsnotify.mask = new_mask;
+		return &real_mount(conn->mnt)->mnt_fsnotify;
+	else
+		return NULL;
 }
 
 /*
@@ -131,13 +142,15 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
  * this by holding a mark->lock or mark->group->mark_mutex for a mark on this
  * list.
  */
-void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+void fsnotify_recalc_mask(struct fsnotify_obj *obj)
 {
+	struct fsnotify_mark_connector *conn = obj->marks;
+
 	if (!conn)
 		return;
 
 	spin_lock(&conn->lock);
-	__fsnotify_recalc_mask(conn);
+	__fsnotify_recalc_mask(obj);
 	spin_unlock(&conn->lock);
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		__fsnotify_update_child_dentry_flags(conn->inode);
@@ -219,7 +232,7 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 		inode = fsnotify_detach_connector_from_object(conn);
 		free_conn = true;
 	} else {
-		__fsnotify_recalc_mask(conn);
+		__fsnotify_recalc_mask(fsnotify_connector_obj(conn));
 	}
 	mark->connector = NULL;
 	spin_unlock(&conn->lock);
@@ -589,7 +602,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 		goto err;
 
 	if (mark->mask)
-		fsnotify_recalc_mask(mark->connector);
+		fsnotify_recalc_mask(obj);
 
 	return ret;
 err:
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 4a034e6832ea..9a908b96909e 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -400,8 +400,8 @@ extern struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group
 
 /* functions used to manipulate the marks attached to inodes */
 
-/* Calculate mask of events for a list of marks */
-extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn);
+/* Calculate mask of events for a list of object marks */
+extern void fsnotify_recalc_mask(struct fsnotify_obj *obj);
 extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 			       struct fsnotify_group *group);
 /* Find mark belonging to given group in the list of object marks */
-- 
2.7.4

Re: [PATCH v2 15/20] fsnotify: make fsnotify_recalc_mask() unaware of object type

[kbuild test robot]

Hi Amir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16]
[cannot apply to linus/master next-20180405]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-super-block-mark/20180406-132931
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   fs/notify/mark.c:94:19: sparse: symbol 'fsnotify_mark_connector_cachep' was not declared. Should it be static?
>> fs/notify/mark.c:114:51: sparse: incorrect type in initializer (different address spaces) @@    expected struct fsnotify_mark_connector *conn @@    got struct fsnotify_mstruct fsnotify_mark_connector *conn @@
   fs/notify/mark.c:114:51:    expected struct fsnotify_mark_connector *conn
   fs/notify/mark.c:114:51:    got struct fsnotify_mark_connector [noderef] <asn:4>*marks
   fs/notify/mark.c:147:51: sparse: incorrect type in initializer (different address spaces) @@    expected struct fsnotify_mark_connector *conn @@    got struct fsnotify_mstruct fsnotify_mark_connector *conn @@
   fs/notify/mark.c:147:51:    expected struct fsnotify_mark_connector *conn
   fs/notify/mark.c:147:51:    got struct fsnotify_mark_connector [noderef] <asn:4>*marks
   fs/notify/mark.c:472:13: sparse: incorrect type in initializer (different address spaces) @@    expected struct fsnotify_mark_connector [noderef] <asn:4>*__new @@    got  fsnotify_mark_connector [noderef] <asn:4>*__new @@
   fs/notify/mark.c:472:13:    expected struct fsnotify_mark_connector [noderef] <asn:4>*__new
   fs/notify/mark.c:472:13:    got struct fsnotify_mark_connector *[assigned] conn
   fs/notify/mark.c:237:9: sparse: context imbalance in 'fsnotify_put_mark' - unexpected unlock
   fs/notify/mark.c:323:25: sparse: context imbalance in 'fsnotify_prepare_user_wait' - unexpected unlock
   fs/notify/mark.c:338:9: sparse: context imbalance in 'fsnotify_finish_user_wait' - wrong count at exit
   fs/notify/mark.c:488:39: sparse: context imbalance in 'fsnotify_grab_connector' - different lock contexts for basic block
   fs/notify/mark.c:567:20: sparse: context imbalance in 'fsnotify_add_mark_list' - unexpected unlock
   fs/notify/mark.c:649:25: sparse: context imbalance in 'fsnotify_find_mark' - unexpected unlock
   fs/notify/mark.c:721:17: sparse: context imbalance in 'fsnotify_destroy_marks' - unexpected unlock

vim +114 fs/notify/mark.c

    92	
    93	struct srcu_struct fsnotify_mark_srcu;
  > 94	struct kmem_cache *fsnotify_mark_connector_cachep;
    95	
    96	static DEFINE_SPINLOCK(destroy_lock);
    97	static LIST_HEAD(destroy_list);
    98	static struct fsnotify_mark_connector *connector_destroy_list;
    99	
   100	static void fsnotify_mark_destroy_workfn(struct work_struct *work);
   101	static DECLARE_DELAYED_WORK(reaper_work, fsnotify_mark_destroy_workfn);
   102	
   103	static void fsnotify_connector_destroy_workfn(struct work_struct *work);
   104	static DECLARE_WORK(connector_reaper_work, fsnotify_connector_destroy_workfn);
   105	
   106	void fsnotify_get_mark(struct fsnotify_mark *mark)
   107	{
   108		WARN_ON_ONCE(!refcount_read(&mark->refcnt));
   109		refcount_inc(&mark->refcnt);
   110	}
   111	
   112	static void __fsnotify_recalc_mask(struct fsnotify_obj *obj)
   113	{
 > 114		struct fsnotify_mark_connector *conn = obj->marks;
   115		u32 new_mask = 0;
   116		struct fsnotify_mark *mark;
   117	
   118		assert_spin_locked(&conn->lock);
   119		hlist_for_each_entry(mark, &conn->list, obj_list) {
   120			if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
   121				new_mask |= mark->mask;
   122		}
   123		obj->mask = new_mask;
   124	}
   125	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH v2 16/20] fsnotify: generalize fsnotify_detach_connector_from_object()

[Amir Goldstein]

Use helper fsnotify_connector_obj() helper to reduce object type
dependent code.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/mark.c                 | 21 ++++++++-------------
 include/linux/fsnotify_backend.h |  1 +
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 53bacbce1145..3b19ed700ac3 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -177,19 +177,14 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work)
 static struct inode *fsnotify_detach_connector_from_object(
 					struct fsnotify_mark_connector *conn)
 {
-	struct inode *inode = NULL;
-
-	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
-		inode = conn->inode;
-		rcu_assign_pointer(inode->i_fsnotify.marks, NULL);
-		inode->i_fsnotify.mask = 0;
-		conn->inode = NULL;
-		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
-	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
-		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify.marks,
-				   NULL);
-		real_mount(conn->mnt)->mnt_fsnotify.mask = 0;
-		conn->mnt = NULL;
+	struct fsnotify_obj *obj = fsnotify_connector_obj(conn);
+	struct inode *inode = (conn->type == FSNOTIFY_OBJ_TYPE_INODE) ?
+			      conn->inode : NULL;
+
+	if (obj) {
+		rcu_assign_pointer(obj->marks, NULL);
+		obj->mask = 0;
+		conn->object = NULL;
 		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
 	}
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9a908b96909e..abbd7311077f 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -275,6 +275,7 @@ struct fsnotify_mark_connector {
 	spinlock_t lock;
 	unsigned int type;	/* Type of object [lock] */
 	union {	/* Object pointer [lock] */
+		void *object;
 		struct inode *inode;
 		struct vfsmount *mnt;
 	};
-- 
2.7.4

Re: [PATCH v2 16/20] fsnotify: generalize fsnotify_detach_connector_from_object()

[Jan Kara]

On Thu 05-04-18 16:18:17, Amir Goldstein wrote:
> Use helper fsnotify_connector_obj() helper to reduce object type
> dependent code.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

So up to this patch I had only minor comments so once you address them
could you please send these preparatory cleanups, I'll go through them and
can then queue them up for 4.18-rc1? I like them and I think they are
worthwhile cleanups regardless of the new watch type so we don't need to
delay them more than necessary...

								Honza

> ---
>  fs/notify/mark.c                 | 21 ++++++++-------------
>  include/linux/fsnotify_backend.h |  1 +
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 53bacbce1145..3b19ed700ac3 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -177,19 +177,14 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work)
>  static struct inode *fsnotify_detach_connector_from_object(
>  					struct fsnotify_mark_connector *conn)
>  {
> -	struct inode *inode = NULL;
> -
> -	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
> -		inode = conn->inode;
> -		rcu_assign_pointer(inode->i_fsnotify.marks, NULL);
> -		inode->i_fsnotify.mask = 0;
> -		conn->inode = NULL;
> -		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
> -	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
> -		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify.marks,
> -				   NULL);
> -		real_mount(conn->mnt)->mnt_fsnotify.mask = 0;
> -		conn->mnt = NULL;
> +	struct fsnotify_obj *obj = fsnotify_connector_obj(conn);
> +	struct inode *inode = (conn->type == FSNOTIFY_OBJ_TYPE_INODE) ?
> +			      conn->inode : NULL;
> +
> +	if (obj) {
> +		rcu_assign_pointer(obj->marks, NULL);
> +		obj->mask = 0;
> +		conn->object = NULL;
>  		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
>  	}
>  
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 9a908b96909e..abbd7311077f 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -275,6 +275,7 @@ struct fsnotify_mark_connector {
>  	spinlock_t lock;
>  	unsigned int type;	/* Type of object [lock] */
>  	union {	/* Object pointer [lock] */
> +		void *object;
>  		struct inode *inode;
>  		struct vfsmount *mnt;
>  	};
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 16/20] fsnotify: generalize fsnotify_detach_connector_from_object()

[Amir Goldstein]

On Thu, Apr 19, 2018 at 4:59 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:17, Amir Goldstein wrote:
>> Use helper fsnotify_connector_obj() helper to reduce object type
>> dependent code.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> So up to this patch I had only minor comments so once you address them
> could you please send these preparatory cleanups, I'll go through them and
> can then queue them up for 4.18-rc1? I like them and I think they are
> worthwhile cleanups regardless of the new watch type so we don't need to
> delay them more than necessary...
>

Will do. Will also top them up with this fanotify refactoring patch:
[19/20] fanotify: factor out helpers to add/remove mark

Thanks,
Amir.

[PATCH v2 17/20] fsnotify: add super block object type

[Amir Goldstein]

Add the infrastructure to attach a mark to a super_block struct
and detach all attached marks when super block is destroyed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             |  8 +++++++-
 fs/notify/fsnotify.h             | 10 ++++++++++
 fs/notify/mark.c                 |  4 ++++
 fs/super.c                       |  2 +-
 include/linux/fs.h               |  4 ++++
 include/linux/fsnotify.h         |  8 ++++++++
 include/linux/fsnotify_backend.h | 15 ++++++++++++---
 7 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 6a235091d2e4..2ef97bc27e20 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -48,7 +48,7 @@ void __fsnotify_vfsmount_delete(struct vfsmount *mnt)
  * Called during unmount with no locks held, so needs to be safe against
  * concurrent modifiers. We temporarily drop sb->s_inode_list_lock and CAN block.
  */
-void fsnotify_unmount_inodes(struct super_block *sb)
+static void fsnotify_unmount_inodes(struct super_block *sb)
 {
 	struct inode *inode, *iput_inode = NULL;
 
@@ -98,6 +98,12 @@ void fsnotify_unmount_inodes(struct super_block *sb)
 		iput(iput_inode);
 }
 
+void __fsnotify_sb_delete(struct super_block *sb)
+{
+	fsnotify_unmount_inodes(sb);
+	fsnotify_clear_marks_by_sb(sb);
+}
+
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index e70d39aed9d8..2ef9f6e9084f 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -31,6 +31,11 @@ static inline void fsnotify_clear_marks_by_mount(struct vfsmount *mnt)
 {
 	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify);
 }
+/* run the list of all marks associated with sb and destroy them */
+static inline void fsnotify_clear_marks_by_sb(struct super_block *sb)
+{
+	fsnotify_destroy_marks(&sb->s_fsnotify);
+}
 /* Wait until all marks queued for destruction are destroyed */
 extern void fsnotify_wait_marks_destroyed(void);
 
@@ -54,4 +59,9 @@ static inline struct vfsmount *fsnotify_vfsmount(struct fsnotify_obj *obj)
 	return &(container_of(obj, struct mount, mnt_fsnotify))->mnt;
 }
 
+static inline struct super_block *fsnotify_sb(struct fsnotify_obj *obj)
+{
+	return container_of(obj, struct super_block, s_fsnotify);
+}
+
 #endif	/* __FS_NOTIFY_FSNOTIFY_H_ */
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 3b19ed700ac3..994f6131d307 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -132,6 +132,8 @@ static struct fsnotify_obj *fsnotify_connector_obj(
 		return &conn->inode->i_fsnotify;
 	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 		return &real_mount(conn->mnt)->mnt_fsnotify;
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_SB)
+		return &conn->sb->s_fsnotify;
 	else
 		return NULL;
 }
@@ -460,6 +462,8 @@ static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
 		inode = conn->inode = igrab(fsnotify_inode(obj));
 	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 		conn->mnt = fsnotify_vfsmount(obj);
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_SB)
+		conn->sb = fsnotify_sb(obj);
 	/*
 	 * cmpxchg() provides the barrier so that readers of obj->marks can see
 	 * only initialized structure
diff --git a/fs/super.c b/fs/super.c
index 672538ca9831..875c5ed9a29c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -425,7 +425,7 @@ void generic_shutdown_super(struct super_block *sb)
 		sync_filesystem(sb);
 		sb->s_flags &= ~SB_ACTIVE;
 
-		fsnotify_unmount_inodes(sb);
+		fsnotify_sb_delete(sb);
 		cgroup_writeback_umount();
 
 		evict_inodes(sb);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 144a488fb74b..1c13b2d901f5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1445,6 +1445,10 @@ struct super_block {
 
 	spinlock_t		s_inode_wblist_lock;
 	struct list_head	s_inodes_wb;	/* writeback inodes */
+
+#ifdef CONFIG_FSNOTIFY
+	struct fsnotify_obj	s_fsnotify;
+#endif
 } __randomize_layout;
 
 /* Helper functions so that in most cases filesystems will
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index bdaf22582f6e..7d70bdc8af04 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -115,6 +115,14 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
 }
 
 /*
+ * fsnotify_sb_delete - a super block is being destroyed, clean up is needed
+ */
+static inline void fsnotify_sb_delete(struct super_block *sb)
+{
+	__fsnotify_sb_delete(sb);
+}
+
+/*
  * fsnotify_nameremove - a filename was removed from a directory
  */
 static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index abbd7311077f..3c1cc6ee5b49 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -203,12 +203,14 @@ struct fsnotify_group {
 enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_INODE,
 	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
+	FSNOTIFY_OBJ_TYPE_SB,
 	FSNOTIFY_OBJ_TYPE_MAX,
 	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_MAX
 };
 
 #define FSNOTIFY_OBJ_TYPE_INODE_FL	(1U << FSNOTIFY_OBJ_TYPE_INODE)
 #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+#define FSNOTIFY_OBJ_TYPE_SB_FL		(1U << FSNOTIFY_OBJ_TYPE_SB)
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
 
 static inline bool fsnotify_valid_obj_type(unsigned int type)
@@ -256,6 +258,7 @@ static inline void fsnotify_iter_set_##name##_mark( \
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
+FSNOTIFY_ITER_FUNCS(sb, SB)
 
 #define fsnotify_foreach_obj_type(type) \
 	for (type = 0; type < FSNOTIFY_OBJ_TYPE_MAX; type++)
@@ -266,8 +269,8 @@ FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 	     type++, mark = fsnotify_iter_type_mark(iter, type)) \
 
 /*
- * Inode / vfsmount point to this structure which tracks all marks attached to
- * the inode / vfsmount. The reference to inode / vfsmount is held by this
+ * Inode/vfsmount/sb point to this structure which tracks all marks attached to
+ * the inode/vfsmount/sb. The reference to inode/vfsmount/sb is held by this
  * structure. We destroy this structure when there are no more marks attached
  * to it. The structure is protected by fsnotify_mark_srcu.
  */
@@ -278,6 +281,7 @@ struct fsnotify_mark_connector {
 		void *object;
 		struct inode *inode;
 		struct vfsmount *mnt;
+		struct super_block *sb;
 	};
 	union {
 		struct hlist_head list;
@@ -337,6 +341,7 @@ extern int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int dat
 extern int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask);
 extern void __fsnotify_inode_delete(struct inode *inode);
 extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt);
+extern void __fsnotify_sb_delete(struct super_block *sb);
 extern u32 fsnotify_get_cookie(void);
 
 static inline int fsnotify_inode_watches_children(struct inode *inode)
@@ -449,9 +454,13 @@ static inline void fsnotify_clear_inode_marks_by_group(struct fsnotify_group *gr
 {
 	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE_FL);
 }
+/* run all the marks in a group, and clear all of the sn marks */
+static inline void fsnotify_clear_sb_marks_by_group(struct fsnotify_group *group)
+{
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_SB_FL);
+}
 extern void fsnotify_get_mark(struct fsnotify_mark *mark);
 extern void fsnotify_put_mark(struct fsnotify_mark *mark);
-extern void fsnotify_unmount_inodes(struct super_block *sb);
 extern void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info);
 extern bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info);
 
-- 
2.7.4

Re: [PATCH v2 17/20] fsnotify: add super block object type

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1401 bytes --]

Hi Amir,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.16]
[cannot apply to linus/master next-20180405]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-super-block-mark/20180406-132931
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from fs//attr.c:15:0:
   include/linux/fsnotify.h: In function 'fsnotify_sb_delete':
>> include/linux/fsnotify.h:122:2: error: implicit declaration of function '__fsnotify_sb_delete'; did you mean 'fsnotify_sb_delete'? [-Werror=implicit-function-declaration]
     __fsnotify_sb_delete(sb);
     ^~~~~~~~~~~~~~~~~~~~
     fsnotify_sb_delete
   cc1: some warnings being treated as errors

vim +122 include/linux/fsnotify.h

   116	
   117	/*
   118	 * fsnotify_sb_delete - a super block is being destroyed, clean up is needed
   119	 */
   120	static inline void fsnotify_sb_delete(struct super_block *sb)
   121	{
 > 122		__fsnotify_sb_delete(sb);
   123	}
   124	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6721 bytes --]

Re: [PATCH v2 17/20] fsnotify: add super block object type

[Amir Goldstein]

On Fri, Apr 6, 2018 at 9:01 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Amir,
>
> I love your patch! Yet something to improve:

Thank you robot :)

>
> [auto build test ERROR on v4.16]
> [cannot apply to linus/master next-20180405]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-super-block-mark/20180406-132931
> config: i386-tinyconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
>    In file included from fs//attr.c:15:0:
>    include/linux/fsnotify.h: In function 'fsnotify_sb_delete':
>>> include/linux/fsnotify.h:122:2: error: implicit declaration of function '__fsnotify_sb_delete'; did you mean 'fsnotify_sb_delete'? [-Werror=implicit-function-declaration]
>      __fsnotify_sb_delete(sb);
>      ^~~~~~~~~~~~~~~~~~~~
>      fsnotify_sb_delete
>    cc1: some warnings being treated as errors
>
> vim +122 include/linux/fsnotify.h
>
>    116
>    117  /*
>    118   * fsnotify_sb_delete - a super block is being destroyed, clean up is needed
>    119   */
>    120  static inline void fsnotify_sb_delete(struct super_block *sb)
>    121  {
>  > 122          __fsnotify_sb_delete(sb);
>    123  }
>    124
>

Jan,

What do you think about ifdefing away everything in fsnotify.h
for CONFIG_FSNOTIFY=n?

Seems like the right thing to do anyway and then we won't need
those __fsnotify_XXX_delete() noops in fsnotify_backend.h

Thanks,
Amir.

Re: [PATCH v2 17/20] fsnotify: add super block object type

[Jan Kara]

On Fri 06-04-18 14:04:23, Amir Goldstein wrote:
> On Fri, Apr 6, 2018 at 9:01 AM, kbuild test robot <lkp@intel.com> wrote:
> > Hi Amir,
> >
> > I love your patch! Yet something to improve:
> 
> Thank you robot :)
> 
> >
> > [auto build test ERROR on v4.16]
> > [cannot apply to linus/master next-20180405]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-super-block-mark/20180406-132931
> > config: i386-tinyconfig (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=i386
> >
> > All errors (new ones prefixed by >>):
> >
> >    In file included from fs//attr.c:15:0:
> >    include/linux/fsnotify.h: In function 'fsnotify_sb_delete':
> >>> include/linux/fsnotify.h:122:2: error: implicit declaration of function '__fsnotify_sb_delete'; did you mean 'fsnotify_sb_delete'? [-Werror=implicit-function-declaration]
> >      __fsnotify_sb_delete(sb);
> >      ^~~~~~~~~~~~~~~~~~~~
> >      fsnotify_sb_delete
> >    cc1: some warnings being treated as errors
> >
> > vim +122 include/linux/fsnotify.h
> >
> >    116
> >    117  /*
> >    118   * fsnotify_sb_delete - a super block is being destroyed, clean up is needed
> >    119   */
> >    120  static inline void fsnotify_sb_delete(struct super_block *sb)
> >    121  {
> >  > 122          __fsnotify_sb_delete(sb);
> >    123  }
> >    124
> >
> 
> Jan,
> 
> What do you think about ifdefing away everything in fsnotify.h
> for CONFIG_FSNOTIFY=n?

I'm fine with that but I suspect you'll then have to define empty stubs for
the stuff defined in fsnotify.h when CONFIG_FSNOTIFY=n, won't you?

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

Re: [PATCH v2 17/20] fsnotify: add super block object type

[Amir Goldstein]

On Thu, Apr 19, 2018 at 3:41 PM, Jan Kara <jack@suse.cz> wrote:
> On Fri 06-04-18 14:04:23, Amir Goldstein wrote:
>> On Fri, Apr 6, 2018 at 9:01 AM, kbuild test robot <lkp@intel.com> wrote:
>> > Hi Amir,
>> >
>> > I love your patch! Yet something to improve:
>>
>> Thank you robot :)
>>
>> >
>> > [auto build test ERROR on v4.16]
>> > [cannot apply to linus/master next-20180405]
>> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>> >
>> > url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-super-block-mark/20180406-132931
>> > config: i386-tinyconfig (attached as .config)
>> > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
>> > reproduce:
>> >         # save the attached .config to linux build tree
>> >         make ARCH=i386
>> >
>> > All errors (new ones prefixed by >>):
>> >
>> >    In file included from fs//attr.c:15:0:
>> >    include/linux/fsnotify.h: In function 'fsnotify_sb_delete':
>> >>> include/linux/fsnotify.h:122:2: error: implicit declaration of function '__fsnotify_sb_delete'; did you mean 'fsnotify_sb_delete'? [-Werror=implicit-function-declaration]
>> >      __fsnotify_sb_delete(sb);
>> >      ^~~~~~~~~~~~~~~~~~~~
>> >      fsnotify_sb_delete
>> >    cc1: some warnings being treated as errors
>> >
>> > vim +122 include/linux/fsnotify.h
>> >
>> >    116
>> >    117  /*
>> >    118   * fsnotify_sb_delete - a super block is being destroyed, clean up is needed
>> >    119   */
>> >    120  static inline void fsnotify_sb_delete(struct super_block *sb)
>> >    121  {
>> >  > 122          __fsnotify_sb_delete(sb);
>> >    123  }
>> >    124
>> >
>>
>> Jan,
>>
>> What do you think about ifdefing away everything in fsnotify.h
>> for CONFIG_FSNOTIFY=n?
>
> I'm fine with that but I suspect you'll then have to define empty stubs for
> the stuff defined in fsnotify.h when CONFIG_FSNOTIFY=n, won't you?
>

Yes, I though that will be cleaner - zero footprint of fsnotify for
CONFIG_FSNOTIFY=n,
but now I realize that audit_inode_child() piggy backs on the
fsnotify_ hooks and
CONFIG_AUDIT is independent of  CONFIG_FSNOTIFY, so I will just get rid of
__fsnotify_sb_delete() as you suggested.

Thanks,
Amir.

Re: [PATCH v2 17/20] fsnotify: add super block object type

[Jan Kara]

On Thu 05-04-18 16:18:18, Amir Goldstein wrote:
> @@ -98,6 +98,12 @@ void fsnotify_unmount_inodes(struct super_block *sb)
>  		iput(iput_inode);
>  }
>  
> +void __fsnotify_sb_delete(struct super_block *sb)
> +{
> +	fsnotify_unmount_inodes(sb);
> +	fsnotify_clear_marks_by_sb(sb);
> +}
> +

I think it is pointless to have fsnotify_sb_delete() and
__fsnotify_sb_delete(). I know some other fsnotify API is like that and I'm
slowly trying to get rid of that overabstraction. Just make this
fsnotify_sb_delete() directly.

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

[PATCH v2 18/20] fsnotify: send path type events to group with super block marks

[Amir Goldstein]

Send events to group if super block mark mask matches the event
and unless the same group has an ignore mask on the vfsmount or
the inode on which the event occurred.

Soon, fanotify backend is going to support super block marks and
fanotify currently only supports path type events.

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

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 2ef97bc27e20..f507d12a9e99 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -320,15 +320,18 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	     const unsigned char *file_name, u32 cookie)
 {
 	struct fsnotify_iter_info iter_info = {};
-	struct mount *mnt;
+	struct super_block *sb = NULL;
+	struct mount *mnt = NULL;
+	__u32 mnt_or_sb_mask = 0;
 	int ret = 0;
 	/* global tests shouldn't care about events on child only the specific event */
 	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
 
-	if (data_is == FSNOTIFY_EVENT_PATH)
+	if (data_is == FSNOTIFY_EVENT_PATH) {
 		mnt = real_mount(((const struct path *)data)->mnt);
-	else
-		mnt = NULL;
+		sb = mnt->mnt.mnt_sb;
+		mnt_or_sb_mask = mnt->mnt_fsnotify.mask | sb->s_fsnotify.mask;
+	}
 
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
@@ -338,7 +341,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 * need SRCU to keep them "alive".
 	 */
 	if (!to_tell->i_fsnotify.marks &&
-	    (!mnt || !mnt->mnt_fsnotify.marks))
+	    (!mnt || (!mnt->mnt_fsnotify.marks && !sb->s_fsnotify.marks)))
 		return 0;
 	/*
 	 * if this is a modify event we may need to clear the ignored masks
@@ -347,7 +350,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 */
 	if (!(mask & FS_MODIFY) &&
 	    !(test_mask & to_tell->i_fsnotify.mask) &&
-	    !(mnt && test_mask & mnt->mnt_fsnotify.mask))
+	    !(mnt && (test_mask & mnt_or_sb_mask)))
 		return 0;
 
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
@@ -359,11 +362,15 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	}
 
 	if (mnt && ((mask & FS_MODIFY) ||
-		    (test_mask & mnt->mnt_fsnotify.mask))) {
+		    (test_mask & mnt_or_sb_mask))) {
 		fsnotify_iter_set_inode_mark(&iter_info,
 			fsnotify_first_mark(&to_tell->i_fsnotify.marks));
 		fsnotify_iter_set_vfsmount_mark(&iter_info,
 			fsnotify_first_mark(&mnt->mnt_fsnotify.marks));
+		if ((mask & FS_MODIFY) ||
+		    (test_mask & sb->s_fsnotify.mask))
+			fsnotify_iter_set_sb_mark(&iter_info,
+				fsnotify_first_mark(&sb->s_fsnotify.marks));
 	}
 
 	/*
-- 
2.7.4

Re: [PATCH v2 18/20] fsnotify: send path type events to group with super block marks

[Jan Kara]

On Thu 05-04-18 16:18:19, Amir Goldstein wrote:
> Send events to group if super block mark mask matches the event
> and unless the same group has an ignore mask on the vfsmount or
> the inode on which the event occurred.
> 
> Soon, fanotify backend is going to support super block marks and
> fanotify currently only supports path type events.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

So what I miss in this patch set is a description (manpage style) of what
is the desired semantics of the new functionality. Then also what usecases
motivate this. Probably this belongs to the initial patch. Also linux-api
should be CCed as this is a new API so it should get wider scrutiny.

Also I'm somewhat concerned with the security of superblock marks - sure
fanotify is currently guarded by CAP_SYS_ADMIN but that seriously limits
its usefulness so long-term we might need to get rid of that at least for
some subset of the functionality or at least relieve that to CAP_SYS_ADMIN
inside current namespace. And I'm not sure superblock marks are safe even
for CAP_SYS_ADMIN process in the current namespace as the process could
escape from its current mount namespace by that. But maybe I'm wrong. I'll
try to extract more knowledge about this from some guys at LSF/MM...

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

Re: [PATCH v2 18/20] fsnotify: send path type events to group with super block marks

[Amir Goldstein]

On Thu, Apr 19, 2018 at 4:49 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:19, Amir Goldstein wrote:
>> Send events to group if super block mark mask matches the event
>> and unless the same group has an ignore mask on the vfsmount or
>> the inode on which the event occurred.
>>
>> Soon, fanotify backend is going to support super block marks and
>> fanotify currently only supports path type events.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> So what I miss in this patch set is a description (manpage style) of what
> is the desired semantics of the new functionality. Then also what usecases
> motivate this. Probably this belongs to the initial patch. Also linux-api
> should be CCed as this is a new API so it should get wider scrutiny.
>
> Also I'm somewhat concerned with the security of superblock marks - sure
> fanotify is currently guarded by CAP_SYS_ADMIN but that seriously limits
> its usefulness so long-term we might need to get rid of that at least for

My long term goal is for fanotify to be a super set of inotify, so that is part
of the plan. The easiest way would be to introduce fanotify_init() flags that
hide all the information in the fanotify event that is sensitive and
allow unprivileged
user to setup a file/dir watch according to same permission ruled of inotify.

> some subset of the functionality or at least relieve that to CAP_SYS_ADMIN
> inside current namespace. And I'm not sure superblock marks are safe even
> for CAP_SYS_ADMIN process in the current namespace as the process could
> escape from its current mount namespace by that. But maybe I'm wrong.

IMO, setup of super block watch should require same permissions as mount of
block device, so if fs has no FS_USERNS_MOUNT flag, only root ns admin should
be able to setup a super block watch and then there is no issue with security.

> I'll try to extract more knowledge about this from some guys at LSF/MM...

Yeh, let's do that.
I have a 30 minute slot for "persistent change journal", I will start with
a 5 min overview of super block watch roadmap to see if there are any
objections.

Thanks,
Amir.

[PATCH v2 19/20] fanotify: factor out helpers to add/remove mark

[Amir Goldstein]

Factor out helpers fanotify_add_mark() and fanotify_remove_mark()
to reduce duplicated code.

These helpers are going to be used for adding functions to add/remove
a super block mark.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 90 ++++++++++++++------------------------
 1 file changed, 32 insertions(+), 58 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 9f2e7e2c33b3..5e78f6672da2 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -524,16 +524,16 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 	return mask & oldmask;
 }
 
-static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
-					 struct vfsmount *mnt, __u32 mask,
-					 unsigned int flags)
+static int fanotify_remove_mark(struct fsnotify_group *group,
+				struct fsnotify_obj *obj,
+				__u32 mask, unsigned int flags)
 {
 	struct fsnotify_mark *fsn_mark = NULL;
 	__u32 removed;
 	int destroy_mark;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify, group);
+	fsn_mark = fsnotify_find_mark(obj, group);
 	if (!fsn_mark) {
 		mutex_unlock(&group->mark_mutex);
 		return -ENOENT;
@@ -541,47 +541,32 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 &destroy_mark);
-	if (removed & real_mount(mnt)->mnt_fsnotify.mask)
-		fsnotify_recalc_mask(&real_mount(mnt)->mnt_fsnotify);
+	if (removed & obj->mask)
+		fsnotify_recalc_mask(obj);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
 	if (destroy_mark)
 		fsnotify_free_mark(fsn_mark);
 
+	/* matches the fsnotify_find_mark() */
 	fsnotify_put_mark(fsn_mark);
 	return 0;
 }
 
+static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
+					 struct vfsmount *mnt, __u32 mask,
+					 unsigned int flags)
+{
+	return fanotify_remove_mark(group, &real_mount(mnt)->mnt_fsnotify,
+				    mask, flags);
+}
+
 static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 				      struct inode *inode, __u32 mask,
 				      unsigned int flags)
 {
-	struct fsnotify_mark *fsn_mark = NULL;
-	__u32 removed;
-	int destroy_mark;
-
-	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, group);
-	if (!fsn_mark) {
-		mutex_unlock(&group->mark_mutex);
-		return -ENOENT;
-	}
-
-	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
-						 &destroy_mark);
-	if (removed & inode->i_fsnotify.mask)
-		fsnotify_recalc_mask(&inode->i_fsnotify);
-	if (destroy_mark)
-		fsnotify_detach_mark(fsn_mark);
-	mutex_unlock(&group->mark_mutex);
-	if (destroy_mark)
-		fsnotify_free_mark(fsn_mark);
-
-	/* matches the fsnotify_find_mark() */
-	fsnotify_put_mark(fsn_mark);
-
-	return 0;
+	return fanotify_remove_mark(group, &inode->i_fsnotify, mask, flags);
 }
 
 static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
@@ -638,40 +623,43 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 }
 
 
-static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
-				      struct vfsmount *mnt, __u32 mask,
-				      unsigned int flags)
+static int fanotify_add_mark(struct fsnotify_group *group,
+			     struct fsnotify_obj *obj, unsigned int type,
+			     __u32 mask, unsigned int flags)
 {
-	struct fsnotify_obj *obj = &real_mount(mnt)->mnt_fsnotify;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
 	fsn_mark = fsnotify_find_mark(obj, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, obj, FSNOTIFY_OBJ_TYPE_VFSMOUNT);
+		fsn_mark = fanotify_add_new_mark(group, obj, type);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
 		}
 	}
 	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
-	if (added & ~real_mount(mnt)->mnt_fsnotify.mask)
-		fsnotify_recalc_mask(&real_mount(mnt)->mnt_fsnotify);
+	if (added & ~obj->mask)
+		fsnotify_recalc_mask(obj);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
 	return 0;
 }
 
+static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
+				      struct vfsmount *mnt, __u32 mask,
+				      unsigned int flags)
+{
+	return fanotify_add_mark(group, &real_mount(mnt)->mnt_fsnotify,
+				 FSNOTIFY_OBJ_TYPE_VFSMOUNT, mask, flags);
+}
+
 static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
 {
-	struct fsnotify_obj *obj = &inode->i_fsnotify;
-	struct fsnotify_mark *fsn_mark;
-	__u32 added;
-
 	pr_debug("%s: group=%p inode=%p\n", __func__, group, inode);
 
 	/*
@@ -684,22 +672,8 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 	    (atomic_read(&inode->i_writecount) > 0))
 		return 0;
 
-	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(obj, group);
-	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, obj, FSNOTIFY_OBJ_TYPE_INODE);
-		if (IS_ERR(fsn_mark)) {
-			mutex_unlock(&group->mark_mutex);
-			return PTR_ERR(fsn_mark);
-		}
-	}
-	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
-	if (added & ~inode->i_fsnotify.mask)
-		fsnotify_recalc_mask(&inode->i_fsnotify);
-	mutex_unlock(&group->mark_mutex);
-
-	fsnotify_put_mark(fsn_mark);
-	return 0;
+	return fanotify_add_mark(group, &inode->i_fsnotify,
+				 FSNOTIFY_OBJ_TYPE_INODE, mask, flags);
 }
 
 /* fanotify syscalls */
-- 
2.7.4

[PATCH v2 20/20] fanotify: add API to attach/detach super block mark

[Amir Goldstein]

Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
of super block mark type.

Only one of FAN_MARK_MOUNT and FAN_MARK_FILESYSTEM mark type flags
may be provided to fanotify_mark() or no mark type flag for inode mark.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 37 ++++++++++++++++++++++++++++++++-----
 include/uapi/linux/fanotify.h      |  7 ++++++-
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 5e78f6672da2..733386cc1307 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -562,6 +562,13 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 				    mask, flags);
 }
 
+static int fanotify_remove_sb_mark(struct fsnotify_group *group,
+				      struct super_block *sb, __u32 mask,
+				      unsigned int flags)
+{
+	return fanotify_remove_mark(group, &sb->s_fsnotify, mask, flags);
+}
+
 static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 				      struct inode *inode, __u32 mask,
 				      unsigned int flags)
@@ -656,6 +663,14 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 				 FSNOTIFY_OBJ_TYPE_VFSMOUNT, mask, flags);
 }
 
+static int fanotify_add_sb_mark(struct fsnotify_group *group,
+				      struct super_block *sb, __u32 mask,
+				      unsigned int flags)
+{
+	return fanotify_add_mark(group, &sb->s_fsnotify,
+				 FSNOTIFY_OBJ_TYPE_SB, mask, flags);
+}
+
 static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
@@ -804,6 +819,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 	struct fd f;
 	struct path path;
 	u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD;
+	unsigned int mark_type = flags & FAN_ALL_MARK_TYPE_FLAGS;
 	int ret;
 
 	pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
@@ -815,6 +831,11 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 
 	if (flags & ~FAN_ALL_MARK_FLAGS)
 		return -EINVAL;
+
+	/* mixed mark type flags are invalid */
+	if (mark_type && !is_power_of_2(mark_type))
+		return -EINVAL;
+
 	switch (flags & (FAN_MARK_ADD | FAN_MARK_REMOVE | FAN_MARK_FLUSH)) {
 	case FAN_MARK_ADD:		/* fallthrough */
 	case FAN_MARK_REMOVE:
@@ -822,7 +843,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 			return -EINVAL;
 		break;
 	case FAN_MARK_FLUSH:
-		if (flags & ~(FAN_MARK_MOUNT | FAN_MARK_FLUSH))
+		if (flags & ~(FAN_ALL_MARK_TYPE_FLAGS | FAN_MARK_FLUSH))
 			return -EINVAL;
 		break;
 	default:
@@ -861,8 +882,10 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 
 	if (flags & FAN_MARK_FLUSH) {
 		ret = 0;
-		if (flags & FAN_MARK_MOUNT)
+		if (mark_type == FAN_MARK_MOUNT)
 			fsnotify_clear_vfsmount_marks_by_group(group);
+		else if (mark_type == FAN_MARK_FILESYSTEM)
+			fsnotify_clear_sb_marks_by_group(group);
 		else
 			fsnotify_clear_inode_marks_by_group(group);
 		goto fput_and_out;
@@ -873,7 +896,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 		goto fput_and_out;
 
 	/* inode held in place by reference to path; group by fget on fd */
-	if (!(flags & FAN_MARK_MOUNT))
+	if (mark_type == FAN_MARK_INODE)
 		inode = path.dentry->d_inode;
 	else
 		mnt = path.mnt;
@@ -881,14 +904,18 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 	/* create/update an inode mark */
 	switch (flags & (FAN_MARK_ADD | FAN_MARK_REMOVE)) {
 	case FAN_MARK_ADD:
-		if (flags & FAN_MARK_MOUNT)
+		if (mark_type == FAN_MARK_MOUNT)
 			ret = fanotify_add_vfsmount_mark(group, mnt, mask, flags);
+		else if (mark_type == FAN_MARK_FILESYSTEM)
+			ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, flags);
 		else
 			ret = fanotify_add_inode_mark(group, inode, mask, flags);
 		break;
 	case FAN_MARK_REMOVE:
-		if (flags & FAN_MARK_MOUNT)
+		if (mark_type == FAN_MARK_MOUNT)
 			ret = fanotify_remove_vfsmount_mark(group, mnt, mask, flags);
+		else if (mark_type == FAN_MARK_FILESYSTEM)
+			ret = fanotify_remove_sb_mark(group, mnt->mnt_sb, mask, flags);
 		else
 			ret = fanotify_remove_inode_mark(group, inode, mask, flags);
 		break;
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 74247917de04..7345b9a57f66 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -43,6 +43,7 @@
 				 FAN_UNLIMITED_MARKS)
 
 /* flags used for fanotify_modify_mark() */
+#define FAN_MARK_INODE		0x00000000
 #define FAN_MARK_ADD		0x00000001
 #define FAN_MARK_REMOVE		0x00000002
 #define FAN_MARK_DONT_FOLLOW	0x00000004
@@ -51,6 +52,7 @@
 #define FAN_MARK_IGNORED_MASK	0x00000020
 #define FAN_MARK_IGNORED_SURV_MODIFY	0x00000040
 #define FAN_MARK_FLUSH		0x00000080
+#define FAN_MARK_FILESYSTEM	0x00000100
 
 #define FAN_ALL_MARK_FLAGS	(FAN_MARK_ADD |\
 				 FAN_MARK_REMOVE |\
@@ -59,7 +61,10 @@
 				 FAN_MARK_MOUNT |\
 				 FAN_MARK_IGNORED_MASK |\
 				 FAN_MARK_IGNORED_SURV_MODIFY |\
-				 FAN_MARK_FLUSH)
+				 FAN_MARK_FLUSH|\
+				 FAN_MARK_FILESYSTEM)
+
+#define FAN_ALL_MARK_TYPE_FLAGS (FAN_MARK_MOUNT | FAN_MARK_FILESYSTEM)
 
 /*
  * All of the events - we build the list by hand so that we can add flags in
-- 
2.7.4