[PATCH v4 0/5] Fsnotify cleanup - part 2

[PATCH v4 0/5] Fsnotify cleanup - part 2

[Amir Goldstein]

Jan,

This is the part of the series that was not merged to v4.18-rc1, after
addressing Linus' concerns.  We already had several review cycles on
the branch in my tree since v3, but this is 4th official posting.

Note that per your request, I changed fsnotify_obj_{inode,mount}
to fsnotify_conn_{inode,mount}, but that was easier to do after
the patch that introduces obj->conn, so the helpers
fsnotify_obj_{inode,mount} are added and then modified.

Thanks,
Amir.

Changes since v3:
- Some cleanup patches already merged to v4.18-rc1
- Replace struct fsnotify_obj with fsnotify_connp_t type
- Leave explicit connector pointer in struct inode (Linus)
- Use helper to get mask from any connector type

Amir Goldstein (5):
  fsnotify: use typedef fsnotify_connp_t for brevity
  fsnotify: pass connp and object type to fsnotify_add_mark()
  fsnotify: let connector point to an abstract object
  fsnotify: add helper to get mask from connector
  fanotify: factor out helpers to add/remove mark

 fs/notify/fanotify/fanotify_user.c |  99 +++++++++++++----------------------
 fs/notify/fdinfo.c                 |   8 +--
 fs/notify/fsnotify.h               |  16 +++++-
 fs/notify/mark.c                   | 103 ++++++++++++++++++++-----------------
 include/linux/fsnotify_backend.h   |  40 +++++++++-----
 kernel/audit_tree.c                |  17 +++---
 6 files changed, 147 insertions(+), 136 deletions(-)

-- 
2.7.4

[PATCH v4 1/5] fsnotify: use typedef fsnotify_connp_t for brevity

[Amir Goldstein]

The object marks manipulation functions fsnotify_destroy_marks()
fsnotify_find_mark() and their helpers take an argument of type
struct fsnotify_mark_connector __rcu ** to dereference the connector
pointer. use a typedef to describe this type for brevity.

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

diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 34515d2c4ba3..94cedf8264ba 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -19,8 +19,8 @@ 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 attached to an object via connector */
+extern void fsnotify_destroy_marks(fsnotify_connp_t *connp);
 /* run the list of all marks associated with inode and destroy them */
 static inline void fsnotify_clear_marks_by_inode(struct inode *inode)
 {
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 61f4c5fa34c7..7b595acd8ec9 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(fsnotify_connp_t *connp,
+					       struct inode *inode,
+					       struct vfsmount *mnt)
 {
 	struct fsnotify_mark_connector *conn;
 
@@ -476,7 +475,7 @@ 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)
+						fsnotify_connp_t *connp)
 {
 	struct fsnotify_mark_connector *conn;
 	int idx;
@@ -508,7 +507,7 @@ 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;
+	fsnotify_connp_t *connp;
 	int cmp;
 	int err = 0;
 
@@ -629,9 +628,8 @@ 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.
  */
-struct fsnotify_mark *fsnotify_find_mark(
-				struct fsnotify_mark_connector __rcu **connp,
-				struct fsnotify_group *group)
+struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp,
+					 struct fsnotify_group *group)
 {
 	struct fsnotify_mark_connector *conn;
 	struct fsnotify_mark *mark;
@@ -697,8 +695,8 @@ 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 via connector */
+void fsnotify_destroy_marks(fsnotify_connp_t *connp)
 {
 	struct fsnotify_mark_connector *conn;
 	struct fsnotify_mark *mark, *old_mark = NULL;
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index b38964a7a521..e7cd7fc6e3cf 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -268,6 +268,8 @@ struct fsnotify_mark_connector {
 	struct hlist_head list;
 };
 
+typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
+
 /*
  * A mark is simply an object attached to an in core inode which allows an
  * fsnotify listener to indicate they are either no longer interested in events
@@ -394,9 +396,8 @@ 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);
+extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp,
+						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);
-- 
2.7.4

[PATCH v4 2/5] fsnotify: pass connp 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 object pointer and the object
type.

The helpers fsnotify_obj_{inode,mount} are added to get the concrete
object pointer from abstract object pointer.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 19 +++++++++--------
 fs/notify/fsnotify.h               | 10 +++++++++
 fs/notify/mark.c                   | 42 +++++++++++++++-----------------------
 include/linux/fsnotify_backend.h   | 20 ++++++++++++------
 4 files changed, 52 insertions(+), 39 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index ec4d8c59d0e3..81212b251189 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -615,8 +615,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)
+						   fsnotify_connp_t *connp,
+						   unsigned int type)
 {
 	struct fsnotify_mark *mark;
 	int ret;
@@ -629,7 +629,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, connp, type, 0);
 	if (ret) {
 		fsnotify_put_mark(mark);
 		return ERR_PTR(ret);
@@ -643,14 +643,15 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 				      struct vfsmount *mnt, __u32 mask,
 				      unsigned int flags)
 {
+	fsnotify_connp_t *connp = &real_mount(mnt)->mnt_fsnotify_marks;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify_marks,
-				      group);
+	fsn_mark = fsnotify_find_mark(connp, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, NULL, mnt);
+		fsn_mark = fanotify_add_new_mark(group, connp,
+						 FSNOTIFY_OBJ_TYPE_VFSMOUNT);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
@@ -669,6 +670,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
 {
+	fsnotify_connp_t *connp = &inode->i_fsnotify_marks;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
@@ -685,9 +687,10 @@ 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(connp, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, inode, NULL);
+		fsn_mark = fanotify_add_new_mark(group, connp,
+						 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 94cedf8264ba..caeee042d1cc 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -9,6 +9,16 @@
 
 #include "../mount.h"
 
+static inline struct inode *fsnotify_obj_inode(fsnotify_connp_t *connp)
+{
+	return container_of(connp, struct inode, i_fsnotify_marks);
+}
+
+static inline struct mount *fsnotify_obj_mount(fsnotify_connp_t *connp)
+{
+	return container_of(connp, struct mount, mnt_fsnotify_marks);
+}
+
 /* destroy all events sitting in this groups notification queue */
 extern void fsnotify_flush_notify(struct fsnotify_group *group);
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 7b595acd8ec9..7abb73b5beba 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(fsnotify_connp_t *connp,
-					       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(fsnotify_connp_t *connp,
 		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_obj_inode(connp));
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+		conn->mnt = &fsnotify_obj_mount(connp)->mnt;
 	/*
 	 * cmpxchg() provides the barrier so that readers of *connp 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,
+				  fsnotify_connp_t *connp, unsigned int type,
 				  int allow_dups)
 {
 	struct fsnotify_mark *lmark, *last = NULL;
 	struct fsnotify_mark_connector *conn;
-	fsnotify_connp_t *connp;
 	int cmp;
 	int err = 0;
 
-	if (WARN_ON(!inode && !mnt))
+	if (WARN_ON(!fsnotify_valid_obj_type(type)))
 		return -EINVAL;
-	if (inode)
-		connp = &inode->i_fsnotify_marks;
-	else
-		connp = &real_mount(mnt)->mnt_fsnotify_marks;
 restart:
 	spin_lock(&mark->lock);
 	conn = fsnotify_grab_connector(connp);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(connp, inode, mnt);
+		err = fsnotify_attach_connector_to_object(connp, 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,
+			     fsnotify_connp_t *connp, 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, connp, 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, fsnotify_connp_t *connp,
+		      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, connp, 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 e7cd7fc6e3cf..8efb8663453d 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -210,6 +210,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_COUNT) - 1)
 
+static inline bool fsnotify_valid_obj_type(unsigned int type)
+{
+	return (type < FSNOTIFY_OBJ_TYPE_COUNT);
+}
+
 struct fsnotify_iter_info {
 	struct fsnotify_mark *marks[FSNOTIFY_OBJ_TYPE_COUNT];
 	unsigned int report_mask;
@@ -398,24 +403,27 @@ extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 /* Find mark belonging to given group in the list of marks */
 extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp,
 						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);
+/* attach the mark to the object */
+extern int fsnotify_add_mark(struct fsnotify_mark *mark,
+			     fsnotify_connp_t *connp, unsigned int type,
+			     int allow_dups);
 extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-				    struct inode *inode, struct vfsmount *mnt,
+				    fsnotify_connp_t *connp, 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_marks,
+				 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_marks,
+					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

[PATCH v4 3/5] fsnotify: let connector point to an abstract object

[Amir Goldstein]

Make the code to attach/detach a connector to object more generic
by letting the fsnotify connector point to an abstract fsnotify_connp_t.
Code that needs to dereference an inode or mount object now uses the
helpers fsnotify_conn_{inode,mount}.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fdinfo.c               |  8 ++++----
 fs/notify/fsnotify.h             | 10 ++++++----
 fs/notify/mark.c                 | 32 ++++++++++++++++----------------
 include/linux/fsnotify_backend.h | 15 ++++++++++-----
 kernel/audit_tree.c              | 17 +++++++++--------
 5 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 10aac1942c9f..86fcf5814279 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -15,7 +15,7 @@
 #include <linux/exportfs.h>
 
 #include "inotify/inotify.h"
-#include "../fs/mount.h"
+#include "fsnotify.h"
 
 #if defined(CONFIG_PROC_FS)
 
@@ -81,7 +81,7 @@ static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		return;
 
 	inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark);
-	inode = igrab(mark->connector->inode);
+	inode = igrab(fsnotify_conn_inode(mark->connector));
 	if (inode) {
 		/*
 		 * IN_ALL_EVENTS represents all of the mask bits
@@ -117,7 +117,7 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
 
 	if (mark->connector->type == FSNOTIFY_OBJ_TYPE_INODE) {
-		inode = igrab(mark->connector->inode);
+		inode = igrab(fsnotify_conn_inode(mark->connector));
 		if (!inode)
 			return;
 		seq_printf(m, "fanotify ino:%lx sdev:%x mflags:%x mask:%x ignored_mask:%x ",
@@ -127,7 +127,7 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		seq_putc(m, '\n');
 		iput(inode);
 	} else if (mark->connector->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
-		struct mount *mnt = real_mount(mark->connector->mnt);
+		struct mount *mnt = fsnotify_conn_mount(mark->connector);
 
 		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
 			   mnt->mnt_id, mflags, mark->mask, mark->ignored_mask);
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index caeee042d1cc..7902653dd577 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -9,14 +9,16 @@
 
 #include "../mount.h"
 
-static inline struct inode *fsnotify_obj_inode(fsnotify_connp_t *connp)
+static inline struct inode *fsnotify_conn_inode(
+				struct fsnotify_mark_connector *conn)
 {
-	return container_of(connp, struct inode, i_fsnotify_marks);
+	return container_of(conn->obj, struct inode, i_fsnotify_marks);
 }
 
-static inline struct mount *fsnotify_obj_mount(fsnotify_connp_t *connp)
+static inline struct mount *fsnotify_conn_mount(
+				struct fsnotify_mark_connector *conn)
 {
-	return container_of(connp, struct mount, mnt_fsnotify_marks);
+	return container_of(conn->obj, struct mount, mnt_fsnotify_marks);
 }
 
 /* destroy all events sitting in this groups notification queue */
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 7abb73b5beba..959bc73aaae7 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -120,14 +120,14 @@ 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;
+		fsnotify_conn_inode(conn)->i_fsnotify_mask = new_mask;
 	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-		real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask;
+		fsnotify_conn_mount(conn)->mnt_fsnotify_mask = new_mask;
 }
 
 /*
  * Calculate mask of events for a list of marks. The caller must make sure
- * connector and connector->inode cannot disappear under us.  Callers achieve
+ * connector and connector->obj cannot disappear under us.  Callers achieve
  * this by holding a mark->lock or mark->group->mark_mutex for a mark on this
  * list.
  */
@@ -140,7 +140,8 @@ void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	__fsnotify_recalc_mask(conn);
 	spin_unlock(&conn->lock);
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		__fsnotify_update_child_dentry_flags(conn->inode);
+		__fsnotify_update_child_dentry_flags(
+					fsnotify_conn_inode(conn));
 }
 
 /* Free all connectors queued for freeing once SRCU period ends */
@@ -166,20 +167,20 @@ static struct inode *fsnotify_detach_connector_from_object(
 {
 	struct inode *inode = NULL;
 
+	if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED)
+		return NULL;
+
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
-		inode = conn->inode;
-		rcu_assign_pointer(inode->i_fsnotify_marks, NULL);
+		inode = fsnotify_conn_inode(conn);
 		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;
-		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
+		fsnotify_conn_mount(conn)->mnt_fsnotify_mask = 0;
 	}
 
+	rcu_assign_pointer(*(conn->obj), NULL);
+	conn->obj = NULL;
+	conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
+
 	return inode;
 }
 
@@ -448,10 +449,9 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	spin_lock_init(&conn->lock);
 	INIT_HLIST_HEAD(&conn->list);
 	conn->type = type;
+	conn->obj = connp;
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		inode = conn->inode = igrab(fsnotify_obj_inode(connp));
-	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-		conn->mnt = &fsnotify_obj_mount(connp)->mnt;
+		inode = igrab(fsnotify_conn_inode(conn));
 	/*
 	 * cmpxchg() provides the barrier so that readers of *connp can see
 	 * only initialized structure
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 8efb8663453d..381cfb0e67fa 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -256,6 +256,13 @@ FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 	for (type = 0; type < FSNOTIFY_OBJ_TYPE_COUNT; type++)
 
 /*
+ * fsnotify_connp_t is what we embed in objects which connector can be attached
+ * to. fsnotify_connp_t * is how we refer from connector back to object.
+ */
+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
  * structure. We destroy this structure when there are no more marks attached
@@ -264,17 +271,15 @@ FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 struct fsnotify_mark_connector {
 	spinlock_t lock;
 	unsigned int type;	/* Type of object [lock] */
-	union {	/* Object pointer [lock] */
-		struct inode *inode;
-		struct vfsmount *mnt;
+	union {
+		/* Object pointer [lock] */
+		fsnotify_connp_t *obj;
 		/* Used listing heads to free after srcu period expires */
 		struct fsnotify_mark_connector *destroy_next;
 	};
 	struct hlist_head list;
 };
 
-typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
-
 /*
  * A mark is simply an object attached to an in core inode which allows an
  * fsnotify listener to indicate they are either no longer interested in events
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index c99ebaae5abc..02feef939560 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -168,7 +168,8 @@ static __cacheline_aligned_in_smp DEFINE_SPINLOCK(hash_lock);
 /* Function to return search key in our hash from inode. */
 static unsigned long inode_to_key(const struct inode *inode)
 {
-	return (unsigned long)inode;
+	/* Use address pointed to by connector->obj as the key */
+	return (unsigned long)&inode->i_fsnotify_marks;
 }
 
 /*
@@ -183,7 +184,7 @@ static unsigned long chunk_to_key(struct audit_chunk *chunk)
 	 */
 	if (WARN_ON_ONCE(!chunk->mark.connector))
 		return 0;
-	return (unsigned long)chunk->mark.connector->inode;
+	return (unsigned long)chunk->mark.connector->obj;
 }
 
 static inline struct list_head *chunk_hash(unsigned long key)
@@ -258,7 +259,7 @@ static void untag_chunk(struct node *p)
 	spin_lock(&entry->lock);
 	/*
 	 * mark_mutex protects mark from getting detached and thus also from
-	 * mark->connector->inode getting NULL.
+	 * mark->connector->obj getting NULL.
 	 */
 	if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
 		spin_unlock(&entry->lock);
@@ -288,8 +289,8 @@ static void untag_chunk(struct node *p)
 	if (!new)
 		goto Fallback;
 
-	if (fsnotify_add_inode_mark_locked(&new->mark, entry->connector->inode,
-					   1)) {
+	if (fsnotify_add_mark_locked(&new->mark, entry->connector->obj,
+				     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
 		fsnotify_put_mark(&new->mark);
 		goto Fallback;
 	}
@@ -423,7 +424,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 	spin_lock(&old_entry->lock);
 	/*
 	 * mark_mutex protects mark from getting detached and thus also from
-	 * mark->connector->inode getting NULL.
+	 * mark->connector->obj getting NULL.
 	 */
 	if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
 		/* old_entry is being shot, lets just lie */
@@ -434,8 +435,8 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
 		return -ENOENT;
 	}
 
-	if (fsnotify_add_inode_mark_locked(chunk_entry,
-			     old_entry->connector->inode, 1)) {
+	if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj,
+				     FSNOTIFY_OBJ_TYPE_INODE, 1)) {
 		spin_unlock(&old_entry->lock);
 		mutex_unlock(&old_entry->group->mark_mutex);
 		fsnotify_put_mark(chunk_entry);
-- 
2.7.4

[PATCH v4 4/5] fsnotify: add helper to get mask from connector

[Amir Goldstein]

Use a helper to get the mask from the object (i.e. i_fsnotify_mask)
to generalize code of add/remove inode/vfsmount mark.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 16 ++++++++--------
 fs/notify/mark.c                   | 25 +++++++++++++++++++++----
 include/linux/fsnotify_backend.h   |  2 ++
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 81212b251189..3899ad177651 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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 & fsnotify_conn_mask(fsn_mark->connector))
+		fsnotify_recalc_mask(fsn_mark->connector);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
@@ -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 & fsnotify_conn_mask(fsn_mark->connector))
+		fsnotify_recalc_mask(fsn_mark->connector);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	mutex_unlock(&group->mark_mutex);
@@ -658,8 +658,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 & ~fsnotify_conn_mask(fsn_mark->connector))
+		fsnotify_recalc_mask(fsn_mark->connector);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
@@ -697,8 +697,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 & ~fsnotify_conn_mask(fsn_mark->connector))
+		fsnotify_recalc_mask(fsn_mark->connector);
 	mutex_unlock(&group->mark_mutex);
 
 	fsnotify_put_mark(fsn_mark);
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 959bc73aaae7..05506d60131c 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -109,6 +109,23 @@ void fsnotify_get_mark(struct fsnotify_mark *mark)
 	refcount_inc(&mark->refcnt);
 }
 
+static __u32 *fsnotify_conn_mask_p(struct fsnotify_mark_connector *conn)
+{
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
+		return &fsnotify_conn_inode(conn)->i_fsnotify_mask;
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+		return &fsnotify_conn_mount(conn)->mnt_fsnotify_mask;
+	return NULL;
+}
+
+__u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn)
+{
+	if (WARN_ON(!fsnotify_valid_obj_type(conn->type)))
+		return 0;
+
+	return *fsnotify_conn_mask_p(conn);
+}
+
 static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 {
 	u32 new_mask = 0;
@@ -119,10 +136,10 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
 			new_mask |= mark->mask;
 	}
-	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		fsnotify_conn_inode(conn)->i_fsnotify_mask = new_mask;
-	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-		fsnotify_conn_mount(conn)->mnt_fsnotify_mask = new_mask;
+	if (WARN_ON(!fsnotify_valid_obj_type(conn->type)))
+		return;
+
+	*fsnotify_conn_mask_p(conn) = new_mask;
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 381cfb0e67fa..2b9b6f1ff5e0 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -401,6 +401,8 @@ extern struct fsnotify_event *fsnotify_remove_first_event(struct fsnotify_group
 
 /* functions used to manipulate the marks attached to inodes */
 
+/* Get mask of events for a list of marks */
+extern __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn);
 /* Calculate mask of events for a list of marks */
 extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn);
 extern void fsnotify_init_mark(struct fsnotify_mark *mark,
-- 
2.7.4

[PATCH v4 5/5] fanotify: factor out helpers to add/remove mark

[Amir Goldstein]

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

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 3899ad177651..d736a833fe39 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -524,17 +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,
+				fsnotify_connp_t *connp, __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_marks,
-				      group);
+	fsn_mark = fsnotify_find_mark(connp, group);
 	if (!fsn_mark) {
 		mutex_unlock(&group->mark_mutex);
 		return -ENOENT;
@@ -550,39 +549,25 @@ static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 	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_marks,
+				    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_marks, 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 & fsnotify_conn_mask(fsn_mark->connector))
-		fsnotify_recalc_mask(fsn_mark->connector);
-	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_marks, mask,
+				    flags);
 }
 
 static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
@@ -639,19 +624,17 @@ 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,
+			     fsnotify_connp_t *connp, unsigned int type,
+			     __u32 mask, unsigned int flags)
 {
-	fsnotify_connp_t *connp = &real_mount(mnt)->mnt_fsnotify_marks;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
 	fsn_mark = fsnotify_find_mark(connp, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, connp,
-						 FSNOTIFY_OBJ_TYPE_VFSMOUNT);
+		fsn_mark = fanotify_add_new_mark(group, connp, type);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
@@ -666,14 +649,18 @@ static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 	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_marks,
+				 FSNOTIFY_OBJ_TYPE_VFSMOUNT, mask, flags);
+}
+
 static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
 {
-	fsnotify_connp_t *connp = &inode->i_fsnotify_marks;
-	struct fsnotify_mark *fsn_mark;
-	__u32 added;
-
 	pr_debug("%s: group=%p inode=%p\n", __func__, group, inode);
 
 	/*
@@ -686,23 +673,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(connp, group);
-	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, connp,
-						 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 & ~fsnotify_conn_mask(fsn_mark->connector))
-		fsnotify_recalc_mask(fsn_mark->connector);
-	mutex_unlock(&group->mark_mutex);
-
-	fsnotify_put_mark(fsn_mark);
-	return 0;
+	return fanotify_add_mark(group, &inode->i_fsnotify_marks,
+				 FSNOTIFY_OBJ_TYPE_INODE, mask, flags);
 }
 
 /* fanotify syscalls */
-- 
2.7.4

Re: [PATCH v4 1/5] fsnotify: use typedef fsnotify_connp_t for brevity

[Al Viro]

On Sat, Jun 23, 2018 at 05:54:47PM +0300, Amir Goldstein wrote:
> The object marks manipulation functions fsnotify_destroy_marks()
> fsnotify_find_mark() and their helpers take an argument of type
> struct fsnotify_mark_connector __rcu ** to dereference the connector
> pointer. use a typedef to describe this type for brevity.

That kind of typedefs is generally considered a bad style kernel-side...

Re: [PATCH v4 1/5] fsnotify: use typedef fsnotify_connp_t for brevity

[Amir Goldstein]

On Sun, Jun 24, 2018 at 12:13 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sat, Jun 23, 2018 at 05:54:47PM +0300, Amir Goldstein wrote:
>> The object marks manipulation functions fsnotify_destroy_marks()
>> fsnotify_find_mark() and their helpers take an argument of type
>> struct fsnotify_mark_connector __rcu ** to dereference the connector
>> pointer. use a typedef to describe this type for brevity.
>
> That kind of typedefs is generally considered a bad style kernel-side...

Yeh... look. Before we decide fold and open code struct
fsnotify_mark_connector __rcu ** everywhere making fsnotify code uglier
and harder to understand (IMO), I'll just say that I still hold the opinion
that struct fsnotify_obj *is* the correct coding pattern here:
https://marc.info/?l=linux-fsdevel&m=152426580218852&w=2

The rejections we got from Linus on v3 were just -
1. We were not able to guaranty that the change doesn't grow
    struct inode doesn't grow on some architectures/compilers
2. Linus dislikes seeing embedded structs in struct inode, that
    may silently grow in the future, below his radar
3. Linus generally preferred if we could get along with an
    abstract pointer in struct inode not having to include another
    header file in fs.h

For the sake of considering all options, I would like to propose that
we bring back fsnotify_obj, but let it contain only a connector pointer,
leaving i_fsnotify_mask independent.

Doing so will have addressed the main concern about struct inode size.
Concern #2 can be addressed by documenting the size of fsnotify_obj
in a comment in struct inode and adding BUILD_BUG_ON to preserve
that guaranty. Concern #3 will need to remain and be a compromise
for the sake of better (IMO) fsnotify code.

Thought?

Amir.

Re: [PATCH v4 1/5] fsnotify: use typedef fsnotify_connp_t for brevity

[Jan Kara]

On Sat 23-06-18 22:13:41, Al Viro wrote:
> On Sat, Jun 23, 2018 at 05:54:47PM +0300, Amir Goldstein wrote:
> > The object marks manipulation functions fsnotify_destroy_marks()
> > fsnotify_find_mark() and their helpers take an argument of type
> > struct fsnotify_mark_connector __rcu ** to dereference the connector
> > pointer. use a typedef to describe this type for brevity.
> 
> That kind of typedefs is generally considered a bad style kernel-side...

I know that such typedefs are discouraged as they often reduce readability
(you have to look up the typedef to know what the type is about). However
in this case the type name is long enough and we also often pass 'pointer
to pointer' so that I think the readability of the result is actually
better. And Amir has chosen reasonably clear name fsnotify_connp_t to show
this is about a pointer to fsnotify_connector so that you should not need
to look up the typedef more than once ;)
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 0/5] Fsnotify cleanup - part 2

[Jan Kara]

On Sat 23-06-18 17:54:46, Amir Goldstein wrote:
> Jan,
> 
> This is the part of the series that was not merged to v4.18-rc1, after
> addressing Linus' concerns.  We already had several review cycles on
> the branch in my tree since v3, but this is 4th official posting.
> 
> Note that per your request, I changed fsnotify_obj_{inode,mount}
> to fsnotify_conn_{inode,mount}, but that was easier to do after
> the patch that introduces obj->conn, so the helpers
> fsnotify_obj_{inode,mount} are added and then modified.

Thanks Amir! I have pulled these patches to my tree.

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