[RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Amir Goldstein]

Jan,

I thought this would turn out simpler, so you may be able to use it
for your work, but I'm afraid that's not the case.

Anyway, since I am leaving for new year's vacation, I am posting
what I have in case you want to use any of it.

It passed some initial tests I ran, but when I wanted to test the
corner case referred to in patch 1, I found that my test program
hangs open() syscalls with kernel 4.10-rc1 before any of my changes.

This is the mark setup I was testing [1]:
  fanotify_mark(fd, FAN_MARK_ADD,
                FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
                path);
  fanotify_mark(fd, FAN_MARK_ADD | \
                FAN_MARK_IGNORED_SURV_MODIFY | FAN_MARK_IGNORED_MASK
                FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
                FAN_CLOSE_WRITE, AT_FDCWD,
                path);
  fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
                FAN_OPEN_PERM | FAN_CLOSE_WRITE, AT_FDCWD,
                path);

Without FAN_EVENT_ON_CHILD it works fine, but with FAN_EVENT_ON_CHILD,
something bad is going on and I did not have time to look into it.

In general, I would like to start working on an fsnotify testsuite,
so if you have any plans wrt writing extra tests or ideas about specific
missing tests, please let me know about them.

Thanks,
Amir.

[1] https://github.com/amir73il/fsnotify-utils/blob/master/fanotify_example.c

Amir Goldstein (4):
  fsnotify: process inode/vfsmount marks independently
  fsnotify: helper to update marks ignored_mask
  fsnotify: return FSNOTIFY_DROPPED when handle_event() dropped event
  fsnotify: pass single mark to handle_event()

 fs/notify/dnotify/dnotify.c          |   4 +-
 fs/notify/fanotify/fanotify.c        |  44 ++++++---------
 fs/notify/fsnotify.c                 | 102 +++++++++++++++++++++--------------
 fs/notify/inotify/inotify.h          |   2 +-
 fs/notify/inotify/inotify_fsnotify.c |   4 +-
 fs/notify/inotify/inotify_user.c     |   2 +-
 include/linux/fsnotify_backend.h     |  20 ++++++-
 kernel/audit_fsnotify.c              |   2 +-
 kernel/audit_tree.c                  |   2 +-
 kernel/audit_watch.c                 |   2 +-
 10 files changed, 103 insertions(+), 81 deletions(-)

-- 
2.7.4

[RFC][PATCH 1/4] fsnotify: process inode/vfsmount marks independently

[Amir Goldstein]

Re-arrange code in send_to_group() and in fanotify_should_send_event()
so that the code processing inode_mark does not refer directly to
vfsmount_mark and vice versa.

The new code has indetical behavior to old code with one exception -
the corner case of event with FS_EVENT_ON_CHILD bit handled by fanotify
group when both inode_mark and vfsmount_mark are present and when
inode_mark->mask does not have the FS_EVENT_ON_CHILD bit set.

With old code, fanotify_should_send_event() may return true if other
event bits match the marks mask.
With new code, fanotify_should_send_event() will return false.

Normally, this event should not reach fanotify_should_send_event() at all,
because this condition is being tested earlier in __fsnotify_parent().

But even in case the event does reach fanotify_should_send_event(), the
change of behavior actually prevents the same event from being reported
twice to a group on (i.e. with and w/o FS_EVENT_ON_CHILD bit).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 20 ++++++++------------
 fs/notify/fsnotify.c          | 15 ++++++++-------
 2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index bbc175d..fc7658f8 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -92,7 +92,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 				       u32 event_mask,
 				       const void *data, int data_type)
 {
-	__u32 marks_mask, marks_ignored_mask;
+	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	const struct path *path = data;
 
 	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
@@ -108,10 +108,7 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	    !d_can_lookup(path->dentry))
 		return false;
 
-	if (inode_mark && vfsmnt_mark) {
-		marks_mask = (vfsmnt_mark->mask | inode_mark->mask);
-		marks_ignored_mask = (vfsmnt_mark->ignored_mask | inode_mark->ignored_mask);
-	} else if (inode_mark) {
+	if (inode_mark) {
 		/*
 		 * if the event is for a child and this inode doesn't care about
 		 * events on the child, don't send it!
@@ -119,13 +116,12 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 		if ((event_mask & FS_EVENT_ON_CHILD) &&
 		    !(inode_mark->mask & FS_EVENT_ON_CHILD))
 			return false;
-		marks_mask = inode_mark->mask;
-		marks_ignored_mask = inode_mark->ignored_mask;
-	} else if (vfsmnt_mark) {
-		marks_mask = vfsmnt_mark->mask;
-		marks_ignored_mask = vfsmnt_mark->ignored_mask;
-	} else {
-		BUG();
+		marks_mask |= inode_mark->mask;
+		marks_ignored_mask |= inode_mark->ignored_mask;
+	}
+	if (vfsmnt_mark) {
+		marks_mask |= vfsmnt_mark->mask;
+		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
 	}
 
 	if (d_is_dir(path->dentry) &&
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index b41515d..138e066 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -132,6 +132,7 @@ static int send_to_group(struct inode *to_tell,
 	struct fsnotify_group *group = NULL;
 	__u32 inode_test_mask = 0;
 	__u32 vfsmount_test_mask = 0;
+	__u32 ignored_mask = 0;
 
 	if (unlikely(!inode_mark && !vfsmount_mark)) {
 		BUG();
@@ -153,7 +154,8 @@ static int send_to_group(struct inode *to_tell,
 		group = inode_mark->group;
 		inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 		inode_test_mask &= inode_mark->mask;
-		inode_test_mask &= ~inode_mark->ignored_mask;
+		ignored_mask |= inode_mark->ignored_mask;
+		inode_test_mask &= ~ignored_mask;
 	}
 
 	/* does the vfsmount_mark tell us to do something? */
@@ -161,17 +163,16 @@ static int send_to_group(struct inode *to_tell,
 		vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 		group = vfsmount_mark->group;
 		vfsmount_test_mask &= vfsmount_mark->mask;
-		vfsmount_test_mask &= ~vfsmount_mark->ignored_mask;
-		if (inode_mark)
-			vfsmount_test_mask &= ~inode_mark->ignored_mask;
+		ignored_mask |= vfsmount_mark->ignored_mask;
+		vfsmount_test_mask &= ~ignored_mask;
 	}
 
 	pr_debug("%s: group=%p to_tell=%p mask=%x inode_mark=%p"
 		 " inode_test_mask=%x vfsmount_mark=%p vfsmount_test_mask=%x"
-		 " data=%p data_is=%d cookie=%d\n",
+		 " ignored_mask=%x data=%p data_is=%d cookie=%d\n",
 		 __func__, group, to_tell, mask, inode_mark,
-		 inode_test_mask, vfsmount_mark, vfsmount_test_mask, data,
-		 data_is, cookie);
+		 inode_test_mask, vfsmount_mark, vfsmount_test_mask,
+		 ignored_mask, data, data_is, cookie);
 
 	if (!inode_test_mask && !vfsmount_test_mask)
 		return 0;
-- 
2.7.4

[RFC][PATCH 2/4] fsnotify: helper to update marks ignored_mask

[Amir Goldstein]

Factor out a helper update_ignored_mask() to update ignored_mask
of inode and vfsmount marks and calculate their combined mask.

With this change, in should_send_to_group() the inode mark mask
is masked with the combined ignored mask, instead of just its own
ignored mask, just like the vfsmount mark mask.

This behavior is similar to the test in fanotify_should_send_event(),
so it does not have any visible effect.

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

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 138e066..b631df5 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -122,6 +122,30 @@ int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask
 }
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 
+static __u32 update_ignored_mask(struct fsnotify_mark *inode_mark,
+				 struct fsnotify_mark *vfsmount_mark,
+				 __u32 mask)
+{
+	__u32 ignored_mask = 0;
+
+	/* clear ignored on inode modification */
+	if (mask & FS_MODIFY) {
+		if (inode_mark &&
+		    !(inode_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
+			inode_mark->ignored_mask = 0;
+		if (vfsmount_mark &&
+		    !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
+			vfsmount_mark->ignored_mask = 0;
+	}
+
+	if (inode_mark)
+		ignored_mask |= inode_mark->ignored_mask;
+	if (vfsmount_mark)
+		ignored_mask |= vfsmount_mark->ignored_mask;
+
+	return ignored_mask;
+}
+
 static int send_to_group(struct inode *to_tell,
 			 struct fsnotify_mark *inode_mark,
 			 struct fsnotify_mark *vfsmount_mark,
@@ -132,29 +156,20 @@ static int send_to_group(struct inode *to_tell,
 	struct fsnotify_group *group = NULL;
 	__u32 inode_test_mask = 0;
 	__u32 vfsmount_test_mask = 0;
-	__u32 ignored_mask = 0;
+	__u32 ignored_mask;
 
 	if (unlikely(!inode_mark && !vfsmount_mark)) {
 		BUG();
 		return 0;
 	}
 
-	/* clear ignored on inode modification */
-	if (mask & FS_MODIFY) {
-		if (inode_mark &&
-		    !(inode_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-			inode_mark->ignored_mask = 0;
-		if (vfsmount_mark &&
-		    !(vfsmount_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-			vfsmount_mark->ignored_mask = 0;
-	}
+	ignored_mask = update_ignored_mask(inode_mark, vfsmount_mark, mask);
 
 	/* does the inode mark tell us to do something? */
 	if (inode_mark) {
 		group = inode_mark->group;
 		inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 		inode_test_mask &= inode_mark->mask;
-		ignored_mask |= inode_mark->ignored_mask;
 		inode_test_mask &= ~ignored_mask;
 	}
 
@@ -163,7 +178,6 @@ static int send_to_group(struct inode *to_tell,
 		vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
 		group = vfsmount_mark->group;
 		vfsmount_test_mask &= vfsmount_mark->mask;
-		ignored_mask |= vfsmount_mark->ignored_mask;
 		vfsmount_test_mask &= ~ignored_mask;
 	}
 
-- 
2.7.4

[RFC][PATCH 3/4] fsnotify: return FSNOTIFY_DROPPED when handle_event() dropped event

[Amir Goldstein]

The handle_event() op returns a negative value on error and 0 on success -
when event was either queued, merged, delivered (permission) or dropped.

Split the single success 0 value to 2 return values:

FSNOTIFY_DONE (0) indicates that the group has processed the event
(e.g. queued, merged, delivered), which may have also changed group
interenal state.
FSNOTIFY_DROPPED (1) indicates that event was not acted upon by group
nor did it change any group internal state.

This semantic difference matters in case there are several marks of the same
group (i.e. vfsmount_mark and inode_mark) that may process the same event.
If handle_event() with one of the marks returns FSNOTIFY_DONE, the event
won't be sent to the other group marks.
If handle_event() with one of the marks returns FSNOTIFY_DROPPED, the event
will be sent to the other group marks.

This change has no visible effects, because fsnotify() ignores non-negative
return values from handle_event().

Only fanotify_handle_event() and the common should_send_to_group() functions
were modified to return FSNOTIFY_DROPPED, because the rest of the backends
don't support vfsmount marks, so they are not aware of this semantic change.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index fc7658f8..9b26e2f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -72,7 +72,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	/* userspace responded, convert to something usable */
 	switch (event->response) {
 	case FAN_ALLOW:
-		ret = 0;
+		ret = FSNOTIFY_DONE;
 		break;
 	case FAN_DENY:
 	default:
@@ -193,7 +193,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 
 	if (!fanotify_should_send_event(inode_mark, fanotify_mark, mask, data,
 					data_type))
-		return 0;
+		return FSNOTIFY_DROPPED;
 
 	pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode,
 		 mask);
@@ -210,7 +210,7 @@ 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;
+		return FSNOTIFY_DONE;
 	}
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index b631df5..caed3c4 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -189,7 +189,7 @@ static int send_to_group(struct inode *to_tell,
 		 ignored_mask, data, data_is, cookie);
 
 	if (!inode_test_mask && !vfsmount_test_mask)
-		return 0;
+		return FSNOTIFY_DROPPED;
 
 	return group->ops->handle_event(group, to_tell, inode_mark,
 					vfsmount_mark, mask, data, data_is,
@@ -290,7 +290,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 		ret = send_to_group(to_tell, inode_mark, vfsmount_mark, mask,
 				    data, data_is, cookie, file_name);
 
-		if (ret && (mask & ALL_FSNOTIFY_PERM_EVENTS))
+		if (ret < 0 && (mask & ALL_FSNOTIFY_PERM_EVENTS))
 			goto out;
 
 		if (inode_group)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 0cf34d6..cbf9e92 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -81,6 +81,22 @@ struct fsnotify_event_private_data;
 struct fsnotify_fname;
 
 /*
+ * handle_event() may return a negative error value or one of the values below.
+ * FSNOTIFY_DONE indicates that the group has processed the event and that
+ * there is no need for the group to handle the same event again.
+ * FSNOTIFY_DROPPED indicates that event was not acted upon by group nor did
+ * it change any group internal state.
+ * This semantic difference matters in case there are several marks of the same
+ * group (i.e. vfsmount_mark and inode_mark) that may process the same event.
+ * If handle_event() with one of the marks returns FSNOTIFY_DONE, the event
+ * won't be sent to the other group marks.
+ * If handle_event() with one of the marks returns FSNOTIFY_DROPPED, the event
+ * will be sent to the other group marks.
+ */
+#define FSNOTIFY_DONE		0 /* group is done handling this event */
+#define FSNOTIFY_DROPPED	1 /* group did not handle this event */
+
+/*
  * Each group much define these ops.  The fsnotify infrastructure will call
  * these operations for each relevant group.
  *
-- 
2.7.4

[RFC][PATCH 4/4] fsnotify: pass single mark to handle_event()

[Amir Goldstein]

The only reason to pass both inode mark and vfsmount mark to
handle_event() is for masking the inode mark ignored_mask from
the vfsmount mark mask.

In case an event is destined for both inode and vfsmount marks on the
same group, instead of passing both inode and vfsmount mark to
handle_event(), start by passing the event with the inode mark and check
return value from handle_event().

If event was handled by group with inode mark (FSNOTIFY_DONE) then we
don't need to send it again to the same group with the vfsmount mark.

If event was dropped by group with inode mark (FSNOTIFY_DROPPED),
call handle_event() of the same group again with vfsmount mark.

This change gets rid of some excessive code that was needed to deal
with passing the two marks to handle_event().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/dnotify/dnotify.c          |  4 +-
 fs/notify/fanotify/fanotify.c        | 34 ++++++---------
 fs/notify/fsnotify.c                 | 85 +++++++++++++++++++-----------------
 fs/notify/inotify/inotify.h          |  2 +-
 fs/notify/inotify/inotify_fsnotify.c |  4 +-
 fs/notify/inotify/inotify_user.c     |  2 +-
 include/linux/fsnotify_backend.h     |  4 +-
 kernel/audit_fsnotify.c              |  2 +-
 kernel/audit_tree.c                  |  2 +-
 kernel/audit_watch.c                 |  2 +-
 10 files changed, 68 insertions(+), 73 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 5a4ec30..4ad969b 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -84,7 +84,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 static int dnotify_handle_event(struct fsnotify_group *group,
 				struct inode *inode,
 				struct fsnotify_mark *inode_mark,
-				struct fsnotify_mark *vfsmount_mark,
+				u32 unused,
 				u32 mask, const void *data, int data_type,
 				const unsigned char *file_name, u32 cookie)
 {
@@ -98,8 +98,6 @@ static int dnotify_handle_event(struct fsnotify_group *group,
 	if (!S_ISDIR(inode->i_mode))
 		return 0;
 
-	BUG_ON(vfsmount_mark);
-
 	dn_mark = container_of(inode_mark, struct dnotify_mark, fsn_mark);
 
 	spin_lock(&inode_mark->lock);
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 9b26e2f..6a2db65 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -87,17 +87,17 @@ static int fanotify_get_response(struct fsnotify_group *group,
 }
 #endif
 
-static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
-				       struct fsnotify_mark *vfsmnt_mark,
+static bool fanotify_should_send_event(struct fsnotify_mark *mark,
+				       u32 ignored_mask,
 				       u32 event_mask,
 				       const void *data, int data_type)
 {
-	__u32 marks_mask = 0, marks_ignored_mask = 0;
+	__u32 mark_mask = mark->mask;
 	const struct path *path = data;
 
-	pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p"
-		 " data_type=%d\n", __func__, inode_mark, vfsmnt_mark,
-		 event_mask, data, data_type);
+	pr_debug("%s: mark=%p mark_mask=%x ignored_mask=%x event_mask=%x"
+		 " data=%p data_type=%d\n", __func__, mark, mark_mask,
+		 ignored_mask, event_mask, data, data_type);
 
 	/* if we don't have enough info to send an event to userspace say no */
 	if (data_type != FSNOTIFY_EVENT_PATH)
@@ -108,28 +108,22 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,
 	    !d_can_lookup(path->dentry))
 		return false;
 
-	if (inode_mark) {
+	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
 		/*
 		 * if the event is for a child and this inode doesn't care about
 		 * events on the child, don't send it!
 		 */
 		if ((event_mask & FS_EVENT_ON_CHILD) &&
-		    !(inode_mark->mask & FS_EVENT_ON_CHILD))
+		    !(mark_mask & FS_EVENT_ON_CHILD))
 			return false;
-		marks_mask |= inode_mark->mask;
-		marks_ignored_mask |= inode_mark->ignored_mask;
-	}
-	if (vfsmnt_mark) {
-		marks_mask |= vfsmnt_mark->mask;
-		marks_ignored_mask |= vfsmnt_mark->ignored_mask;
 	}
 
 	if (d_is_dir(path->dentry) &&
-	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
+	    !(mark_mask & FS_ISDIR & ~ignored_mask))
 		return false;
 
-	if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask &
-				 ~marks_ignored_mask)
+	if (event_mask & FAN_ALL_OUTGOING_EVENTS & mark_mask &
+				 ~ignored_mask)
 		return true;
 
 	return false;
@@ -171,8 +165,8 @@ init: __maybe_unused
 
 static int fanotify_handle_event(struct fsnotify_group *group,
 				 struct inode *inode,
-				 struct fsnotify_mark *inode_mark,
-				 struct fsnotify_mark *fanotify_mark,
+				 struct fsnotify_mark *mark,
+				 u32 ignored_mask,
 				 u32 mask, const void *data, int data_type,
 				 const unsigned char *file_name, u32 cookie)
 {
@@ -191,7 +185,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	BUILD_BUG_ON(FAN_ACCESS_PERM != FS_ACCESS_PERM);
 	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
 
-	if (!fanotify_should_send_event(inode_mark, fanotify_mark, mask, data,
+	if (!fanotify_should_send_event(mark, ignored_mask, mask, data,
 					data_type))
 		return FSNOTIFY_DROPPED;
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index caed3c4..6af8f81 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -146,56 +146,61 @@ static __u32 update_ignored_mask(struct fsnotify_mark *inode_mark,
 	return ignored_mask;
 }
 
-static int send_to_group(struct inode *to_tell,
-			 struct fsnotify_mark *inode_mark,
-			 struct fsnotify_mark *vfsmount_mark,
-			 __u32 mask, const void *data,
-			 int data_is, u32 cookie,
-			 const unsigned char *file_name)
+static int send_to_group_mark(struct inode *to_tell,
+			      struct fsnotify_mark *mark,
+			      __u32 ignored_mask,
+			      __u32 mask, const void *data,
+			      int data_is, u32 cookie,
+			      const unsigned char *file_name)
 {
-	struct fsnotify_group *group = NULL;
-	__u32 inode_test_mask = 0;
-	__u32 vfsmount_test_mask = 0;
-	__u32 ignored_mask;
-
-	if (unlikely(!inode_mark && !vfsmount_mark)) {
-		BUG();
-		return 0;
-	}
+	struct fsnotify_group *group = mark->group;
+	__u32 test_mask;
 
-	ignored_mask = update_ignored_mask(inode_mark, vfsmount_mark, mask);
-
-	/* does the inode mark tell us to do something? */
-	if (inode_mark) {
-		group = inode_mark->group;
-		inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
-		inode_test_mask &= inode_mark->mask;
-		inode_test_mask &= ~ignored_mask;
-	}
+	/* does the mark tell us to do something? */
+	test_mask = (mask & mark->mask & ~FS_EVENT_ON_CHILD);
 
-	/* does the vfsmount_mark tell us to do something? */
-	if (vfsmount_mark) {
-		vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
-		group = vfsmount_mark->group;
-		vfsmount_test_mask &= vfsmount_mark->mask;
-		vfsmount_test_mask &= ~ignored_mask;
-	}
-
-	pr_debug("%s: group=%p to_tell=%p mask=%x inode_mark=%p"
-		 " inode_test_mask=%x vfsmount_mark=%p vfsmount_test_mask=%x"
-		 " ignored_mask=%x data=%p data_is=%d cookie=%d\n",
-		 __func__, group, to_tell, mask, inode_mark,
-		 inode_test_mask, vfsmount_mark, vfsmount_test_mask,
+	pr_debug("%s: group=%p to_tell=%p mask=%x mark=%p test_mask=%x"
+		 " ignored_mask=%x"" data=%p data_is=%d cookie=%d\n",
+		 __func__, group, to_tell, mask, mark, test_mask,
 		 ignored_mask, data, data_is, cookie);
 
-	if (!inode_test_mask && !vfsmount_test_mask)
+	if (!(test_mask & ~ignored_mask))
 		return FSNOTIFY_DROPPED;
 
-	return group->ops->handle_event(group, to_tell, inode_mark,
-					vfsmount_mark, mask, data, data_is,
+	return group->ops->handle_event(group, to_tell, mark,
+					ignored_mask, mask, data, data_is,
 					file_name, cookie);
 }
 
+static int send_to_group(struct inode *to_tell,
+			 struct fsnotify_mark *inode_mark,
+			 struct fsnotify_mark *vfsmount_mark,
+			 __u32 mask, const void *data,
+			 int data_is, u32 cookie,
+			 const unsigned char *file_name)
+{
+	int ret = FSNOTIFY_DROPPED;
+	__u32 ignored_mask = update_ignored_mask(inode_mark, vfsmount_mark,
+						 mask);
+
+	if (inode_mark)
+		ret = send_to_group_mark(to_tell, inode_mark,
+					 ignored_mask, mask,
+					 data, data_is, cookie, file_name);
+	/*
+	 * If event was dropped by group when handling with inode mark
+	 * (FSNOTIFY_DROPPED), resend to the group with vfsmount mark.
+	 * If event was handled by group with inode mark (FSNOTIFY_DONE)
+	 * or error was returned, then we don't need to send the event
+	 * to the same group again with vfsmount mark.
+	 */
+	if (vfsmount_mark && ret == FSNOTIFY_DROPPED)
+		ret = send_to_group_mark(to_tell, vfsmount_mark,
+					 ignored_mask, mask,
+					 data, data_is, cookie, file_name);
+
+	return ret;
+}
 /*
  * 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
diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index a6f5907..f25ad80 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -25,7 +25,7 @@ extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 extern int inotify_handle_event(struct fsnotify_group *group,
 				struct inode *inode,
 				struct fsnotify_mark *inode_mark,
-				struct fsnotify_mark *vfsmount_mark,
+				u32 unused,
 				u32 mask, const void *data, int data_type,
 				const unsigned char *file_name, u32 cookie);
 
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 19e7ec1..bef7e0d 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -65,7 +65,7 @@ static int inotify_merge(struct list_head *list,
 int inotify_handle_event(struct fsnotify_group *group,
 			 struct inode *inode,
 			 struct fsnotify_mark *inode_mark,
-			 struct fsnotify_mark *vfsmount_mark,
+			 u32 unused,
 			 u32 mask, const void *data, int data_type,
 			 const unsigned char *file_name, u32 cookie)
 {
@@ -76,8 +76,6 @@ int inotify_handle_event(struct fsnotify_group *group,
 	int len = 0;
 	int alloc_len = sizeof(struct inotify_event_info);
 
-	BUG_ON(vfsmount_mark);
-
 	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
 	    (data_type == FSNOTIFY_EVENT_PATH)) {
 		const struct path *path = data;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 69d1ea3..eba9ed3 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -493,7 +493,7 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 	struct inotify_inode_mark *i_mark;
 
 	/* Queue ignore event for the watch */
-	inotify_handle_event(group, NULL, fsn_mark, NULL, FS_IN_IGNORED,
+	inotify_handle_event(group, NULL, fsn_mark, 0, FS_IN_IGNORED,
 			     NULL, FSNOTIFY_EVENT_NONE, NULL, 0);
 
 	i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index cbf9e92..594d6db 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -110,8 +110,8 @@ struct fsnotify_fname;
 struct fsnotify_ops {
 	int (*handle_event)(struct fsnotify_group *group,
 			    struct inode *inode,
-			    struct fsnotify_mark *inode_mark,
-			    struct fsnotify_mark *vfsmount_mark,
+			    struct fsnotify_mark *mark,
+			    u32 ignored_mask,
 			    u32 mask, const void *data, int data_type,
 			    const unsigned char *file_name, u32 cookie);
 	void (*free_group_priv)(struct fsnotify_group *group);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 7ea57e5..6db88ad 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -166,7 +166,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
 static int audit_mark_handle_event(struct fsnotify_group *group,
 				    struct inode *to_tell,
 				    struct fsnotify_mark *inode_mark,
-				    struct fsnotify_mark *vfsmount_mark,
+				    u32 unused,
 				    u32 mask, const void *data, int data_type,
 				    const unsigned char *dname, u32 cookie)
 {
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 8b1dde9..4393553 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -946,7 +946,7 @@ static void evict_chunk(struct audit_chunk *chunk)
 static int audit_tree_handle_event(struct fsnotify_group *group,
 				   struct inode *to_tell,
 				   struct fsnotify_mark *inode_mark,
-				   struct fsnotify_mark *vfsmount_mark,
+				   u32 unused,
 				   u32 mask, const void *data, int data_type,
 				   const unsigned char *file_name, u32 cookie)
 {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f79e465..6c3f811 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -470,7 +470,7 @@ void audit_remove_watch_rule(struct audit_krule *krule)
 static int audit_watch_handle_event(struct fsnotify_group *group,
 				    struct inode *to_tell,
 				    struct fsnotify_mark *inode_mark,
-				    struct fsnotify_mark *vfsmount_mark,
+				    u32 unused,
 				    u32 mask, const void *data, int data_type,
 				    const unsigned char *dname, u32 cookie)
 {
-- 
2.7.4

Re: [RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Jan Kara]

Hi,

On Tue 27-12-16 21:32:24, Amir Goldstein wrote:
> I thought this would turn out simpler, so you may be able to use it
> for your work, but I'm afraid that's not the case.
> 
> Anyway, since I am leaving for new year's vacation, I am posting
> what I have in case you want to use any of it.
> 
> It passed some initial tests I ran, but when I wanted to test the
> corner case referred to in patch 1, I found that my test program
> hangs open() syscalls with kernel 4.10-rc1 before any of my changes.
> 
> This is the mark setup I was testing [1]:
>   fanotify_mark(fd, FAN_MARK_ADD,
>                 FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>                 path);
>   fanotify_mark(fd, FAN_MARK_ADD | \
>                 FAN_MARK_IGNORED_SURV_MODIFY | FAN_MARK_IGNORED_MASK
>                 FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>                 FAN_CLOSE_WRITE, AT_FDCWD,
>                 path);
>   fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
>                 FAN_OPEN_PERM | FAN_CLOSE_WRITE, AT_FDCWD,
>                 path);
> 
> Without FAN_EVENT_ON_CHILD it works fine, but with FAN_EVENT_ON_CHILD,
> something bad is going on and I did not have time to look into it.

I had a look at the patches and the result does not look simpler than what
we had before AFAICT. Sure we don't have to pass both marks into
->handle_event but is that really such a big win? And actually my patches
for dropping SRCU lock when waiting for userspace response to fanotify
permission event need both marks in ->handle_event because they both need
to be protected against freeing when SRCU lock is dropped... So I don't
think this is really viable path.

However one thing that may be worth cleaning up is that
fanotify_should_send_event() needlessly checks the masks - send to group
already did this. So I'd move the check for FS_EVENT_ON_CHILD from
fanotify_should_send_event() to send_to_group() - arguably it belongs there
- and then just completely drop checking of the masks from
fanotify_should_send_event(). What do you think?

> In general, I would like to start working on an fsnotify testsuite,
> so if you have any plans wrt writing extra tests or ideas about specific
> missing tests, please let me know about them.

That would be certainly worthwhile. Actually when I find some useful
testcase I add it to LTP under the
testcases/kernel/syscalls/{fanotify|inotify}. So please extend that if you
have some more ideas for testcases.

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

Re: [RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Amir Goldstein]

On Wed, Jan 4, 2017 at 10:28 AM, Jan Kara <jack@suse.cz> wrote:
> Hi,
>
> On Tue 27-12-16 21:32:24, Amir Goldstein wrote:
>> I thought this would turn out simpler, so you may be able to use it
>> for your work, but I'm afraid that's not the case.
>>
>> Anyway, since I am leaving for new year's vacation, I am posting
>> what I have in case you want to use any of it.
>>
>> It passed some initial tests I ran, but when I wanted to test the
>> corner case referred to in patch 1, I found that my test program
>> hangs open() syscalls with kernel 4.10-rc1 before any of my changes.
>>
>> This is the mark setup I was testing [1]:
>>   fanotify_mark(fd, FAN_MARK_ADD,
>>                 FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>>                 path);
>>   fanotify_mark(fd, FAN_MARK_ADD | \
>>                 FAN_MARK_IGNORED_SURV_MODIFY | FAN_MARK_IGNORED_MASK
>>                 FAN_OPEN_PERM | FAN_EVENT_ON_CHILD, AT_FDCWD,
>>                 FAN_CLOSE_WRITE, AT_FDCWD,
>>                 path);
>>   fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
>>                 FAN_OPEN_PERM | FAN_CLOSE_WRITE, AT_FDCWD,
>>                 path);
>>
>> Without FAN_EVENT_ON_CHILD it works fine, but with FAN_EVENT_ON_CHILD,
>> something bad is going on and I did not have time to look into it.
>
> I had a look at the patches and the result does not look simpler than what
> we had before AFAICT. Sure we don't have to pass both marks into
> ->handle_event but is that really such a big win? And actually my patches
> for dropping SRCU lock when waiting for userspace response to fanotify
> permission event need both marks in ->handle_event because they both need
> to be protected against freeing when SRCU lock is dropped... So I don't
> think this is really viable path.
>

Sure.

> However one thing that may be worth cleaning up is that
> fanotify_should_send_event() needlessly checks the masks - send to group
> already did this. So I'd move the check for FS_EVENT_ON_CHILD from
> fanotify_should_send_event() to send_to_group() - arguably it belongs there
> - and then just completely drop checking of the masks from
> fanotify_should_send_event(). What do you think?
>

Right, so patch [1/4] plus deduplicating the tests in
fanotify_should_send_event().
In principle it makes sense. However, you probably noticed that the logic used
by fanotify_should_send_event() for FS_EVENT_ON_CHILD is different from
the generic logic in send_to_group().

The test in fanotify_should_send_event() is skipped if both inode and vfsmount
marks are present. My patch [1/4] changes this logic, because I thought it was
a bug, but my tests indicate that a bug related to FS_EVENT_ON_CHILD exists
before AND after my change.

So first, I need to isolate and analyse the bug. When I propose a fix, I will
make sure the FS_EVENT_ON_CHILD test ends up only in send_to_group().

>> In general, I would like to start working on an fsnotify testsuite,
>> so if you have any plans wrt writing extra tests or ideas about specific
>> missing tests, please let me know about them.
>
> That would be certainly worthwhile. Actually when I find some useful
> testcase I add it to LTP under the
> testcases/kernel/syscalls/{fanotify|inotify}. So please extend that if you
> have some more ideas for testcases.
>

Yes, I am aware of those testcases.
I find LTP quite heavy to build, so I though I would spin a dedicated
testsuite that will contain your testcases, but will also include
infrastructure for stress testing and profiling.
Keeping track of performance regressions is clearly a major aspect
of maintaining fsnotify.

Amir.

Re: [RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Jan Kara]

On Wed 04-01-17 11:57:04, Amir Goldstein wrote:
> On Wed, Jan 4, 2017 at 10:28 AM, Jan Kara <jack@suse.cz> wrote:
> > However one thing that may be worth cleaning up is that
> > fanotify_should_send_event() needlessly checks the masks - send to group
> > already did this. So I'd move the check for FS_EVENT_ON_CHILD from
> > fanotify_should_send_event() to send_to_group() - arguably it belongs there
> > - and then just completely drop checking of the masks from
> > fanotify_should_send_event(). What do you think?
> >
> 
> Right, so patch [1/4] plus deduplicating the tests in
> fanotify_should_send_event().
> In principle it makes sense. However, you probably noticed that the logic used
> by fanotify_should_send_event() for FS_EVENT_ON_CHILD is different from
> the generic logic in send_to_group().

Yeah, and my head spins when I try to think how the checks in those two
places actually interact - one more reason to check masks only in one place
:).

> The test in fanotify_should_send_event() is skipped if both inode and vfsmount
> marks are present. My patch [1/4] changes this logic, because I thought it was
> a bug, but my tests indicate that a bug related to FS_EVENT_ON_CHILD exists
> before AND after my change.
> 
> So first, I need to isolate and analyse the bug. When I propose a fix, I will
> make sure the FS_EVENT_ON_CHILD test ends up only in send_to_group().

Thanks!

> >> In general, I would like to start working on an fsnotify testsuite,
> >> so if you have any plans wrt writing extra tests or ideas about specific
> >> missing tests, please let me know about them.
> >
> > That would be certainly worthwhile. Actually when I find some useful
> > testcase I add it to LTP under the
> > testcases/kernel/syscalls/{fanotify|inotify}. So please extend that if you
> > have some more ideas for testcases.
> >
> 
> Yes, I am aware of those testcases.
> I find LTP quite heavy to build, so I though I would spin a dedicated
> testsuite that will contain your testcases, but will also include
> infrastructure for stress testing and profiling.
> Keeping track of performance regressions is clearly a major aspect
> of maintaining fsnotify.

Yeah, I don't build full LTP, just testcases in those two directories. The
advantage of LTP is that quite a few people run it so you get a decent test
coverage on different systems (and I've got reports from people running LTP
about regressions in fsnotify code). So I'd prefer to have the functional
tests in LTP if reasonably feasible. But with respect to performance
testing or some crazy stress tests which take long to execute I can definitely
see space for a dedicated test suite.

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

Re: [RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Amir Goldstein]

On Wed, Jan 4, 2017 at 12:39 PM, Jan Kara <jack@suse.cz> wrote:
> On Wed 04-01-17 11:57:04, Amir Goldstein wrote:
...
>> Yes, I am aware of those testcases.
>> I find LTP quite heavy to build, so I though I would spin a dedicated
>> testsuite that will contain your testcases, but will also include
>> infrastructure for stress testing and profiling.
>> Keeping track of performance regressions is clearly a major aspect
>> of maintaining fsnotify.
>
> Yeah, I don't build full LTP, just testcases in those two directories.

LTP seems to have this massive build system around it.
So just to give me a head start, do you use any custom build scripts
to build just those testcases? or simple make inside their directory
will do the trick (it didn't seem so simple from first look).

> The
> advantage of LTP is that quite a few people run it so you get a decent test
> coverage on different systems (and I've got reports from people running LTP
> about regressions in fsnotify code). So I'd prefer to have the functional
> tests in LTP if reasonably feasible.

Sure. That's makes a lot of sense.

> But with respect to performance
> testing or some crazy stress tests which take long to execute I can definitely
> see space for a dedicated test suite.
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [RFC][PATCH 0/4] fsnotify: pass single mark to handle_event()

[Jan Kara]

On Wed 04-01-17 12:45:41, Amir Goldstein wrote:
> On Wed, Jan 4, 2017 at 12:39 PM, Jan Kara <jack@suse.cz> wrote:
> > On Wed 04-01-17 11:57:04, Amir Goldstein wrote:
> ...
> >> Yes, I am aware of those testcases.
> >> I find LTP quite heavy to build, so I though I would spin a dedicated
> >> testsuite that will contain your testcases, but will also include
> >> infrastructure for stress testing and profiling.
> >> Keeping track of performance regressions is clearly a major aspect
> >> of maintaining fsnotify.
> >
> > Yeah, I don't build full LTP, just testcases in those two directories.
> 
> LTP seems to have this massive build system around it.
> So just to give me a head start, do you use any custom build scripts
> to build just those testcases? or simple make inside their directory
> will do the trick (it didn't seem so simple from first look).

I just run make inside the directories I'm interested in...

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