[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 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 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.