[PATCH v3 0/3] fanotify super block marks

[PATCH v3 0/3] fanotify super block marks

[Amir Goldstein]

Jan,

The following patches implement the new mark type FAN_MARK_FILESYSTEM
to enable monitoring of filesystem events on all filesystem objects
regardless of the mount where event was generated.

These patches are available on my github [1] along with LTP tests [2]
and man-pages update [3].

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fanotify_sb
[2] https://github.com/amir73il/ltp/commits/fanotify_sb
[3] https://github.com/amir73il/man-pages/commits/fanotify_sb

Changes since v2:
- Rebased on top of merged cleanup patches
- Show super block marks in fanotify_fdinfo()
- Add man pages patch [3]

Amir Goldstein (3):
  fsnotify: add super block object type
  fsnotify: send path type events to group with super block marks
  fanotify: add API to attach/detach super block mark

 fs/notify/fanotify/fanotify_user.c | 37 ++++++++++++++++++++++++++++++++-----
 fs/notify/fdinfo.c                 |  5 +++++
 fs/notify/fsnotify.c               | 35 ++++++++++++++++++++++++-----------
 fs/notify/fsnotify.h               | 11 +++++++++++
 fs/notify/mark.c                   |  4 ++++
 fs/super.c                         |  2 +-
 include/linux/fs.h                 |  5 +++++
 include/linux/fsnotify_backend.h   | 17 ++++++++++++++---
 include/uapi/linux/fanotify.h      |  7 ++++++-
 9 files changed, 102 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] 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.

This is going to be used by fanotify backend to setup super block
marks.

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

diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 86fcf5814279..25385e336ac7 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -131,6 +131,11 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 
 		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
 			   mnt->mnt_id, mflags, mark->mask, mark->ignored_mask);
+	} else if (mark->connector->type == FSNOTIFY_OBJ_TYPE_SB) {
+		struct super_block *sb = fsnotify_conn_sb(mark->connector);
+
+		seq_printf(m, "fanotify sdev:%x mflags:%x mask:%x ignored_mask:%x\n",
+			   sb->s_dev, mflags, mark->mask, mark->ignored_mask);
 	}
 }
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index f174397b63a0..775e731d3016 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 7902653dd577..5a00121fb219 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -21,6 +21,12 @@ static inline struct mount *fsnotify_conn_mount(
 	return container_of(conn->obj, struct mount, mnt_fsnotify_marks);
 }
 
+static inline struct super_block *fsnotify_conn_sb(
+				struct fsnotify_mark_connector *conn)
+{
+	return container_of(conn->obj, struct super_block, s_fsnotify_marks);
+}
+
 /* destroy all events sitting in this groups notification queue */
 extern void fsnotify_flush_notify(struct fsnotify_group *group);
 
@@ -43,6 +49,11 @@ static inline void fsnotify_clear_marks_by_mount(struct vfsmount *mnt)
 {
 	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify_marks);
 }
+/* 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_marks);
+}
 /* Wait until all marks queued for destruction are destroyed */
 extern void fsnotify_wait_marks_destroyed(void);
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 59cdb27826de..b5172ccb2e60 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -115,6 +115,8 @@ static __u32 *fsnotify_conn_mask_p(struct fsnotify_mark_connector *conn)
 		return &fsnotify_conn_inode(conn)->i_fsnotify_mask;
 	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 		return &fsnotify_conn_mount(conn)->mnt_fsnotify_mask;
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_SB)
+		return &fsnotify_conn_sb(conn)->s_fsnotify_mask;
 	return NULL;
 }
 
@@ -192,6 +194,8 @@ static struct inode *fsnotify_detach_connector_from_object(
 		inode->i_fsnotify_mask = 0;
 	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
 		fsnotify_conn_mount(conn)->mnt_fsnotify_mask = 0;
+	} else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) {
+		fsnotify_conn_sb(conn)->s_fsnotify_mask = 0;
 	}
 
 	rcu_assign_pointer(*(conn->obj), NULL);
diff --git a/fs/super.c b/fs/super.c
index f3a8c008e164..ca53a08497ed 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -442,7 +442,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 6c0b4a1c22ff..4f0a11d0d296 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1464,6 +1464,11 @@ struct super_block {
 
 	spinlock_t		s_inode_wblist_lock;
 	struct list_head	s_inodes_wb;	/* writeback inodes */
+
+#ifdef CONFIG_FSNOTIFY
+	__u32			s_fsnotify_mask;
+	struct fsnotify_mark_connector __rcu	*s_fsnotify_marks;
+#endif
 } __randomize_layout;
 
 /* Helper functions so that in most cases filesystems will
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b8f4182f42f1..81b88fc9df31 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -206,12 +206,14 @@ struct fsnotify_group {
 enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_INODE,
 	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
+	FSNOTIFY_OBJ_TYPE_SB,
 	FSNOTIFY_OBJ_TYPE_COUNT,
 	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_COUNT
 };
 
 #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_COUNT) - 1)
 
 static inline bool fsnotify_valid_obj_type(unsigned int type)
@@ -255,6 +257,7 @@ static inline struct fsnotify_mark *fsnotify_iter_##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_COUNT; type++)
@@ -267,8 +270,8 @@ struct fsnotify_mark_connector;
 typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
 
 /*
- * 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.
  */
@@ -335,6 +338,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)
@@ -455,9 +459,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);
 
@@ -484,6 +492,9 @@ static inline void __fsnotify_inode_delete(struct inode *inode)
 static inline void __fsnotify_vfsmount_delete(struct vfsmount *mnt)
 {}
 
+static inline void fsnotify_sb_delete(struct super_block *sb)
+{}
+
 static inline void fsnotify_update_flags(struct dentry *dentry)
 {}
 
-- 
2.7.4

[PATCH v3 2/3] 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 backend only supports path type events.

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

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 775e731d3016..89a71242b786 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -325,15 +325,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
@@ -343,16 +346,16 @@ 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
-	 * otherwise return if neither the inode nor the vfsmount care about
+	 * otherwise return if neither the inode nor the vfsmount/sb care about
 	 * this type of event.
 	 */
 	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);
@@ -364,16 +367,20 @@ 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))) {
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
 			fsnotify_first_mark(&to_tell->i_fsnotify_marks);
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
 			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
+		if ((mask & FS_MODIFY) ||
+		    (test_mask & sb->s_fsnotify_mask))
+			iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
+				fsnotify_first_mark(&sb->s_fsnotify_marks);
 	}
 
 	/*
-	 * We need to merge inode & vfsmount mark lists so that inode mark
-	 * ignore masks are properly reflected for mount mark notifications.
+	 * We need to merge inode/vfsmount/sb mark lists so that e.g. inode mark
+	 * ignore masks are properly reflected for mount/sb mark notifications.
 	 * That's why this traversal is so complicated...
 	 */
 	while (fsnotify_iter_select_report_types(&iter_info)) {
-- 
2.7.4

[PATCH v3 3/3] 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.

A super block watch gets all events on the filesystem, regardless of
the mount from which the mark was added, unless an ignore mask exists
on either the inode or the mount where the event was generated.

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.

Cc: <linux-api@vger.kernel.org>
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 69054886915b..dff64c7e75fd 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -563,6 +563,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_marks, mask, flags);
+}
+
 static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 				      struct inode *inode, __u32 mask,
 				      unsigned int flags)
@@ -658,6 +665,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_marks,
+				 FSNOTIFY_OBJ_TYPE_SB, mask, flags);
+}
+
 static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
@@ -806,6 +821,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	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",
@@ -817,6 +833,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 
 	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:
@@ -824,7 +845,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 			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:
@@ -863,8 +884,10 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 
 	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;
@@ -875,7 +898,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		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;
@@ -883,14 +906,18 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	/* 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

Re: [PATCH v3 2/3] fsnotify: send path type events to group with super block marks

[Jan Kara]

On Thu 30-08-18 18:15:50, 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 backend only supports path type events.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Two small questions below. Otherwise the patch looks good to me.

> ---
>  fs/notify/fsnotify.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
...
>  	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)))

When you use mnt_or_sb_mask, the 'mnt' check is useless, right?

>  	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
> @@ -364,16 +367,20 @@ 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))) {
>  		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
>  			fsnotify_first_mark(&to_tell->i_fsnotify_marks);
>  		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
>  			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
> +		if ((mask & FS_MODIFY) ||
> +		    (test_mask & sb->s_fsnotify_mask))

Why is here this additional test? We might need to clear ignore masks on SB
list if nothing else. Also we need to reflect ignore mask from the
superblock marks... I agree there's probably no huge use for either of
these two functionalities but I just don't see a strong reason for
sb & mnt marks to behave differently.

> +			iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
> +				fsnotify_first_mark(&sb->s_fsnotify_marks);
>  	}
>  
>  	/*
> -	 * We need to merge inode & vfsmount mark lists so that inode mark
> -	 * ignore masks are properly reflected for mount mark notifications.
> +	 * We need to merge inode/vfsmount/sb mark lists so that e.g. inode mark
> +	 * ignore masks are properly reflected for mount/sb mark notifications.
>  	 * That's why this traversal is so complicated...
>  	 */
>  	while (fsnotify_iter_select_report_types(&iter_info)) {
> -- 
> 2.7.4
> 

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

Re: [PATCH v3 1/3] fsnotify: add super block object type

[Jan Kara]

On Thu 30-08-18 18:15:49, Amir Goldstein wrote:
> Add the infrastructure to attach a mark to a super_block struct
> and detach all attached marks when super block is destroyed.
> 
> This is going to be used by fanotify backend to setup super block
> marks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

This patch looks OK to me.

								Honza

> ---
>  fs/notify/fdinfo.c               |  5 +++++
>  fs/notify/fsnotify.c             |  8 +++++++-
>  fs/notify/fsnotify.h             | 11 +++++++++++
>  fs/notify/mark.c                 |  4 ++++
>  fs/super.c                       |  2 +-
>  include/linux/fs.h               |  5 +++++
>  include/linux/fsnotify_backend.h | 17 ++++++++++++++---
>  7 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index 86fcf5814279..25385e336ac7 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -131,6 +131,11 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
>  
>  		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
>  			   mnt->mnt_id, mflags, mark->mask, mark->ignored_mask);
> +	} else if (mark->connector->type == FSNOTIFY_OBJ_TYPE_SB) {
> +		struct super_block *sb = fsnotify_conn_sb(mark->connector);
> +
> +		seq_printf(m, "fanotify sdev:%x mflags:%x mask:%x ignored_mask:%x\n",
> +			   sb->s_dev, mflags, mark->mask, mark->ignored_mask);
>  	}
>  }
>  
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index f174397b63a0..775e731d3016 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 7902653dd577..5a00121fb219 100644
> --- a/fs/notify/fsnotify.h
> +++ b/fs/notify/fsnotify.h
> @@ -21,6 +21,12 @@ static inline struct mount *fsnotify_conn_mount(
>  	return container_of(conn->obj, struct mount, mnt_fsnotify_marks);
>  }
>  
> +static inline struct super_block *fsnotify_conn_sb(
> +				struct fsnotify_mark_connector *conn)
> +{
> +	return container_of(conn->obj, struct super_block, s_fsnotify_marks);
> +}
> +
>  /* destroy all events sitting in this groups notification queue */
>  extern void fsnotify_flush_notify(struct fsnotify_group *group);
>  
> @@ -43,6 +49,11 @@ static inline void fsnotify_clear_marks_by_mount(struct vfsmount *mnt)
>  {
>  	fsnotify_destroy_marks(&real_mount(mnt)->mnt_fsnotify_marks);
>  }
> +/* 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_marks);
> +}
>  /* Wait until all marks queued for destruction are destroyed */
>  extern void fsnotify_wait_marks_destroyed(void);
>  
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 59cdb27826de..b5172ccb2e60 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -115,6 +115,8 @@ static __u32 *fsnotify_conn_mask_p(struct fsnotify_mark_connector *conn)
>  		return &fsnotify_conn_inode(conn)->i_fsnotify_mask;
>  	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
>  		return &fsnotify_conn_mount(conn)->mnt_fsnotify_mask;
> +	else if (conn->type == FSNOTIFY_OBJ_TYPE_SB)
> +		return &fsnotify_conn_sb(conn)->s_fsnotify_mask;
>  	return NULL;
>  }
>  
> @@ -192,6 +194,8 @@ static struct inode *fsnotify_detach_connector_from_object(
>  		inode->i_fsnotify_mask = 0;
>  	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
>  		fsnotify_conn_mount(conn)->mnt_fsnotify_mask = 0;
> +	} else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) {
> +		fsnotify_conn_sb(conn)->s_fsnotify_mask = 0;
>  	}
>  
>  	rcu_assign_pointer(*(conn->obj), NULL);
> diff --git a/fs/super.c b/fs/super.c
> index f3a8c008e164..ca53a08497ed 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -442,7 +442,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 6c0b4a1c22ff..4f0a11d0d296 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1464,6 +1464,11 @@ struct super_block {
>  
>  	spinlock_t		s_inode_wblist_lock;
>  	struct list_head	s_inodes_wb;	/* writeback inodes */
> +
> +#ifdef CONFIG_FSNOTIFY
> +	__u32			s_fsnotify_mask;
> +	struct fsnotify_mark_connector __rcu	*s_fsnotify_marks;
> +#endif
>  } __randomize_layout;
>  
>  /* Helper functions so that in most cases filesystems will
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index b8f4182f42f1..81b88fc9df31 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -206,12 +206,14 @@ struct fsnotify_group {
>  enum fsnotify_obj_type {
>  	FSNOTIFY_OBJ_TYPE_INODE,
>  	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
> +	FSNOTIFY_OBJ_TYPE_SB,
>  	FSNOTIFY_OBJ_TYPE_COUNT,
>  	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_COUNT
>  };
>  
>  #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_COUNT) - 1)
>  
>  static inline bool fsnotify_valid_obj_type(unsigned int type)
> @@ -255,6 +257,7 @@ static inline struct fsnotify_mark *fsnotify_iter_##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_COUNT; type++)
> @@ -267,8 +270,8 @@ struct fsnotify_mark_connector;
>  typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
>  
>  /*
> - * 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.
>   */
> @@ -335,6 +338,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)
> @@ -455,9 +459,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);
>  
> @@ -484,6 +492,9 @@ static inline void __fsnotify_inode_delete(struct inode *inode)
>  static inline void __fsnotify_vfsmount_delete(struct vfsmount *mnt)
>  {}
>  
> +static inline void fsnotify_sb_delete(struct super_block *sb)
> +{}
> +
>  static inline void fsnotify_update_flags(struct dentry *dentry)
>  {}
>  
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark

[Jan Kara]

On Thu 30-08-18 18:15:51, Amir Goldstein wrote:
> Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
> of super block mark type.
> 
> A super block watch gets all events on the filesystem, regardless of
> the mount from which the mark was added, unless an ignore mask exists
> on either the inode or the mount where the event was generated.
> 
> 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.
> 
> Cc: <linux-api@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Just one nit below, otherwise the patch look good to me.

> 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

So for completeness I'd add FAN_MARK_INODE to FAN_ALL_MARK_FLAGS and
FAN_ALL_MARK_TYPE_FLAGS. I know it doesn't change the actual value but
logically it belongs there when you defined it...

Also one more thing to consider: Different mark types cannot be combined.
So it could save some bits in 'flags' in future if we had something like
FAN_MARK_TYPE_MASK and (flags & FAN_MARK_TYPE_MASK) would enumerate
different mark types - 0 for inode mark, FAN_MARK_MOUNT for mount mark,
FAN_MARK_FILESYSTEM for superblock mark etc. Again, currently there's no
practical difference in the values, just the names and tests for validity
would be slightly different.

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

Re: [PATCH v3 2/3] fsnotify: send path type events to group with super block marks

[Amir Goldstein]

On Fri, Aug 31, 2018 at 4:56 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 30-08-18 18:15:50, 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 backend only supports path type events.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Two small questions below. Otherwise the patch looks good to me.
>
> > ---
> >  fs/notify/fsnotify.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> ...
> >       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)))
>
> When you use mnt_or_sb_mask, the 'mnt' check is useless, right?

Right. it could be !(test_mask & (to_tell->i_fsnotify_mask | mnt_or_sb_mask))
if you think that is nicer.

>
> >       iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
> > @@ -364,16 +367,20 @@ 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))) {
> >               iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> >                       fsnotify_first_mark(&to_tell->i_fsnotify_marks);
> >               iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
> >                       fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
> > +             if ((mask & FS_MODIFY) ||
> > +                 (test_mask & sb->s_fsnotify_mask))
>
> Why is here this additional test? We might need to clear ignore masks on SB
> list if nothing else. Also we need to reflect ignore mask from the
> superblock marks... I agree there's probably no huge use for either of
> these two functionalities but I just don't see a strong reason for
> sb & mnt marks to behave differently.
>

Hmm, that is indeed not pretty.
It seems that I perpetuated the asymetric ignore relations between inode and
mnt mark that the test above implemented forever.

In this thread [1], we already agreed that include-the-exclude is the desired
semantics for ignore masks and the result was commit 92183a42898d
("fsnotify: fix ignore mask logic in send_to_group()").
However, it seems we have missed this subtle spot here and need to fix
it as well. The end result should look like this with no tests at all: (?)

        iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
                fsnotify_first_mark(&to_tell->i_fsnotify_marks);
        iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
                fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
        iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
                fsnotify_first_mark(&sb->s_fsnotify_marks);

Right?

Thanks,
Amir.

[1] https://marc.info/?l=linux-fsdevel&m=152284295703053&w=2

Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark

[Amir Goldstein]

On Fri, Aug 31, 2018 at 5:05 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 30-08-18 18:15:51, Amir Goldstein wrote:
> > Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
> > of super block mark type.
> >
> > A super block watch gets all events on the filesystem, regardless of
> > the mount from which the mark was added, unless an ignore mask exists
> > on either the inode or the mount where the event was generated.
> >
> > 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.
> >
> > Cc: <linux-api@vger.kernel.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Just one nit below, otherwise the patch look good to me.
>
> > 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
>
> So for completeness I'd add FAN_MARK_INODE to FAN_ALL_MARK_FLAGS and
> FAN_ALL_MARK_TYPE_FLAGS. I know it doesn't change the actual value but
> logically it belongs there when you defined it...
>

Ok.

> Also one more thing to consider: Different mark types cannot be combined.
> So it could save some bits in 'flags' in future if we had something like
> FAN_MARK_TYPE_MASK and (flags & FAN_MARK_TYPE_MASK) would enumerate
> different mark types - 0 for inode mark, FAN_MARK_MOUNT for mount mark,
> FAN_MARK_FILESYSTEM for superblock mark etc. Again, currently there's no
> practical difference in the values, just the names and tests for validity
> would be slightly different.
>

So do you prefer that I replace the test (mark_type &&
!is_power_of_2(mark_type))
with a switch cases statement for supported types?
Makes sense.

Shall I go as far as:
#define FAN_MARK_TYPE_BIT1      0x00000010
#define FAN_MARK_TYPE_BIT2      0x00000100
#define FAN_MARK_TYPE_MASK (FAN_MARK_TYPE_BIT1 | FAN_MARK_TYPE_BIT2)

/* mark type can be a combination of mark type bits */
#define FAN_MARK_INODE          0
#define FAN_MARK_MOUNT          FAN_MARK_TYPE_BIT1
#define FAN_MARK_FILESYSTEM     FAN_MARK_TYPE_BIT2


Thanks,
Amir.

Re: [PATCH v3 2/3] fsnotify: send path type events to group with super block marks

[Jan Kara]

On Fri 31-08-18 18:07:27, Amir Goldstein wrote:
> On Fri, Aug 31, 2018 at 4:56 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 30-08-18 18:15:50, 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 backend only supports path type events.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Two small questions below. Otherwise the patch looks good to me.
> >
> > > ---
> > >  fs/notify/fsnotify.c | 27 +++++++++++++++++----------
> > >  1 file changed, 17 insertions(+), 10 deletions(-)
> > >
> > ...
> > >       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)))
> >
> > When you use mnt_or_sb_mask, the 'mnt' check is useless, right?
> 
> Right. it could be !(test_mask & (to_tell->i_fsnotify_mask | mnt_or_sb_mask))
> if you think that is nicer.
> 
> >
> > >       iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
> > > @@ -364,16 +367,20 @@ 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))) {
> > >               iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> > >                       fsnotify_first_mark(&to_tell->i_fsnotify_marks);
> > >               iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
> > >                       fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
> > > +             if ((mask & FS_MODIFY) ||
> > > +                 (test_mask & sb->s_fsnotify_mask))
> >
> > Why is here this additional test? We might need to clear ignore masks on SB
> > list if nothing else. Also we need to reflect ignore mask from the
> > superblock marks... I agree there's probably no huge use for either of
> > these two functionalities but I just don't see a strong reason for
> > sb & mnt marks to behave differently.
> >
> 
> Hmm, that is indeed not pretty.
> It seems that I perpetuated the asymetric ignore relations between inode and
> mnt mark that the test above implemented forever.
> 
> In this thread [1], we already agreed that include-the-exclude is the desired
> semantics for ignore masks and the result was commit 92183a42898d
> ("fsnotify: fix ignore mask logic in send_to_group()").

Right.

> However, it seems we have missed this subtle spot here and need to fix
> it as well. The end result should look like this with no tests at all: (?)
> 
>         iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
>                 fsnotify_first_mark(&to_tell->i_fsnotify_marks);
>         iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
>                 fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
>         iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
>                 fsnotify_first_mark(&sb->s_fsnotify_marks);
> 
> Right?

Let me think loud and please correct me if I'm wrong somewhere. We need all
three lists if:

1) It is a FS_MODIFY event as that may need to clear ignore masks on some
list.

or

2) Mask for any list type (inode, mnt, sb) matches the mask of event - so
that we can collect also ignore masks and thus find out whether event
really should be reported.

This is already checked by a condition early in fsnotify(). So I agree that
the iter_info initialization should look like:

        iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
		fsnotify_first_mark(&to_tell->i_fsnotify_marks);
	if (mnt) {
		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
		iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
			fsnotify_first_mark(&sb->s_fsnotify_marks);
	}


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

Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark

[Jan Kara]

On Fri 31-08-18 18:30:32, Amir Goldstein wrote:
> On Fri, Aug 31, 2018 at 5:05 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 30-08-18 18:15:51, Amir Goldstein wrote:
> > > Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
> > > of super block mark type.
> > >
> > > A super block watch gets all events on the filesystem, regardless of
> > > the mount from which the mark was added, unless an ignore mask exists
> > > on either the inode or the mount where the event was generated.
> > >
> > > 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.
> > >
> > > Cc: <linux-api@vger.kernel.org>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Just one nit below, otherwise the patch look good to me.
> >
> > > 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
> >
> > So for completeness I'd add FAN_MARK_INODE to FAN_ALL_MARK_FLAGS and
> > FAN_ALL_MARK_TYPE_FLAGS. I know it doesn't change the actual value but
> > logically it belongs there when you defined it...
> >
> 
> Ok.
> 
> > Also one more thing to consider: Different mark types cannot be combined.
> > So it could save some bits in 'flags' in future if we had something like
> > FAN_MARK_TYPE_MASK and (flags & FAN_MARK_TYPE_MASK) would enumerate
> > different mark types - 0 for inode mark, FAN_MARK_MOUNT for mount mark,
> > FAN_MARK_FILESYSTEM for superblock mark etc. Again, currently there's no
> > practical difference in the values, just the names and tests for validity
> > would be slightly different.
> >
> 
> So do you prefer that I replace the test (mark_type &&
> !is_power_of_2(mark_type))
> with a switch cases statement for supported types?
> Makes sense.

Yes.

> Shall I go as far as:
> #define FAN_MARK_TYPE_BIT1      0x00000010
> #define FAN_MARK_TYPE_BIT2      0x00000100
> #define FAN_MARK_TYPE_MASK (FAN_MARK_TYPE_BIT1 | FAN_MARK_TYPE_BIT2)
> 
> /* mark type can be a combination of mark type bits */
> #define FAN_MARK_INODE          0
> #define FAN_MARK_MOUNT          FAN_MARK_TYPE_BIT1
> #define FAN_MARK_FILESYSTEM     FAN_MARK_TYPE_BIT2

Probably I would not go as far as defining FAN_MARK_TYPE_BIT?. That looks a
bit confusing and it's in userspace-visible headers. I'd just define the
mask and add it into FAN_ALL_MARK_FLAGS instead of FAN_MARK_MOUNT. That
should protect us (together with flags & supported-type checks in
do_fanotify_mark()) against messing up the definitions (at least I hope ;).

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

Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark

[Amir Goldstein]

On Mon, Sep 3, 2018 at 11:49 AM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 31-08-18 18:30:32, Amir Goldstein wrote:
> > On Fri, Aug 31, 2018 at 5:05 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Thu 30-08-18 18:15:51, Amir Goldstein wrote:
> > > > Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush
> > > > of super block mark type.
> > > >
> > > > A super block watch gets all events on the filesystem, regardless of
> > > > the mount from which the mark was added, unless an ignore mask exists
> > > > on either the inode or the mount where the event was generated.
> > > >
> > > > 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.
> > > >
> > > > Cc: <linux-api@vger.kernel.org>
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
[...]
> > Shall I go as far as:
> > #define FAN_MARK_TYPE_BIT1      0x00000010
> > #define FAN_MARK_TYPE_BIT2      0x00000100
> > #define FAN_MARK_TYPE_MASK (FAN_MARK_TYPE_BIT1 | FAN_MARK_TYPE_BIT2)
> >
> > /* mark type can be a combination of mark type bits */
> > #define FAN_MARK_INODE          0
> > #define FAN_MARK_MOUNT          FAN_MARK_TYPE_BIT1
> > #define FAN_MARK_FILESYSTEM     FAN_MARK_TYPE_BIT2
>
> Probably I would not go as far as defining FAN_MARK_TYPE_BIT?. That looks a
> bit confusing and it's in userspace-visible headers. I'd just define the
> mask and add it into FAN_ALL_MARK_FLAGS instead of FAN_MARK_MOUNT. That
> should protect us (together with flags & supported-type checks in
> do_fanotify_mark()) against messing up the definitions (at least I hope ;).
>

That's what I figured.. already posted v4 with a comment similar to
that in the similar case of FAN_ALL_CLASS_BITS.

Thanks,
Amir.