[PATCH v5 00/22] fanotify events with name info

[PATCH v5 00/22] fanotify events with name info

[Amir Goldstein]

Hi Jan,

This patch set implements the FAN_REPORT_NAME and FAN_REPORT_DIR_FID
group flags.

I previously posted v3 of prep patch series [1] and v4 of follow up
series [2].  Since then you pick up several prep patches, so this
posting includes the rest of the prep patches along with the followup
patches with most of your comments addressed.

Regarding the use of flag FS_EVENT_ON_CHILD and the TYPE_CHILD mark
iterator, I did not change that because I was not sure about it and it
is an internal implementation detail that we can change later.
But the discussion about it made me realize that dnotify event handler
wasn't properly adapted, so I added a patch to fix it.

The patches are available on github [3] based on your fsnotify branch.
man-pages [4] LTP tests [5] and a demo [6] are also available.

Thanks,
Amir.

Changes since v4 (and since prep series v3):
- Some prep patches already picked up - the rest are included here
- Parcel variable event info according to your suggestions
- Separate unrelated change from FAN_MOVE_SELF patch
- Add adaptation of dnotify to unified event on parent/child
- Re-structure fsnotify_parent() based on your review comments

[1] https://lore.kernel.org/linux-fsdevel/20200708111156.24659-21-amir73il@gmail.com/
[2] https://lore.kernel.org/linux-fsdevel/20200702125744.10535-1-amir73il@gmail.com/
[3] https://github.com/amir73il/man-pages/commits/fanotify_name_fid-v5
[4] https://github.com/amir73il/linux/commits/fanotify_name_fid
[5] https://github.com/amir73il/ltp/commits/fanotify_name_fid
[6] https://github.com/amir73il/inotify-tools/commits/fanotify_name


*** BLURB HERE ***

Amir Goldstein (22):
  fanotify: generalize the handling of extra event flags
  fanotify: generalize merge logic of events on dir
  fanotify: distinguish between fid encode error and null fid
  fanotify: generalize test for FAN_REPORT_FID
  fanotify: mask out special event flags from ignored mask
  fanotify: prepare for implicit event flags in mark mask
  fanotify: use FAN_EVENT_ON_CHILD as implicit flag on sb/mount/non-dir
    marks
  fsnotify: add object type "child" to object type iterator
  fanotify: use struct fanotify_info to parcel the variable size buffer
  fanotify: no external fh buffer in fanotify_name_event
  dnotify: report both events on parent and child with single callback
  inotify: report both events on parent and child with single callback
  fanotify: report both events on parent and child with single callback
  fsnotify: send event to parent and child with single callback
  fsnotify: send event with parent/name info to sb/mount/non-dir marks
  fsnotify: remove check that source dentry is positive
  fsnotify: send MOVE_SELF event with parent/name info
  fanotify: add basic support for FAN_REPORT_DIR_FID
  fanotify: report events with parent dir fid to sb/mount/non-dir marks
  fanotify: add support for FAN_REPORT_NAME
  fanotify: report parent fid + name + child fid
  fanotify: report parent fid + child fid

 fs/kernfs/file.c                     |  10 +-
 fs/notify/dnotify/dnotify.c          |  42 +++--
 fs/notify/fanotify/fanotify.c        | 273 ++++++++++++++++++++-------
 fs/notify/fanotify/fanotify.h        | 103 ++++++++--
 fs/notify/fanotify/fanotify_user.c   | 192 ++++++++++++++-----
 fs/notify/fsnotify.c                 | 132 +++++++++----
 fs/notify/inotify/inotify_fsnotify.c |  44 ++++-
 include/linux/fanotify.h             |   6 +-
 include/linux/fsnotify.h             |  15 +-
 include/linux/fsnotify_backend.h     |  35 +++-
 include/uapi/linux/fanotify.h        |  15 +-
 11 files changed, 662 insertions(+), 205 deletions(-)

-- 
2.17.1

[PATCH v5 01/22] fanotify: generalize the handling of extra event flags

[Amir Goldstein]

In fanotify_group_event_mask() there is logic in place to make sure we
are not going to handle an event with no type and just FAN_ONDIR flag.
Generalize this logic to any FANOTIFY_EVENT_FLAGS.

There is only one more flag in this group at the moment -
FAN_EVENT_ON_CHILD. We never report it to user, but we do pass it in to
fanotify_alloc_event() when group is reporting fid as indication that
event happened on child. We will have use for this indication later on.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e6ba605732d7..110835a9bf99 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -211,7 +211,8 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 				     int data_type)
 {
 	__u32 marks_mask = 0, marks_ignored_mask = 0;
-	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS;
+	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
+				     FANOTIFY_EVENT_FLAGS;
 	const struct path *path = fsnotify_data_path(data, data_type);
 	struct fsnotify_mark *mark;
 	int type;
@@ -264,14 +265,18 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	 *
 	 * For backward compatibility and consistency, do not report FAN_ONDIR
 	 * to user in legacy fanotify mode (reporting fd) and report FAN_ONDIR
-	 * to user in FAN_REPORT_FID mode for all event types.
+	 * to user in fid mode for all event types.
+	 *
+	 * We never report FAN_EVENT_ON_CHILD to user, but we do pass it in to
+	 * fanotify_alloc_event() when group is reporting fid as indication
+	 * that event happened on child.
 	 */
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
-		/* Do not report FAN_ONDIR without any event */
-		if (!(test_mask & ~FAN_ONDIR))
+		/* Do not report event flags without any event */
+		if (!(test_mask & ~FANOTIFY_EVENT_FLAGS))
 			return 0;
 	} else {
-		user_mask &= ~FAN_ONDIR;
+		user_mask &= ~FANOTIFY_EVENT_FLAGS;
 	}
 
 	return test_mask & user_mask;
-- 
2.17.1

[PATCH v5 02/22] fanotify: generalize merge logic of events on dir

[Amir Goldstein]

An event on directory should never be merged with an event on
non-directory regardless of the event struct type.

This change has no visible effect, because currently, with struct
fanotify_path_event, the relevant events will not be merged because
event path of dir will be different than event path of non-dir.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 110835a9bf99..84c86a45874c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -83,22 +83,22 @@ static bool fanotify_should_merge(struct fsnotify_event *old_fsn,
 	    old->type != new->type || old->pid != new->pid)
 		return false;
 
+	/*
+	 * We want to merge many dirent events in the same dir (i.e.
+	 * creates/unlinks/renames), but we do not want to merge dirent
+	 * events referring to subdirs with dirent events referring to
+	 * non subdirs, otherwise, user won't be able to tell from a
+	 * mask FAN_CREATE|FAN_DELETE|FAN_ONDIR if it describes mkdir+
+	 * unlink pair or rmdir+create pair of events.
+	 */
+	if ((old->mask & FS_ISDIR) != (new->mask & FS_ISDIR))
+		return false;
+
 	switch (old->type) {
 	case FANOTIFY_EVENT_TYPE_PATH:
 		return fanotify_path_equal(fanotify_event_path(old),
 					   fanotify_event_path(new));
 	case FANOTIFY_EVENT_TYPE_FID:
-		/*
-		 * We want to merge many dirent events in the same dir (i.e.
-		 * creates/unlinks/renames), but we do not want to merge dirent
-		 * events referring to subdirs with dirent events referring to
-		 * non subdirs, otherwise, user won't be able to tell from a
-		 * mask FAN_CREATE|FAN_DELETE|FAN_ONDIR if it describes mkdir+
-		 * unlink pair or rmdir+create pair of events.
-		 */
-		if ((old->mask & FS_ISDIR) != (new->mask & FS_ISDIR))
-			return false;
-
 		return fanotify_fid_event_equal(FANOTIFY_FE(old),
 						FANOTIFY_FE(new));
 	case FANOTIFY_EVENT_TYPE_FID_NAME:
-- 
2.17.1

[PATCH v5 03/22] fanotify: distinguish between fid encode error and null fid

[Amir Goldstein]

In fanotify_encode_fh(), both cases of NULL inode and failure to encode
ended up with fh type FILEID_INVALID.

Distiguish the case of NULL inode, by setting fh type to FILEID_ROOT.
This is just a semantic difference at this point.

Remove stale comment and unneeded check from fid event compare helpers.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 84c86a45874c..3dc71a8e795a 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -34,10 +34,6 @@ static bool fanotify_fh_equal(struct fanotify_fh *fh1,
 	if (fh1->type != fh2->type || fh1->len != fh2->len)
 		return false;
 
-	/* Do not merge events if we failed to encode fh */
-	if (fh1->type == FILEID_INVALID)
-		return false;
-
 	return !fh1->len ||
 		!memcmp(fanotify_fh_buf(fh1), fanotify_fh_buf(fh2), fh1->len);
 }
@@ -56,10 +52,7 @@ static bool fanotify_fid_event_equal(struct fanotify_fid_event *ffe1,
 static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
 				      struct fanotify_name_event *fne2)
 {
-	/*
-	 * Do not merge name events without dir fh.
-	 * FAN_DIR_MODIFY does not encode object fh, so it may be empty.
-	 */
+	/* Do not merge name events without dir fh */
 	if (!fne1->dir_fh.len)
 		return false;
 
@@ -290,8 +283,10 @@ static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	void *buf = fh->buf;
 	int err;
 
+	fh->type = FILEID_ROOT;
+	fh->len = 0;
 	if (!inode)
-		goto out;
+		return;
 
 	dwords = 0;
 	err = -ENOENT;
@@ -326,7 +321,6 @@ static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 			    type, bytes, err);
 	kfree(ext_buf);
 	*fanotify_fh_ext_buf_ptr(fh) = NULL;
-out:
 	/* Report the event without a file identifier on encode error */
 	fh->type = FILEID_INVALID;
 	fh->len = 0;
-- 
2.17.1

[PATCH v5 04/22] fanotify: generalize test for FAN_REPORT_FID

[Amir Goldstein]

As preparation to new flags that report fids, define a bit set
of flags for a group reporting fids, currently containing the
only bit FAN_REPORT_FID.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 3dc71a8e795a..b8c04a6f04c5 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -207,13 +207,14 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
 				     FANOTIFY_EVENT_FLAGS;
 	const struct path *path = fsnotify_data_path(data, data_type);
+	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct fsnotify_mark *mark;
 	int type;
 
 	pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
 		 __func__, iter_info->report_mask, event_mask, data, data_type);
 
-	if (!FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	if (!fid_mode) {
 		/* Do we have path to open a file descriptor? */
 		if (!path)
 			return 0;
@@ -264,7 +265,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	 * fanotify_alloc_event() when group is reporting fid as indication
 	 * that event happened on child.
 	 */
-	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	if (fid_mode) {
 		/* Do not report event flags without any event */
 		if (!(test_mask & ~FANOTIFY_EVENT_FLAGS))
 			return 0;
@@ -424,6 +425,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
 	struct inode *id = fanotify_fid_inode(mask, data, data_type, dir);
 	const struct path *path = fsnotify_data_path(data, data_type);
+	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	bool name_event = false;
 
 	/*
@@ -444,7 +446,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		event = fanotify_alloc_perm_event(path, gfp);
 	} else if (name_event && file_name) {
 		event = fanotify_alloc_name_event(id, fsid, file_name, gfp);
-	} else if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	} else if (fid_mode) {
 		event = fanotify_alloc_fid_event(id, fsid, gfp);
 	} else {
 		event = fanotify_alloc_path_event(path, gfp);
@@ -551,7 +553,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 			return 0;
 	}
 
-	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	if (FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS)) {
 		fsid = fanotify_get_fsid(iter_info);
 		/* Racing with mark destruction or creation? */
 		if (!fsid.val[0] && !fsid.val[1])
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c9a824e5c045..1e04caf8d6ba 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -100,7 +100,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
 	if (fsnotify_notify_queue_is_empty(group))
 		goto out;
 
-	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	if (FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS)) {
 		event_size += fanotify_event_info_len(
 			FANOTIFY_E(fsnotify_peek_first_event(group)));
 	}
@@ -882,7 +882,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 		return -EINVAL;
 	}
 
-	if ((flags & FAN_REPORT_FID) &&
+	if ((flags & FANOTIFY_FID_BITS) &&
 	    (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF)
 		return -EINVAL;
 
@@ -1040,7 +1040,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	__kernel_fsid_t __fsid, *fsid = NULL;
 	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
-	unsigned int obj_type;
+	unsigned int obj_type, fid_mode;
 	int ret;
 
 	pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
@@ -1113,9 +1113,9 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	 * inode events are not supported on a mount mark, because they do not
 	 * carry enough information (i.e. path) to be filtered by mount point.
 	 */
+	fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	if (mask & FANOTIFY_INODE_EVENTS &&
-	    (!FAN_GROUP_FLAG(group, FAN_REPORT_FID) ||
-	     mark_type == FAN_MARK_MOUNT))
+	    (!fid_mode || mark_type == FAN_MARK_MOUNT))
 		goto fput_and_out;
 
 	if (flags & FAN_MARK_FLUSH) {
@@ -1140,7 +1140,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 			goto path_put_and_out;
 	}
 
-	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	if (fid_mode) {
 		ret = fanotify_test_fid(&path, &__fsid);
 		if (ret)
 			goto path_put_and_out;
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index b79fa9bb7359..bbbee11d2521 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -18,8 +18,10 @@
 #define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
 				 FAN_CLASS_PRE_CONTENT)
 
-#define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | \
-				 FAN_REPORT_TID | FAN_REPORT_FID | \
+#define FANOTIFY_FID_BITS	(FAN_REPORT_FID)
+
+#define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | FANOTIFY_FID_BITS | \
+				 FAN_REPORT_TID | \
 				 FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 
-- 
2.17.1

[PATCH v5 05/22] fanotify: mask out special event flags from ignored mask

[Amir Goldstein]

The special event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) never had
any meaning in ignored mask. Mask them out explicitly.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 1e04caf8d6ba..6d30beb320f3 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1040,6 +1040,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	__kernel_fsid_t __fsid, *fsid = NULL;
 	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
+	bool ignored = flags & FAN_MARK_IGNORED_MASK;
 	unsigned int obj_type, fid_mode;
 	int ret;
 
@@ -1087,6 +1088,10 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	if (mask & ~valid_mask)
 		return -EINVAL;
 
+	/* Event flags (ONDIR, ON_CHILD) are meaningless in ignored mask */
+	if (ignored)
+		mask &= ~FANOTIFY_EVENT_FLAGS;
+
 	f = fdget(fanotify_fd);
 	if (unlikely(!f.file))
 		return -EBADF;
-- 
2.17.1

[PATCH v5 06/22] fanotify: prepare for implicit event flags in mark mask

[Amir Goldstein]

So far, all flags that can be set in an fanotify mark mask can be set
explicitly by a call to fanotify_mark(2).

Prepare for defining implicit event flags that cannot be set by user with
fanotify_mark(2), similar to how inotify/dnotify implicitly set the
FS_EVENT_ON_CHILD flag.

Implicit event flags cannot be removed by user and mark gets destroyed
when only implicit event flags remain in the mask.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 6d30beb320f3..ab974cd234f7 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -656,12 +656,13 @@ static int fanotify_find_path(int dfd, const char __user *filename,
 }
 
 static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
-					    __u32 mask,
-					    unsigned int flags,
-					    int *destroy)
+					    __u32 mask, unsigned int flags,
+					    __u32 umask, int *destroy)
 {
 	__u32 oldmask = 0;
 
+	/* umask bits cannot be removed by user */
+	mask &= ~umask;
 	spin_lock(&fsn_mark->lock);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
 		oldmask = fsn_mark->mask;
@@ -669,7 +670,13 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 	} else {
 		fsn_mark->ignored_mask &= ~mask;
 	}
-	*destroy = !(fsn_mark->mask | fsn_mark->ignored_mask);
+	/*
+	 * We need to keep the mark around even if remaining mask cannot
+	 * result in any events (e.g. mask == FAN_ONDIR) to support incremenal
+	 * changes to the mask.
+	 * Destroy mark when only umask bits remain.
+	 */
+	*destroy = !((fsn_mark->mask | fsn_mark->ignored_mask) & ~umask);
 	spin_unlock(&fsn_mark->lock);
 
 	return mask & oldmask;
@@ -677,7 +684,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 
 static int fanotify_remove_mark(struct fsnotify_group *group,
 				fsnotify_connp_t *connp, __u32 mask,
-				unsigned int flags)
+				unsigned int flags, __u32 umask)
 {
 	struct fsnotify_mark *fsn_mark = NULL;
 	__u32 removed;
@@ -691,7 +698,7 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
 	}
 
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
-						 &destroy_mark);
+						 umask, &destroy_mark);
 	if (removed & fsnotify_conn_mask(fsn_mark->connector))
 		fsnotify_recalc_mask(fsn_mark->connector);
 	if (destroy_mark)
@@ -707,25 +714,26 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
 
 static int fanotify_remove_vfsmount_mark(struct fsnotify_group *group,
 					 struct vfsmount *mnt, __u32 mask,
-					 unsigned int flags)
+					 unsigned int flags, __u32 umask)
 {
 	return fanotify_remove_mark(group, &real_mount(mnt)->mnt_fsnotify_marks,
-				    mask, flags);
+				    mask, flags, umask);
 }
 
 static int fanotify_remove_sb_mark(struct fsnotify_group *group,
-				      struct super_block *sb, __u32 mask,
-				      unsigned int flags)
+				   struct super_block *sb, __u32 mask,
+				   unsigned int flags, __u32 umask)
 {
-	return fanotify_remove_mark(group, &sb->s_fsnotify_marks, mask, flags);
+	return fanotify_remove_mark(group, &sb->s_fsnotify_marks, mask,
+				    flags, umask);
 }
 
 static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 				      struct inode *inode, __u32 mask,
-				      unsigned int flags)
+				      unsigned int flags, __u32 umask)
 {
 	return fanotify_remove_mark(group, &inode->i_fsnotify_marks, mask,
-				    flags);
+				    flags, umask);
 }
 
 static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
@@ -1175,13 +1183,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	case FAN_MARK_REMOVE:
 		if (mark_type == FAN_MARK_MOUNT)
 			ret = fanotify_remove_vfsmount_mark(group, mnt, mask,
-							    flags);
+							    flags, 0);
 		else if (mark_type == FAN_MARK_FILESYSTEM)
 			ret = fanotify_remove_sb_mark(group, mnt->mnt_sb, mask,
-						      flags);
+						      flags, 0);
 		else
 			ret = fanotify_remove_inode_mark(group, inode, mask,
-							 flags);
+							 flags, 0);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.17.1

[PATCH v5 07/22] fanotify: use FAN_EVENT_ON_CHILD as implicit flag on sb/mount/non-dir marks

[Amir Goldstein]

Up to now, fanotify allowed to set the FAN_EVENT_ON_CHILD flag on
sb/mount marks and non-directory inode mask, but the flag was ignored.

Mask out the flag if it is provided by user on sb/mount/non-dir marks
and define it as an implicit flag that cannot be removed by user.

This flag is going to be used internally to request for events with
parent and name info.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index ab974cd234f7..16d70a8e90f9 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1050,6 +1050,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
 	bool ignored = flags & FAN_MARK_IGNORED_MASK;
 	unsigned int obj_type, fid_mode;
+	u32 umask = 0;
 	int ret;
 
 	pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",
@@ -1167,6 +1168,12 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	else
 		mnt = path.mnt;
 
+	/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount/non-dir marks */
+	if (mnt || !S_ISDIR(inode->i_mode)) {
+		mask &= ~FAN_EVENT_ON_CHILD;
+		umask = FAN_EVENT_ON_CHILD;
+	}
+
 	/* create/update an inode mark */
 	switch (flags & (FAN_MARK_ADD | FAN_MARK_REMOVE)) {
 	case FAN_MARK_ADD:
@@ -1183,13 +1190,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	case FAN_MARK_REMOVE:
 		if (mark_type == FAN_MARK_MOUNT)
 			ret = fanotify_remove_vfsmount_mark(group, mnt, mask,
-							    flags, 0);
+							    flags, umask);
 		else if (mark_type == FAN_MARK_FILESYSTEM)
 			ret = fanotify_remove_sb_mark(group, mnt->mnt_sb, mask,
-						      flags, 0);
+						      flags, umask);
 		else
 			ret = fanotify_remove_inode_mark(group, inode, mask,
-							 flags, 0);
+							 flags, umask);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.17.1

[PATCH v5 08/22] fsnotify: add object type "child" to object type iterator

[Amir Goldstein]

The object type iterator is used to collect all the marks of
a specific group that have interest in an event.

It is used by fanotify to get a single handle_event callback
when an event has a match to either of inode/sb/mount marks
of the group.

The nature of fsnotify events is that they are associated with
at most one sb at most one mount and at most one inode.

When a parent and child are both watching, two events are sent
to backend, one associated to parent inode and one associated
to the child inode.

This results in duplicate events in fanotify, which usually
get merged before user reads them, but this is sub-optimal.

It would be better if the same event is sent to backend with
an object type iterator that has both the child inode and its
parent, and let the backend decide if the event should be reported
once (fanotify) or twice (inotify).

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

diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 860c847c5bfa..2c62628566c5 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -255,6 +255,7 @@ static inline const struct path *fsnotify_data_path(const void *data,
 
 enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_INODE,
+	FSNOTIFY_OBJ_TYPE_CHILD,
 	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
 	FSNOTIFY_OBJ_TYPE_SB,
 	FSNOTIFY_OBJ_TYPE_COUNT,
@@ -262,6 +263,7 @@ enum fsnotify_obj_type {
 };
 
 #define FSNOTIFY_OBJ_TYPE_INODE_FL	(1U << FSNOTIFY_OBJ_TYPE_INODE)
+#define FSNOTIFY_OBJ_TYPE_CHILD_FL	(1U << FSNOTIFY_OBJ_TYPE_CHILD)
 #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 #define FSNOTIFY_OBJ_TYPE_SB_FL		(1U << FSNOTIFY_OBJ_TYPE_SB)
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_COUNT) - 1)
@@ -306,6 +308,7 @@ static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 }
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
+FSNOTIFY_ITER_FUNCS(child, CHILD)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 FSNOTIFY_ITER_FUNCS(sb, SB)
 
-- 
2.17.1

[PATCH v5 09/22] fanotify: use struct fanotify_info to parcel the variable size buffer

[Amir Goldstein]

An fanotify event name is always recorded relative to a dir fh.
Encapsulate the name_len member of fanotify_name_event in a new struct
fanotify_info, which describes the parceling of the variable size
buffer of an fanotify_name_event.

The dir_fh member of fanotify_name_event is renamed to _dir_fh and is not
accessed directly, but via the fanotify_info_dir_fh() accessor.
Although the dir_fh len information is already available in struct
fanotify_fh, we store it also in dif_fh_totlen member of fanotify_info,
including the size of fanotify_fh header, so we know the offset of the
name in the buffer without looking inside the dir_fh.

We also add a file_fh_totlen member to allow packing another file handle
in the variable size buffer after the dir_fh and before the name.
We are going to use that space to store the child fid.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 66 ++++++++++++++++------
 fs/notify/fanotify/fanotify.h      | 91 +++++++++++++++++++++++++-----
 fs/notify/fanotify/fanotify_user.c | 25 ++++----
 3 files changed, 139 insertions(+), 43 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index b8c04a6f04c5..31fd41e91575 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -49,22 +49,44 @@ static bool fanotify_fid_event_equal(struct fanotify_fid_event *ffe1,
 		fanotify_fh_equal(&ffe1->object_fh, &ffe2->object_fh);
 }
 
+static bool fanotify_info_equal(struct fanotify_info *info1,
+				struct fanotify_info *info2)
+{
+	if (info1->dir_fh_totlen != info2->dir_fh_totlen ||
+	    info1->file_fh_totlen != info2->file_fh_totlen ||
+	    info1->name_len != info2->name_len)
+		return false;
+
+	if (info1->dir_fh_totlen &&
+	    !fanotify_fh_equal(fanotify_info_dir_fh(info1),
+			       fanotify_info_dir_fh(info2)))
+		return false;
+
+	if (info1->file_fh_totlen &&
+	    !fanotify_fh_equal(fanotify_info_file_fh(info1),
+			       fanotify_info_file_fh(info2)))
+		return false;
+
+	return !info1->name_len ||
+		!memcmp(fanotify_info_name(info1), fanotify_info_name(info2),
+			info1->name_len);
+}
+
 static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
 				      struct fanotify_name_event *fne2)
 {
-	/* Do not merge name events without dir fh */
-	if (!fne1->dir_fh.len)
-		return false;
+	struct fanotify_info *info1 = &fne1->info;
+	struct fanotify_info *info2 = &fne2->info;
 
-	if (fne1->name_len != fne2->name_len ||
-	    !fanotify_fh_equal(&fne1->dir_fh, &fne2->dir_fh))
+	/* Do not merge name events without dir fh */
+	if (!info1->dir_fh_totlen)
 		return false;
 
-	return !memcmp(fne1->name, fne2->name, fne1->name_len);
+	return fanotify_info_equal(info1, info2);
 }
 
 static bool fanotify_should_merge(struct fsnotify_event *old_fsn,
-			 struct fsnotify_event *new_fsn)
+				  struct fsnotify_event *new_fsn)
 {
 	struct fanotify_event *old, *new;
 
@@ -276,8 +298,14 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	return test_mask & user_mask;
 }
 
-static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
-			       gfp_t gfp)
+/*
+ * Encode fanotify_fh.
+ *
+ * Return total size of encoded fh including fanotify_fh header.
+ * Return 0 on failure to encode.
+ */
+static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
+			      gfp_t gfp)
 {
 	int dwords, type, bytes = 0;
 	char *ext_buf = NULL;
@@ -287,7 +315,7 @@ static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	fh->type = FILEID_ROOT;
 	fh->len = 0;
 	if (!inode)
-		return;
+		return 0;
 
 	dwords = 0;
 	err = -ENOENT;
@@ -315,7 +343,7 @@ static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	fh->type = type;
 	fh->len = bytes;
 
-	return;
+	return FANOTIFY_FH_HDR_LEN + bytes;
 
 out_err:
 	pr_warn_ratelimited("fanotify: failed to encode fid (type=%d, len=%d, err=%i)\n",
@@ -325,6 +353,7 @@ static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	/* Report the event without a file identifier on encode error */
 	fh->type = FILEID_INVALID;
 	fh->len = 0;
+	return 0;
 }
 
 /*
@@ -401,6 +430,8 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 							gfp_t gfp)
 {
 	struct fanotify_name_event *fne;
+	struct fanotify_info *info;
+	struct fanotify_fh *dfh;
 
 	fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp);
 	if (!fne)
@@ -408,9 +439,11 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 
 	fne->fae.type = FANOTIFY_EVENT_TYPE_FID_NAME;
 	fne->fsid = *fsid;
-	fanotify_encode_fh(&fne->dir_fh, id, gfp);
-	fne->name_len = file_name->len;
-	strcpy(fne->name, file_name->name);
+	info = &fne->info;
+	fanotify_info_init(info);
+	dfh = fanotify_info_dir_fh(info);
+	info->dir_fh_totlen = fanotify_encode_fh(dfh, id, gfp);
+	fanotify_info_copy_name(info, file_name);
 
 	return &fne->fae;
 }
@@ -626,9 +659,10 @@ static void fanotify_free_fid_event(struct fanotify_event *event)
 static void fanotify_free_name_event(struct fanotify_event *event)
 {
 	struct fanotify_name_event *fne = FANOTIFY_NE(event);
+	struct fanotify_fh *dfh = fanotify_info_dir_fh(&fne->info);
 
-	if (fanotify_fh_has_ext_buf(&fne->dir_fh))
-		kfree(fanotify_fh_ext_buf(&fne->dir_fh));
+	if (fanotify_fh_has_ext_buf(dfh))
+		kfree(fanotify_fh_ext_buf(dfh));
 	kfree(fne);
 }
 
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 1b2a3bbe6008..5e104fc56abb 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -23,11 +23,29 @@ enum {
  * stored in either the first or last 2 dwords.
  */
 #define FANOTIFY_INLINE_FH_LEN	(3 << 2)
+#define FANOTIFY_FH_HDR_LEN	offsetof(struct fanotify_fh, buf)
 
+/* Fixed size struct for file handle */
 struct fanotify_fh {
-	unsigned char buf[FANOTIFY_INLINE_FH_LEN];
 	u8 type;
 	u8 len;
+	u8 pad[2];
+	unsigned char buf[FANOTIFY_INLINE_FH_LEN];
+} __aligned(4);
+
+/* Variable size struct for dir file handle + child file handle + name */
+struct fanotify_info {
+	/* size of dir_fh/file_fh including fanotify_fh hdr size */
+	u8 dir_fh_totlen;
+	u8 file_fh_totlen;
+	u8 name_len;
+	u8 pad;
+	unsigned char buf[];
+	/*
+	 * (struct fanotify_fh) dir_fh starts at buf[0]
+	 * (optional) file_fh starts at buf[dir_fh_totlen]
+	 * name starts at buf[dir_fh_totlen + file_fh_totlen]
+	 */
 } __aligned(4);
 
 static inline bool fanotify_fh_has_ext_buf(struct fanotify_fh *fh)
@@ -37,6 +55,7 @@ static inline bool fanotify_fh_has_ext_buf(struct fanotify_fh *fh)
 
 static inline char **fanotify_fh_ext_buf_ptr(struct fanotify_fh *fh)
 {
+	BUILD_BUG_ON(FANOTIFY_FH_HDR_LEN % 4);
 	BUILD_BUG_ON(__alignof__(char *) - 4 + sizeof(char *) >
 		     FANOTIFY_INLINE_FH_LEN);
 	return (char **)ALIGN((unsigned long)(fh->buf), __alignof__(char *));
@@ -52,6 +71,56 @@ static inline void *fanotify_fh_buf(struct fanotify_fh *fh)
 	return fanotify_fh_has_ext_buf(fh) ? fanotify_fh_ext_buf(fh) : fh->buf;
 }
 
+static inline int fanotify_info_dir_fh_len(struct fanotify_info *info)
+{
+	if (!info->dir_fh_totlen ||
+	    WARN_ON_ONCE(info->dir_fh_totlen < FANOTIFY_FH_HDR_LEN))
+		return 0;
+
+	return info->dir_fh_totlen - FANOTIFY_FH_HDR_LEN;
+}
+
+static inline struct fanotify_fh *fanotify_info_dir_fh(struct fanotify_info *info)
+{
+	BUILD_BUG_ON(offsetof(struct fanotify_info, buf) % 4);
+
+	return (struct fanotify_fh *)info->buf;
+}
+
+static inline int fanotify_info_file_fh_len(struct fanotify_info *info)
+{
+	if (!info->file_fh_totlen ||
+	    WARN_ON_ONCE(info->file_fh_totlen < FANOTIFY_FH_HDR_LEN))
+		return 0;
+
+	return info->file_fh_totlen - FANOTIFY_FH_HDR_LEN;
+}
+
+static inline struct fanotify_fh *fanotify_info_file_fh(struct fanotify_info *info)
+{
+	return (struct fanotify_fh *)(info->buf + info->dir_fh_totlen);
+}
+
+static inline const char *fanotify_info_name(struct fanotify_info *info)
+{
+	return info->buf + info->dir_fh_totlen + info->file_fh_totlen;
+}
+
+static inline void fanotify_info_init(struct fanotify_info *info)
+{
+	info->dir_fh_totlen = 0;
+	info->file_fh_totlen = 0;
+	info->name_len = 0;
+}
+
+static inline void fanotify_info_copy_name(struct fanotify_info *info,
+					   const struct qstr *name)
+{
+	info->name_len = name->len;
+	strcpy(info->buf + info->dir_fh_totlen + info->file_fh_totlen,
+	       name->name);
+}
+
 /*
  * Common structure for fanotify events. Concrete structs are allocated in
  * fanotify_handle_event() and freed when the information is retrieved by
@@ -96,9 +165,9 @@ FANOTIFY_FE(struct fanotify_event *event)
 struct fanotify_name_event {
 	struct fanotify_event fae;
 	__kernel_fsid_t fsid;
-	struct fanotify_fh dir_fh;
-	u8 name_len;
-	char name[];
+	struct fanotify_info info;
+	/* Reserve space in info.buf[] - access with fanotify_info_dir_fh() */
+	struct fanotify_fh _dir_fh;
 };
 
 static inline struct fanotify_name_event *
@@ -126,11 +195,11 @@ static inline struct fanotify_fh *fanotify_event_object_fh(
 		return NULL;
 }
 
-static inline struct fanotify_fh *fanotify_event_dir_fh(
+static inline struct fanotify_info *fanotify_event_info(
 						struct fanotify_event *event)
 {
 	if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
-		return &FANOTIFY_NE(event)->dir_fh;
+		return &FANOTIFY_NE(event)->info;
 	else
 		return NULL;
 }
@@ -142,15 +211,11 @@ static inline int fanotify_event_object_fh_len(struct fanotify_event *event)
 	return fh ? fh->len : 0;
 }
 
-static inline bool fanotify_event_has_name(struct fanotify_event *event)
+static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)
 {
-	return event->type == FANOTIFY_EVENT_TYPE_FID_NAME;
-}
+	struct fanotify_info *info = fanotify_event_info(event);
 
-static inline int fanotify_event_name_len(struct fanotify_event *event)
-{
-	return fanotify_event_has_name(event) ?
-		FANOTIFY_NE(event)->name_len : 0;
+	return info ? fanotify_info_dir_fh_len(info) : 0;
 }
 
 struct fanotify_path_event {
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 16d70a8e90f9..3842ef00b52e 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -66,19 +66,17 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
 
 static int fanotify_event_info_len(struct fanotify_event *event)
 {
-	int info_len = 0;
+	struct fanotify_info *info = fanotify_event_info(event);
+	int dir_fh_len = fanotify_event_dir_fh_len(event);
 	int fh_len = fanotify_event_object_fh_len(event);
+	int info_len = 0;
+
+	if (dir_fh_len)
+		info_len += fanotify_fid_info_len(dir_fh_len, info->name_len);
 
 	if (fh_len)
 		info_len += fanotify_fid_info_len(fh_len, 0);
 
-	if (fanotify_event_name_len(event)) {
-		struct fanotify_name_event *fne = FANOTIFY_NE(event);
-
-		info_len += fanotify_fid_info_len(fne->dir_fh.len,
-						  fne->name_len);
-	}
-
 	return info_len;
 }
 
@@ -305,6 +303,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 {
 	struct fanotify_event_metadata metadata;
 	struct path *path = fanotify_event_path(event);
+	struct fanotify_info *info = fanotify_event_info(event);
 	struct file *f = NULL;
 	int ret, fd = FAN_NOFD;
 
@@ -346,13 +345,11 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		fd_install(fd, f);
 
 	/* Event info records order is: dir fid + name, child fid */
-	if (fanotify_event_name_len(event)) {
-		struct fanotify_name_event *fne = FANOTIFY_NE(event);
-
+	if (fanotify_event_dir_fh_len(event)) {
 		ret = copy_info_to_user(fanotify_event_fsid(event),
-					fanotify_event_dir_fh(event),
-					fne->name, fne->name_len,
-					buf, count);
+					fanotify_info_dir_fh(info),
+					fanotify_info_name(info),
+					info->name_len, buf, count);
 		if (ret < 0)
 			return ret;
 
-- 
2.17.1

[PATCH v5 10/22] fanotify: no external fh buffer in fanotify_name_event

[Amir Goldstein]

The fanotify_fh struct has an inline buffer of size 12 which is enough
to store the most common local filesystem file handles (e.g. ext4, xfs).
For file handles that do not fit in the inline buffer (e.g. btrfs), an
external buffer is allocated to store the file handle.

When allocating a variable size fanotify_name_event, there is no point
in allocating also an external fh buffer when file handle does not fit
in the inline buffer.

Check required size for encoding fh, preallocate an event buffer
sufficient to contain both file handle and name and store the name after
the file handle.

At this time, when not reporting name in event, we still allocate
the fixed size fanotify_fid_event and an external buffer for large
file handles, but fanotify_alloc_name_event() has already been prepared
to accept a NULL file_name.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 75 ++++++++++++++++++++++++-----------
 fs/notify/fanotify/fanotify.h | 12 +++---
 2 files changed, 59 insertions(+), 28 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 31fd41e91575..c107974d6830 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -298,6 +298,24 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	return test_mask & user_mask;
 }
 
+/*
+ * Check size needed to encode fanotify_fh.
+ *
+ * Return size of encoded fh without fanotify_fh header.
+ * Return 0 on failure to encode.
+ */
+static int fanotify_encode_fh_len(struct inode *inode)
+{
+	int dwords = 0;
+
+	if (!inode)
+		return 0;
+
+	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
+
+	return dwords << 2;
+}
+
 /*
  * Encode fanotify_fh.
  *
@@ -305,49 +323,54 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
  * Return 0 on failure to encode.
  */
 static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
-			      gfp_t gfp)
+			      unsigned int fh_len, gfp_t gfp)
 {
-	int dwords, type, bytes = 0;
+	int dwords, type = 0;
 	char *ext_buf = NULL;
 	void *buf = fh->buf;
 	int err;
 
 	fh->type = FILEID_ROOT;
 	fh->len = 0;
+	fh->flags = 0;
 	if (!inode)
 		return 0;
 
-	dwords = 0;
+	/*
+	 * !gpf means preallocated variable size fh, but fh_len could
+	 * be zero in that case if encoding fh len failed.
+	 */
 	err = -ENOENT;
-	type = exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
-	if (!dwords)
+	if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4))
 		goto out_err;
 
-	bytes = dwords << 2;
-	if (bytes > FANOTIFY_INLINE_FH_LEN) {
-		/* Treat failure to allocate fh as failure to allocate event */
+	/* No external buffer in a variable size allocated fh */
+	if (gfp && fh_len > FANOTIFY_INLINE_FH_LEN) {
+		/* Treat failure to allocate fh as failure to encode fh */
 		err = -ENOMEM;
-		ext_buf = kmalloc(bytes, gfp);
+		ext_buf = kmalloc(fh_len, gfp);
 		if (!ext_buf)
 			goto out_err;
 
 		*fanotify_fh_ext_buf_ptr(fh) = ext_buf;
 		buf = ext_buf;
+		fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
 	}
 
+	dwords = fh_len >> 2;
 	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
 	err = -EINVAL;
-	if (!type || type == FILEID_INVALID || bytes != dwords << 2)
+	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
 		goto out_err;
 
 	fh->type = type;
-	fh->len = bytes;
+	fh->len = fh_len;
 
-	return FANOTIFY_FH_HDR_LEN + bytes;
+	return FANOTIFY_FH_HDR_LEN + fh_len;
 
 out_err:
 	pr_warn_ratelimited("fanotify: failed to encode fid (type=%d, len=%d, err=%i)\n",
-			    type, bytes, err);
+			    type, fh_len, err);
 	kfree(ext_buf);
 	*fanotify_fh_ext_buf_ptr(fh) = NULL;
 	/* Report the event without a file identifier on encode error */
@@ -419,7 +442,8 @@ static struct fanotify_event *fanotify_alloc_fid_event(struct inode *id,
 
 	ffe->fae.type = FANOTIFY_EVENT_TYPE_FID;
 	ffe->fsid = *fsid;
-	fanotify_encode_fh(&ffe->object_fh, id, gfp);
+	fanotify_encode_fh(&ffe->object_fh, id, fanotify_encode_fh_len(id),
+			   gfp);
 
 	return &ffe->fae;
 }
@@ -432,8 +456,13 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 	struct fanotify_name_event *fne;
 	struct fanotify_info *info;
 	struct fanotify_fh *dfh;
+	unsigned int dir_fh_len = fanotify_encode_fh_len(id);
+	unsigned int size;
 
-	fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp);
+	size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len;
+	if (file_name)
+		size += file_name->len + 1;
+	fne = kmalloc(size, gfp);
 	if (!fne)
 		return NULL;
 
@@ -442,8 +471,13 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 	info = &fne->info;
 	fanotify_info_init(info);
 	dfh = fanotify_info_dir_fh(info);
-	info->dir_fh_totlen = fanotify_encode_fh(dfh, id, gfp);
-	fanotify_info_copy_name(info, file_name);
+	info->dir_fh_totlen = fanotify_encode_fh(dfh, id, dir_fh_len, 0);
+	if (file_name)
+		fanotify_info_copy_name(info, file_name);
+
+	pr_debug("%s: ino=%lu size=%u dir_fh_len=%u name_len=%u name='%.*s'\n",
+		 __func__, id->i_ino, size, dir_fh_len,
+		 info->name_len, info->name_len, fanotify_info_name(info));
 
 	return &fne->fae;
 }
@@ -658,12 +692,7 @@ static void fanotify_free_fid_event(struct fanotify_event *event)
 
 static void fanotify_free_name_event(struct fanotify_event *event)
 {
-	struct fanotify_name_event *fne = FANOTIFY_NE(event);
-	struct fanotify_fh *dfh = fanotify_info_dir_fh(&fne->info);
-
-	if (fanotify_fh_has_ext_buf(dfh))
-		kfree(fanotify_fh_ext_buf(dfh));
-	kfree(fne);
+	kfree(FANOTIFY_NE(event));
 }
 
 static void fanotify_free_event(struct fsnotify_event *fsn_event)
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 5e104fc56abb..12c204b1489f 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -29,8 +29,10 @@ enum {
 struct fanotify_fh {
 	u8 type;
 	u8 len;
-	u8 pad[2];
-	unsigned char buf[FANOTIFY_INLINE_FH_LEN];
+#define FANOTIFY_FH_FLAG_EXT_BUF 1
+	u8 flags;
+	u8 pad;
+	unsigned char buf[];
 } __aligned(4);
 
 /* Variable size struct for dir file handle + child file handle + name */
@@ -50,7 +52,7 @@ struct fanotify_info {
 
 static inline bool fanotify_fh_has_ext_buf(struct fanotify_fh *fh)
 {
-	return fh->len > FANOTIFY_INLINE_FH_LEN;
+	return (fh->flags & FANOTIFY_FH_FLAG_EXT_BUF);
 }
 
 static inline char **fanotify_fh_ext_buf_ptr(struct fanotify_fh *fh)
@@ -154,6 +156,8 @@ struct fanotify_fid_event {
 	struct fanotify_event fae;
 	__kernel_fsid_t fsid;
 	struct fanotify_fh object_fh;
+	/* Reserve space in object_fh.buf[] - access with fanotify_fh_buf() */
+	unsigned char _inline_fh_buf[FANOTIFY_INLINE_FH_LEN];
 };
 
 static inline struct fanotify_fid_event *
@@ -166,8 +170,6 @@ struct fanotify_name_event {
 	struct fanotify_event fae;
 	__kernel_fsid_t fsid;
 	struct fanotify_info info;
-	/* Reserve space in info.buf[] - access with fanotify_info_dir_fh() */
-	struct fanotify_fh _dir_fh;
 };
 
 static inline struct fanotify_name_event *
-- 
2.17.1

Re: [PATCH v5 10/22] fanotify: no external fh buffer in fanotify_name_event

[Jan Kara]

On Thu 16-07-20 11:42:18, Amir Goldstein wrote:
> The fanotify_fh struct has an inline buffer of size 12 which is enough
> to store the most common local filesystem file handles (e.g. ext4, xfs).
> For file handles that do not fit in the inline buffer (e.g. btrfs), an
> external buffer is allocated to store the file handle.
> 
> When allocating a variable size fanotify_name_event, there is no point
> in allocating also an external fh buffer when file handle does not fit
> in the inline buffer.
> 
> Check required size for encoding fh, preallocate an event buffer
> sufficient to contain both file handle and name and store the name after
> the file handle.
> 
> At this time, when not reporting name in event, we still allocate
> the fixed size fanotify_fid_event and an external buffer for large
> file handles, but fanotify_alloc_name_event() has already been prepared
> to accept a NULL file_name.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

When reading this, I've got one cleanup idea for later: For FID events, we
could now easily check fh len in fanotify_alloc_fid_event(). If it fits in
inline size, allocate the event from kmem cache, if it does not, allocate
appropriately sized event from kmalloc(). Similarly when freeing event we
could check fh len to determine how to free the event. This way we can
completely get rid of the external buffer code, somewhat simplify all
the fh handling, remove the alignment restrictions on fanotify_fh and
fanotify_info...

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

Re: [PATCH v5 10/22] fanotify: no external fh buffer in fanotify_name_event

[Amir Goldstein]

On Thu, Jul 16, 2020 at 3:44 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 11:42:18, Amir Goldstein wrote:
> > The fanotify_fh struct has an inline buffer of size 12 which is enough
> > to store the most common local filesystem file handles (e.g. ext4, xfs).
> > For file handles that do not fit in the inline buffer (e.g. btrfs), an
> > external buffer is allocated to store the file handle.
> >
> > When allocating a variable size fanotify_name_event, there is no point
> > in allocating also an external fh buffer when file handle does not fit
> > in the inline buffer.
> >
> > Check required size for encoding fh, preallocate an event buffer
> > sufficient to contain both file handle and name and store the name after
> > the file handle.
> >
> > At this time, when not reporting name in event, we still allocate
> > the fixed size fanotify_fid_event and an external buffer for large
> > file handles, but fanotify_alloc_name_event() has already been prepared
> > to accept a NULL file_name.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> When reading this, I've got one cleanup idea for later: For FID events, we
> could now easily check fh len in fanotify_alloc_fid_event(). If it fits in
> inline size, allocate the event from kmem cache, if it does not, allocate
> appropriately sized event from kmalloc(). Similarly when freeing event we
> could check fh len to determine how to free the event. This way we can
> completely get rid of the external buffer code, somewhat simplify all
> the fh handling, remove the alignment restrictions on fanotify_fh and
> fanotify_info...
>

OK. Note that the f_handle buffer passed to filesystems is expected to be
aligned anyway, although for encoding it may be less important, see:

cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory")

Thanks,
Amir.

[PATCH v5 11/22] dnotify: report both events on parent and child with single callback

[Amir Goldstein]

For some events (e.g. DN_ATTRIB on sub-directory) fsnotify may call
dnotify_handle_event() once for watching parent and once again for
the watching sub-directory.

Do the same thing with a single callback instead of two callbacks when
marks iterator contains both inode and child entries.

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

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 608c3e70e81f..305e5559560a 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -70,26 +70,15 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
  * destroy the dnotify struct if it was not registered to receive multiple
  * events.
  */
-static int dnotify_handle_event(struct fsnotify_group *group, u32 mask,
-				const void *data, int data_type,
-				struct inode *dir,
-				const struct qstr *file_name, u32 cookie,
-				struct fsnotify_iter_info *iter_info)
+static void dnotify_one_event(struct fsnotify_group *group, u32 mask,
+			      struct fsnotify_mark *inode_mark)
 {
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct dnotify_mark *dn_mark;
 	struct dnotify_struct *dn;
 	struct dnotify_struct **prev;
 	struct fown_struct *fown;
 	__u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
 
-	/* not a dir, dnotify doesn't care */
-	if (!dir)
-		return 0;
-
-	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
-		return 0;
-
 	dn_mark = container_of(inode_mark, struct dnotify_mark, fsn_mark);
 
 	spin_lock(&inode_mark->lock);
@@ -111,6 +100,33 @@ static int dnotify_handle_event(struct fsnotify_group *group, u32 mask,
 	}
 
 	spin_unlock(&inode_mark->lock);
+}
+
+static int dnotify_handle_event(struct fsnotify_group *group, u32 mask,
+				const void *data, int data_type,
+				struct inode *dir,
+				const struct qstr *file_name, u32 cookie,
+				struct fsnotify_iter_info *iter_info)
+{
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
+	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
+
+	/* not a dir, dnotify doesn't care */
+	if (!dir)
+		return 0;
+
+	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
+		return 0;
+
+	/*
+	 * Some events can be sent on both parent dir and subdir marks
+	 * (e.g. DN_ATTRIB).  If both parent dir and subdir are watching,
+	 * report the event once to parent dir and once to subdir.
+	 */
+	if (inode_mark)
+		dnotify_one_event(group, mask, inode_mark);
+	if (child_mark)
+		dnotify_one_event(group, mask, child_mark);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v5 12/22] inotify: report both events on parent and child with single callback

[Amir Goldstein]

fsnotify usually calls inotify_handle_event() once for watching parent
to report event with child's name and once for watching child to report
event without child's name.

Do the same thing with a single callback instead of two callbacks when
marks iterator contains both inode and child entries.

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

diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index dfd455798a1b..a65cf8c9f600 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -55,13 +55,11 @@ static int inotify_merge(struct list_head *list,
 	return event_compare(last_event, event);
 }
 
-int inotify_handle_event(struct fsnotify_group *group, u32 mask,
-			 const void *data, int data_type, struct inode *dir,
-			 const struct qstr *file_name, u32 cookie,
-			 struct fsnotify_iter_info *iter_info)
+static int inotify_one_event(struct fsnotify_group *group, u32 mask,
+			     struct fsnotify_mark *inode_mark,
+			     const struct path *path,
+			     const struct qstr *file_name, u32 cookie)
 {
-	const struct path *path = fsnotify_data_path(data, data_type);
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct inotify_inode_mark *i_mark;
 	struct inotify_event_info *event;
 	struct fsnotify_event *fsn_event;
@@ -69,9 +67,6 @@ int inotify_handle_event(struct fsnotify_group *group, u32 mask,
 	int len = 0;
 	int alloc_len = sizeof(struct inotify_event_info);
 
-	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
-		return 0;
-
 	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
 	    path && d_unlinked(path->dentry))
 		return 0;
@@ -135,6 +130,37 @@ int inotify_handle_event(struct fsnotify_group *group, u32 mask,
 	return 0;
 }
 
+int inotify_handle_event(struct fsnotify_group *group, u32 mask,
+			 const void *data, int data_type, struct inode *dir,
+			 const struct qstr *file_name, u32 cookie,
+			 struct fsnotify_iter_info *iter_info)
+{
+	const struct path *path = fsnotify_data_path(data, data_type);
+	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
+	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
+	int ret = 0;
+
+	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
+		return 0;
+
+	/*
+	 * Some events cannot be sent on both parent and child marks
+	 * (e.g. IN_CREATE).  Those events are always sent on inode_mark.
+	 * For events that are possible on both parent and child (e.g. IN_OPEN),
+	 * event is sent on inode_mark with name if the parent is watching and
+	 * is sent on child_mark without name if child is watching.
+	 * If both parent and child are watching, report the event with child's
+	 * name here and report another event without child's name below.
+	 */
+	if (inode_mark)
+		ret = inotify_one_event(group, mask, inode_mark, path,
+					file_name, cookie);
+	if (ret || !child_mark)
+		return ret;
+
+	return inotify_one_event(group, mask, child_mark, path, NULL, 0);
+}
+
 static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group)
 {
 	inotify_ignored_and_remove_idr(fsn_mark, group);
-- 
2.17.1

Re: [PATCH v5 12/22] inotify: report both events on parent and child with single callback

[Jan Kara]

On Thu 16-07-20 11:42:20, Amir Goldstein wrote:
> fsnotify usually calls inotify_handle_event() once for watching parent
> to report event with child's name and once for watching child to report
> event without child's name.
> 
> Do the same thing with a single callback instead of two callbacks when
> marks iterator contains both inode and child entries.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Another idea for possible future cleanup here: Everybody except for
fanotify cares only about inode marks and reporting both parent and child
only complicates things for them (and I can imagine bugs being created by
in-kernel fsnotify users because they misunderstand inode-vs-child mark
types etc.). So maybe we can create another fsnotify_group operation
similar to ->handle_event but with simpler signature for these simple
notification handlers and send_to_group() will take care of translating 
the complex fsnotify() call into a sequence of these simple callbacks.

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

Re: [PATCH v5 12/22] inotify: report both events on parent and child with single callback

[Amir Goldstein]

On Thu, Jul 16, 2020 at 3:52 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 11:42:20, Amir Goldstein wrote:
> > fsnotify usually calls inotify_handle_event() once for watching parent
> > to report event with child's name and once for watching child to report
> > event without child's name.
> >
> > Do the same thing with a single callback instead of two callbacks when
> > marks iterator contains both inode and child entries.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Another idea for possible future cleanup here: Everybody except for
> fanotify cares only about inode marks and reporting both parent and child
> only complicates things for them (and I can imagine bugs being created by
> in-kernel fsnotify users because they misunderstand inode-vs-child mark
> types etc.). So maybe we can create another fsnotify_group operation
> similar to ->handle_event but with simpler signature for these simple
> notification handlers and send_to_group() will take care of translating
> the complex fsnotify() call into a sequence of these simple callbacks.
>

Yeh we could do that.
But then it's not every day that a new in-kernel fsnotify_group is added...

Thanks,
Amir.

Re: [PATCH v5 12/22] inotify: report both events on parent and child with single callback

[Jan Kara]

On Thu 16-07-20 17:25:27, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 3:52 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-07-20 11:42:20, Amir Goldstein wrote:
> > > fsnotify usually calls inotify_handle_event() once for watching parent
> > > to report event with child's name and once for watching child to report
> > > event without child's name.
> > >
> > > Do the same thing with a single callback instead of two callbacks when
> > > marks iterator contains both inode and child entries.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Another idea for possible future cleanup here: Everybody except for
> > fanotify cares only about inode marks and reporting both parent and child
> > only complicates things for them (and I can imagine bugs being created by
> > in-kernel fsnotify users because they misunderstand inode-vs-child mark
> > types etc.). So maybe we can create another fsnotify_group operation
> > similar to ->handle_event but with simpler signature for these simple
> > notification handlers and send_to_group() will take care of translating
> > the complex fsnotify() call into a sequence of these simple callbacks.
> >
> 
> Yeh we could do that.
> But then it's not every day that a new in-kernel fsnotify_group is added...

Definitely. But then we often do not notice when it is added (to review the
usage) or when e.g. audit decides to tweak its event mask and things
suddently subtly break for it...

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

[PATCH v5 13/22] fanotify: report both events on parent and child with single callback

[Amir Goldstein]

fsnotify usually calls fanotify_handle_event() once for watching parent
and once for watching child, even though both events are exactly the
same and will most likely get merged before user reads them.

Add support for handling both event flavors with a single callback
instead of two callbacks when marks iterator contains both inode and
child entries.

fanotify will queue a single event in that case and the unneeded merge
will be avoided.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index c107974d6830..1ec760960c93 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -263,8 +263,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		/*
 		 * If the event is for a child and this mark doesn't care about
 		 * events on a child, don't send it!
+		 * The special object type "child" always cares about events on
+		 * a child, because it refers to the child inode itself.
 		 */
 		if (event_mask & FS_EVENT_ON_CHILD &&
+		    type != FSNOTIFY_OBJ_TYPE_CHILD &&
 		    (type != FSNOTIFY_OBJ_TYPE_INODE ||
 		     !(mark->mask & FS_EVENT_ON_CHILD)))
 			continue;
-- 
2.17.1

[PATCH v5 14/22] fsnotify: send event to parent and child with single callback

[Amir Goldstein]

Instead of calling fsnotify() twice, once with parent inode and once
with child inode, if event should be sent to parent inode, send it
with both parent and child inodes marks in object type iterator and call
the backend handle_event() callback only once.

The parent inode is assigned to the standard "inode" iterator type and
the child inode is assigned to the special "child" iterator type.

In that case, the bit FS_EVENT_ON_CHILD will be set in the event mask,
the dir argment to handle_event will be the parent inode, the file_name
argument to handle_event is non NULL and refers to the name of the child
and the child inode can be accessed with fsnotify_data_inode().

This will allow fanotify to make decisions based on child or parent's
ignored mask.  For example, when a parent is interested in a specific
event on its children, but a specific child wishes to ignore this event,
the event will not be reported.  This is not what happens with current
code, but according to man page, it is the expected behavior.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/kernfs/file.c     | 10 +++++---
 fs/notify/fsnotify.c | 60 ++++++++++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index e23b3f62483c..5b1468bc509e 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -883,6 +883,7 @@ static void kernfs_notify_workfn(struct work_struct *work)
 
 	list_for_each_entry(info, &kernfs_root(kn)->supers, node) {
 		struct kernfs_node *parent;
+		struct inode *p_inode = NULL;
 		struct inode *inode;
 		struct qstr name;
 
@@ -899,8 +900,6 @@ static void kernfs_notify_workfn(struct work_struct *work)
 		name = (struct qstr)QSTR_INIT(kn->name, strlen(kn->name));
 		parent = kernfs_get_parent(kn);
 		if (parent) {
-			struct inode *p_inode;
-
 			p_inode = ilookup(info->sb, kernfs_ino(parent));
 			if (p_inode) {
 				fsnotify(p_inode, FS_MODIFY | FS_EVENT_ON_CHILD,
@@ -911,8 +910,11 @@ static void kernfs_notify_workfn(struct work_struct *work)
 			kernfs_put(parent);
 		}
 
-		fsnotify(inode, FS_MODIFY, inode, FSNOTIFY_EVENT_INODE,
-			 NULL, 0);
+		if (!p_inode) {
+			fsnotify(inode, FS_MODIFY, inode, FSNOTIFY_EVENT_INODE,
+				 NULL, 0);
+		}
+
 		iput(inode);
 	}
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 51ada3cfd2ff..7120c675e9a6 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -145,17 +145,21 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
 /*
  * Notify this dentry's parent about a child's events with child name info
  * if parent is watching.
- * Notify also the child without name info if child inode is watching.
+ * Notify only the child without name info if parent is not watching.
  */
 int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 		      int data_type)
 {
+	struct inode *inode = d_inode(dentry);
 	struct dentry *parent;
 	struct inode *p_inode;
+	struct name_snapshot name;
+	struct qstr *file_name = NULL;
 	int ret = 0;
 
+	parent = NULL;
 	if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
-		goto notify_child;
+		goto notify;
 
 	parent = dget_parent(dentry);
 	p_inode = parent->d_inode;
@@ -163,25 +167,24 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 	if (unlikely(!fsnotify_inode_watches_children(p_inode))) {
 		__fsnotify_update_child_dentry_flags(p_inode);
 	} else if (p_inode->i_fsnotify_mask & mask & ALL_FSNOTIFY_EVENTS) {
-		struct name_snapshot name;
+		/* When notifying parent, child should be passed as data */
+		WARN_ON_ONCE(inode != fsnotify_data_inode(data, data_type));
 
-		/*
-		 * We are notifying a parent, so set a flag in mask to inform
-		 * backend that event has information about a child entry.
-		 */
+		/* Notify both parent and child with child name info */
+		inode = p_inode;
 		take_dentry_name_snapshot(&name, dentry);
-		ret = fsnotify(p_inode, mask | FS_EVENT_ON_CHILD, data,
-			       data_type, &name.name, 0);
-		release_dentry_name_snapshot(&name);
+		file_name = &name.name;
+		mask |= FS_EVENT_ON_CHILD;
 	}
 
-	dput(parent);
+notify:
+	ret = fsnotify(inode, mask, data, data_type, file_name, 0);
 
-	if (ret)
-		return ret;
+	if (file_name)
+		release_dentry_name_snapshot(&name);
+	dput(parent);
 
-notify_child:
-	return fsnotify(d_inode(dentry), mask, data, data_type, NULL, 0);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 
@@ -322,12 +325,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	struct super_block *sb = to_tell->i_sb;
 	struct inode *dir = S_ISDIR(to_tell->i_mode) ? to_tell : NULL;
 	struct mount *mnt = NULL;
+	struct inode *child = NULL;
 	int ret = 0;
 	__u32 test_mask, marks_mask;
 
 	if (path)
 		mnt = real_mount(path->mnt);
 
+	if (mask & FS_EVENT_ON_CHILD)
+		child = fsnotify_data_inode(data, data_type);
+
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
 	 * be expensive.  It protects walking the *_fsnotify_marks lists.
@@ -336,21 +343,20 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	 * need SRCU to keep them "alive".
 	 */
 	if (!to_tell->i_fsnotify_marks && !sb->s_fsnotify_marks &&
-	    (!mnt || !mnt->mnt_fsnotify_marks))
+	    (!mnt || !mnt->mnt_fsnotify_marks) &&
+	    (!child || !child->i_fsnotify_marks))
 		return 0;
 
-	/* An event "on child" is not intended for a mount/sb mark */
-	marks_mask = to_tell->i_fsnotify_mask;
-	if (!(mask & FS_EVENT_ON_CHILD)) {
-		marks_mask |= sb->s_fsnotify_mask;
-		if (mnt)
-			marks_mask |= mnt->mnt_fsnotify_mask;
-	}
+	marks_mask = to_tell->i_fsnotify_mask | sb->s_fsnotify_mask;
+	if (mnt)
+		marks_mask |= mnt->mnt_fsnotify_mask;
+	if (child)
+		marks_mask |= child->i_fsnotify_mask;
+
 
 	/*
 	 * if this is a modify event we may need to clear the ignored masks
-	 * otherwise return if neither the inode nor the vfsmount/sb care about
-	 * this type of event.
+	 * otherwise return if none of the marks care about this type of event.
 	 */
 	test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 	if (!(mask & FS_MODIFY) && !(test_mask & marks_mask))
@@ -366,6 +372,10 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
 			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
 	}
+	if (child) {
+		iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] =
+			fsnotify_first_mark(&child->i_fsnotify_marks);
+	}
 
 	/*
 	 * We need to merge inode/vfsmount/sb mark lists so that e.g. inode mark
-- 
2.17.1

[PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Amir Goldstein]

Similar to events "on child" to watching directory, send event "on child"
with parent/name info if sb/mount/non-dir marks are interested in
parent/name info.

The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
interest in parent/name info for events on non-directory inodes.

Events on "orphan" children (disconnected dentries) are sent without
parent/name info.

Events on direcories are send with parent/name info only if the parent
directory is watching.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             | 54 ++++++++++++++++++++++++++++----
 include/linux/fsnotify.h         | 10 ++++--
 include/linux/fsnotify_backend.h | 32 ++++++++++++++++---
 3 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 7120c675e9a6..efa5c1c4908a 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -142,31 +142,73 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
 	spin_unlock(&inode->i_lock);
 }
 
+/* Are inode/sb/mount interested in parent and name info with this event? */
+static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt,
+					__u32 mask)
+{
+	__u32 marks_mask = 0;
+
+	/* We only send parent/name to inode/sb/mount for events on non-dir */
+	if (mask & FS_ISDIR)
+		return false;
+
+	/* Did either inode/sb/mount subscribe for events with parent/name? */
+	marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
+	marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
+	if (mnt)
+		marks_mask |= fsnotify_parent_needed_mask(mnt->mnt_fsnotify_mask);
+
+	/* Did they subscribe for this event with parent/name info? */
+	return mask & marks_mask;
+}
+
 /*
  * Notify this dentry's parent about a child's events with child name info
- * if parent is watching.
- * Notify only the child without name info if parent is not watching.
+ * if parent is watching or if inode/sb/mount are interested in events with
+ * parent and name info.
+ *
+ * Notify only the child without name info if parent is not watching and
+ * inode/sb/mount are not interested in events with parent and name info.
  */
 int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 		      int data_type)
 {
+	const struct path *path = fsnotify_data_path(data, data_type);
+	struct mount *mnt = path ? real_mount(path->mnt) : NULL;
 	struct inode *inode = d_inode(dentry);
 	struct dentry *parent;
+	bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED;
+	__u32 p_mask;
 	struct inode *p_inode;
 	struct name_snapshot name;
 	struct qstr *file_name = NULL;
 	int ret = 0;
 
+	/*
+	 * Do inode/sb/mount care about parent and name info on non-dir?
+	 * Do they care about any event at all?
+	 */
+	if (!inode->i_fsnotify_marks && !inode->i_sb->s_fsnotify_marks &&
+	    (!mnt || !mnt->mnt_fsnotify_marks) && !parent_watched)
+		return 0;
+
 	parent = NULL;
-	if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
+	if (!parent_watched && !fsnotify_event_needs_parent(inode, mnt, mask))
 		goto notify;
 
+	/* Does parent inode care about events on children? */
 	parent = dget_parent(dentry);
 	p_inode = parent->d_inode;
-
-	if (unlikely(!fsnotify_inode_watches_children(p_inode))) {
+	p_mask = fsnotify_inode_watches_children(p_inode);
+	if (unlikely(parent_watched && !p_mask))
 		__fsnotify_update_child_dentry_flags(p_inode);
-	} else if (p_inode->i_fsnotify_mask & mask & ALL_FSNOTIFY_EVENTS) {
+
+	/*
+	 * Include parent/name in notification either if some notification
+	 * groups require parent info (!parent_watched case) or the parent is
+	 * interested in this event.
+	 */
+	if (!parent_watched || (mask & p_mask & ALL_FSNOTIFY_EVENTS)) {
 		/* When notifying parent, child should be passed as data */
 		WARN_ON_ONCE(inode != fsnotify_data_inode(data, data_type));
 
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 9b2566d273a9..044cae3a0628 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -44,10 +44,16 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
 {
 	struct inode *inode = d_inode(dentry);
 
-	if (S_ISDIR(inode->i_mode))
+	if (S_ISDIR(inode->i_mode)) {
 		mask |= FS_ISDIR;
 
-	if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
+		/* sb/mount marks are not interested in name of directory */
+		if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
+			goto notify_child;
+	}
+
+	/* disconnected dentry cannot notify parent */
+	if (IS_ROOT(dentry))
 		goto notify_child;
 
 	return __fsnotify_parent(dentry, mask, data, data_type);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 2c62628566c5..6f0df110e9f8 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -49,8 +49,11 @@
 #define FS_OPEN_EXEC_PERM	0x00040000	/* open/exec event in a permission hook */
 
 #define FS_EXCL_UNLINK		0x04000000	/* do not send events if object is unlinked */
-/* This inode cares about things that happen to its children.  Always set for
- * dnotify and inotify. */
+/*
+ * Set on inode mark that cares about things that happen to its children.
+ * Always set for dnotify and inotify.
+ * Set on inode/sb/mount marks that care about parenet/name info.
+ */
 #define FS_EVENT_ON_CHILD	0x08000000
 
 #define FS_DN_RENAME		0x10000000	/* file renamed */
@@ -72,14 +75,22 @@
 				  FS_OPEN_EXEC_PERM)
 
 /*
- * This is a list of all events that may get sent to a parent based on fs event
- * happening to inodes inside that directory.
+ * This is a list of all events that may get sent to a parent that is watching
+ * with flag FS_EVENT_ON_CHILD based on fs event on a child of that directory.
  */
 #define FS_EVENTS_POSS_ON_CHILD   (ALL_FSNOTIFY_PERM_EVENTS | \
 				   FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
 				   FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | \
 				   FS_OPEN | FS_OPEN_EXEC)
 
+/*
+ * This is a list of all events that may get sent with the parent inode as the
+ * @to_tell argument of fsnotify().
+ * It may include events that can be sent to an inode/sb/mount mark, but cannot
+ * be sent to a parent watching children.
+ */
+#define FS_EVENTS_POSS_TO_PARENT (FS_EVENTS_POSS_ON_CHILD)
+
 /* Events that can be reported to backends */
 #define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
 			     FS_EVENTS_POSS_ON_CHILD | \
@@ -398,6 +409,19 @@ extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt);
 extern void fsnotify_sb_delete(struct super_block *sb);
 extern u32 fsnotify_get_cookie(void);
 
+static inline __u32 fsnotify_parent_needed_mask(__u32 mask)
+{
+	/* FS_EVENT_ON_CHILD is set on marks that want parent/name info */
+	if (!(mask & FS_EVENT_ON_CHILD))
+		return 0;
+	/*
+	 * This object might be watched by a mark that cares about parent/name
+	 * info, does it care about the specific set of events that can be
+	 * reported with parent/name info?
+	 */
+	return mask & FS_EVENTS_POSS_TO_PARENT;
+}
+
 static inline int fsnotify_inode_watches_children(struct inode *inode)
 {
 	/* FS_EVENT_ON_CHILD is set if the inode may care */
-- 
2.17.1

Re: [PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Jan Kara]

On Thu 16-07-20 11:42:23, Amir Goldstein wrote:
> Similar to events "on child" to watching directory, send event "on child"
> with parent/name info if sb/mount/non-dir marks are interested in
> parent/name info.
> 
> The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
> interest in parent/name info for events on non-directory inodes.
> 
> Events on "orphan" children (disconnected dentries) are sent without
> parent/name info.
> 
> Events on direcories are send with parent/name info only if the parent
> directory is watching.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Hum, doesn't this break ignore mask handling in
fanotify_group_event_mask()? Because parent's ignore mask will be included
even though parent is added into the iter only to carry the parent info...

Also I'm constantly getting confused about mark->mask handling in that
function WRT __fsnotify_parent() sending FS_EVENT_ON_CHILD event. But in
the end I've convinced myself it is correct ;)

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

Re: [PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Amir Goldstein]

On Thu, Jul 16, 2020 at 8:01 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 11:42:23, Amir Goldstein wrote:
> > Similar to events "on child" to watching directory, send event "on child"
> > with parent/name info if sb/mount/non-dir marks are interested in
> > parent/name info.
> >
> > The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
> > interest in parent/name info for events on non-directory inodes.
> >
> > Events on "orphan" children (disconnected dentries) are sent without
> > parent/name info.
> >
> > Events on direcories are send with parent/name info only if the parent
> > directory is watching.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Hum, doesn't this break ignore mask handling in
> fanotify_group_event_mask()? Because parent's ignore mask will be included
> even though parent is added into the iter only to carry the parent info...
>

Hmm, break ignore mask handling? or fix it?

Man page said:
"Having these two types of masks permits a mount point or directory to be
 marked for receiving events, while at the  same time ignoring events for
 specific objects under that mount point or directory."

The author did not say what to expect from marking a mount and ignoring
a directory.

As it turns out, I need this exact functionality for my snapshot.
- sb is watching all (pre) modify events
- after dir has been marked with a change in snapshot an exclude
  mark is set on that dir inode
- further modification events on files inside that dir are ignored
  without calling event handler

I am sure you are aware that we have been fixing a lot of problems
in handling combinations of mark masks.
I see the unified event as another step in the direction to fix those
issues and to get consistent and expected behavior.

Thanks,
Amir.

Re: [PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Jan Kara]

On Thu 16-07-20 20:20:04, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 8:01 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-07-20 11:42:23, Amir Goldstein wrote:
> > > Similar to events "on child" to watching directory, send event "on child"
> > > with parent/name info if sb/mount/non-dir marks are interested in
> > > parent/name info.
> > >
> > > The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
> > > interest in parent/name info for events on non-directory inodes.
> > >
> > > Events on "orphan" children (disconnected dentries) are sent without
> > > parent/name info.
> > >
> > > Events on direcories are send with parent/name info only if the parent
> > > directory is watching.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Hum, doesn't this break ignore mask handling in
> > fanotify_group_event_mask()? Because parent's ignore mask will be included
> > even though parent is added into the iter only to carry the parent info...
> >
> 
> Hmm, break ignore mask handling? or fix it?
> 
> Man page said:
> "Having these two types of masks permits a mount point or directory to be
>  marked for receiving events, while at the  same time ignoring events for
>  specific objects under that mount point or directory."

Right, but presumably that speaks of the case of a mark where the parent
has FS_EVENT_ON_CHILD set. For case of parent watching events of a child, I
agree it makes sense to apply ignore masks of both the parent and the child.

> The author did not say what to expect from marking a mount and ignoring
> a directory.

Yes and I'd expect to apply ignore mask on events for that directory but
not for events on files in that directory... Even more so because this will
be currently inconsistent wrt whether the child is dir (parent's ignore mask
does not apply) or file (parent's ignore mask does apply).

> As it turns out, I need this exact functionality for my snapshot.
> - sb is watching all (pre) modify events
> - after dir has been marked with a change in snapshot an exclude
>   mark is set on that dir inode
> - further modification events on files inside that dir are ignored
>   without calling event handler

Yes, I can see how the functionality is useful. Maybe with
FS_EVENT_ON_CHILD in the ignore mask, applying the mask to child's events
would make sense... But that's IMO a dispute for a different patch.

> I am sure you are aware that we have been fixing a lot of problems
> in handling combinations of mark masks.

Very well aware :)

> I see the unified event as another step in the direction to fix those
> issues and to get consistent and expected behavior.

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

Re: [PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Amir Goldstein]

On Thu, Jul 16, 2020 at 8:57 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 20:20:04, Amir Goldstein wrote:
> > On Thu, Jul 16, 2020 at 8:01 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Thu 16-07-20 11:42:23, Amir Goldstein wrote:
> > > > Similar to events "on child" to watching directory, send event "on child"
> > > > with parent/name info if sb/mount/non-dir marks are interested in
> > > > parent/name info.
> > > >
> > > > The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
> > > > interest in parent/name info for events on non-directory inodes.
> > > >
> > > > Events on "orphan" children (disconnected dentries) are sent without
> > > > parent/name info.
> > > >
> > > > Events on direcories are send with parent/name info only if the parent
> > > > directory is watching.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > Hum, doesn't this break ignore mask handling in
> > > fanotify_group_event_mask()? Because parent's ignore mask will be included
> > > even though parent is added into the iter only to carry the parent info...
> > >
> >
> > Hmm, break ignore mask handling? or fix it?
> >
> > Man page said:
> > "Having these two types of masks permits a mount point or directory to be
> >  marked for receiving events, while at the  same time ignoring events for
> >  specific objects under that mount point or directory."
>
> Right, but presumably that speaks of the case of a mark where the parent
> has FS_EVENT_ON_CHILD set. For case of parent watching events of a child, I
> agree it makes sense to apply ignore masks of both the parent and the child.
>
> > The author did not say what to expect from marking a mount and ignoring
> > a directory.
>
> Yes and I'd expect to apply ignore mask on events for that directory but
> not for events on files in that directory... Even more so because this will
> be currently inconsistent wrt whether the child is dir (parent's ignore mask
> does not apply) or file (parent's ignore mask does apply).
>

Indeed. For that I used this trick in my POC:

        /* Set the mark mask, so fsnotify_parent() will find this mark */
        ovm->fsn_mark.mask = mask | FS_EVENT_ON_CHILD;
        ovm->fsn_mark.ignored_mask = mask;

It's not how users are expected to configure an ignored mask on children
but we can work the ignored mask information into the object mask, like
I already did w.r.t FS_MODIFY and get the same result without the hack.

Thanks,
Amir.

P.S. for whoever is interested, my POC is on ovl-fsnotify branch.
It seems to be working well. I am just trying to get those "ephemeral
exclude marks" to not pin the dir inodes to cache, so that those inodes
could be evicted.

Re: [PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Jan Kara]

On Thu 16-07-20 21:42:20, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 8:57 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-07-20 20:20:04, Amir Goldstein wrote:
> > > On Thu, Jul 16, 2020 at 8:01 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Thu 16-07-20 11:42:23, Amir Goldstein wrote:
> > > > > Similar to events "on child" to watching directory, send event "on child"
> > > > > with parent/name info if sb/mount/non-dir marks are interested in
> > > > > parent/name info.
> > > > >
> > > > > The FS_EVENT_ON_CHILD flag can be set on sb/mount/non-dir marks to specify
> > > > > interest in parent/name info for events on non-directory inodes.
> > > > >
> > > > > Events on "orphan" children (disconnected dentries) are sent without
> > > > > parent/name info.
> > > > >
> > > > > Events on direcories are send with parent/name info only if the parent
> > > > > directory is watching.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > >
> > > > Hum, doesn't this break ignore mask handling in
> > > > fanotify_group_event_mask()? Because parent's ignore mask will be included
> > > > even though parent is added into the iter only to carry the parent info...
> > > >
> > >
> > > Hmm, break ignore mask handling? or fix it?
> > >
> > > Man page said:
> > > "Having these two types of masks permits a mount point or directory to be
> > >  marked for receiving events, while at the  same time ignoring events for
> > >  specific objects under that mount point or directory."
> >
> > Right, but presumably that speaks of the case of a mark where the parent
> > has FS_EVENT_ON_CHILD set. For case of parent watching events of a child, I
> > agree it makes sense to apply ignore masks of both the parent and the child.
> >
> > > The author did not say what to expect from marking a mount and ignoring
> > > a directory.
> >
> > Yes and I'd expect to apply ignore mask on events for that directory but
> > not for events on files in that directory... Even more so because this will
> > be currently inconsistent wrt whether the child is dir (parent's ignore mask
> > does not apply) or file (parent's ignore mask does apply).
> >
> 
> Indeed. For that I used this trick in my POC:
> 
>         /* Set the mark mask, so fsnotify_parent() will find this mark */
>         ovm->fsn_mark.mask = mask | FS_EVENT_ON_CHILD;
>         ovm->fsn_mark.ignored_mask = mask;
> 
> It's not how users are expected to configure an ignored mask on children
> but we can work the ignored mask information into the object mask, like
> I already did w.r.t FS_MODIFY and get the same result without the hack.

OK, nice trick but for this series, I'd like to keep the original ignore
mask behavior (bug to bug compatibility) or possibly let parent's ignore
mask be applied only for events being sent to the parent due to its
FS_EVENT_ON_CHILD. Can you please fix that up? I won't get to it before I
leave for vacation but once I return, I'd like to just pick the fixed up
commit and push everything to linux-next... Thanks!

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

Re: [PATCH v5 15/22] fsnotify: send event with parent/name info to sb/mount/non-dir marks

[Amir Goldstein]

> OK, nice trick but for this series, I'd like to keep the original ignore
> mask behavior (bug to bug compatibility) or possibly let parent's ignore
> mask be applied only for events being sent to the parent due to its
> FS_EVENT_ON_CHILD.

That should be easy if we set the FS_EVENT_ON_CHILD flag only for
the case of a watching parent.
And I believe that would make the flag meaningful again and not
redundant as you now see it.
Please note that the series is already not "bug compatible", because
the patch " send event to parent and child with single callback"
already fixes the combination parent watch + child ignore, which
did not work before and this is declared in commit message.

> Can you please fix that up?

Will do.
I will also take care of LTP test coverage for the ignore mask cases
that have been fixed and for dnotify/inotify parent+child watching case.

> I won't get to it before I
> leave for vacation but once I return, I'd like to just pick the fixed up
> commit and push everything to linux-next... Thanks!
>

Thanks a lot for everything!

Amir.

[PATCH v5 16/22] fsnotify: remove check that source dentry is positive

[Amir Goldstein]

Remove the unneeded check for positive source dentry in
fsnotify_move().

fsnotify_move() hook is mostly called from vfs_rename() under
lock_rename() and vfs_rename() starts with may_delete() test that
verifies positive source dentry.  The only other caller of
fsnotify_move() - debugfs_rename() also verifies positive source.

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

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 044cae3a0628..fe4f2bc5b4c2 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -149,8 +149,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	if (target)
 		fsnotify_link_count(target);
 
-	if (source)
-		fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
 	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
 }
 
-- 
2.17.1

Re: [PATCH v5 16/22] fsnotify: remove check that source dentry is positive

[Jan Kara]

On Thu 16-07-20 11:42:24, Amir Goldstein wrote:
> Remove the unneeded check for positive source dentry in
> fsnotify_move().
> 
> fsnotify_move() hook is mostly called from vfs_rename() under
> lock_rename() and vfs_rename() starts with may_delete() test that
> verifies positive source dentry.  The only other caller of
> fsnotify_move() - debugfs_rename() also verifies positive source.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL,
new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it?

								Honza

> ---
>  include/linux/fsnotify.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 044cae3a0628..fe4f2bc5b4c2 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -149,8 +149,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	if (target)
>  		fsnotify_link_count(target);
>  
> -	if (source)
> -		fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
>  	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
>  }
>  
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v5 16/22] fsnotify: remove check that source dentry is positive

[Jan Kara]

On Thu 16-07-20 15:13:11, Jan Kara wrote:
> On Thu 16-07-20 11:42:24, Amir Goldstein wrote:
> > Remove the unneeded check for positive source dentry in
> > fsnotify_move().
> > 
> > fsnotify_move() hook is mostly called from vfs_rename() under
> > lock_rename() and vfs_rename() starts with may_delete() test that
> > verifies positive source dentry.  The only other caller of
> > fsnotify_move() - debugfs_rename() also verifies positive source.
> > 
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> 
> But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL,
> new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it?

FWIW, renameat2() doesn't allow RENAME_EXCHANGE with non-existent target
and I didn't find any other place that could call vfs_rename() with
RENAME_EXCHANGE and negative target but still vfs_rename() seems to support
that and so fsnotify should likely handle that as well.

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

Re: [PATCH v5 16/22] fsnotify: remove check that source dentry is positive

[Amir Goldstein]

On Thu, Jul 16, 2020 at 4:29 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 15:13:11, Jan Kara wrote:
> > On Thu 16-07-20 11:42:24, Amir Goldstein wrote:
> > > Remove the unneeded check for positive source dentry in
> > > fsnotify_move().
> > >
> > > fsnotify_move() hook is mostly called from vfs_rename() under
> > > lock_rename() and vfs_rename() starts with may_delete() test that
> > > verifies positive source dentry.  The only other caller of
> > > fsnotify_move() - debugfs_rename() also verifies positive source.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL,
> > new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it?
>
> FWIW, renameat2() doesn't allow RENAME_EXCHANGE with non-existent target
> and I didn't find any other place that could call vfs_rename() with
> RENAME_EXCHANGE and negative target but still vfs_rename() seems to support
> that and so fsnotify should likely handle that as well.

If some code did call vfs_rename() like that d_exchange() will barf:

void d_exchange(struct dentry *dentry1, struct dentry *dentry2)
{
        write_seqlock(&rename_lock);

        WARN_ON(!dentry1->d_inode);
        WARN_ON(!dentry2->d_inode);

Thanks,
Amir.

Re: [PATCH v5 16/22] fsnotify: remove check that source dentry is positive

[Jan Kara]

On Thu 16-07-20 16:54:08, Amir Goldstein wrote:
> On Thu, Jul 16, 2020 at 4:29 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-07-20 15:13:11, Jan Kara wrote:
> > > On Thu 16-07-20 11:42:24, Amir Goldstein wrote:
> > > > Remove the unneeded check for positive source dentry in
> > > > fsnotify_move().
> > > >
> > > > fsnotify_move() hook is mostly called from vfs_rename() under
> > > > lock_rename() and vfs_rename() starts with may_delete() test that
> > > > verifies positive source dentry.  The only other caller of
> > > > fsnotify_move() - debugfs_rename() also verifies positive source.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > But in vfs_rename() if RENAME_EXCHANGE is set and target is NULL,
> > > new_dentry can be negative when calling fsnotify_move() AFAICT, cannot it?
> >
> > FWIW, renameat2() doesn't allow RENAME_EXCHANGE with non-existent target
> > and I didn't find any other place that could call vfs_rename() with
> > RENAME_EXCHANGE and negative target but still vfs_rename() seems to support
> > that and so fsnotify should likely handle that as well.
> 
> If some code did call vfs_rename() like that d_exchange() will barf:
> 
> void d_exchange(struct dentry *dentry1, struct dentry *dentry2)
> {
>         write_seqlock(&rename_lock);
> 
>         WARN_ON(!dentry1->d_inode);
>         WARN_ON(!dentry2->d_inode);

Good point. Thanks for explanation!

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

[PATCH v5 17/22] fsnotify: send MOVE_SELF event with parent/name info

[Amir Goldstein]

MOVE_SELF event does not get reported to a parent watching children
when a child is moved, but it can be reported to sb/mount mark or to
the moved inode itself with parent/name info if group is interested
in parent/name info.

Use the fsnotify_parent() helper to send a MOVE_SELF event and adjust
fsnotify() to handle the case of an event "on child" that should not
be sent to the watching parent's inode mark.

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

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index efa5c1c4908a..fa84aea47b20 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -367,6 +367,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	struct super_block *sb = to_tell->i_sb;
 	struct inode *dir = S_ISDIR(to_tell->i_mode) ? to_tell : NULL;
 	struct mount *mnt = NULL;
+	struct inode *inode = NULL;
 	struct inode *child = NULL;
 	int ret = 0;
 	__u32 test_mask, marks_mask;
@@ -377,6 +378,14 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	if (mask & FS_EVENT_ON_CHILD)
 		child = fsnotify_data_inode(data, data_type);
 
+	/*
+	 * If event is "on child" then to_tell is a watching parent.
+	 * An event "on child" may be sent to mount/sb mark with parent/name
+	 * info, but not appropriate for watching parent (e.g. FS_MOVE_SELF).
+	 */
+	if (!child || (mask & FS_EVENTS_POSS_ON_CHILD))
+		inode = to_tell;
+
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
 	 * be expensive.  It protects walking the *_fsnotify_marks lists.
@@ -384,14 +393,17 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 	 * SRCU because we have no references to any objects and do not
 	 * need SRCU to keep them "alive".
 	 */
-	if (!to_tell->i_fsnotify_marks && !sb->s_fsnotify_marks &&
+	if (!sb->s_fsnotify_marks &&
 	    (!mnt || !mnt->mnt_fsnotify_marks) &&
+	    (!inode || !inode->i_fsnotify_marks) &&
 	    (!child || !child->i_fsnotify_marks))
 		return 0;
 
-	marks_mask = to_tell->i_fsnotify_mask | sb->s_fsnotify_mask;
+	marks_mask = sb->s_fsnotify_mask;
 	if (mnt)
 		marks_mask |= mnt->mnt_fsnotify_mask;
+	if (inode)
+		marks_mask |= inode->i_fsnotify_mask;
 	if (child)
 		marks_mask |= child->i_fsnotify_mask;
 
@@ -406,14 +418,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
 
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
 
-	iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
-		fsnotify_first_mark(&to_tell->i_fsnotify_marks);
 	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
 		fsnotify_first_mark(&sb->s_fsnotify_marks);
 	if (mnt) {
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
 			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
 	}
+	if (inode) {
+		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
+			fsnotify_first_mark(&inode->i_fsnotify_marks);
+	}
 	if (child) {
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] =
 			fsnotify_first_mark(&child->i_fsnotify_marks);
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index fe4f2bc5b4c2..61dccaf21e7b 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -131,7 +131,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	u32 fs_cookie = fsnotify_get_cookie();
 	__u32 old_dir_mask = FS_MOVED_FROM;
 	__u32 new_dir_mask = FS_MOVED_TO;
-	__u32 mask = FS_MOVE_SELF;
 	const struct qstr *new_name = &moved->d_name;
 
 	if (old_dir == new_dir)
@@ -140,7 +139,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	if (isdir) {
 		old_dir_mask |= FS_ISDIR;
 		new_dir_mask |= FS_ISDIR;
-		mask |= FS_ISDIR;
 	}
 
 	fsnotify_name(old_dir, old_dir_mask, source, old_name, fs_cookie);
@@ -149,7 +147,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 	if (target)
 		fsnotify_link_count(target);
 
-	fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
+	fsnotify_dentry(moved, FS_MOVE_SELF);
 	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
 }
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 6f0df110e9f8..acce2ec17edf 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -89,7 +89,7 @@
  * It may include events that can be sent to an inode/sb/mount mark, but cannot
  * be sent to a parent watching children.
  */
-#define FS_EVENTS_POSS_TO_PARENT (FS_EVENTS_POSS_ON_CHILD)
+#define FS_EVENTS_POSS_TO_PARENT (FS_EVENTS_POSS_ON_CHILD | FS_MOVE_SELF)
 
 /* Events that can be reported to backends */
 #define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
-- 
2.17.1

Re: [PATCH v5 17/22] fsnotify: send MOVE_SELF event with parent/name info

[Jan Kara]

On Thu 16-07-20 11:42:25, Amir Goldstein wrote:
> MOVE_SELF event does not get reported to a parent watching children
> when a child is moved, but it can be reported to sb/mount mark or to
> the moved inode itself with parent/name info if group is interested
> in parent/name info.
> 
> Use the fsnotify_parent() helper to send a MOVE_SELF event and adjust
> fsnotify() to handle the case of an event "on child" that should not
> be sent to the watching parent's inode mark.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

What I find strange about this is that the MOVE_SELF event will be reported
to the new parent under the new name (just due to the way how dentry
handling in vfs_rename() works). That seems rather arbitrary and I'm not
sure it would be useful? I guess anybody needing dir info with renames
will rather use FS_MOVED_FROM / FS_MOVED_TO where it is well defined?

So can we leave FS_MOVE_SELF as one of those cases that doesn't report
parent + name info?

								Honza


> ---
>  fs/notify/fsnotify.c             | 22 ++++++++++++++++++----
>  include/linux/fsnotify.h         |  4 +---
>  include/linux/fsnotify_backend.h |  2 +-
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index efa5c1c4908a..fa84aea47b20 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -367,6 +367,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	struct super_block *sb = to_tell->i_sb;
>  	struct inode *dir = S_ISDIR(to_tell->i_mode) ? to_tell : NULL;
>  	struct mount *mnt = NULL;
> +	struct inode *inode = NULL;
>  	struct inode *child = NULL;
>  	int ret = 0;
>  	__u32 test_mask, marks_mask;
> @@ -377,6 +378,14 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	if (mask & FS_EVENT_ON_CHILD)
>  		child = fsnotify_data_inode(data, data_type);
>  
> +	/*
> +	 * If event is "on child" then to_tell is a watching parent.
> +	 * An event "on child" may be sent to mount/sb mark with parent/name
> +	 * info, but not appropriate for watching parent (e.g. FS_MOVE_SELF).
> +	 */
> +	if (!child || (mask & FS_EVENTS_POSS_ON_CHILD))
> +		inode = to_tell;
> +
>  	/*
>  	 * Optimization: srcu_read_lock() has a memory barrier which can
>  	 * be expensive.  It protects walking the *_fsnotify_marks lists.
> @@ -384,14 +393,17 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  	 * SRCU because we have no references to any objects and do not
>  	 * need SRCU to keep them "alive".
>  	 */
> -	if (!to_tell->i_fsnotify_marks && !sb->s_fsnotify_marks &&
> +	if (!sb->s_fsnotify_marks &&
>  	    (!mnt || !mnt->mnt_fsnotify_marks) &&
> +	    (!inode || !inode->i_fsnotify_marks) &&
>  	    (!child || !child->i_fsnotify_marks))
>  		return 0;
>  
> -	marks_mask = to_tell->i_fsnotify_mask | sb->s_fsnotify_mask;
> +	marks_mask = sb->s_fsnotify_mask;
>  	if (mnt)
>  		marks_mask |= mnt->mnt_fsnotify_mask;
> +	if (inode)
> +		marks_mask |= inode->i_fsnotify_mask;
>  	if (child)
>  		marks_mask |= child->i_fsnotify_mask;
>  
> @@ -406,14 +418,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_type,
>  
>  	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
>  
> -	iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> -		fsnotify_first_mark(&to_tell->i_fsnotify_marks);
>  	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
>  		fsnotify_first_mark(&sb->s_fsnotify_marks);
>  	if (mnt) {
>  		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
>  			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
>  	}
> +	if (inode) {
> +		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
> +			fsnotify_first_mark(&inode->i_fsnotify_marks);
> +	}
>  	if (child) {
>  		iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] =
>  			fsnotify_first_mark(&child->i_fsnotify_marks);
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index fe4f2bc5b4c2..61dccaf21e7b 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -131,7 +131,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	u32 fs_cookie = fsnotify_get_cookie();
>  	__u32 old_dir_mask = FS_MOVED_FROM;
>  	__u32 new_dir_mask = FS_MOVED_TO;
> -	__u32 mask = FS_MOVE_SELF;
>  	const struct qstr *new_name = &moved->d_name;
>  
>  	if (old_dir == new_dir)
> @@ -140,7 +139,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	if (isdir) {
>  		old_dir_mask |= FS_ISDIR;
>  		new_dir_mask |= FS_ISDIR;
> -		mask |= FS_ISDIR;
>  	}
>  
>  	fsnotify_name(old_dir, old_dir_mask, source, old_name, fs_cookie);
> @@ -149,7 +147,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  	if (target)
>  		fsnotify_link_count(target);
>  
> -	fsnotify(source, mask, source, FSNOTIFY_EVENT_INODE, NULL, 0);
> +	fsnotify_dentry(moved, FS_MOVE_SELF);
>  	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
>  }
>  
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 6f0df110e9f8..acce2ec17edf 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -89,7 +89,7 @@
>   * It may include events that can be sent to an inode/sb/mount mark, but cannot
>   * be sent to a parent watching children.
>   */
> -#define FS_EVENTS_POSS_TO_PARENT (FS_EVENTS_POSS_ON_CHILD)
> +#define FS_EVENTS_POSS_TO_PARENT (FS_EVENTS_POSS_ON_CHILD | FS_MOVE_SELF)
>  
>  /* Events that can be reported to backends */
>  #define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v5 17/22] fsnotify: send MOVE_SELF event with parent/name info

[Amir Goldstein]

On Thu, Jul 16, 2020 at 4:45 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 11:42:25, Amir Goldstein wrote:
> > MOVE_SELF event does not get reported to a parent watching children
> > when a child is moved, but it can be reported to sb/mount mark or to
> > the moved inode itself with parent/name info if group is interested
> > in parent/name info.
> >
> > Use the fsnotify_parent() helper to send a MOVE_SELF event and adjust
> > fsnotify() to handle the case of an event "on child" that should not
> > be sent to the watching parent's inode mark.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> What I find strange about this is that the MOVE_SELF event will be reported
> to the new parent under the new name (just due to the way how dentry
> handling in vfs_rename() works). That seems rather arbitrary and I'm not
> sure it would be useful? I guess anybody needing dir info with renames
> will rather use FS_MOVED_FROM / FS_MOVED_TO where it is well defined?
>
> So can we leave FS_MOVE_SELF as one of those cases that doesn't report
> parent + name info?
>

I can live with that.
I didn't have a use case for it.
This patch may be dropped from the series without affecting the rest.

Thanks,
Amir.

Re: [PATCH v5 17/22] fsnotify: send MOVE_SELF event with parent/name info

[Amir Goldstein]

On Thu, Jul 16, 2020 at 4:59 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 4:45 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-07-20 11:42:25, Amir Goldstein wrote:
> > > MOVE_SELF event does not get reported to a parent watching children
> > > when a child is moved, but it can be reported to sb/mount mark or to
> > > the moved inode itself with parent/name info if group is interested
> > > in parent/name info.
> > >
> > > Use the fsnotify_parent() helper to send a MOVE_SELF event and adjust
> > > fsnotify() to handle the case of an event "on child" that should not
> > > be sent to the watching parent's inode mark.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > What I find strange about this is that the MOVE_SELF event will be reported
> > to the new parent under the new name (just due to the way how dentry
> > handling in vfs_rename() works). That seems rather arbitrary and I'm not
> > sure it would be useful? I guess anybody needing dir info with renames
> > will rather use FS_MOVED_FROM / FS_MOVED_TO where it is well defined?
> >
> > So can we leave FS_MOVE_SELF as one of those cases that doesn't report
> > parent + name info?
> >
>
> I can live with that.
> I didn't have a use case for it.
> This patch may be dropped from the series without affecting the rest.
>

BTW, I checked my man page and it doesn't say anything about whether
parent fid and child fid can be reported together with MOVE_SELF.
The language is generic enough on that part.

Thanks,
Amir.

Re: [PATCH v5 17/22] fsnotify: send MOVE_SELF event with parent/name info

[Amir Goldstein]

On Thu, Jul 16, 2020 at 5:10 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 4:59 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, Jul 16, 2020 at 4:45 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Thu 16-07-20 11:42:25, Amir Goldstein wrote:
> > > > MOVE_SELF event does not get reported to a parent watching children
> > > > when a child is moved, but it can be reported to sb/mount mark or to
> > > > the moved inode itself with parent/name info if group is interested
> > > > in parent/name info.
> > > >
> > > > Use the fsnotify_parent() helper to send a MOVE_SELF event and adjust
> > > > fsnotify() to handle the case of an event "on child" that should not
> > > > be sent to the watching parent's inode mark.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > What I find strange about this is that the MOVE_SELF event will be reported
> > > to the new parent under the new name (just due to the way how dentry
> > > handling in vfs_rename() works). That seems rather arbitrary and I'm not
> > > sure it would be useful? I guess anybody needing dir info with renames
> > > will rather use FS_MOVED_FROM / FS_MOVED_TO where it is well defined?
> > >
> > > So can we leave FS_MOVE_SELF as one of those cases that doesn't report
> > > parent + name info?
> > >
> >
> > I can live with that.
> > I didn't have a use case for it.
> > This patch may be dropped from the series without affecting the rest.
> >
>
> BTW, I checked my man page and it doesn't say anything about whether
> parent fid and child fid can be reported together with MOVE_SELF.
> The language is generic enough on that part.
>

FYI, I pushed a commit to the LTP branch that adapts the test to MOVE_SELF
that does not report name and tested with this patch reverted.

The test now has less special cases when setting expected values,
which is generally a good sign ;-)

Thanks,
Amir.

[PATCH v5 18/22] fanotify: add basic support for FAN_REPORT_DIR_FID

[Amir Goldstein]

For now, the flag is mutually exclusive with FAN_REPORT_FID.
Events include a single info record of type FAN_EVENT_INFO_TYPE_DFID
with a directory file handle.

For now, events are only reported for:
- Directory modification events
- Events on children of a watching directory
- Events on directory objects

Soon, we will add support for reporting the parent directory fid
for events on non-directories with filesystem/mount mark and
support for reporting both parent directory fid and child fid.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 33 +++++++++++++-
 fs/notify/fanotify/fanotify_user.c | 71 +++++++++++++++++++++++++-----
 include/linux/fanotify.h           |  2 +-
 include/uapi/linux/fanotify.h      | 11 +++--
 4 files changed, 100 insertions(+), 17 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 1ec760960c93..4cbdb40e0d54 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -223,7 +223,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
 static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 				     struct fsnotify_iter_info *iter_info,
 				     u32 event_mask, const void *data,
-				     int data_type)
+				     int data_type, struct inode *dir)
 {
 	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
@@ -243,6 +243,10 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		/* Path type events are only relevant for files and dirs */
 		if (!d_is_reg(path->dentry) && !d_can_lookup(path->dentry))
 			return 0;
+	} else if (!(fid_mode & FAN_REPORT_FID)) {
+		/* Do we have a directory inode to report? */
+		if (!dir)
+			return 0;
 	}
 
 	fsnotify_foreach_obj_type(type) {
@@ -399,6 +403,28 @@ static struct inode *fanotify_fid_inode(u32 event_mask, const void *data,
 	return fsnotify_data_inode(data, data_type);
 }
 
+/*
+ * The inode to use as identifier when reporting dir fid depends on the event.
+ * Report the modified directory inode on dirent modification events.
+ * Report the "victim" inode if "victim" is a directory.
+ * Report the parent inode if "victim" is not a directory and event is
+ * reported to parent.
+ * Otherwise, do not report dir fid.
+ */
+static struct inode *fanotify_dfid_inode(u32 event_mask, const void *data,
+					 int data_type, struct inode *dir)
+{
+	struct inode *inode = fsnotify_data_inode(data, data_type);
+
+	if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
+		return dir;
+
+	if (S_ISDIR(inode->i_mode))
+		return inode;
+
+	return dir;
+}
+
 static struct fanotify_event *fanotify_alloc_path_event(const struct path *path,
 							gfp_t gfp)
 {
@@ -498,6 +524,9 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	bool name_event = false;
 
+	if ((fid_mode & FAN_REPORT_DIR_FID) && dir)
+		id = fanotify_dfid_inode(mask, data, data_type, dir);
+
 	/*
 	 * For queues with unlimited length lost events are not expected and
 	 * can possibly have security implications. Avoid losing events when
@@ -608,7 +637,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
 
 	mask = fanotify_group_event_mask(group, iter_info, mask, data,
-					 data_type);
+					 data_type, dir);
 	if (!mask)
 		return 0;
 
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 3842ef00b52e..e494400711c9 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -216,7 +216,7 @@ static int process_access_response(struct fsnotify_group *group,
 }
 
 static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
-			     const char *name, size_t name_len,
+			     int info_type, const char *name, size_t name_len,
 			     char __user *buf, size_t count)
 {
 	struct fanotify_event_info_fid info = { };
@@ -229,7 +229,7 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
 	pr_debug("%s: fh_len=%zu name_len=%zu, info_len=%zu, count=%zu\n",
 		 __func__, fh_len, name_len, info_len, count);
 
-	if (!fh_len || (name && !name_len))
+	if (!fh_len)
 		return 0;
 
 	if (WARN_ON_ONCE(len < sizeof(info) || len > count))
@@ -239,8 +239,21 @@ static int copy_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
 	 * Copy event info fid header followed by variable sized file handle
 	 * and optionally followed by variable sized filename.
 	 */
-	info.hdr.info_type = name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
-					FAN_EVENT_INFO_TYPE_FID;
+	switch (info_type) {
+	case FAN_EVENT_INFO_TYPE_FID:
+	case FAN_EVENT_INFO_TYPE_DFID:
+		if (WARN_ON_ONCE(name_len))
+			return -EFAULT;
+		break;
+	case FAN_EVENT_INFO_TYPE_DFID_NAME:
+		if (WARN_ON_ONCE(!name || !name_len))
+			return -EFAULT;
+		break;
+	default:
+		return -EFAULT;
+	}
+
+	info.hdr.info_type = info_type;
 	info.hdr.len = len;
 	info.fsid = *fsid;
 	if (copy_to_user(buf, &info, sizeof(info)))
@@ -304,8 +317,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	struct fanotify_event_metadata metadata;
 	struct path *path = fanotify_event_path(event);
 	struct fanotify_info *info = fanotify_event_info(event);
+	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct file *f = NULL;
 	int ret, fd = FAN_NOFD;
+	int info_type = 0;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
@@ -346,9 +361,10 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
 	/* Event info records order is: dir fid + name, child fid */
 	if (fanotify_event_dir_fh_len(event)) {
+		info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
 		ret = copy_info_to_user(fanotify_event_fsid(event),
 					fanotify_info_dir_fh(info),
-					fanotify_info_name(info),
+					info_type, fanotify_info_name(info),
 					info->name_len, buf, count);
 		if (ret < 0)
 			return ret;
@@ -358,9 +374,33 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	}
 
 	if (fanotify_event_object_fh_len(event)) {
+		if (fid_mode == FAN_REPORT_FID || info_type) {
+			/*
+			 * With only group flag FAN_REPORT_FID only type FID is
+			 * reported. Second info record type is always FID.
+			 */
+			info_type = FAN_EVENT_INFO_TYPE_FID;
+		} else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
+			   (event->mask & FAN_ONDIR)) {
+			/*
+			 * With group flag FAN_REPORT_DIR_FID, a single info
+			 * record has type DFID for directory entry modification
+			 * event and for event on a directory.
+			 */
+			info_type = FAN_EVENT_INFO_TYPE_DFID;
+		} else {
+			/*
+			 * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
+			 * a single info record has type FID for event on a
+			 * non-directory, when there is no directory to report.
+			 * For example, on FAN_DELETE_SELF event.
+			 */
+			info_type = FAN_EVENT_INFO_TYPE_FID;
+		}
+
 		ret = copy_info_to_user(fanotify_event_fsid(event),
 					fanotify_event_object_fh(event),
-					NULL, 0, buf, count);
+					info_type, NULL, 0, buf, count);
 		if (ret < 0)
 			return ret;
 
@@ -861,6 +901,8 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	struct fsnotify_group *group;
 	int f_flags, fd;
 	struct user_struct *user;
+	unsigned int fid_mode = flags & FANOTIFY_FID_BITS;
+	unsigned int class = flags & FANOTIFY_CLASS_BITS;
 
 	pr_debug("%s: flags=%x event_f_flags=%x\n",
 		 __func__, flags, event_f_flags);
@@ -887,10 +929,19 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 		return -EINVAL;
 	}
 
-	if ((flags & FANOTIFY_FID_BITS) &&
-	    (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF)
+	if (fid_mode && class != FAN_CLASS_NOTIF)
 		return -EINVAL;
 
+	/* Reporting either object fid or dir fid */
+	switch (fid_mode) {
+	case 0:
+	case FAN_REPORT_FID:
+	case FAN_REPORT_DIR_FID:
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	user = get_current_user();
 	if (atomic_read(&user->fanotify_listeners) > FANOTIFY_DEFAULT_MAX_LISTENERS) {
 		free_uid(user);
@@ -926,7 +977,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	group->fanotify_data.f_flags = event_f_flags;
 	init_waitqueue_head(&group->fanotify_data.access_waitq);
 	INIT_LIST_HEAD(&group->fanotify_data.access_list);
-	switch (flags & FANOTIFY_CLASS_BITS) {
+	switch (class) {
 	case FAN_CLASS_NOTIF:
 		group->priority = FS_PRIO_0;
 		break;
@@ -1236,7 +1287,7 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 8);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 9);
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index bbbee11d2521..4ddac97b2bf7 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -18,7 +18,7 @@
 #define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
 				 FAN_CLASS_PRE_CONTENT)
 
-#define FANOTIFY_FID_BITS	(FAN_REPORT_FID)
+#define FANOTIFY_FID_BITS	(FAN_REPORT_FID | FAN_REPORT_DIR_FID)
 
 #define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | FANOTIFY_FID_BITS | \
 				 FAN_REPORT_TID | \
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 7f2f17eacbf9..21afebf77fd7 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -53,6 +53,7 @@
 /* Flags to determine fanotify event format */
 #define FAN_REPORT_TID		0x00000100	/* event->pid is thread id */
 #define FAN_REPORT_FID		0x00000200	/* Report unique file id */
+#define FAN_REPORT_DIR_FID	0x00000400	/* Report unique directory id */
 
 /* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
@@ -117,6 +118,7 @@ struct fanotify_event_metadata {
 
 #define FAN_EVENT_INFO_TYPE_FID		1
 #define FAN_EVENT_INFO_TYPE_DFID_NAME	2
+#define FAN_EVENT_INFO_TYPE_DFID	3
 
 /* Variable length info record following event metadata */
 struct fanotify_event_info_header {
@@ -126,10 +128,11 @@ struct fanotify_event_info_header {
 };
 
 /*
- * Unique file identifier info record. This is used both for
- * FAN_EVENT_INFO_TYPE_FID records and for FAN_EVENT_INFO_TYPE_DFID_NAME
- * records. For FAN_EVENT_INFO_TYPE_DFID_NAME there is additionally a null
- * terminated name immediately after the file handle.
+ * Unique file identifier info record.
+ * This structure is used for records of types FAN_EVENT_INFO_TYPE_FID,
+ * FAN_EVENT_INFO_TYPE_DFID and FAN_EVENT_INFO_TYPE_DFID_NAME.
+ * For FAN_EVENT_INFO_TYPE_DFID_NAME there is additionally a null terminated
+ * name immediately after the file handle.
  */
 struct fanotify_event_info_fid {
 	struct fanotify_event_info_header hdr;
-- 
2.17.1

[PATCH v5 19/22] fanotify: report events with parent dir fid to sb/mount/non-dir marks

[Amir Goldstein]

In a group with flag FAN_REPORT_DIR_FID, when adding an inode mark with
FAN_EVENT_ON_CHILD, events on non-directory children are reported with
the fid of the parent.

When adding a filesystem or mount mark or mark on a non-dir inode, we
want to report events that are "possible on child" (e.g. open/close)
also with fid of the parent, as if the victim inode's parent is
interested in events "on child".

Some events, currently only FAN_MOVE_SELF, should be reported to a
sb/mount/non-dir mark with parent fid even though they are not
reported to a watching parent.

To get the desired behavior we set the flag FAN_EVENT_ON_CHILD on
all the sb/mount/non-dir mark masks in a group with FAN_REPORT_DIR_FID.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 3 +--
 fs/notify/fanotify/fanotify_user.c | 7 +++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 4cbdb40e0d54..09331b0acaf2 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -272,8 +272,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		 */
 		if (event_mask & FS_EVENT_ON_CHILD &&
 		    type != FSNOTIFY_OBJ_TYPE_CHILD &&
-		    (type != FSNOTIFY_OBJ_TYPE_INODE ||
-		     !(mark->mask & FS_EVENT_ON_CHILD)))
+		    !(mark->mask & FS_EVENT_ON_CHILD))
 			continue;
 
 		marks_mask |= mark->mask;
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index e494400711c9..7caa64d028ba 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1220,6 +1220,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	if (mnt || !S_ISDIR(inode->i_mode)) {
 		mask &= ~FAN_EVENT_ON_CHILD;
 		umask = FAN_EVENT_ON_CHILD;
+		/*
+		 * If group needs to report parent fid, register for getting
+		 * events with parent/name info for non-directory.
+		 */
+		if ((fid_mode & FAN_REPORT_DIR_FID) &&
+		    (flags & FAN_MARK_ADD) && !ignored)
+			mask |= FAN_EVENT_ON_CHILD;
 	}
 
 	/* create/update an inode mark */
-- 
2.17.1

[PATCH v5 20/22] fanotify: add support for FAN_REPORT_NAME

[Amir Goldstein]

Introduce a new fanotify_init() flag FAN_REPORT_NAME.  It requires the
flag FAN_REPORT_DIR_FID and there is a constant for setting both flags
named FAN_REPORT_DFID_NAME.

For a group with flag FAN_REPORT_NAME, the parent fid and name are
reported for directory entry modification events (create/detete/move)
and for events on non-directory objects.

Events on directories themselves are reported with their own fid and
"." as the name.

The parent fid and name are reported with an info record of type
FAN_EVENT_INFO_TYPE_DFID_NAME, similar to the way that parent fid is
reported with into type FAN_EVENT_INFO_TYPE_DFID, but with an appended
null terminated name string.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 18 +++++++++++-
 fs/notify/fanotify/fanotify_user.c | 45 ++++++++++++++++++++++++------
 include/linux/fanotify.h           |  2 +-
 include/uapi/linux/fanotify.h      |  4 +++
 4 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 09331b0acaf2..c77b37eb33a9 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -523,9 +523,25 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	bool name_event = false;
 
-	if ((fid_mode & FAN_REPORT_DIR_FID) && dir)
+	if ((fid_mode & FAN_REPORT_DIR_FID) && dir) {
 		id = fanotify_dfid_inode(mask, data, data_type, dir);
 
+		/*
+		 * We record file name only in a group with FAN_REPORT_NAME
+		 * and when we have a directory inode to report.
+		 *
+		 * For directory entry modification event, we record the fid of
+		 * the directory and the name of the modified entry.
+		 *
+		 * For event on non-directory that is reported to parent, we
+		 * record the fid of the parent and the name of the child.
+		 */
+		if ((fid_mode & FAN_REPORT_NAME) &&
+		    ((mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
+		     !(mask & FAN_ONDIR)))
+			name_event = true;
+	}
+
 	/*
 	 * For queues with unlimited length lost events are not expected and
 	 * can possibly have security implications. Avoid losing events when
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 7caa64d028ba..6b839790cb42 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -64,18 +64,27 @@ static int fanotify_fid_info_len(int fh_len, int name_len)
 	return roundup(FANOTIFY_INFO_HDR_LEN + info_len, FANOTIFY_EVENT_ALIGN);
 }
 
-static int fanotify_event_info_len(struct fanotify_event *event)
+static int fanotify_event_info_len(unsigned int fid_mode,
+				   struct fanotify_event *event)
 {
 	struct fanotify_info *info = fanotify_event_info(event);
 	int dir_fh_len = fanotify_event_dir_fh_len(event);
 	int fh_len = fanotify_event_object_fh_len(event);
 	int info_len = 0;
+	int dot_len = 0;
 
-	if (dir_fh_len)
+	if (dir_fh_len) {
 		info_len += fanotify_fid_info_len(dir_fh_len, info->name_len);
+	} else if ((fid_mode & FAN_REPORT_NAME) && (event->mask & FAN_ONDIR)) {
+		/*
+		 * With group flag FAN_REPORT_NAME, if name was not recorded in
+		 * event on a directory, we will report the name ".".
+		 */
+		dot_len = 1;
+	}
 
 	if (fh_len)
-		info_len += fanotify_fid_info_len(fh_len, 0);
+		info_len += fanotify_fid_info_len(fh_len, dot_len);
 
 	return info_len;
 }
@@ -91,6 +100,7 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
 {
 	size_t event_size = FAN_EVENT_METADATA_LEN;
 	struct fanotify_event *event = NULL;
+	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 
 	pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
 
@@ -98,8 +108,8 @@ static struct fanotify_event *get_one_event(struct fsnotify_group *group,
 	if (fsnotify_notify_queue_is_empty(group))
 		goto out;
 
-	if (FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS)) {
-		event_size += fanotify_event_info_len(
+	if (fid_mode) {
+		event_size += fanotify_event_info_len(fid_mode,
 			FANOTIFY_E(fsnotify_peek_first_event(group)));
 	}
 
@@ -325,7 +335,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
 	metadata.event_len = FAN_EVENT_METADATA_LEN +
-					fanotify_event_info_len(event);
+				fanotify_event_info_len(fid_mode, event);
 	metadata.metadata_len = FAN_EVENT_METADATA_LEN;
 	metadata.vers = FANOTIFY_METADATA_VERSION;
 	metadata.reserved = 0;
@@ -374,12 +384,25 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	}
 
 	if (fanotify_event_object_fh_len(event)) {
+		const char *dot = NULL;
+		int dot_len = 0;
+
 		if (fid_mode == FAN_REPORT_FID || info_type) {
 			/*
 			 * With only group flag FAN_REPORT_FID only type FID is
 			 * reported. Second info record type is always FID.
 			 */
 			info_type = FAN_EVENT_INFO_TYPE_FID;
+		} else if ((fid_mode & FAN_REPORT_NAME) &&
+			   (event->mask & FAN_ONDIR)) {
+			/*
+			 * With group flag FAN_REPORT_NAME, if name was not
+			 * recorded in an event on a directory, report the
+			 * name "." with info type DFID_NAME.
+			 */
+			info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
+			dot = ".";
+			dot_len = 1;
 		} else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
 			   (event->mask & FAN_ONDIR)) {
 			/*
@@ -400,7 +423,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
 		ret = copy_info_to_user(fanotify_event_fsid(event),
 					fanotify_event_object_fh(event),
-					info_type, NULL, 0, buf, count);
+					info_type, dot, dot_len, buf, count);
 		if (ret < 0)
 			return ret;
 
@@ -932,11 +955,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	if (fid_mode && class != FAN_CLASS_NOTIF)
 		return -EINVAL;
 
-	/* Reporting either object fid or dir fid */
+	/*
+	 * Reporting either object fid or dir fid.
+	 * Child name is reported with parent fid so requires dir fid.
+	 */
 	switch (fid_mode) {
 	case 0:
 	case FAN_REPORT_FID:
 	case FAN_REPORT_DIR_FID:
+	case FAN_REPORT_DFID_NAME:
 		break;
 	default:
 		return -EINVAL;
@@ -1294,7 +1321,7 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 9);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 10);
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 4ddac97b2bf7..3e9c56ee651f 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -18,7 +18,7 @@
 #define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
 				 FAN_CLASS_PRE_CONTENT)
 
-#define FANOTIFY_FID_BITS	(FAN_REPORT_FID | FAN_REPORT_DIR_FID)
+#define FANOTIFY_FID_BITS	(FAN_REPORT_FID | FAN_REPORT_DFID_NAME)
 
 #define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | FANOTIFY_FID_BITS | \
 				 FAN_REPORT_TID | \
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 21afebf77fd7..fbf9c5c7dd59 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -54,6 +54,10 @@
 #define FAN_REPORT_TID		0x00000100	/* event->pid is thread id */
 #define FAN_REPORT_FID		0x00000200	/* Report unique file id */
 #define FAN_REPORT_DIR_FID	0x00000400	/* Report unique directory id */
+#define FAN_REPORT_NAME		0x00000800	/* Report events with name */
+
+/* Convenience macro - FAN_REPORT_NAME requires FAN_REPORT_DIR_FID */
+#define FAN_REPORT_DFID_NAME	(FAN_REPORT_DIR_FID | FAN_REPORT_NAME)
 
 /* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
-- 
2.17.1

[PATCH v5 21/22] fanotify: report parent fid + name + child fid

[Amir Goldstein]

For a group with fanotify_init() flag FAN_REPORT_DFID_NAME, the parent
fid and name are reported for events on non-directory objects with an
info record of type FAN_EVENT_INFO_TYPE_DFID_NAME.

If the group also has the init flag FAN_REPORT_FID, the child fid
is also reported with another info record that follows the first info
record. The second info record is the same info record that would have
been reported to a group with only FAN_REPORT_FID flag.

When the child fid needs to be recorded, the variable size struct
fanotify_name_event is preallocated with enough space to store the
child fh between the dir fh and the name.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 30 ++++++++++++++++++++++++++----
 fs/notify/fanotify/fanotify.h      |  8 +++++++-
 fs/notify/fanotify/fanotify_user.c |  3 ++-
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index c77b37eb33a9..1d8eb886fe08 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -479,15 +479,22 @@ static struct fanotify_event *fanotify_alloc_fid_event(struct inode *id,
 static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 							__kernel_fsid_t *fsid,
 							const struct qstr *file_name,
+							struct inode *child,
 							gfp_t gfp)
 {
 	struct fanotify_name_event *fne;
 	struct fanotify_info *info;
-	struct fanotify_fh *dfh;
+	struct fanotify_fh *dfh, *ffh;
 	unsigned int dir_fh_len = fanotify_encode_fh_len(id);
+	unsigned int child_fh_len = fanotify_encode_fh_len(child);
 	unsigned int size;
 
+	if (WARN_ON_ONCE(dir_fh_len % FANOTIFY_FH_HDR_LEN))
+		child_fh_len = 0;
+
 	size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len;
+	if (child_fh_len)
+		size += FANOTIFY_FH_HDR_LEN + child_fh_len;
 	if (file_name)
 		size += file_name->len + 1;
 	fne = kmalloc(size, gfp);
@@ -500,11 +507,15 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 	fanotify_info_init(info);
 	dfh = fanotify_info_dir_fh(info);
 	info->dir_fh_totlen = fanotify_encode_fh(dfh, id, dir_fh_len, 0);
+	if (child_fh_len) {
+		ffh = fanotify_info_file_fh(info);
+		info->file_fh_totlen = fanotify_encode_fh(ffh, child, child_fh_len, 0);
+	}
 	if (file_name)
 		fanotify_info_copy_name(info, file_name);
 
-	pr_debug("%s: ino=%lu size=%u dir_fh_len=%u name_len=%u name='%.*s'\n",
-		 __func__, id->i_ino, size, dir_fh_len,
+	pr_debug("%s: ino=%lu size=%u dir_fh_len=%u child_fh_len=%u name_len=%u name='%.*s'\n",
+		 __func__, id->i_ino, size, dir_fh_len, child_fh_len,
 		 info->name_len, info->name_len, fanotify_info_name(info));
 
 	return &fne->fae;
@@ -521,9 +532,19 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	struct inode *id = fanotify_fid_inode(mask, data, data_type, dir);
 	const struct path *path = fsnotify_data_path(data, data_type);
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
+	struct inode *child = NULL;
 	bool name_event = false;
 
 	if ((fid_mode & FAN_REPORT_DIR_FID) && dir) {
+		/*
+		 * With both flags FAN_REPORT_DIR_FID and FAN_REPORT_FID, we
+		 * report the child fid for events reported on a non-dir child
+		 * in addition to reporting the parent fid and child name.
+		 */
+		if ((fid_mode & FAN_REPORT_FID) &&
+		    (mask & FAN_EVENT_ON_CHILD) && !(mask & FAN_ONDIR))
+			child = id;
+
 		id = fanotify_dfid_inode(mask, data, data_type, dir);
 
 		/*
@@ -559,7 +580,8 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	if (fanotify_is_perm_event(mask)) {
 		event = fanotify_alloc_perm_event(path, gfp);
 	} else if (name_event && file_name) {
-		event = fanotify_alloc_name_event(id, fsid, file_name, gfp);
+		event = fanotify_alloc_name_event(id, fsid, file_name, child,
+						  gfp);
 	} else if (fid_mode) {
 		event = fanotify_alloc_fid_event(id, fsid, gfp);
 	} else {
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 12c204b1489f..896c819a1786 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -193,6 +193,8 @@ static inline struct fanotify_fh *fanotify_event_object_fh(
 {
 	if (event->type == FANOTIFY_EVENT_TYPE_FID)
 		return &FANOTIFY_FE(event)->object_fh;
+	else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME)
+		return fanotify_info_file_fh(&FANOTIFY_NE(event)->info);
 	else
 		return NULL;
 }
@@ -208,9 +210,13 @@ static inline struct fanotify_info *fanotify_event_info(
 
 static inline int fanotify_event_object_fh_len(struct fanotify_event *event)
 {
+	struct fanotify_info *info = fanotify_event_info(event);
 	struct fanotify_fh *fh = fanotify_event_object_fh(event);
 
-	return fh ? fh->len : 0;
+	if (info)
+		return info->file_fh_totlen ? fh->len : 0;
+	else
+		return fh ? fh->len : 0;
 }
 
 static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 6b839790cb42..be328d7ee283 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -956,14 +956,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 		return -EINVAL;
 
 	/*
-	 * Reporting either object fid or dir fid.
 	 * Child name is reported with parent fid so requires dir fid.
+	 * If reporting child name, we can report both child fid and dir fid.
 	 */
 	switch (fid_mode) {
 	case 0:
 	case FAN_REPORT_FID:
 	case FAN_REPORT_DIR_FID:
 	case FAN_REPORT_DFID_NAME:
+	case FAN_REPORT_DFID_NAME | FAN_REPORT_FID:
 		break;
 	default:
 		return -EINVAL;
-- 
2.17.1

Re: [PATCH v5 21/22] fanotify: report parent fid + name + child fid

[Jan Kara]

On Thu 16-07-20 11:42:29, Amir Goldstein wrote:
> For a group with fanotify_init() flag FAN_REPORT_DFID_NAME, the parent
> fid and name are reported for events on non-directory objects with an
> info record of type FAN_EVENT_INFO_TYPE_DFID_NAME.
> 
> If the group also has the init flag FAN_REPORT_FID, the child fid
> is also reported with another info record that follows the first info
> record. The second info record is the same info record that would have
> been reported to a group with only FAN_REPORT_FID flag.
> 
> When the child fid needs to be recorded, the variable size struct
> fanotify_name_event is preallocated with enough space to store the
> child fh between the dir fh and the name.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify.c      | 30 ++++++++++++++++++++++++++----
>  fs/notify/fanotify/fanotify.h      |  8 +++++++-
>  fs/notify/fanotify/fanotify_user.c |  3 ++-
>  3 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index c77b37eb33a9..1d8eb886fe08 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -479,15 +479,22 @@ static struct fanotify_event *fanotify_alloc_fid_event(struct inode *id,
>  static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>  							__kernel_fsid_t *fsid,
>  							const struct qstr *file_name,
> +							struct inode *child,
>  							gfp_t gfp)
>  {
>  	struct fanotify_name_event *fne;
>  	struct fanotify_info *info;
> -	struct fanotify_fh *dfh;
> +	struct fanotify_fh *dfh, *ffh;
>  	unsigned int dir_fh_len = fanotify_encode_fh_len(id);
> +	unsigned int child_fh_len = fanotify_encode_fh_len(child);
>  	unsigned int size;
>  
> +	if (WARN_ON_ONCE(dir_fh_len % FANOTIFY_FH_HDR_LEN))
> +		child_fh_len = 0;
> +

Why this check? Do you want to check everything is 4-byte aligned? But then
FANOTIFY_FH_HDR_LEN works mostly by accident...

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

Re: [PATCH v5 21/22] fanotify: report parent fid + name + child fid

[Amir Goldstein]

On Thu, Jul 16, 2020 at 6:59 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-07-20 11:42:29, Amir Goldstein wrote:
> > For a group with fanotify_init() flag FAN_REPORT_DFID_NAME, the parent
> > fid and name are reported for events on non-directory objects with an
> > info record of type FAN_EVENT_INFO_TYPE_DFID_NAME.
> >
> > If the group also has the init flag FAN_REPORT_FID, the child fid
> > is also reported with another info record that follows the first info
> > record. The second info record is the same info record that would have
> > been reported to a group with only FAN_REPORT_FID flag.
> >
> > When the child fid needs to be recorded, the variable size struct
> > fanotify_name_event is preallocated with enough space to store the
> > child fh between the dir fh and the name.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/notify/fanotify/fanotify.c      | 30 ++++++++++++++++++++++++++----
> >  fs/notify/fanotify/fanotify.h      |  8 +++++++-
> >  fs/notify/fanotify/fanotify_user.c |  3 ++-
> >  3 files changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index c77b37eb33a9..1d8eb886fe08 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -479,15 +479,22 @@ static struct fanotify_event *fanotify_alloc_fid_event(struct inode *id,
> >  static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
> >                                                       __kernel_fsid_t *fsid,
> >                                                       const struct qstr *file_name,
> > +                                                     struct inode *child,
> >                                                       gfp_t gfp)
> >  {
> >       struct fanotify_name_event *fne;
> >       struct fanotify_info *info;
> > -     struct fanotify_fh *dfh;
> > +     struct fanotify_fh *dfh, *ffh;
> >       unsigned int dir_fh_len = fanotify_encode_fh_len(id);
> > +     unsigned int child_fh_len = fanotify_encode_fh_len(child);
> >       unsigned int size;
> >
> > +     if (WARN_ON_ONCE(dir_fh_len % FANOTIFY_FH_HDR_LEN))
> > +             child_fh_len = 0;
> > +
>
> Why this check? Do you want to check everything is 4-byte aligned? But then
> FANOTIFY_FH_HDR_LEN works mostly by accident...

Good catch.
That's indeed a mistake.
Should be % 4 which is expected because
...
        return dwords << 2;

If you think that is over defensive, you can drop it.

Thanks,
Amir.

[PATCH v5 22/22] fanotify: report parent fid + child fid

[Amir Goldstein]

Add support for FAN_REPORT_FID | FAN_REPORT_DIR_FID.
Internally, it is implemented as a private case of reporting both
parent and child fids and name, the parent and child fids are recorded
in a variable length fanotify_name_event, but there is no name.

It should be noted that directory modification events are recorded
in fixed size fanotify_fid_event when not reporting name, just like
with group flags FAN_REPORT_FID.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 1d8eb886fe08..8cb4fdeb567a 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -539,7 +539,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		/*
 		 * With both flags FAN_REPORT_DIR_FID and FAN_REPORT_FID, we
 		 * report the child fid for events reported on a non-dir child
-		 * in addition to reporting the parent fid and child name.
+		 * in addition to reporting the parent fid and maybe child name.
 		 */
 		if ((fid_mode & FAN_REPORT_FID) &&
 		    (mask & FAN_EVENT_ON_CHILD) && !(mask & FAN_ONDIR))
@@ -556,11 +556,17 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		 *
 		 * For event on non-directory that is reported to parent, we
 		 * record the fid of the parent and the name of the child.
+		 *
+		 * Even if not reporting name, we need a variable length
+		 * fanotify_name_event if reporting both parent and child fids.
 		 */
-		if ((fid_mode & FAN_REPORT_NAME) &&
-		    ((mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
-		     !(mask & FAN_ONDIR)))
+		if (!(fid_mode & FAN_REPORT_NAME)) {
+			name_event = !!child;
+			file_name = NULL;
+		} else if ((mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
+			   !(mask & FAN_ONDIR)) {
 			name_event = true;
+		}
 	}
 
 	/*
@@ -579,7 +585,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 
 	if (fanotify_is_perm_event(mask)) {
 		event = fanotify_alloc_perm_event(path, gfp);
-	} else if (name_event && file_name) {
+	} else if (name_event && (file_name || child)) {
 		event = fanotify_alloc_name_event(id, fsid, file_name, child,
 						  gfp);
 	} else if (fid_mode) {
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index be328d7ee283..559de311deca 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -371,7 +371,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 
 	/* Event info records order is: dir fid + name, child fid */
 	if (fanotify_event_dir_fh_len(event)) {
-		info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
+		info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
+					     FAN_EVENT_INFO_TYPE_DFID;
 		ret = copy_info_to_user(fanotify_event_fsid(event),
 					fanotify_info_dir_fh(info),
 					info_type, fanotify_info_name(info),
@@ -957,18 +958,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
 	/*
 	 * Child name is reported with parent fid so requires dir fid.
-	 * If reporting child name, we can report both child fid and dir fid.
+	 * We can report both child fid and dir fid with or without name.
 	 */
-	switch (fid_mode) {
-	case 0:
-	case FAN_REPORT_FID:
-	case FAN_REPORT_DIR_FID:
-	case FAN_REPORT_DFID_NAME:
-	case FAN_REPORT_DFID_NAME | FAN_REPORT_FID:
-		break;
-	default:
+	if ((fid_mode & FAN_REPORT_NAME) && !(fid_mode & FAN_REPORT_DIR_FID))
 		return -EINVAL;
-	}
 
 	user = get_current_user();
 	if (atomic_read(&user->fanotify_listeners) > FANOTIFY_DEFAULT_MAX_LISTENERS) {
-- 
2.17.1

Re: [PATCH v5 00/22] fanotify events with name info

[Jan Kara]

Hi Amir!

On Thu 16-07-20 11:42:08, Amir Goldstein wrote:
> This patch set implements the FAN_REPORT_NAME and FAN_REPORT_DIR_FID
> group flags.
> 
> I previously posted v3 of prep patch series [1] and v4 of follow up
> series [2].  Since then you pick up several prep patches, so this
> posting includes the rest of the prep patches along with the followup
> patches with most of your comments addressed.
> 
> Regarding the use of flag FS_EVENT_ON_CHILD and the TYPE_CHILD mark
> iterator, I did not change that because I was not sure about it and it
> is an internal implementation detail that we can change later.
> But the discussion about it made me realize that dnotify event handler
> wasn't properly adapted, so I added a patch to fix it.
> 
> The patches are available on github [3] based on your fsnotify branch.
> man-pages [4] LTP tests [5] and a demo [6] are also available.

Phew! So I went through the patches. I didn't find any bug besides couple
of typos I've fixed and then couple of things I've flagged at individual
patches (which I've fixed up locally as well). There's just that
ignore_mask handling issue outstanding. Overall I have to say I'm unhappy
about the complexity of juggling with dir/inode/child, dirfh vs objfh, etc.
I acknowledge that the stuff is at least well commented so I was able to
grok it but still... That being said I don't have a great idea how to
simplify all this so at this point I'm ok with merging things as they are
but once all the functionality is in I want to have a look at how to
simplify all the special cases.

Anyway, for now, thanks for your persistence and work when developing this
series!

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