[PATCH v3 0/7] fanotify events on child with name info

[PATCH v3 0/7] fanotify events on child with name info

[Amir Goldstein]

Jan,

In the v3 posting of the name info patches [1] I dropped the
FAN_REPORT_NAME patches as agreed to defer them to next cycle.

Following is remainder of the series to complement the FAN_DIR_MODIFY
patches that were merged to v5.7-rc1.

The v3 patches are available on my github branch fanotify_name [2].
Same branch names for LTP tests [3], man page draft [4] and a demo [5].

Patches 1-4 are cleanup and minor re-factoring in prep for the name
info patches.

Patch 5 adds the FAN_REPORT_NAME flag and the new event reporting format
combined of FAN_EVENT_INFO_TYPE_DFID_NAME and FAN_EVENT_INFO_TYPE_FID
info records, but provides not much added value beyond inotify.

Patches 6-7 add the new capability of filesystem/mount watch with events
including name info.

I have made an API decision that stems from consolidating the
implementation with fsnotify_parent() that requires your approval -
A filesystem/mount mark with FAN_REPORT_NAME behaves as if all the
directories and inodes are marked.  This results in user getting all
relevant events in two flavors - one with both info records and one with just
FAN_EVENT_INFO_TYPE_FID.  I have tries several approaches to work around this
bizarrity, but in the end I decided that would be the lesser evil and that
bizarre behavior is at least easy to document.

Let me know what you think.
Thanks,
Amir.

Main changes since v2:
- FAN_DIR_MODIFY patches have been merged
- A few more clean patches
- More text about the motivation (in "report parent fid + name" patch)
- Reduce code duplication with fsnotify_parent()

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

Amir Goldstein (7):
  fanotify: create overflow event type
  fanotify: break up fanotify_alloc_event()
  fanotify: generalize the handling of extra event flags
  fanotify: distinguish between fid encode error and null fid
  fanotify: report parent fid + name for events on children
  fsnotify: send event "on child" to sb/mount marks
  fanotify: report events "on child" with name info to sb/mount marks

 fs/notify/fanotify/fanotify.c      | 213 +++++++++++++++++------------
 fs/notify/fanotify/fanotify.h      |  18 ++-
 fs/notify/fanotify/fanotify_user.c |  46 +++++--
 fs/notify/fsnotify.c               |  38 ++++-
 include/linux/fanotify.h           |   2 +-
 include/linux/fsnotify_backend.h   |  23 +++-
 include/uapi/linux/fanotify.h      |   4 +
 7 files changed, 231 insertions(+), 113 deletions(-)

-- 
2.17.1

[PATCH v3 1/7] fanotify: create overflow event type

[Amir Goldstein]

The special overflow event is allocated as struct fanotify_path_event,
but with a null path.

Use a special event type to identify the overflow event, so the helper
fanotify_has_event_path() will always indicate a non null path.

Allocating the overflow event doesn't need any of the fancy stuff in
fanotify_alloc_event(), so create a simplified helper for allocating the
overflow event.

There is also no need to store and report the pid with an overflow event.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 27 +++++++++++----------------
 fs/notify/fanotify/fanotify.h      | 15 +++++++++------
 fs/notify/fanotify/fanotify_user.c | 21 ++++++++++++++++-----
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 5435a40f82be..ce4677077118 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -341,11 +341,11 @@ static struct inode *fanotify_fid_inode(struct inode *to_tell, u32 event_mask,
 	return (struct inode *)fsnotify_data_inode(data, data_type);
 }
 
-struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
-					    struct inode *inode, u32 mask,
-					    const void *data, int data_type,
-					    const struct qstr *file_name,
-					    __kernel_fsid_t *fsid)
+static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
+						struct inode *inode, u32 mask,
+						const void *data, int data_type,
+						const struct qstr *file_name,
+						__kernel_fsid_t *fsid)
 {
 	struct fanotify_event *event = NULL;
 	struct fanotify_fid_event *ffe = NULL;
@@ -423,8 +423,7 @@ struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	 * event queue, so event reported on parent is merged with event
 	 * reported on child when both directory and child watches exist.
 	 */
-	fsnotify_init_event(&event->fse, (unsigned long)id);
-	event->mask = mask;
+	fanotify_init_event(event, (unsigned long)id, mask);
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_TID))
 		event->pid = get_pid(task_pid(current));
 	else
@@ -440,15 +439,8 @@ struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		fanotify_encode_fh(fanotify_event_dir_fh(event), id, gfp);
 
 	if (fanotify_event_has_path(event)) {
-		struct path *p = fanotify_event_path(event);
-
-		if (path) {
-			*p = *path;
-			path_get(path);
-		} else {
-			p->mnt = NULL;
-			p->dentry = NULL;
-		}
+		*fanotify_event_path(event) = *path;
+		path_get(path);
 	}
 out:
 	memalloc_unuse_memcg();
@@ -637,6 +629,9 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 	case FANOTIFY_EVENT_TYPE_FID_NAME:
 		fanotify_free_name_event(event);
 		break;
+	case FANOTIFY_EVENT_TYPE_OVERFLOW:
+		kfree(event);
+		break;
 	default:
 		WARN_ON_ONCE(1);
 	}
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 35bfbf4a7aac..3adbb9f7d1a8 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -63,6 +63,7 @@ enum fanotify_event_type {
 	FANOTIFY_EVENT_TYPE_FID_NAME, /* variable length */
 	FANOTIFY_EVENT_TYPE_PATH,
 	FANOTIFY_EVENT_TYPE_PATH_PERM,
+	FANOTIFY_EVENT_TYPE_OVERFLOW, /* struct fanotify_event */
 };
 
 struct fanotify_event {
@@ -72,6 +73,14 @@ struct fanotify_event {
 	struct pid *pid;
 };
 
+static inline void fanotify_init_event(struct fanotify_event *event,
+				       unsigned long id, u32 mask)
+{
+	fsnotify_init_event(&event->fse, id);
+	event->mask = mask;
+	event->pid = NULL;
+}
+
 struct fanotify_fid_event {
 	struct fanotify_event fae;
 	__kernel_fsid_t fsid;
@@ -202,9 +211,3 @@ static inline struct path *fanotify_event_path(struct fanotify_event *event)
 	else
 		return NULL;
 }
-
-struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
-					    struct inode *inode, u32 mask,
-					    const void *data, int data_type,
-					    const struct qstr *file_name,
-					    __kernel_fsid_t *fsid);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 42cb794c62ac..8c6d22ec7b41 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -829,13 +829,26 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				 FSNOTIFY_OBJ_TYPE_INODE, mask, flags, fsid);
 }
 
+static struct fsnotify_event *fanotify_alloc_overflow_event(void)
+{
+	struct fanotify_event *oevent;
+
+	oevent = kmalloc(sizeof(*oevent), GFP_KERNEL_ACCOUNT);
+	if (!oevent)
+		return NULL;
+
+	fanotify_init_event(oevent, 0, FS_Q_OVERFLOW);
+	oevent->type = FANOTIFY_EVENT_TYPE_OVERFLOW;
+
+	return &oevent->fse;
+}
+
 /* fanotify syscalls */
 SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 {
 	struct fsnotify_group *group;
 	int f_flags, fd;
 	struct user_struct *user;
-	struct fanotify_event *oevent;
 
 	pr_debug("%s: flags=%x event_f_flags=%x\n",
 		 __func__, flags, event_f_flags);
@@ -890,13 +903,11 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	atomic_inc(&user->fanotify_listeners);
 	group->memcg = get_mem_cgroup_from_mm(current->mm);
 
-	oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL,
-				      FSNOTIFY_EVENT_NONE, NULL, NULL);
-	if (unlikely(!oevent)) {
+	group->overflow_event = fanotify_alloc_overflow_event();
+	if (unlikely(!group->overflow_event)) {
 		fd = -ENOMEM;
 		goto out_destroy_group;
 	}
-	group->overflow_event = &oevent->fse;
 
 	if (force_o_largefile())
 		event_f_flags |= O_LARGEFILE;
-- 
2.17.1

[PATCH v3 2/7] fanotify: break up fanotify_alloc_event()

[Amir Goldstein]

Break up fanotify_alloc_event() into helpers by event struct type.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 146 ++++++++++++++++++++--------------
 1 file changed, 85 insertions(+), 61 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index ce4677077118..f5c21949fdaa 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -341,6 +341,77 @@ static struct inode *fanotify_fid_inode(struct inode *to_tell, u32 event_mask,
 	return (struct inode *)fsnotify_data_inode(data, data_type);
 }
 
+struct fanotify_event *fanotify_alloc_path_event(const struct path *path,
+						 gfp_t gfp)
+{
+	struct fanotify_path_event *pevent;
+
+	pevent = kmem_cache_alloc(fanotify_path_event_cachep, gfp);
+	if (!pevent)
+		return NULL;
+
+	pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH;
+	pevent->path = *path;
+	path_get(path);
+
+	return &pevent->fae;
+}
+
+struct fanotify_event *fanotify_alloc_perm_event(const struct path *path,
+						 gfp_t gfp)
+{
+	struct fanotify_perm_event *pevent;
+
+	pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
+	if (!pevent)
+		return NULL;
+
+	pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM;
+	pevent->response = 0;
+	pevent->state = FAN_EVENT_INIT;
+	pevent->path = *path;
+	path_get(path);
+
+	return &pevent->fae;
+}
+
+struct fanotify_event *fanotify_alloc_fid_event(struct inode *id,
+						__kernel_fsid_t *fsid,
+						gfp_t gfp)
+{
+	struct fanotify_fid_event *ffe;
+
+	ffe = kmem_cache_alloc(fanotify_fid_event_cachep, gfp);
+	if (!ffe)
+		return NULL;
+
+	ffe->fae.type = FANOTIFY_EVENT_TYPE_FID;
+	ffe->fsid = *fsid;
+	fanotify_encode_fh(&ffe->object_fh, id, gfp);
+
+	return &ffe->fae;
+}
+
+struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
+						 __kernel_fsid_t *fsid,
+						 const struct qstr *file_name,
+						 gfp_t gfp)
+{
+	struct fanotify_name_event *fne;
+
+	fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp);
+	if (!fne)
+		return NULL;
+
+	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);
+
+	return &fne->fae;
+}
+
 static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						struct inode *inode, u32 mask,
 						const void *data, int data_type,
@@ -348,8 +419,6 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						__kernel_fsid_t *fsid)
 {
 	struct fanotify_event *event = NULL;
-	struct fanotify_fid_event *ffe = NULL;
-	struct fanotify_name_event *fne = NULL;
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
 	struct inode *id = fanotify_fid_inode(inode, mask, data, data_type);
 	const struct path *path = fsnotify_data_path(data, data_type);
@@ -369,55 +438,23 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	memalloc_use_memcg(group->memcg);
 
 	if (fanotify_is_perm_event(mask)) {
-		struct fanotify_perm_event *pevent;
-
-		pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
-		if (!pevent)
-			goto out;
-
-		event = &pevent->fae;
-		event->type = FANOTIFY_EVENT_TYPE_PATH_PERM;
-		pevent->response = 0;
-		pevent->state = FAN_EVENT_INIT;
-		goto init;
-	}
-
-	/*
-	 * For FAN_DIR_MODIFY event, we report the fid of the directory and
-	 * the name of the modified entry.
-	 * Allocate an fanotify_name_event struct and copy the name.
-	 */
-	if (mask & FAN_DIR_MODIFY && !(WARN_ON_ONCE(!file_name))) {
-		fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp);
-		if (!fne)
-			goto out;
-
-		event = &fne->fae;
-		event->type = FANOTIFY_EVENT_TYPE_FID_NAME;
-		fne->name_len = file_name->len;
-		strcpy(fne->name, file_name->name);
-		goto init;
-	}
-
-	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
-		ffe = kmem_cache_alloc(fanotify_fid_event_cachep, gfp);
-		if (!ffe)
-			goto out;
-
-		event = &ffe->fae;
-		event->type = FANOTIFY_EVENT_TYPE_FID;
+		event = fanotify_alloc_perm_event(path, gfp);
+	} else if (mask & FAN_DIR_MODIFY && !(WARN_ON_ONCE(!file_name))) {
+		/*
+		 * For FAN_DIR_MODIFY event, we report the fid of the directory
+		 * and the name of the modified entry.
+		 * Allocate an fanotify_name_event struct and copy the name.
+		 */
+		event = fanotify_alloc_name_event(id, fsid, file_name, gfp);
+	} else if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+		event = fanotify_alloc_fid_event(id, fsid, gfp);
 	} else {
-		struct fanotify_path_event *pevent;
-
-		pevent = kmem_cache_alloc(fanotify_path_event_cachep, gfp);
-		if (!pevent)
-			goto out;
-
-		event = &pevent->fae;
-		event->type = FANOTIFY_EVENT_TYPE_PATH;
+		event = fanotify_alloc_path_event(path, gfp);
 	}
 
-init:
+	if (!event)
+		goto out;
+
 	/*
 	 * Use the victim inode instead of the watching inode as the id for
 	 * event queue, so event reported on parent is merged with event
@@ -429,19 +466,6 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	else
 		event->pid = get_pid(task_tgid(current));
 
-	if (fsid && fanotify_event_fsid(event))
-		*fanotify_event_fsid(event) = *fsid;
-
-	if (fanotify_event_object_fh(event))
-		fanotify_encode_fh(fanotify_event_object_fh(event), id, gfp);
-
-	if (fanotify_event_dir_fh(event))
-		fanotify_encode_fh(fanotify_event_dir_fh(event), id, gfp);
-
-	if (fanotify_event_has_path(event)) {
-		*fanotify_event_path(event) = *path;
-		path_get(path);
-	}
 out:
 	memalloc_unuse_memcg();
 	return event;
-- 
2.17.1

[PATCH v3 3/7] 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, and we will soon need to mask it out.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index f5c21949fdaa..1e4a345155dd 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -264,11 +264,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 	 * to user in FAN_REPORT_FID mode for all event types.
 	 */
 	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 v3 4/7] 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 needed because fanotify_fh_equal() treats FILEID_INVALID
specially and we are going to need fanotify_fh_equal() to return true
when comparing two null fids.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 1e4a345155dd..bdafc76cc258 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -282,8 +282,11 @@ static void fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	void *buf = fh->buf;
 	int err;
 
+	/* FAN_DIR_MODIFY does not encode object fh */
+	fh->type = FILEID_ROOT;
+	fh->len = 0;
 	if (!inode)
-		goto out;
+		return;
 
 	dwords = 0;
 	err = -ENOENT;
@@ -318,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 v3 5/7] fanotify: report parent fid + name for events on children

[Amir Goldstein]

An application that needs to index or mirror the content of a large
directory needs an efficient way to be notified on changes in that
directory without re-scanning the entire directory on every change.

The event FAN_DIR_MODIFY provides the means to get a notification on
directory entry changes, but not on child modifications. fanotify
provides the capability to get events on changes to children of a watched
directory, but getting the name of the child is not reliable and this
is something that an application of that sort needs.

As a matter of fact, inotify already provides this capability, but it
does not scale to watching the entire filesystem.
We are adding the basic capability to fanotify only for directory marks
for now and soon we will add the ability to watch an entire filesystem
or mount with a reliable [*] way to get the path on the event.

[*] reliable above means that we can stat the file by dirfid+name and
know for sure of the event reported was for that file.

A new fanotify_init() flag FAN_REPORT_NAME is introduced.  It requires
flag FAN_REPORT_FID and there is a constant for setting both flags named
FAN_REPORT_FID_NAME.

For a group with fanotify_init() flag FAN_REPORT_NAME, the parent fid and
name are reported for events "on child" to a directory watching children.
The parent fid and name are reported with an info record of type
FAN_EVENT_INFO_TYPE_DFID_NAME, similar to the way that name info is
reported for FAN_DIR_MODIFY events.

The child fid itself is reported with another info record of type
FAN_EVENT_INFO_TYPE_FID that follows the first info record, with the
same fid info as reported to a group with only FAN_REPORT_FID flag.

FAN_DIR_MODIFY events do not record nor report the "child" fid, but in
order to stay consistent with events "on child", we store those events
in struct fanotify_name_event with an empty object_fh.

This wastes a few unused bytes (16) of memory per FAN_DIR_MODIFY event,
but keeps the code simpler and avoids creating a custom event struct
just for FAN_DIR_MODIFY events.

Cc: <linux-api@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 33 ++++++++++++++++++++++++------
 fs/notify/fanotify/fanotify.h      |  3 +++
 fs/notify/fanotify/fanotify_user.c |  6 +++++-
 include/linux/fanotify.h           |  2 +-
 include/uapi/linux/fanotify.h      |  4 ++++
 5 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index bdafc76cc258..e91a8cc1b83c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -64,7 +64,8 @@ static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
 		return false;
 
 	if (fne1->name_len != fne2->name_len ||
-	    !fanotify_fh_equal(&fne1->dir_fh, &fne2->dir_fh))
+	    !fanotify_fh_equal(&fne1->dir_fh, &fne2->dir_fh) ||
+	    !fanotify_fh_equal(&fne1->object_fh, &fne2->object_fh))
 		return false;
 
 	return !memcmp(fne1->name, fne2->name, fne1->name_len);
@@ -211,7 +212,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;
@@ -262,11 +264,17 @@ 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.
+	 *
+	 * We never report FAN_EVENT_ON_CHILD to user, but we do pass it in to
+	 * fanotify_alloc_event() for group with FAN_REPORT_NAME as indication
+	 * of how the event should be reported.
 	 */
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
 		/* Do not report event flags without any event */
 		if (!(test_mask & ~FANOTIFY_EVENT_FLAGS))
 			return 0;
+		if (!FAN_GROUP_FLAG(group, FAN_REPORT_NAME))
+			user_mask &= ~FAN_EVENT_ON_CHILD;
 	} else {
 		user_mask &= ~FANOTIFY_EVENT_FLAGS;
 	}
@@ -394,10 +402,10 @@ struct fanotify_event *fanotify_alloc_fid_event(struct inode *id,
 	return &ffe->fae;
 }
 
-struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
+struct fanotify_event *fanotify_alloc_name_event(struct inode *dir,
 						 __kernel_fsid_t *fsid,
 						 const struct qstr *file_name,
-						 gfp_t gfp)
+						 struct inode *id, gfp_t gfp)
 {
 	struct fanotify_name_event *fne;
 
@@ -407,7 +415,8 @@ 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);
+	fanotify_encode_fh(&fne->dir_fh, dir, gfp);
+	fanotify_encode_fh(&fne->object_fh, id, gfp);
 	fne->name_len = file_name->len;
 	strcpy(fne->name, file_name->name);
 
@@ -447,7 +456,17 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		 * and the name of the modified entry.
 		 * Allocate an fanotify_name_event struct and copy the name.
 		 */
-		event = fanotify_alloc_name_event(id, fsid, file_name, gfp);
+		event = fanotify_alloc_name_event(id, fsid, file_name, NULL,
+						  gfp);
+	} else if (FAN_GROUP_FLAG(group, FAN_REPORT_NAME) &&
+		   (mask & FAN_EVENT_ON_CHILD) && likely(inode != id)) {
+		/*
+		 * With flag FAN_REPORT_NAME, we report the parent fid and name
+		 * for events reported "on child" in addition to reporting the
+		 * child fid.
+		 */
+		event = fanotify_alloc_name_event(inode, fsid, file_name, id,
+						  gfp);
 	} else if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
 		event = fanotify_alloc_fid_event(id, fsid, gfp);
 	} else {
@@ -631,6 +650,8 @@ static void fanotify_free_name_event(struct fanotify_event *event)
 {
 	struct fanotify_name_event *fne = FANOTIFY_NE(event);
 
+	if (fanotify_fh_has_ext_buf(&fne->object_fh))
+		kfree(fanotify_fh_ext_buf(&fne->object_fh));
 	if (fanotify_fh_has_ext_buf(&fne->dir_fh))
 		kfree(fanotify_fh_ext_buf(&fne->dir_fh));
 	kfree(fne);
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 3adbb9f7d1a8..9aafccc23c75 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -96,6 +96,7 @@ FANOTIFY_FE(struct fanotify_event *event)
 struct fanotify_name_event {
 	struct fanotify_event fae;
 	__kernel_fsid_t fsid;
+	struct fanotify_fh object_fh;
 	struct fanotify_fh dir_fh;
 	u8 name_len;
 	char name[0];
@@ -122,6 +123,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_NE(event)->object_fh;
 	else
 		return NULL;
 }
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8c6d22ec7b41..030534da49e2 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -879,6 +879,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	    (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF)
 		return -EINVAL;
 
+	/* Child name is reported with parent fid */
+	if ((flags & FAN_REPORT_NAME) && !(flags & FAN_REPORT_FID))
+		return -EINVAL;
+
 	user = get_current_user();
 	if (atomic_read(&user->fanotify_listeners) > FANOTIFY_DEFAULT_MAX_LISTENERS) {
 		free_uid(user);
@@ -1212,7 +1216,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 3049a6c06d9e..5412a25c54c0 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -19,7 +19,7 @@
 				 FAN_CLASS_PRE_CONTENT)
 
 #define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | \
-				 FAN_REPORT_TID | FAN_REPORT_FID | \
+				 FAN_REPORT_TID | FAN_REPORT_FID_NAME | \
 				 FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index a88c7c6d0692..41f54a0f360f 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -54,6 +54,10 @@
 /* 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_NAME		0x00000400	/* Report events with name */
+
+/* Convenience macro - FAN_REPORT_NAME requires FAN_REPORT_FID */
+#define FAN_REPORT_FID_NAME	(FAN_REPORT_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 v3 6/7] fsnotify: send event "on child" to sb/mount marks

[Amir Goldstein]

Similar to events "on child" to watching directory, send event flavor
"on child" to sb/mount marks that specified interest in this flavor.
Event "on child" will not be sent on "orphan" children, that is, on
disconnected dentries and on a mount/sb root.

Currently, fanotify allows to set the FAN_EVENT_ON_CHILD flag on
sb/mount marks, but it was ignored.  Mask the flag explicitly until
fanotify implements support for events "on child" with name info
for sb/mount marks.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c |  7 ++++--
 fs/notify/fsnotify.c               | 38 ++++++++++++++++++++++++------
 include/linux/fsnotify_backend.h   | 23 ++++++++++++------
 3 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 030534da49e2..36c1327b32f4 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1146,10 +1146,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	}
 
 	/* inode held in place by reference to path; group by fget on fd */
-	if (mark_type == FAN_MARK_INODE)
+	if (mark_type == FAN_MARK_INODE) {
 		inode = path.dentry->d_inode;
-	else
+	} else {
 		mnt = path.mnt;
+		/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount marks */
+		mask &= ~FAN_EVENT_ON_CHILD;
+	}
 
 	/* create/update an inode mark */
 	switch (flags & (FAN_MARK_ADD | FAN_MARK_REMOVE)) {
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 72d332ce8e12..f6da8f263bc0 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -142,23 +142,50 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
 	spin_unlock(&inode->i_lock);
 }
 
-/* Notify this dentry's parent about a child's events. */
+/*
+ * Notify this dentry's parent about a child's events if parent is watching
+ * children or if sb/mount marks are interested in events with file_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 = NULL;
+	bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED;
+	__u32 test_mask = mask & FS_EVENTS_POSS_ON_CHILD;
+	__u32 p_mask, marks_mask = 0;
 	struct dentry *parent;
 	struct inode *p_inode;
 	int ret = 0;
 
-	if (!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))
+	if (path && path->mnt->mnt_root != dentry)
+		mnt = real_mount(path->mnt);
+
+	/*
+	 * FS_EVENT_ON_CHILD on sb/mount mask implies reporting events as if all
+	 * directories are watched.
+	 */
+	if (!IS_ROOT(dentry))
+		marks_mask |= fsnotify_watches_children(
+						dentry->d_sb->s_fsnotify_mask);
+	if (mnt)
+		marks_mask |= fsnotify_watches_children(
+						mnt->mnt_fsnotify_mask);
+
+
+	if (!(marks_mask & test_mask) && !parent_watched)
 		return 0;
 
 	parent = dget_parent(dentry);
 	p_inode = parent->d_inode;
+	p_mask = fsnotify_inode_watches_children(p_inode);
 
-	if (unlikely(!fsnotify_inode_watches_children(p_inode))) {
+	if (p_mask)
+		marks_mask |= p_mask;
+	else if (unlikely(parent_watched))
 		__fsnotify_update_child_dentry_flags(p_inode);
-	} else if (p_inode->i_fsnotify_mask & mask & ALL_FSNOTIFY_EVENTS) {
+
+	if (marks_mask & test_mask) {
 		struct name_snapshot name;
 
 		/* we are notifying a parent so come up with the new mask which
@@ -323,9 +350,6 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 		mnt = real_mount(path->mnt);
 		mnt_or_sb_mask |= mnt->mnt_fsnotify_mask;
 	}
-	/* An event "on child" is not intended for a mount/sb mark */
-	if (mask & FS_EVENT_ON_CHILD)
-		mnt_or_sb_mask = 0;
 
 	/*
 	 * Optimization: srcu_read_lock() has a memory barrier which can
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index f0c506405b54..ca461b95662a 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -50,8 +50,12 @@
 #define FS_DIR_MODIFY		0x00080000	/* Directory entry was modified */
 
 #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. */
+/*
+ * This inode cares about things that happen to its children.
+ * Always set for dnotify and inotify.
+ * Set on sb/mount marks mask to indicate interest in getting events with
+ * file_name information.
+ */
 #define FS_EVENT_ON_CHILD	0x08000000
 
 #define FS_DN_RENAME		0x10000000	/* file renamed */
@@ -386,14 +390,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 int fsnotify_inode_watches_children(struct inode *inode)
+static inline __u32 fsnotify_watches_children(__u32 mask)
 {
-	/* FS_EVENT_ON_CHILD is set if the inode may care */
-	if (!(inode->i_fsnotify_mask & FS_EVENT_ON_CHILD))
+	/* FS_EVENT_ON_CHILD is set if the object may care */
+	if (!(mask & FS_EVENT_ON_CHILD))
 		return 0;
-	/* this inode might care about child events, does it care about the
+	/* This object might care about child events, does it care about the
 	 * specific set of events that can happen on a child? */
-	return inode->i_fsnotify_mask & FS_EVENTS_POSS_ON_CHILD;
+	return mask & FS_EVENTS_POSS_ON_CHILD;
+}
+
+static inline int fsnotify_inode_watches_children(struct inode *inode)
+{
+	return fsnotify_watches_children(inode->i_fsnotify_mask);
 }
 
 /*
-- 
2.17.1

[PATCH v3 7/7] fanotify: report events "on child" with name info to sb/mount marks

[Amir Goldstein]

With fanotify_init() flags FAN_REPORT_FID_NAME, when adding an inode
mark with FS_EVENT_ON_CHILD, events are reported with fid of the parent
and the name of the child entry.

When adding a filesystem or mount mark, report events that are "possible
on child" (e.g. open/close) in two flavors, one with just the child fid
and one also with the parent fid and the child entry name, as if all
directories are interested in events "on child".

The flag FAN_EVENT_ON_CHILD was always ignored for sb/mount mark, so we
can safely ignore the value of the flag passed by the user and set the
flag in sb/mount mark mask depending on the FAN_REPORT_NAME group flag.

Cc: <linux-api@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      |  7 +++++--
 fs/notify/fanotify/fanotify_user.c | 16 ++++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index e91a8cc1b83c..ec42c721850c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -244,10 +244,13 @@ 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!
+		 * An event "on child" without name info is not intended for a
+		 * mount/sb mark.
 		 */
 		if (event_mask & FS_EVENT_ON_CHILD &&
-		    (type != FSNOTIFY_OBJ_TYPE_INODE ||
-		     !(mark->mask & FS_EVENT_ON_CHILD)))
+		    (!(mark->mask & FS_EVENT_ON_CHILD) ||
+		     (type != FSNOTIFY_OBJ_TYPE_INODE &&
+		      !FAN_GROUP_FLAG(group, FAN_REPORT_NAME))))
 			continue;
 
 		marks_mask |= mark->mask;
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 36c1327b32f4..89c0554da90e 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1150,8 +1150,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		inode = path.dentry->d_inode;
 	} else {
 		mnt = path.mnt;
-		/* Mask out FAN_EVENT_ON_CHILD flag for sb/mount marks */
-		mask &= ~FAN_EVENT_ON_CHILD;
+		/*
+		 * So far, flag FAN_EVENT_ON_CHILD was ignored for sb/mount
+		 * marks.  Reporting events "on child" with name info for
+		 * sb/mount marks is now implemented, so explicitly mask out
+		 * the flag for backward compatibility with existing programs
+		 * that do not request events with name info.
+		 * On sb/mount mark events with FAN_REPORT_NAME, events are
+		 * reported as if all directories are interested in events
+		 * "on child".
+		 */
+		if (FAN_GROUP_FLAG(group, FAN_REPORT_NAME))
+			mask |= FAN_EVENT_ON_CHILD;
+		else
+			mask &= ~FAN_EVENT_ON_CHILD;
 	}
 
 	/* create/update an inode mark */
-- 
2.17.1

Re: [PATCH v3 0/7] fanotify events on child with name info

[Amir Goldstein]

On Tue, May 5, 2020 at 7:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Jan,
>
> In the v3 posting of the name info patches [1] I dropped the
> FAN_REPORT_NAME patches as agreed to defer them to next cycle.
>
> Following is remainder of the series to complement the FAN_DIR_MODIFY
> patches that were merged to v5.7-rc1.
>
> The v3 patches are available on my github branch fanotify_name [2].
> Same branch names for LTP tests [3], man page draft [4] and a demo [5].
>
> Patches 1-4 are cleanup and minor re-factoring in prep for the name
> info patches.
>
> Patch 5 adds the FAN_REPORT_NAME flag and the new event reporting format
> combined of FAN_EVENT_INFO_TYPE_DFID_NAME and FAN_EVENT_INFO_TYPE_FID
> info records, but provides not much added value beyond inotify.
>
> Patches 6-7 add the new capability of filesystem/mount watch with events
> including name info.
>
> I have made an API decision that stems from consolidating the
> implementation with fsnotify_parent() that requires your approval -
> A filesystem/mount mark with FAN_REPORT_NAME behaves as if all the
> directories and inodes are marked.  This results in user getting all
> relevant events in two flavors - one with both info records and one with just
> FAN_EVENT_INFO_TYPE_FID.  I have tries several approaches to work around this
> bizarrity, but in the end I decided that would be the lesser evil and that
> bizarre behavior is at least easy to document.
>

Hi Jan,

Were you able to give some thought to the API question above?

I would really like to be able to finalize the design of the API, so
that I will be able to continue working on the man page patches.

Re-phrasing the API question that needs addressing:

With FAN_REPORT_NAME, filesystem/mount watches get -
1. ONLY events with name (no events on root and no SELF events)
2. Each event in one flavor (with name info when available)
3. ALL events in both flavors (where name info is available)
4. Something else?

The current v3 patches implement API choice #3, which is derived
from the implementation choice to emit two events via fsnotify_parent().

My v2 patches implemented API choice #2, which was the reason
for duplicating name snapshot code inside handle_event().
I considered implementing merge of event with and without name,
but it seemed too ugly to live.

We could also go for an API that allows any combination of
FAN_REPORT_NAME and FAN_REPORT_FID:
FAN_REPORT_FID - current upstream behavior
FAN_REPORT_FID_NAME - choice #3 above as implemented in v3
FAN_REPORT_NAME - choice #1 above

At one point, I also considered that user needs to opt-in
for named events per filesystem/mount mark with
FAN_EVENT_ON_CHILD. This flag is implied in v3 for
all filesystem/mount marks by FAN_REPORT_NAME, while
for directory marks it is opt-in as it has always been.

At the moment I went with choice #3 in v3 because I felt it
would be the simplest of all choices to document.

I didn't want to invest time in documenting behavior if you find it
unacceptable. If you are swaying between more than one option,
then I am willing to try documenting more than one option, so we
can see what the result looks like.

Thanks,
Amir.