[PATCH v2 0/7] fix fanotify issues with the series in v4.12

[PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Miklos Szeredi]

We discovered some problems in the latest fsnotify/fanotify codebase with
the help of a stress test (Xiong Zhou is working on upstreaming it to
fstests).

This series attempts to fix these.  With the patch applied the stress test
passes.

Please review/test.

Changes in v2 (only cosmetic fixes, no functional change):
 - split first patch into 3 parts to make it more readable
 - checkpatch fixes
 - added cleanup patch for fanotify

Thanks,
Miklos
---

Miklos Szeredi (7):
  fsnotify: clean up fsnotify_prepare/finish_user_wait()
  fsnotify: pin both inode and vfsmount mark
  fsnotify: fix pinning group in fsnotify_prepare_user_wait()
  fsnotify: skip unattached marks
  fanotify: fix fsnotify_prepare_user_wait() failure
  fsnotify: clean up fsnotify()
  fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs

 fs/notify/fanotify/fanotify.c      |  49 ++++++---------
 fs/notify/fanotify/fanotify.h      |   8 ++-
 fs/notify/fanotify/fanotify_user.c | 123 ++++++++++++++++++-------------------
 fs/notify/fsnotify.c               | 108 ++++++++++++++++----------------
 fs/notify/mark.c                   | 100 ++++++++++++++----------------
 include/linux/fsnotify_backend.h   |   2 -
 6 files changed, 184 insertions(+), 206 deletions(-)

-- 
2.5.5

[PATCH v2 1/7] fsnotify: clean up fsnotify_prepare/finish_user_wait()

[Miklos Szeredi]

This patch doesn't actually fix any bug, just paves the way for fixing mark
and group pinning.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: <stable@vger.kernel.org> # v4.12
---
 fs/notify/mark.c | 89 ++++++++++++++++++++++++++------------------------------
 1 file changed, 41 insertions(+), 48 deletions(-)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 9991f8826734..982527ce6a58 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -109,16 +109,6 @@ void fsnotify_get_mark(struct fsnotify_mark *mark)
 	atomic_inc(&mark->refcnt);
 }
 
-/*
- * Get mark reference when we found the mark via lockless traversal of object
- * list. Mark can be already removed from the list by now and on its way to be
- * destroyed once SRCU period ends.
- */
-static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
-{
-	return atomic_inc_not_zero(&mark->refcnt);
-}
-
 static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 {
 	u32 new_mask = 0;
@@ -256,32 +246,56 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 			   FSNOTIFY_REAPER_DELAY);
 }
 
-bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
+/*
+ * Get mark reference when we found the mark via lockless traversal of object
+ * list. Mark can be already removed from the list by now and on its way to be
+ * destroyed once SRCU period ends.
+ */
+static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
 {
 	struct fsnotify_group *group;
 
-	if (WARN_ON_ONCE(!iter_info->inode_mark && !iter_info->vfsmount_mark))
-		return false;
-
-	if (iter_info->inode_mark)
-		group = iter_info->inode_mark->group;
-	else
-		group = iter_info->vfsmount_mark->group;
+	if (!mark)
+		return true;
 
+	group = mark->group;
 	/*
 	 * Since acquisition of mark reference is an atomic op as well, we can
 	 * be sure this inc is seen before any effect of refcount increment.
 	 */
 	atomic_inc(&group->user_waits);
+	if (atomic_inc_not_zero(&mark->refcnt))
+		return true;
+
+	if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
+		wake_up(&group->notification_waitq);
 
-	if (iter_info->inode_mark) {
-		/* This can fail if mark is being removed */
-		if (!fsnotify_get_mark_safe(iter_info->inode_mark))
-			goto out_wait;
+	return false;
+}
+
+static void fsnotify_put_mark_wait(struct fsnotify_mark *mark)
+{
+	if (mark) {
+		struct fsnotify_group *group = mark->group;
+
+		fsnotify_put_mark(mark);
+		/*
+		 * We abuse notification_waitq on group shutdown for waiting for
+		 * all marks pinned when waiting for userspace.
+		 */
+		if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
+			wake_up(&group->notification_waitq);
 	}
-	if (iter_info->vfsmount_mark) {
-		if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark))
-			goto out_inode;
+}
+
+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_wait(iter_info->inode_mark);
+		return false;
 	}
 
 	/*
@@ -292,34 +306,13 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
 	srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
 
 	return true;
-out_inode:
-	if (iter_info->inode_mark)
-		fsnotify_put_mark(iter_info->inode_mark);
-out_wait:
-	if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
-		wake_up(&group->notification_waitq);
-	return false;
 }
 
 void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
 {
-	struct fsnotify_group *group = NULL;
-
 	iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
-	if (iter_info->inode_mark) {
-		group = iter_info->inode_mark->group;
-		fsnotify_put_mark(iter_info->inode_mark);
-	}
-	if (iter_info->vfsmount_mark) {
-		group = iter_info->vfsmount_mark->group;
-		fsnotify_put_mark(iter_info->vfsmount_mark);
-	}
-	/*
-	 * We abuse notification_waitq on group shutdown for waiting for all
-	 * marks pinned when waiting for userspace.
-	 */
-	if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
-		wake_up(&group->notification_waitq);
+	fsnotify_put_mark_wait(iter_info->inode_mark);
+	fsnotify_put_mark_wait(iter_info->vfsmount_mark);
 }
 
 /*
-- 
2.5.5

[PATCH v2 2/7] fsnotify: pin both inode and vfsmount mark

[Miklos Szeredi]

We may fail to pin one of the marks in fsnotify_prepare_user_wait() when
dropping the srcu read lock, resulting in use after free at the next
iteration.

Solution is to store both marks in iter_info instead of just the one we'll
be sending the event for.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: 9385a84d7e1f ("fsnotify: Pass fsnotify_iter_info into handle_event handler")
Cc: <stable@vger.kernel.org> # v4.12
---
 fs/notify/fsnotify.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 0c4583b61717..48ec61f4c4d5 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -336,6 +336,9 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 			vfsmount_group = vfsmount_mark->group;
 		}
 
+		iter_info.inode_mark = inode_mark;
+		iter_info.vfsmount_mark = vfsmount_mark;
+
 		if (inode_group && vfsmount_group) {
 			int cmp = fsnotify_compare_groups(inode_group,
 							  vfsmount_group);
@@ -348,9 +351,6 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 			}
 		}
 
-		iter_info.inode_mark = inode_mark;
-		iter_info.vfsmount_mark = vfsmount_mark;
-
 		ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask,
 				    data, data_is, cookie, file_name,
 				    &iter_info);
-- 
2.5.5

[PATCH v2 3/7] fsnotify: fix pinning group in fsnotify_prepare_user_wait()

[Miklos Szeredi]

Blind increment of group's user_waits is not enough, we could be far enough
in the group's destruction that it isn't taken into account (i.e. grabbing
the mark ref afterwards doesn't guarantee that it was the ref coming from
the _group_ that was grabbed).

Instead we need to check (under lock) that the mark is still attached to
the group after having obtained a ref to the mark.  If not, skip it.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: 9385a84d7e1f ("fsnotify: Pass fsnotify_iter_info into handle_event handler")
Cc: <stable@vger.kernel.org> # v4.12
---
 fs/notify/mark.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 982527ce6a58..14b76a06ab33 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -253,23 +253,20 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
  */
 static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
 {
-	struct fsnotify_group *group;
-
 	if (!mark)
 		return true;
 
-	group = mark->group;
-	/*
-	 * Since acquisition of mark reference is an atomic op as well, we can
-	 * be sure this inc is seen before any effect of refcount increment.
-	 */
-	atomic_inc(&group->user_waits);
-	if (atomic_inc_not_zero(&mark->refcnt))
-		return true;
-
-	if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
-		wake_up(&group->notification_waitq);
-
+	if (atomic_inc_not_zero(&mark->refcnt)) {
+		spin_lock(&mark->lock);
+		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) {
+			/* mark is attached, group is still alive then */
+			atomic_inc(&mark->group->user_waits);
+			spin_unlock(&mark->lock);
+			return true;
+		}
+		spin_unlock(&mark->lock);
+		fsnotify_put_mark(mark);
+	}
 	return false;
 }
 
-- 
2.5.5

[PATCH v2 4/7] fsnotify: skip unattached marks

[Miklos Szeredi]

After having gone through a ref-unref for the mark, dereferencing the group
(e.g. in fsnotify_compare_groups()) is wrong since the group may be
completely gone by that time.  So before continuing to traverse the mark
list, check if the mark is still attached.

This is done in the generic case, not just when we go through
fsnotify_prepare_user_wait()/fsnotify_finish_user_wait(), otherwise it
would introduce unnecessary complexity.  And it shouldn't hurt to skip
unattached marks anyway ("flags" is very likely in same cacheline as
neighbouring "ignored_mask", which is pulled in anyway).

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: 9385a84d7e1f ("fsnotify: Pass fsnotify_iter_info into handle_event handler")
Cc: <stable@vger.kernel.org> # v4.12
---
 fs/notify/fsnotify.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 48ec61f4c4d5..0ab6a7179e4d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -328,12 +328,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 			inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu),
 						 struct fsnotify_mark, obj_list);
 			inode_group = inode_mark->group;
+			if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
+				goto skip_inode;
 		}
 
 		if (vfsmount_node) {
 			vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu),
 						    struct fsnotify_mark, obj_list);
 			vfsmount_group = vfsmount_mark->group;
+			if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
+				goto skip_vfsmount;
 		}
 
 		iter_info.inode_mark = inode_mark;
@@ -357,10 +361,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 
 		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
 			goto out;
-
+skip_inode:
 		if (inode_group)
 			inode_node = srcu_dereference(inode_node->next,
 						      &fsnotify_mark_srcu);
+skip_vfsmount:
 		if (vfsmount_group)
 			vfsmount_node = srcu_dereference(vfsmount_node->next,
 							 &fsnotify_mark_srcu);
-- 
2.5.5

[PATCH v2 5/7] fanotify: fix fsnotify_prepare_user_wait() failure

[Miklos Szeredi]

If fsnotify_prepare_user_wait() fails, we leave the event on the
notification list.  Which will result in a warning in
fsnotify_destroy_event() and later use-after-free.

Instead of adding a new helper to remove the event from the list in this
case, I opted to move the prepare/finish up into fanotify_handle_event().

This will allow these to be moved further out into the generic code later,
and perhaps let us move to non-sleeping RCU.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Fixes: 05f0e38724e8 ("fanotify: Release SRCU lock when waiting for userspace response")
Cc: <stable@vger.kernel.org> # v4.12
---
 fs/notify/fanotify/fanotify.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 2fa99aeaa095..df3f484e458a 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -64,19 +64,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	/*
-	 * fsnotify_prepare_user_wait() fails if we race with mark deletion.
-	 * Just let the operation pass in that case.
-	 */
-	if (!fsnotify_prepare_user_wait(iter_info)) {
-		event->response = FAN_ALLOW;
-		goto out;
-	}
-
 	wait_event(group->fanotify_data.access_waitq, event->response);
 
-	fsnotify_finish_user_wait(iter_info);
-out:
 	/* userspace responded, convert to something usable */
 	switch (event->response) {
 	case FAN_ALLOW:
@@ -211,9 +200,21 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,
 		 mask);
 
+#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
+	if (mask & FAN_ALL_PERM_EVENTS) {
+		/*
+		 * fsnotify_prepare_user_wait() fails if we race with mark
+		 * deletion.  Just let the operation pass in that case.
+		 */
+		if (!fsnotify_prepare_user_wait(iter_info))
+			return 0;
+	}
+#endif
+
 	event = fanotify_alloc_event(inode, mask, data);
+	ret = -ENOMEM;
 	if (unlikely(!event))
-		return -ENOMEM;
+		goto finish;
 
 	fsn_event = &event->fse;
 	ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
@@ -223,7 +224,8 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 		/* Our event wasn't used in the end. Free it. */
 		fsnotify_destroy_event(group, fsn_event);
 
-		return 0;
+		ret = 0;
+		goto finish;
 	}
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
@@ -232,6 +234,11 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 					    iter_info);
 		fsnotify_destroy_event(group, fsn_event);
 	}
+finish:
+	if (mask & FAN_ALL_PERM_EVENTS)
+		fsnotify_finish_user_wait(iter_info);
+#else
+finish:
 #endif
 	return ret;
 }
-- 
2.5.5

[PATCH v2 6/7] fsnotify: clean up fsnotify()

[Miklos Szeredi]

Use helpers to get first and next marks from connector.

Also get rid of inode_node/vfsmount_node local variables, which just refers
to the same objects as iter_info.  There was an srcu_dereference() for
foo_node, but that's completely superfluous since we've already done it
when obtaining foo_node.

Also get rid of inode_group/vfsmount_group local variables; checking
against non-NULL for these is the same as checking against non-NULL
inode_mark/vfsmount_mark.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/notify/fsnotify.c | 113 ++++++++++++++++++++++++---------------------------
 1 file changed, 54 insertions(+), 59 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 0ab6a7179e4d..dcd70e94cbf1 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -243,6 +243,29 @@ static int send_to_group(struct inode *to_tell,
 					file_name, cookie, iter_info);
 }
 
+static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp)
+{
+	struct fsnotify_mark_connector *conn;
+	struct hlist_node *node = NULL;
+
+	conn = srcu_dereference(*connp, &fsnotify_mark_srcu);
+	if (conn)
+		node = srcu_dereference(conn->list.first, &fsnotify_mark_srcu);
+
+	return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
+}
+
+static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
+{
+	struct hlist_node *node = NULL;
+
+	if (mark)
+		node = srcu_dereference(mark->obj_list.next,
+					&fsnotify_mark_srcu);
+
+	return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
+}
+
 /*
  * 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
@@ -252,11 +275,7 @@ static int send_to_group(struct inode *to_tell,
 int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	     const unsigned char *file_name, u32 cookie)
 {
-	struct hlist_node *inode_node = NULL, *vfsmount_node = NULL;
-	struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL;
-	struct fsnotify_group *inode_group, *vfsmount_group;
-	struct fsnotify_mark_connector *inode_conn, *vfsmount_conn;
-	struct fsnotify_iter_info iter_info;
+	struct fsnotify_iter_info iter_info = { NULL, NULL };
 	struct mount *mnt;
 	int ret = 0;
 	/* global tests shouldn't care about events on child only the specific event */
@@ -291,26 +310,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)) {
-		inode_conn = srcu_dereference(to_tell->i_fsnotify_marks,
-					      &fsnotify_mark_srcu);
-		if (inode_conn)
-			inode_node = srcu_dereference(inode_conn->list.first,
-						      &fsnotify_mark_srcu);
+		iter_info.inode_mark =
+			fsnotify_first_mark(&to_tell->i_fsnotify_marks);
 	}
 
 	if (mnt && ((mask & FS_MODIFY) ||
 		    (test_mask & mnt->mnt_fsnotify_mask))) {
-		inode_conn = srcu_dereference(to_tell->i_fsnotify_marks,
-					      &fsnotify_mark_srcu);
-		if (inode_conn)
-			inode_node = srcu_dereference(inode_conn->list.first,
-						      &fsnotify_mark_srcu);
-		vfsmount_conn = srcu_dereference(mnt->mnt_fsnotify_marks,
-					         &fsnotify_mark_srcu);
-		if (vfsmount_conn)
-			vfsmount_node = srcu_dereference(
-						vfsmount_conn->list.first,
-						&fsnotify_mark_srcu);
+		iter_info.inode_mark =
+			fsnotify_first_mark(&to_tell->i_fsnotify_marks);
+		iter_info.vfsmount_mark =
+			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
 	}
 
 	/*
@@ -318,41 +327,28 @@ 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 (inode_node || vfsmount_node) {
-		inode_group = NULL;
-		inode_mark = NULL;
-		vfsmount_group = NULL;
-		vfsmount_mark = NULL;
-
-		if (inode_node) {
-			inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu),
-						 struct fsnotify_mark, obj_list);
-			inode_group = inode_mark->group;
-			if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
-				goto skip_inode;
-		}
+	while (iter_info.inode_mark || iter_info.vfsmount_mark) {
+		struct fsnotify_mark *inode_mark = iter_info.inode_mark;
+		struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark;
 
-		if (vfsmount_node) {
-			vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu),
-						    struct fsnotify_mark, obj_list);
-			vfsmount_group = vfsmount_mark->group;
-			if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
-				goto skip_vfsmount;
+		if (inode_mark &&
+		    !(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
+			vfsmount_mark = NULL;
+			goto skip;
+		}
+		if (vfsmount_mark &&
+		    !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
+			inode_mark = NULL;
+			goto skip;
 		}
 
-		iter_info.inode_mark = inode_mark;
-		iter_info.vfsmount_mark = vfsmount_mark;
-
-		if (inode_group && vfsmount_group) {
-			int cmp = fsnotify_compare_groups(inode_group,
-							  vfsmount_group);
-			if (cmp > 0) {
-				inode_group = NULL;
+		if (inode_mark && vfsmount_mark) {
+			int cmp = fsnotify_compare_groups(inode_mark->group,
+							  vfsmount_mark->group);
+			if (cmp > 0)
 				inode_mark = NULL;
-			} else if (cmp < 0) {
-				vfsmount_group = NULL;
+			else if (cmp < 0)
 				vfsmount_mark = NULL;
-			}
 		}
 
 		ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask,
@@ -361,14 +357,13 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 
 		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
 			goto out;
-skip_inode:
-		if (inode_group)
-			inode_node = srcu_dereference(inode_node->next,
-						      &fsnotify_mark_srcu);
-skip_vfsmount:
-		if (vfsmount_group)
-			vfsmount_node = srcu_dereference(vfsmount_node->next,
-							 &fsnotify_mark_srcu);
+skip:
+		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);
 	}
 	ret = 0;
 out:
-- 
2.5.5

[PATCH v2 7/7] fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs

[Miklos Szeredi]

The only negative from this patch should be an addition of 32bytes to
'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not
defined.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/notify/fanotify/fanotify.c      |  30 +++------
 fs/notify/fanotify/fanotify.h      |   8 ++-
 fs/notify/fanotify/fanotify_user.c | 123 ++++++++++++++++++-------------------
 include/linux/fsnotify_backend.h   |   2 -
 4 files changed, 72 insertions(+), 91 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index df3f484e458a..63f56b007280 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -35,15 +35,13 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 
 	pr_debug("%s: list=%p event=%p\n", __func__, list, event);
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 	/*
 	 * Don't merge a permission event with any other event so that we know
 	 * the event structure we have created in fanotify_handle_event() is the
 	 * one we should check for permission response.
 	 */
-	if (event->mask & FAN_ALL_PERM_EVENTS)
+	if (fanotify_is_perm_event(event->mask))
 		return 0;
-#endif
 
 	list_for_each_entry_reverse(test_event, list, list) {
 		if (should_merge(test_event, event)) {
@@ -55,7 +53,6 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 	return 0;
 }
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 static int fanotify_get_response(struct fsnotify_group *group,
 				 struct fanotify_perm_event_info *event,
 				 struct fsnotify_iter_info *iter_info)
@@ -82,7 +79,6 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	
 	return ret;
 }
-#endif
 
 static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 				       struct fsnotify_mark *vfsmnt_mark,
@@ -141,8 +137,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
 {
 	struct fanotify_event_info *event;
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (mask & FAN_ALL_PERM_EVENTS) {
+	if (fanotify_is_perm_event(mask)) {
 		struct fanotify_perm_event_info *pevent;
 
 		pevent = kmem_cache_alloc(fanotify_perm_event_cachep,
@@ -153,7 +148,6 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
 		pevent->response = 0;
 		goto init;
 	}
-#endif
 	event = kmem_cache_alloc(fanotify_event_cachep, GFP_KERNEL);
 	if (!event)
 		return NULL;
@@ -200,8 +194,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,
 		 mask);
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (mask & FAN_ALL_PERM_EVENTS) {
+	if (fanotify_is_perm_event(mask)) {
 		/*
 		 * fsnotify_prepare_user_wait() fails if we race with mark
 		 * deletion.  Just let the operation pass in that case.
@@ -209,7 +202,6 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 		if (!fsnotify_prepare_user_wait(iter_info))
 			return 0;
 	}
-#endif
 
 	event = fanotify_alloc_event(inode, mask, data);
 	ret = -ENOMEM;
@@ -225,21 +217,15 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 		fsnotify_destroy_event(group, fsn_event);
 
 		ret = 0;
-		goto finish;
-	}
-
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (mask & FAN_ALL_PERM_EVENTS) {
+	} else if (fanotify_is_perm_event(mask)) {
 		ret = fanotify_get_response(group, FANOTIFY_PE(fsn_event),
 					    iter_info);
 		fsnotify_destroy_event(group, fsn_event);
 	}
 finish:
-	if (mask & FAN_ALL_PERM_EVENTS)
+	if (fanotify_is_perm_event(mask))
 		fsnotify_finish_user_wait(iter_info);
-#else
-finish:
-#endif
+
 	return ret;
 }
 
@@ -259,13 +245,11 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 	event = FANOTIFY_E(fsn_event);
 	path_put(&event->path);
 	put_pid(event->tgid);
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (fsn_event->mask & FAN_ALL_PERM_EVENTS) {
+	if (fanotify_is_perm_event(fsn_event->mask)) {
 		kmem_cache_free(fanotify_perm_event_cachep,
 				FANOTIFY_PE(fsn_event));
 		return;
 	}
-#endif
 	kmem_cache_free(fanotify_event_cachep, event);
 }
 
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 4eb6f5efa282..dc219cf07a6a 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -21,7 +21,6 @@ struct fanotify_event_info {
 	struct pid *tgid;
 };
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 /*
  * Structure for permission fanotify events. It gets allocated and freed in
  * fanotify_handle_event() since we wait there for user response. When the
@@ -40,7 +39,12 @@ FANOTIFY_PE(struct fsnotify_event *fse)
 {
 	return container_of(fse, struct fanotify_perm_event_info, fae.fse);
 }
-#endif
+
+static inline bool fanotify_is_perm_event(u32 mask)
+{
+	return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) &&
+		mask & FAN_ALL_PERM_EVENTS;
+}
 
 static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse)
 {
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 907a481ac781..44fd12aa17ff 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -142,7 +142,6 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	return ret;
 }
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 static struct fanotify_perm_event_info *dequeue_event(
 				struct fsnotify_group *group, int fd)
 {
@@ -199,7 +198,6 @@ static int process_access_response(struct fsnotify_group *group,
 
 	return 0;
 }
-#endif
 
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
 				  struct fsnotify_event *event,
@@ -221,10 +219,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 			 fanotify_event_metadata.event_len))
 		goto out_close_fd;
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (event->mask & FAN_ALL_PERM_EVENTS)
+	if (fanotify_is_perm_event(event->mask))
 		FANOTIFY_PE(event)->fd = fd;
-#endif
 
 	if (fd != FAN_NOFD)
 		fd_install(fd, f);
@@ -309,10 +305,9 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 		 * Permission events get queued to wait for response.  Other
 		 * events can be destroyed now.
 		 */
-		if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) {
+		if (!fanotify_is_perm_event(kevent->mask)) {
 			fsnotify_destroy_event(group, kevent);
 		} else {
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 			if (ret <= 0) {
 				FANOTIFY_PE(kevent)->response = FAN_DENY;
 				wake_up(&group->fanotify_data.access_waitq);
@@ -322,7 +317,6 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 					&group->fanotify_data.access_list);
 				spin_unlock(&group->notification_lock);
 			}
-#endif
 		}
 		if (ret < 0)
 			break;
@@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 
 static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
 {
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 	struct fanotify_response response = { .fd = -1, .response = -1 };
 	struct fsnotify_group *group;
 	int ret;
 
+	if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
+		return -EINVAL;
+
+
 	group = file->private_data;
 
 	if (count > sizeof(response))
@@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t
 		count = ret;
 
 	return count;
-#else
-	return -EINVAL;
-#endif
 }
 
 static int fanotify_release(struct inode *ignored, struct file *file)
 {
 	struct fsnotify_group *group = file->private_data;
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	struct fanotify_perm_event_info *event, *next;
-	struct fsnotify_event *fsn_event;
+	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
+		struct fanotify_perm_event_info *event, *next;
+		struct fsnotify_event *fsn_event;
 
-	/*
-	 * Stop new events from arriving in the notification queue. since
-	 * userspace cannot use fanotify fd anymore, no event can enter or
-	 * leave access_list by now either.
-	 */
-	fsnotify_group_stop_queueing(group);
+		/*
+		 * Stop new events from arriving in the notification
+		 * queue. since userspace cannot use fanotify fd anymore, no
+		 * event can enter or leave access_list by now either.
+		 */
+		fsnotify_group_stop_queueing(group);
 
-	/*
-	 * Process all permission events on access_list and notification queue
-	 * and simulate reply from userspace.
-	 */
-	spin_lock(&group->notification_lock);
-	list_for_each_entry_safe(event, next, &group->fanotify_data.access_list,
-				 fae.fse.list) {
-		pr_debug("%s: found group=%p event=%p\n", __func__, group,
-			 event);
+		/*
+		 * Process all permission events on access_list and notification
+		 * queue and simulate reply from userspace.
+		 */
+		spin_lock(&group->notification_lock);
+		list_for_each_entry_safe(event, next,
+					 &group->fanotify_data.access_list,
+					 fae.fse.list) {
+			pr_debug("%s: found group=%p event=%p\n", __func__,
+				 group, event);
+
+			list_del_init(&event->fae.fse.list);
+			event->response = FAN_ALLOW;
+		}
 
-		list_del_init(&event->fae.fse.list);
-		event->response = FAN_ALLOW;
-	}
+		/*
+		 * Destroy all non-permission events. For permission events just
+		 * dequeue them and set the response. They will be freed once
+		 * the response is consumed and fanotify_get_response() returns.
+		 */
+		while (!fsnotify_notify_queue_is_empty(group)) {
+			fsn_event = fsnotify_remove_first_event(group);
+			if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) {
+				spin_unlock(&group->notification_lock);
+				fsnotify_destroy_event(group, fsn_event);
+				spin_lock(&group->notification_lock);
+			} else
+				FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
+		}
+		spin_unlock(&group->notification_lock);
 
-	/*
-	 * Destroy all non-permission events. For permission events just
-	 * dequeue them and set the response. They will be freed once the
-	 * response is consumed and fanotify_get_response() returns.
-	 */
-	while (!fsnotify_notify_queue_is_empty(group)) {
-		fsn_event = fsnotify_remove_first_event(group);
-		if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) {
-			spin_unlock(&group->notification_lock);
-			fsnotify_destroy_event(group, fsn_event);
-			spin_lock(&group->notification_lock);
-		} else
-			FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
+		/* Response for all permission events it set, wakeup waiters */
+		wake_up(&group->fanotify_data.access_waitq);
 	}
-	spin_unlock(&group->notification_lock);
-
-	/* Response for all permission events it set, wakeup waiters */
-	wake_up(&group->fanotify_data.access_waitq);
-#endif
 
 	/* matches the fanotify_init->fsnotify_alloc_group */
 	fsnotify_destroy_group(group);
@@ -768,10 +763,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	if (force_o_largefile())
 		event_f_flags |= O_LARGEFILE;
 	group->fanotify_data.f_flags = event_f_flags;
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	init_waitqueue_head(&group->fanotify_data.access_waitq);
-	INIT_LIST_HEAD(&group->fanotify_data.access_list);
-#endif
+	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
+		init_waitqueue_head(&group->fanotify_data.access_waitq);
+		INIT_LIST_HEAD(&group->fanotify_data.access_list);
+	}
 	switch (flags & FAN_ALL_CLASS_BITS) {
 	case FAN_CLASS_NOTIF:
 		group->priority = FS_PRIO_0;
@@ -825,6 +820,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 	struct fsnotify_group *group;
 	struct fd f;
 	struct path path;
+	u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD;
 	int ret;
 
 	pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
@@ -855,11 +851,10 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 		mask &= ~FAN_ONDIR;
 	}
 
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (mask & ~(FAN_ALL_EVENTS | FAN_ALL_PERM_EVENTS | FAN_EVENT_ON_CHILD))
-#else
-	if (mask & ~(FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD))
-#endif
+	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
+		valid_mask |= FAN_ALL_PERM_EVENTS;
+
+	if (mask & ~valid_mask)
 		return -EINVAL;
 
 	f = fdget(fanotify_fd);
@@ -949,10 +944,10 @@ static int __init fanotify_user_setup(void)
 {
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark, SLAB_PANIC);
 	fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC);
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	fanotify_perm_event_cachep = KMEM_CACHE(fanotify_perm_event_info,
-						SLAB_PANIC);
-#endif
+	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
+		fanotify_perm_event_cachep =
+			KMEM_CACHE(fanotify_perm_event_info, SLAB_PANIC);
+	}
 
 	return 0;
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index c6c69318752b..eaea494683ab 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -182,11 +182,9 @@ struct fsnotify_group {
 #endif
 #ifdef CONFIG_FANOTIFY
 		struct fanotify_group_private_data {
-#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
 			/* allows a group to block waiting for a userspace response */
 			struct list_head access_list;
 			wait_queue_head_t access_waitq;
-#endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
 			int f_flags;
 			unsigned int max_marks;
 			struct user_struct *user;
-- 
2.5.5

Re: [PATCH v2 7/7] fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs

[Amir Goldstein]

On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> The only negative from this patch should be an addition of 32bytes to
> 'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not
> defined.
>

Thanks for this cleanup!
See one question and coding style nits below.

> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/notify/fanotify/fanotify.c      |  30 +++------
>  fs/notify/fanotify/fanotify.h      |   8 ++-
>  fs/notify/fanotify/fanotify_user.c | 123 ++++++++++++++++++-------------------
>  include/linux/fsnotify_backend.h   |   2 -
>  4 files changed, 72 insertions(+), 91 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index df3f484e458a..63f56b007280 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -35,15 +35,13 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
>
>         pr_debug("%s: list=%p event=%p\n", __func__, list, event);
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>         /*
>          * Don't merge a permission event with any other event so that we know
>          * the event structure we have created in fanotify_handle_event() is the
>          * one we should check for permission response.
>          */
> -       if (event->mask & FAN_ALL_PERM_EVENTS)
> +       if (fanotify_is_perm_event(event->mask))
>                 return 0;
> -#endif
>
>         list_for_each_entry_reverse(test_event, list, list) {
>                 if (should_merge(test_event, event)) {
> @@ -55,7 +53,6 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
>         return 0;
>  }
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>  static int fanotify_get_response(struct fsnotify_group *group,
>                                  struct fanotify_perm_event_info *event,
>                                  struct fsnotify_iter_info *iter_info)
> @@ -82,7 +79,6 @@ static int fanotify_get_response(struct fsnotify_group *group,
>
>         return ret;
>  }
> -#endif
>
>  static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
>                                        struct fsnotify_mark *vfsmnt_mark,
> @@ -141,8 +137,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
>  {
>         struct fanotify_event_info *event;
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -       if (mask & FAN_ALL_PERM_EVENTS) {
> +       if (fanotify_is_perm_event(mask)) {
>                 struct fanotify_perm_event_info *pevent;
>
>                 pevent = kmem_cache_alloc(fanotify_perm_event_cachep,
> @@ -153,7 +148,6 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask,
>                 pevent->response = 0;
>                 goto init;
>         }
> -#endif
>         event = kmem_cache_alloc(fanotify_event_cachep, GFP_KERNEL);
>         if (!event)
>                 return NULL;
> @@ -200,8 +194,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>         pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,
>                  mask);
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -       if (mask & FAN_ALL_PERM_EVENTS) {
> +       if (fanotify_is_perm_event(mask)) {
>                 /*
>                  * fsnotify_prepare_user_wait() fails if we race with mark
>                  * deletion.  Just let the operation pass in that case.
> @@ -209,7 +202,6 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>                 if (!fsnotify_prepare_user_wait(iter_info))
>                         return 0;
>         }
> -#endif
>
>         event = fanotify_alloc_event(inode, mask, data);
>         ret = -ENOMEM;
> @@ -225,21 +217,15 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>                 fsnotify_destroy_event(group, fsn_event);
>
>                 ret = 0;
> -               goto finish;
> -       }
> -
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -       if (mask & FAN_ALL_PERM_EVENTS) {
> +       } else if (fanotify_is_perm_event(mask)) {
>                 ret = fanotify_get_response(group, FANOTIFY_PE(fsn_event),
>                                             iter_info);
>                 fsnotify_destroy_event(group, fsn_event);
>         }
>  finish:
> -       if (mask & FAN_ALL_PERM_EVENTS)
> +       if (fanotify_is_perm_event(mask))
>                 fsnotify_finish_user_wait(iter_info);
> -#else
> -finish:
> -#endif

Could have reduced messiness of path 5/7 if cleanup was done before - oh well

> +
>         return ret;
>  }
>
> @@ -259,13 +245,11 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
>         event = FANOTIFY_E(fsn_event);
>         path_put(&event->path);
>         put_pid(event->tgid);
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -       if (fsn_event->mask & FAN_ALL_PERM_EVENTS) {
> +       if (fanotify_is_perm_event(fsn_event->mask)) {
>                 kmem_cache_free(fanotify_perm_event_cachep,
>                                 FANOTIFY_PE(fsn_event));
>                 return;
>         }
> -#endif
>         kmem_cache_free(fanotify_event_cachep, event);
>  }
>
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index 4eb6f5efa282..dc219cf07a6a 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -21,7 +21,6 @@ struct fanotify_event_info {
>         struct pid *tgid;
>  };
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>  /*
>   * Structure for permission fanotify events. It gets allocated and freed in
>   * fanotify_handle_event() since we wait there for user response. When the
> @@ -40,7 +39,12 @@ FANOTIFY_PE(struct fsnotify_event *fse)
>  {
>         return container_of(fse, struct fanotify_perm_event_info, fae.fse);
>  }
> -#endif
> +
> +static inline bool fanotify_is_perm_event(u32 mask)
> +{
> +       return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) &&
> +               mask & FAN_ALL_PERM_EVENTS;

I know this is good w.r.t operation precedence, but it gets me eerie to see
bit masking without (). maybe its just me.

> +}
>
>  static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse)
>  {
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 907a481ac781..44fd12aa17ff 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -142,7 +142,6 @@ static int fill_event_metadata(struct fsnotify_group *group,
>         return ret;
>  }
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>  static struct fanotify_perm_event_info *dequeue_event(
>                                 struct fsnotify_group *group, int fd)
>  {
> @@ -199,7 +198,6 @@ static int process_access_response(struct fsnotify_group *group,
>
>         return 0;
>  }
> -#endif
>
>  static ssize_t copy_event_to_user(struct fsnotify_group *group,
>                                   struct fsnotify_event *event,
> @@ -221,10 +219,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>                          fanotify_event_metadata.event_len))
>                 goto out_close_fd;
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -       if (event->mask & FAN_ALL_PERM_EVENTS)
> +       if (fanotify_is_perm_event(event->mask))
>                 FANOTIFY_PE(event)->fd = fd;
> -#endif
>
>         if (fd != FAN_NOFD)
>                 fd_install(fd, f);
> @@ -309,10 +305,9 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>                  * Permission events get queued to wait for response.  Other
>                  * events can be destroyed now.
>                  */
> -               if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) {
> +               if (!fanotify_is_perm_event(kevent->mask)) {
>                         fsnotify_destroy_event(group, kevent);
>                 } else {
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>                         if (ret <= 0) {
>                                 FANOTIFY_PE(kevent)->response = FAN_DENY;
>                                 wake_up(&group->fanotify_data.access_waitq);
> @@ -322,7 +317,6 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>                                         &group->fanotify_data.access_list);
>                                 spin_unlock(&group->notification_lock);
>                         }
> -#endif
>                 }
>                 if (ret < 0)
>                         break;
> @@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>
>  static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
>  {
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>         struct fanotify_response response = { .fd = -1, .response = -1 };
>         struct fsnotify_group *group;
>         int ret;
>
> +       if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
> +               return -EINVAL;
> +
> +
>         group = file->private_data;
>
>         if (count > sizeof(response))
> @@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t
>                 count = ret;
>
>         return count;
> -#else
> -       return -EINVAL;
> -#endif
>  }
>
>  static int fanotify_release(struct inode *ignored, struct file *file)
>  {
>         struct fsnotify_group *group = file->private_data;
>
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -       struct fanotify_perm_event_info *event, *next;
> -       struct fsnotify_event *fsn_event;
> +       if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
> +               struct fanotify_perm_event_info *event, *next;
> +               struct fsnotify_event *fsn_event;
>
> -       /*
> -        * Stop new events from arriving in the notification queue. since
> -        * userspace cannot use fanotify fd anymore, no event can enter or
> -        * leave access_list by now either.
> -        */
> -       fsnotify_group_stop_queueing(group);
> +               /*
> +                * Stop new events from arriving in the notification
> +                * queue. since userspace cannot use fanotify fd anymore, no
> +                * event can enter or leave access_list by now either.
> +                */
> +               fsnotify_group_stop_queueing(group);
>
> -       /*
> -        * Process all permission events on access_list and notification queue
> -        * and simulate reply from userspace.
> -        */
> -       spin_lock(&group->notification_lock);
> -       list_for_each_entry_safe(event, next, &group->fanotify_data.access_list,
> -                                fae.fse.list) {
> -               pr_debug("%s: found group=%p event=%p\n", __func__, group,
> -                        event);
> +               /*
> +                * Process all permission events on access_list and notification
> +                * queue and simulate reply from userspace.
> +                */
> +               spin_lock(&group->notification_lock);
> +               list_for_each_entry_safe(event, next,
> +                                        &group->fanotify_data.access_list,
> +                                        fae.fse.list) {
> +                       pr_debug("%s: found group=%p event=%p\n", __func__,
> +                                group, event);
> +
> +                       list_del_init(&event->fae.fse.list);
> +                       event->response = FAN_ALLOW;
> +               }
>
> -               list_del_init(&event->fae.fse.list);
> -               event->response = FAN_ALLOW;
> -       }
> +               /*
> +                * Destroy all non-permission events. For permission events just
> +                * dequeue them and set the response. They will be freed once
> +                * the response is consumed and fanotify_get_response() returns.
> +                */
> +               while (!fsnotify_notify_queue_is_empty(group)) {
> +                       fsn_event = fsnotify_remove_first_event(group);
> +                       if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) {
> +                               spin_unlock(&group->notification_lock);
> +                               fsnotify_destroy_event(group, fsn_event);
> +                               spin_lock(&group->notification_lock);
> +                       } else
> +                               FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;

Not your change, but if you post another version please add {} after else

> +               }
> +               spin_unlock(&group->notification_lock);
>
> -       /*
> -        * Destroy all non-permission events. For permission events just
> -        * dequeue them and set the response. They will be freed once the
> -        * response is consumed and fanotify_get_response() returns.
> -        */
> -       while (!fsnotify_notify_queue_is_empty(group)) {
> -               fsn_event = fsnotify_remove_first_event(group);
> -               if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) {
> -                       spin_unlock(&group->notification_lock);
> -                       fsnotify_destroy_event(group, fsn_event);
> -                       spin_lock(&group->notification_lock);
> -               } else
> -                       FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
> +               /* Response for all permission events it set, wakeup waiters */
> +               wake_up(&group->fanotify_data.access_waitq);
>         }
> -       spin_unlock(&group->notification_lock);
> -
> -       /* Response for all permission events it set, wakeup waiters */
> -       wake_up(&group->fanotify_data.access_waitq);

So I might as well learn something from this review -
why did you move wake_up inside spinlock? Does it matter at all?

Amir.

Re: [PATCH v2 6/7] fsnotify: clean up fsnotify()

[Amir Goldstein]

On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> Use helpers to get first and next marks from connector.
>
> Also get rid of inode_node/vfsmount_node local variables, which just refers
> to the same objects as iter_info.  There was an srcu_dereference() for
> foo_node, but that's completely superfluous since we've already done it
> when obtaining foo_node.
>
> Also get rid of inode_group/vfsmount_group local variables; checking
> against non-NULL for these is the same as checking against non-NULL
> inode_mark/vfsmount_mark.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/notify/fsnotify.c | 113 ++++++++++++++++++++++++---------------------------
>  1 file changed, 54 insertions(+), 59 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 0ab6a7179e4d..dcd70e94cbf1 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -243,6 +243,29 @@ static int send_to_group(struct inode *to_tell,
>                                         file_name, cookie, iter_info);
>  }
>
> +static struct fsnotify_mark *fsnotify_first_mark(struct fsnotify_mark_connector **connp)
> +{
> +       struct fsnotify_mark_connector *conn;
> +       struct hlist_node *node = NULL;
> +
> +       conn = srcu_dereference(*connp, &fsnotify_mark_srcu);
> +       if (conn)
> +               node = srcu_dereference(conn->list.first, &fsnotify_mark_srcu);
> +
> +       return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
> +}
> +
> +static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
> +{
> +       struct hlist_node *node = NULL;
> +
> +       if (mark)
> +               node = srcu_dereference(mark->obj_list.next,
> +                                       &fsnotify_mark_srcu);
> +
> +       return hlist_entry_safe(node, struct fsnotify_mark, obj_list);
> +}
> +
>  /*
>   * 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
> @@ -252,11 +275,7 @@ static int send_to_group(struct inode *to_tell,
>  int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>              const unsigned char *file_name, u32 cookie)
>  {
> -       struct hlist_node *inode_node = NULL, *vfsmount_node = NULL;
> -       struct fsnotify_mark *inode_mark = NULL, *vfsmount_mark = NULL;
> -       struct fsnotify_group *inode_group, *vfsmount_group;
> -       struct fsnotify_mark_connector *inode_conn, *vfsmount_conn;
> -       struct fsnotify_iter_info iter_info;
> +       struct fsnotify_iter_info iter_info = { NULL, NULL };

Forgot to change to = { };

>         struct mount *mnt;
>         int ret = 0;
>         /* global tests shouldn't care about events on child only the specific event */
> @@ -291,26 +310,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)) {
> -               inode_conn = srcu_dereference(to_tell->i_fsnotify_marks,
> -                                             &fsnotify_mark_srcu);
> -               if (inode_conn)
> -                       inode_node = srcu_dereference(inode_conn->list.first,
> -                                                     &fsnotify_mark_srcu);
> +               iter_info.inode_mark =
> +                       fsnotify_first_mark(&to_tell->i_fsnotify_marks);
>         }
>
>         if (mnt && ((mask & FS_MODIFY) ||
>                     (test_mask & mnt->mnt_fsnotify_mask))) {
> -               inode_conn = srcu_dereference(to_tell->i_fsnotify_marks,
> -                                             &fsnotify_mark_srcu);
> -               if (inode_conn)
> -                       inode_node = srcu_dereference(inode_conn->list.first,
> -                                                     &fsnotify_mark_srcu);
> -               vfsmount_conn = srcu_dereference(mnt->mnt_fsnotify_marks,
> -                                                &fsnotify_mark_srcu);
> -               if (vfsmount_conn)
> -                       vfsmount_node = srcu_dereference(
> -                                               vfsmount_conn->list.first,
> -                                               &fsnotify_mark_srcu);
> +               iter_info.inode_mark =
> +                       fsnotify_first_mark(&to_tell->i_fsnotify_marks);
> +               iter_info.vfsmount_mark =
> +                       fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
>         }
>
>         /*
> @@ -318,41 +327,28 @@ 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 (inode_node || vfsmount_node) {
> -               inode_group = NULL;
> -               inode_mark = NULL;
> -               vfsmount_group = NULL;
> -               vfsmount_mark = NULL;
> -
> -               if (inode_node) {
> -                       inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu),
> -                                                struct fsnotify_mark, obj_list);
> -                       inode_group = inode_mark->group;
> -                       if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
> -                               goto skip_inode;
> -               }
> +       while (iter_info.inode_mark || iter_info.vfsmount_mark) {
> +               struct fsnotify_mark *inode_mark = iter_info.inode_mark;
> +               struct fsnotify_mark *vfsmount_mark = iter_info.vfsmount_mark;
>
> -               if (vfsmount_node) {
> -                       vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu),
> -                                                   struct fsnotify_mark, obj_list);
> -                       vfsmount_group = vfsmount_mark->group;
> -                       if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
> -                               goto skip_vfsmount;
> +               if (inode_mark &&
> +                   !(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
> +                       vfsmount_mark = NULL;
> +                       goto skip;
> +               }
> +               if (vfsmount_mark &&
> +                   !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) {
> +                       inode_mark = NULL;
> +                       goto skip;
>                 }
>
> -               iter_info.inode_mark = inode_mark;
> -               iter_info.vfsmount_mark = vfsmount_mark;
> -
> -               if (inode_group && vfsmount_group) {
> -                       int cmp = fsnotify_compare_groups(inode_group,
> -                                                         vfsmount_group);
> -                       if (cmp > 0) {
> -                               inode_group = NULL;
> +               if (inode_mark && vfsmount_mark) {
> +                       int cmp = fsnotify_compare_groups(inode_mark->group,
> +                                                         vfsmount_mark->group);
> +                       if (cmp > 0)
>                                 inode_mark = NULL;
> -                       } else if (cmp < 0) {
> -                               vfsmount_group = NULL;
> +                       else if (cmp < 0)
>                                 vfsmount_mark = NULL;
> -                       }
>                 }
>
>                 ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask,
> @@ -361,14 +357,13 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>
>                 if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
>                         goto out;
> -skip_inode:
> -               if (inode_group)
> -                       inode_node = srcu_dereference(inode_node->next,
> -                                                     &fsnotify_mark_srcu);
> -skip_vfsmount:
> -               if (vfsmount_group)
> -                       vfsmount_node = srcu_dereference(vfsmount_node->next,
> -                                                        &fsnotify_mark_srcu);
> +skip:
> +               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);
>         }
>         ret = 0;
>  out:
> --
> 2.5.5
>

Re: [PATCH v2 1/7] fsnotify: clean up fsnotify_prepare/finish_user_wait()

[Amir Goldstein]

On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> This patch doesn't actually fix any bug, just paves the way for fixing mark
> and group pinning.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Cc: <stable@vger.kernel.org> # v4.12
> ---
>  fs/notify/mark.c | 89 ++++++++++++++++++++++++++------------------------------
>  1 file changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 9991f8826734..982527ce6a58 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -109,16 +109,6 @@ void fsnotify_get_mark(struct fsnotify_mark *mark)
>         atomic_inc(&mark->refcnt);
>  }
>
> -/*
> - * Get mark reference when we found the mark via lockless traversal of object
> - * list. Mark can be already removed from the list by now and on its way to be
> - * destroyed once SRCU period ends.
> - */
> -static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
> -{
> -       return atomic_inc_not_zero(&mark->refcnt);
> -}
> -
>  static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
>  {
>         u32 new_mask = 0;
> @@ -256,32 +246,56 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
>                            FSNOTIFY_REAPER_DELAY);
>  }
>
> -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
> +/*
> + * Get mark reference when we found the mark via lockless traversal of object
> + * list. Mark can be already removed from the list by now and on its way to be
> + * destroyed once SRCU period ends.
> + */
> +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
>  {
>         struct fsnotify_group *group;
>
> -       if (WARN_ON_ONCE(!iter_info->inode_mark && !iter_info->vfsmount_mark))
> -               return false;
> -
> -       if (iter_info->inode_mark)
> -               group = iter_info->inode_mark->group;
> -       else
> -               group = iter_info->vfsmount_mark->group;
> +       if (!mark)
> +               return true;
>
> +       group = mark->group;
>         /*
>          * Since acquisition of mark reference is an atomic op as well, we can
>          * be sure this inc is seen before any effect of refcount increment.
>          */
>         atomic_inc(&group->user_waits);
> +       if (atomic_inc_not_zero(&mark->refcnt))
> +               return true;
> +

Please replicate the "We abuse notification_waitq on group shutdown" comment
here as well to clarify why this wakeup is needed. Although comment says
"for waiting for all marks pinned" and this code path did not change
mark refcount,
so is the wakeup really needed here?

> +       if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
> +               wake_up(&group->notification_waitq);
>
> -       if (iter_info->inode_mark) {
> -               /* This can fail if mark is being removed */
> -               if (!fsnotify_get_mark_safe(iter_info->inode_mark))
> -                       goto out_wait;
> +       return false;
> +}
> +
> +static void fsnotify_put_mark_wait(struct fsnotify_mark *mark)
> +{
> +       if (mark) {
> +               struct fsnotify_group *group = mark->group;
> +
> +               fsnotify_put_mark(mark);
> +               /*
> +                * We abuse notification_waitq on group shutdown for waiting for
> +                * all marks pinned when waiting for userspace.
> +                */
> +               if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
> +                       wake_up(&group->notification_waitq);
>         }
> -       if (iter_info->vfsmount_mark) {
> -               if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark))
> -                       goto out_inode;
> +}
> +
> +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_wait(iter_info->inode_mark);
> +               return false;
>         }
>
>         /*
> @@ -292,34 +306,13 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>         srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
>
>         return true;
> -out_inode:
> -       if (iter_info->inode_mark)
> -               fsnotify_put_mark(iter_info->inode_mark);
> -out_wait:
> -       if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
> -               wake_up(&group->notification_waitq);
> -       return false;
>  }
>
>  void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
>  {
> -       struct fsnotify_group *group = NULL;
> -
>         iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
> -       if (iter_info->inode_mark) {
> -               group = iter_info->inode_mark->group;
> -               fsnotify_put_mark(iter_info->inode_mark);
> -       }
> -       if (iter_info->vfsmount_mark) {
> -               group = iter_info->vfsmount_mark->group;
> -               fsnotify_put_mark(iter_info->vfsmount_mark);
> -       }
> -       /*
> -        * We abuse notification_waitq on group shutdown for waiting for all
> -        * marks pinned when waiting for userspace.
> -        */
> -       if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
> -               wake_up(&group->notification_waitq);
> +       fsnotify_put_mark_wait(iter_info->inode_mark);
> +       fsnotify_put_mark_wait(iter_info->vfsmount_mark);
>  }
>
>  /*
> --
> 2.5.5
>

Re: [PATCH v2 1/7] fsnotify: clean up fsnotify_prepare/finish_user_wait()

[Amir Goldstein]

On Wed, Oct 25, 2017 at 3:06 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> This patch doesn't actually fix any bug, just paves the way for fixing mark
>> and group pinning.
>>
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> Cc: <stable@vger.kernel.org> # v4.12
>> ---
>>  fs/notify/mark.c | 89 ++++++++++++++++++++++++++------------------------------
>>  1 file changed, 41 insertions(+), 48 deletions(-)
>>
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index 9991f8826734..982527ce6a58 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -109,16 +109,6 @@ void fsnotify_get_mark(struct fsnotify_mark *mark)
>>         atomic_inc(&mark->refcnt);
>>  }
>>
>> -/*
>> - * Get mark reference when we found the mark via lockless traversal of object
>> - * list. Mark can be already removed from the list by now and on its way to be
>> - * destroyed once SRCU period ends.
>> - */
>> -static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
>> -{
>> -       return atomic_inc_not_zero(&mark->refcnt);
>> -}
>> -
>>  static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
>>  {
>>         u32 new_mask = 0;
>> @@ -256,32 +246,56 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
>>                            FSNOTIFY_REAPER_DELAY);
>>  }
>>
>> -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>> +/*
>> + * Get mark reference when we found the mark via lockless traversal of object
>> + * list. Mark can be already removed from the list by now and on its way to be
>> + * destroyed once SRCU period ends.
>> + */
>> +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)

Sorry, hit send too soon...
This helper is now more than "get mark safe"
how about fsnotify_get_mark_wait() to pair with fsnotify_put_mark_wait()?

>>  {
>>         struct fsnotify_group *group;
>>
>> -       if (WARN_ON_ONCE(!iter_info->inode_mark && !iter_info->vfsmount_mark))
>> -               return false;
>> -
>> -       if (iter_info->inode_mark)
>> -               group = iter_info->inode_mark->group;
>> -       else
>> -               group = iter_info->vfsmount_mark->group;
>> +       if (!mark)
>> +               return true;
>>
>> +       group = mark->group;
>>         /*
>>          * Since acquisition of mark reference is an atomic op as well, we can
>>          * be sure this inc is seen before any effect of refcount increment.
>>          */
>>         atomic_inc(&group->user_waits);
>> +       if (atomic_inc_not_zero(&mark->refcnt))
>> +               return true;
>> +
>
> Please replicate the "We abuse notification_waitq on group shutdown" comment
> here as well to clarify why this wakeup is needed. Although comment says
> "for waiting for all marks pinned" and this code path did not change
> mark refcount,
> so is the wakeup really needed here?
>
>> +       if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
>> +               wake_up(&group->notification_waitq);
>>
>> -       if (iter_info->inode_mark) {
>> -               /* This can fail if mark is being removed */
>> -               if (!fsnotify_get_mark_safe(iter_info->inode_mark))
>> -                       goto out_wait;
>> +       return false;
>> +}
>> +
>> +static void fsnotify_put_mark_wait(struct fsnotify_mark *mark)
>> +{
>> +       if (mark) {
>> +               struct fsnotify_group *group = mark->group;
>> +
>> +               fsnotify_put_mark(mark);
>> +               /*
>> +                * We abuse notification_waitq on group shutdown for waiting for
>> +                * all marks pinned when waiting for userspace.
>> +                */
>> +               if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
>> +                       wake_up(&group->notification_waitq);
>>         }
>> -       if (iter_info->vfsmount_mark) {
>> -               if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark))
>> -                       goto out_inode;
>> +}
>> +
>> +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_wait(iter_info->inode_mark);
>> +               return false;
>>         }
>>
>>         /*
>> @@ -292,34 +306,13 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>>         srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
>>
>>         return true;
>> -out_inode:
>> -       if (iter_info->inode_mark)
>> -               fsnotify_put_mark(iter_info->inode_mark);
>> -out_wait:
>> -       if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
>> -               wake_up(&group->notification_waitq);
>> -       return false;
>>  }
>>
>>  void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
>>  {
>> -       struct fsnotify_group *group = NULL;
>> -
>>         iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
>> -       if (iter_info->inode_mark) {
>> -               group = iter_info->inode_mark->group;
>> -               fsnotify_put_mark(iter_info->inode_mark);
>> -       }
>> -       if (iter_info->vfsmount_mark) {
>> -               group = iter_info->vfsmount_mark->group;
>> -               fsnotify_put_mark(iter_info->vfsmount_mark);
>> -       }
>> -       /*
>> -        * We abuse notification_waitq on group shutdown for waiting for all
>> -        * marks pinned when waiting for userspace.
>> -        */
>> -       if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
>> -               wake_up(&group->notification_waitq);
>> +       fsnotify_put_mark_wait(iter_info->inode_mark);
>> +       fsnotify_put_mark_wait(iter_info->vfsmount_mark);
>>  }
>>
>>  /*
>> --
>> 2.5.5
>>

Re: [PATCH v2 1/7] fsnotify: clean up fsnotify_prepare/finish_user_wait()

[Miklos Szeredi]

On Wed, Oct 25, 2017 at 2:07 PM, Amir Goldstein <amir73il@gmail.com> wrote:

>>> -bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
>>> +/*
>>> + * Get mark reference when we found the mark via lockless traversal of object
>>> + * list. Mark can be already removed from the list by now and on its way to be
>>> + * destroyed once SRCU period ends.
>>> + */
>>> +static bool fsnotify_get_mark_safe(struct fsnotify_mark *mark)
>
> Sorry, hit send too soon...
> This helper is now more than "get mark safe"
> how about fsnotify_get_mark_wait() to pair with fsnotify_put_mark_wait()?

It's get mark and pin mark->group.  I think "safe" describes that
adequately.   Added comments to reflect that.

Also renamed fsnotify_put_mark_wait() to fsnotify_put_mark_wake(),
because that's what it does.

Thanks,
Miklos

Re: [PATCH v2 7/7] fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs

[Miklos Szeredi]

On Wed, Oct 25, 2017 at 1:44 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> +static inline bool fanotify_is_perm_event(u32 mask)
>> +{
>> +       return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) &&
>> +               mask & FAN_ALL_PERM_EVENTS;
>
> I know this is good w.r.t operation precedence, but it gets me eerie to see
> bit masking without (). maybe its just me.

Yeah, not easy to get used to, but here I don't think it hurts readability.

>> +                       } else
>> +                               FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
>
> Not your change, but if you post another version please add {} after else

Okay.

>
>> +               }
>> +               spin_unlock(&group->notification_lock);
>>
>> -       /*
>> -        * Destroy all non-permission events. For permission events just
>> -        * dequeue them and set the response. They will be freed once the
>> -        * response is consumed and fanotify_get_response() returns.
>> -        */
>> -       while (!fsnotify_notify_queue_is_empty(group)) {
>> -               fsn_event = fsnotify_remove_first_event(group);
>> -               if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) {
>> -                       spin_unlock(&group->notification_lock);
>> -                       fsnotify_destroy_event(group, fsn_event);
>> -                       spin_lock(&group->notification_lock);
>> -               } else
>> -                       FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
>> +               /* Response for all permission events it set, wakeup waiters */
>> +               wake_up(&group->fanotify_data.access_waitq);
>>         }
>> -       spin_unlock(&group->notification_lock);
>> -
>> -       /* Response for all permission events it set, wakeup waiters */
>> -       wake_up(&group->fanotify_data.access_waitq);
>
> So I might as well learn something from this review -
> why did you move wake_up inside spinlock? Does it matter at all?

It would be strange if I did that.  But I didn't, it's just that diff
sometimes doesn't make it easy to read the (non) change.

Thanks,
Miklos

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Miklos Szeredi]

On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> We discovered some problems in the latest fsnotify/fanotify codebase with
> the help of a stress test (Xiong Zhou is working on upstreaming it to
> fstests).
>
> This series attempts to fix these.  With the patch applied the stress test
> passes.
>
> Please review/test.
>
> Changes in v2 (only cosmetic fixes, no functional change):
>  - split first patch into 3 parts to make it more readable
>  - checkpatch fixes
>  - added cleanup patch for fanotify

Pushed v3 (again with just cosmetics) to:

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3

Thanks,
Miklos

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Amir Goldstein]

On Wed, Oct 25, 2017 at 5:31 PM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> We discovered some problems in the latest fsnotify/fanotify codebase with
>> the help of a stress test (Xiong Zhou is working on upstreaming it to
>> fstests).
>>
>> This series attempts to fix these.  With the patch applied the stress test
>> passes.
>>
>> Please review/test.
>>
>> Changes in v2 (only cosmetic fixes, no functional change):
>>  - split first patch into 3 parts to make it more readable
>>  - checkpatch fixes
>>  - added cleanup patch for fanotify
>
> Pushed v3 (again with just cosmetics) to:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
for the series

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Jan Kara]

On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
> On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> > We discovered some problems in the latest fsnotify/fanotify codebase with
> > the help of a stress test (Xiong Zhou is working on upstreaming it to
> > fstests).
> >
> > This series attempts to fix these.  With the patch applied the stress test
> > passes.
> >
> > Please review/test.
> >
> > Changes in v2 (only cosmetic fixes, no functional change):
> >  - split first patch into 3 parts to make it more readable
> >  - checkpatch fixes
> >  - added cleanup patch for fanotify
> 
> Pushed v3 (again with just cosmetics) to:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3

Sorry, I was at OSS Europe this week so I didn't find time to have a close
look. I'll try to check it early next week and pick it up to my tree.
Also thanks Amir for reviewing Miklos' patches!

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

Re: [PATCH v2 2/7] fsnotify: pin both inode and vfsmount mark

[Jan Kara]

On Wed 25-10-17 10:41:34, Miklos Szeredi wrote:
> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when
> dropping the srcu read lock, resulting in use after free at the next
> iteration.
> 
> Solution is to store both marks in iter_info instead of just the one we'll
> be sending the event for.

I'm sorry but I'm not getting it. Where exactly is use-after-free
happening? And how come because if fsnotify_prepare_user_wait() fails to
pin some mark, it does not drop SRCU and bails out, doesn't it?

								Honza
 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 0c4583b61717..48ec61f4c4d5 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -336,6 +336,9 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>  			vfsmount_group = vfsmount_mark->group;
>  		}
>  
> +		iter_info.inode_mark = inode_mark;
> +		iter_info.vfsmount_mark = vfsmount_mark;
> +
>  		if (inode_group && vfsmount_group) {
>  			int cmp = fsnotify_compare_groups(inode_group,
>  							  vfsmount_group);
> @@ -348,9 +351,6 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>  			}
>  		}
>  
> -		iter_info.inode_mark = inode_mark;
> -		iter_info.vfsmount_mark = vfsmount_mark;
> -
>  		ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask,
>  				    data, data_is, cookie, file_name,
>  				    &iter_info);
> -- 
> 2.5.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/7] fsnotify: pin both inode and vfsmount mark

[Miklos Szeredi]

On Mon, Oct 30, 2017 at 2:34 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 25-10-17 10:41:34, Miklos Szeredi wrote:
>> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when
>> dropping the srcu read lock, resulting in use after free at the next
>> iteration.
>>
>> Solution is to store both marks in iter_info instead of just the one we'll
>> be sending the event for.
>
> I'm sorry but I'm not getting it. Where exactly is use-after-free
> happening? And how come because if fsnotify_prepare_user_wait() fails to
> pin some mark, it does not drop SRCU and bails out, doesn't it?

No no. It's the success case, when we do drop SRCU.

Two marks non-NULL marks: inode_mark, vfsmount_mark.  We select one
with fsnotify_compare_groups(), e.g. cmp < 0: vfsmount_mark set to
NULL.

So we pin inode_mark but not vfsmount_mark.  Then we find the next
mark for inode_mark, and use non-NULL vfsmount_node to find the same
vfsmount_mark that we started out previously.  But that one was not
pinned while SRCU was released -> use after free.

Thanks,
Miklos

Re: [PATCH v2 4/7] fsnotify: skip unattached marks

[Jan Kara]

On Wed 25-10-17 10:41:36, Miklos Szeredi wrote:
> After having gone through a ref-unref for the mark, dereferencing the group
> (e.g. in fsnotify_compare_groups()) is wrong since the group may be
> completely gone by that time.  So before continuing to traverse the mark
> list, check if the mark is still attached.

Are you sure this can happen? The thing is: Group reference from mark is
dropped only in fsnotify_final_mark_destroy(). That gets called after SRCU
period is finished from fsnotify_mark_destroy_workfn(). And SRCU period in
which we have dropped our mark reference in fsnotify_finish_user_wait() has
not yet ended. What am I missing?

								Honza

> This is done in the generic case, not just when we go through
> fsnotify_prepare_user_wait()/fsnotify_finish_user_wait(), otherwise it
> would introduce unnecessary complexity.  And it shouldn't hurt to skip
> unattached marks anyway ("flags" is very likely in same cacheline as
> neighbouring "ignored_mask", which is pulled in anyway).
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Fixes: 9385a84d7e1f ("fsnotify: Pass fsnotify_iter_info into handle_event handler")
> Cc: <stable@vger.kernel.org> # v4.12
> ---
>  fs/notify/fsnotify.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 48ec61f4c4d5..0ab6a7179e4d 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -328,12 +328,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>  			inode_mark = hlist_entry(srcu_dereference(inode_node, &fsnotify_mark_srcu),
>  						 struct fsnotify_mark, obj_list);
>  			inode_group = inode_mark->group;
> +			if (!(inode_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
> +				goto skip_inode;
>  		}
>  
>  		if (vfsmount_node) {
>  			vfsmount_mark = hlist_entry(srcu_dereference(vfsmount_node, &fsnotify_mark_srcu),
>  						    struct fsnotify_mark, obj_list);
>  			vfsmount_group = vfsmount_mark->group;
> +			if (!(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
> +				goto skip_vfsmount;
>  		}
>  
>  		iter_info.inode_mark = inode_mark;
> @@ -357,10 +361,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
>  
>  		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
>  			goto out;
> -
> +skip_inode:
>  		if (inode_group)
>  			inode_node = srcu_dereference(inode_node->next,
>  						      &fsnotify_mark_srcu);
> +skip_vfsmount:
>  		if (vfsmount_group)
>  			vfsmount_node = srcu_dereference(vfsmount_node->next,
>  							 &fsnotify_mark_srcu);
> -- 
> 2.5.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/7] fsnotify: pin both inode and vfsmount mark

[Jan Kara]

On Mon 30-10-17 14:42:11, Miklos Szeredi wrote:
> On Mon, Oct 30, 2017 at 2:34 PM, Jan Kara <jack@suse.cz> wrote:
> > On Wed 25-10-17 10:41:34, Miklos Szeredi wrote:
> >> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when
> >> dropping the srcu read lock, resulting in use after free at the next
> >> iteration.
> >>
> >> Solution is to store both marks in iter_info instead of just the one we'll
> >> be sending the event for.
> >
> > I'm sorry but I'm not getting it. Where exactly is use-after-free
> > happening? And how come because if fsnotify_prepare_user_wait() fails to
> > pin some mark, it does not drop SRCU and bails out, doesn't it?
> 
> No no. It's the success case, when we do drop SRCU.
> 
> Two marks non-NULL marks: inode_mark, vfsmount_mark.  We select one
> with fsnotify_compare_groups(), e.g. cmp < 0: vfsmount_mark set to
> NULL.
> 
> So we pin inode_mark but not vfsmount_mark.  Then we find the next
> mark for inode_mark, and use non-NULL vfsmount_node to find the same
> vfsmount_mark that we started out previously.  But that one was not
> pinned while SRCU was released -> use after free.

Got it. Thanks for explanation! But please add a comment that we need to
protect both inode_node and vfsmount_node against freeing so that we can
continue iteration at the place where iter_info.inode_mark and vfsmount_mark
are set.

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

Re: [PATCH v2 4/7] fsnotify: skip unattached marks

[Miklos Szeredi]

On Mon, Oct 30, 2017 at 3:00 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 25-10-17 10:41:36, Miklos Szeredi wrote:
>> After having gone through a ref-unref for the mark, dereferencing the group
>> (e.g. in fsnotify_compare_groups()) is wrong since the group may be
>> completely gone by that time.  So before continuing to traverse the mark
>> list, check if the mark is still attached.
>
> Are you sure this can happen? The thing is: Group reference from mark is
> dropped only in fsnotify_final_mark_destroy(). That gets called after SRCU
> period is finished from fsnotify_mark_destroy_workfn(). And SRCU period in
> which we have dropped our mark reference in fsnotify_finish_user_wait() has
> not yet ended. What am I missing?

Ah, missed that fact that mark holds a ref on the group until it's destroyed.

Yes, the patch is unnecessary in this case.

Thanks,
Miklos

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Miklos Szeredi]

On Fri, Oct 27, 2017 at 1:53 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
>> On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> > We discovered some problems in the latest fsnotify/fanotify codebase with
>> > the help of a stress test (Xiong Zhou is working on upstreaming it to
>> > fstests).
>> >
>> > This series attempts to fix these.  With the patch applied the stress test
>> > passes.
>> >
>> > Please review/test.
>> >
>> > Changes in v2 (only cosmetic fixes, no functional change):
>> >  - split first patch into 3 parts to make it more readable
>> >  - checkpatch fixes
>> >  - added cleanup patch for fanotify
>>
>> Pushed v3 (again with just cosmetics) to:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3

Pushed v4 to:

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v4

Changes from v3:

 - dropped patch skipping unattached marks (group is protected with refcount)
 - added comment about having to pin both marks

Thanks,
Miklos

Re: [PATCH v2 7/7] fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs

[Jan Kara]

On Wed 25-10-17 10:41:39, Miklos Szeredi wrote:
> The only negative from this patch should be an addition of 32bytes to
> 'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not
> defined.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

I like this but some comments below.

> @@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>  
>  static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
>  {
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
>  	struct fanotify_response response = { .fd = -1, .response = -1 };
>  	struct fsnotify_group *group;
>  	int ret;
>  
> +	if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
> +		return -EINVAL;
> +
> +

Two empty lines here look superfluous.

> @@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t
>  		count = ret;
>  
>  	return count;
> -#else
> -	return -EINVAL;
> -#endif
>  }
>  
>  static int fanotify_release(struct inode *ignored, struct file *file)
>  {
>  	struct fsnotify_group *group = file->private_data;
>  
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -	struct fanotify_perm_event_info *event, *next;
> -	struct fsnotify_event *fsn_event;
> +	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
> +		struct fanotify_perm_event_info *event, *next;
> +		struct fsnotify_event *fsn_event;

Rather than doing this, I'd just let fanotify_release() go through the same
path for both CONFIG_FANOTIFY_ACCESS_PERMISSIONS enabled and disabled.
Enabled path won't be much more expensive since access_list will be empty
and we have to walk & destroy events anyway. That way you also don't have
to reindent everything.

> @@ -768,10 +763,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  	if (force_o_largefile())
>  		event_f_flags |= O_LARGEFILE;
>  	group->fanotify_data.f_flags = event_f_flags;
> -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -	init_waitqueue_head(&group->fanotify_data.access_waitq);
> -	INIT_LIST_HEAD(&group->fanotify_data.access_list);
> -#endif
> +	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
> +		init_waitqueue_head(&group->fanotify_data.access_waitq);
> +		INIT_LIST_HEAD(&group->fanotify_data.access_list);
> +	}

When having space for these allocated, just initialize them properly.
Otherwise it's asking for trouble.

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

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Jan Kara]

On Fri 27-10-17 13:53:20, Jan Kara wrote:
> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
> > On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> > > We discovered some problems in the latest fsnotify/fanotify codebase with
> > > the help of a stress test (Xiong Zhou is working on upstreaming it to
> > > fstests).
> > >
> > > This series attempts to fix these.  With the patch applied the stress test
> > > passes.
> > >
> > > Please review/test.
> > >
> > > Changes in v2 (only cosmetic fixes, no functional change):
> > >  - split first patch into 3 parts to make it more readable
> > >  - checkpatch fixes
> > >  - added cleanup patch for fanotify
> > 
> > Pushed v3 (again with just cosmetics) to:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
> 
> Sorry, I was at OSS Europe this week so I didn't find time to have a close
> look. I'll try to check it early next week and pick it up to my tree.
> Also thanks Amir for reviewing Miklos' patches!

So I went through all patches and commented on those where I had some
questions / suggestions. Once those are addressed, I can pick this up to my
tree.

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

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Miklos Szeredi]

On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara <jack@suse.cz> wrote:
> On Fri 27-10-17 13:53:20, Jan Kara wrote:
>> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
>> > On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> > > We discovered some problems in the latest fsnotify/fanotify codebase with
>> > > the help of a stress test (Xiong Zhou is working on upstreaming it to
>> > > fstests).
>> > >
>> > > This series attempts to fix these.  With the patch applied the stress test
>> > > passes.
>> > >
>> > > Please review/test.
>> > >
>> > > Changes in v2 (only cosmetic fixes, no functional change):
>> > >  - split first patch into 3 parts to make it more readable
>> > >  - checkpatch fixes
>> > >  - added cleanup patch for fanotify
>> >
>> > Pushed v3 (again with just cosmetics) to:
>> >
>> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
>>
>> Sorry, I was at OSS Europe this week so I didn't find time to have a close
>> look. I'll try to check it early next week and pick it up to my tree.
>> Also thanks Amir for reviewing Miklos' patches!
>
> So I went through all patches and commented on those where I had some
> questions / suggestions. Once those are addressed, I can pick this up to my
> tree.

Pushed v5 to:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
fsnotify-fixes-v5

Changes from v4:
- address comments for fanotify cleanup patch

Thanks,
Miklos

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Jan Kara]

On Mon 30-10-17 21:18:09, Miklos Szeredi wrote:
> On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara <jack@suse.cz> wrote:
> > On Fri 27-10-17 13:53:20, Jan Kara wrote:
> >> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
> >> > On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> >> > > We discovered some problems in the latest fsnotify/fanotify codebase with
> >> > > the help of a stress test (Xiong Zhou is working on upstreaming it to
> >> > > fstests).
> >> > >
> >> > > This series attempts to fix these.  With the patch applied the stress test
> >> > > passes.
> >> > >
> >> > > Please review/test.
> >> > >
> >> > > Changes in v2 (only cosmetic fixes, no functional change):
> >> > >  - split first patch into 3 parts to make it more readable
> >> > >  - checkpatch fixes
> >> > >  - added cleanup patch for fanotify
> >> >
> >> > Pushed v3 (again with just cosmetics) to:
> >> >
> >> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
> >>
> >> Sorry, I was at OSS Europe this week so I didn't find time to have a close
> >> look. I'll try to check it early next week and pick it up to my tree.
> >> Also thanks Amir for reviewing Miklos' patches!
> >
> > So I went through all patches and commented on those where I had some
> > questions / suggestions. Once those are addressed, I can pick this up to my
> > tree.
> 
> Pushed v5 to:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
> fsnotify-fixes-v5
> 
> Changes from v4:
> - address comments for fanotify cleanup patch

Thanks! I went through all the patches and they look fine to me. Amir, will
you have time to check the final version of patches so that I can add your
Reviewed-by tag?

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

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Amir Goldstein]

On Tue, Oct 31, 2017 at 11:54 AM, Jan Kara <jack@suse.cz> wrote:
> On Mon 30-10-17 21:18:09, Miklos Szeredi wrote:
>> On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara <jack@suse.cz> wrote:
>> > On Fri 27-10-17 13:53:20, Jan Kara wrote:
>> >> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
>> >> > On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> >> > > We discovered some problems in the latest fsnotify/fanotify codebase with
>> >> > > the help of a stress test (Xiong Zhou is working on upstreaming it to
>> >> > > fstests).
>> >> > >
>> >> > > This series attempts to fix these.  With the patch applied the stress test
>> >> > > passes.
>> >> > >
>> >> > > Please review/test.
>> >> > >
>> >> > > Changes in v2 (only cosmetic fixes, no functional change):
>> >> > >  - split first patch into 3 parts to make it more readable
>> >> > >  - checkpatch fixes
>> >> > >  - added cleanup patch for fanotify
>> >> >
>> >> > Pushed v3 (again with just cosmetics) to:
>> >> >
>> >> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
>> >>
>> >> Sorry, I was at OSS Europe this week so I didn't find time to have a close
>> >> look. I'll try to check it early next week and pick it up to my tree.
>> >> Also thanks Amir for reviewing Miklos' patches!
>> >
>> > So I went through all patches and commented on those where I had some
>> > questions / suggestions. Once those are addressed, I can pick this up to my
>> > tree.
>>
>> Pushed v5 to:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
>> fsnotify-fixes-v5
>>
>> Changes from v4:
>> - address comments for fanotify cleanup patch
>
> Thanks! I went through all the patches and they look fine to me. Amir, will
> you have time to check the final version of patches so that I can add your
> Reviewed-by tag?

You can add for the v5 series:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

My only concern is regarding the comment that you requested
("Need to protect both marks against freeing...")
from "pin both inode and vfsmount mark" patch.

IMO its a bit hard for a bypassing reader to understand why the
comment is there and what exactly it refers to.
Then the "cleanup fsnotify()" patch leaves this comment dangling in
a place that makes this even worse.

I suggest to move the comment into the comment above mark iteration
loop and mention iter_info explicitly, something like this:

                  * That's why this traversal is so complicated...
+                * Need to protect both marks against freeing so that we can
+                * continue iteration from this place, so pass both marks on
+                * iter_info to send_to_group(), regardless of which mark
+                * we actually happen to send an event for.
                  */

Amir.

Re: [PATCH v2 0/7] fix fanotify issues with the series in v4.12

[Jan Kara]

On Tue 31-10-17 13:02:21, Amir Goldstein wrote:
> On Tue, Oct 31, 2017 at 11:54 AM, Jan Kara <jack@suse.cz> wrote:
> > On Mon 30-10-17 21:18:09, Miklos Szeredi wrote:
> >> On Mon, Oct 30, 2017 at 6:27 PM, Jan Kara <jack@suse.cz> wrote:
> >> > On Fri 27-10-17 13:53:20, Jan Kara wrote:
> >> >> On Wed 25-10-17 16:31:39, Miklos Szeredi wrote:
> >> >> > On Wed, Oct 25, 2017 at 10:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> >> >> > > We discovered some problems in the latest fsnotify/fanotify codebase with
> >> >> > > the help of a stress test (Xiong Zhou is working on upstreaming it to
> >> >> > > fstests).
> >> >> > >
> >> >> > > This series attempts to fix these.  With the patch applied the stress test
> >> >> > > passes.
> >> >> > >
> >> >> > > Please review/test.
> >> >> > >
> >> >> > > Changes in v2 (only cosmetic fixes, no functional change):
> >> >> > >  - split first patch into 3 parts to make it more readable
> >> >> > >  - checkpatch fixes
> >> >> > >  - added cleanup patch for fanotify
> >> >> >
> >> >> > Pushed v3 (again with just cosmetics) to:
> >> >> >
> >> >> > git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git fsnotify-fixes-v3
> >> >>
> >> >> Sorry, I was at OSS Europe this week so I didn't find time to have a close
> >> >> look. I'll try to check it early next week and pick it up to my tree.
> >> >> Also thanks Amir for reviewing Miklos' patches!
> >> >
> >> > So I went through all patches and commented on those where I had some
> >> > questions / suggestions. Once those are addressed, I can pick this up to my
> >> > tree.
> >>
> >> Pushed v5 to:
> >>
> >>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
> >> fsnotify-fixes-v5
> >>
> >> Changes from v4:
> >> - address comments for fanotify cleanup patch
> >
> > Thanks! I went through all the patches and they look fine to me. Amir, will
> > you have time to check the final version of patches so that I can add your
> > Reviewed-by tag?
> 
> You can add for the v5 series:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> My only concern is regarding the comment that you requested
> ("Need to protect both marks against freeing...")
> from "pin both inode and vfsmount mark" patch.
> 
> IMO its a bit hard for a bypassing reader to understand why the
> comment is there and what exactly it refers to.
> Then the "cleanup fsnotify()" patch leaves this comment dangling in
> a place that makes this even worse.
> 
> I suggest to move the comment into the comment above mark iteration
> loop and mention iter_info explicitly, something like this:
> 
>                   * That's why this traversal is so complicated...
> +                * Need to protect both marks against freeing so that we can
> +                * continue iteration from this place, so pass both marks on
> +                * iter_info to send_to_group(), regardless of which mark
> +                * we actually happen to send an event for.
>                   */

That's a good point. Actually I don't think any comment is needed after the
cleanup as the code is self-explaining then. I'll just delete the comment
as a part of the cleanup patch.

I've pulled Miklos' patches to my tree and will push them to Linus during
the merge window.

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