[PATCH v4 00/15] fanotify: add support for more event types

[PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

Jan,

This is the 4th revision of patch series to add support for filesystem
change monitoring to fanotify.
It incorporates the changes you requested in review of v3 FAN_REPORT_FID
patches.
The complete work is available on fanotify_dirent branch [1] on my tree.

The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
dirent modification events is demonstrated with a prototype of global
filesystem monitor based on inotify-tools [2].

In your review of v3 patched you only got as far as patch v3 9/13.
Because this patch marks the end of the FAN_REPORT_FID sub series,
I found it best to re-post the entire series with the changes you
requested thus far. For convenience of review, I pushed branches
fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
reviewed so far and its re-worked version.

One thing that we discussed and I did NOT do is move struct file_handle
to uapi headers. This got complicated due to existing definitions in
glibc header files and I realized we could do without it.

I have added the vfs_get_fsid() helper as you requested, but since it
wasn't required by the patch set, I added it as two new cleanup patches
at the end of the FAN_REPORT_FID series, so you will be able to stage
the feature with or without the VFS change.

Thanks,
Amir.

Changes since v3:
- Re-organize structs fsnotify_event and fanotify_event to pack
  the fields better on 64bit arch (NEW patch "move mask out of...")
- Re-arrange building event fid in two main patches: "encode fid..."
  to in-kernel event struct and "copy event fid..." to uapi format
- Avoid exporting statfs_by_dentry() and add 2 NEW cleanup patches
  to use new helper vfs_get_fsid()
- Replace dodgy code using pointer bit 0 with proper type field
  in fanotify_event to indicate if path or fid info was stored
- Store up to 12 bytes (on 32bit) or 16 bytes (on 64bit) of embedded
  file handle in fanotify_event to refrain from small memory allocations
- Re-arrange code in copy_event_to_user(), so adding copy of fid to
  user is less convoluted
- Replace convoluted code to verify all marks for a reported event
  have the same cached fsid with simpler code than uses any cached fsid

Changes since v2:
- Discard FSNOTIFY_EVENT_DENTRY data type changes
- Cache fsid in connector instead of calling vfs_statfs() on every event
- Deny setting fid watch on filesystem with no fsid (tmpfs)
- Deny setting fid watch on filesystem with non root fsid (btrfs subvol)
- Report FAN_ONDIR for all event types with FAN_REPORT_FID

[1] https://github.com/amir73il/linux/commits/fanotify_dirent
[2] https://github.com/amir73il/inotify-tools/commits/fanotify_dirent
[3] https://github.com/amir73il/linux/commits/fanotify_fid-v3
[4] https://github.com/amir73il/linux/commits/fanotify_fid-v4

Amir Goldstein (15):
  fsnotify: annotate directory entry modification events
  fsnotify: send all event types to super block marks
  fsnotify: move mask out of struct fsnotify_event
  fanotify: rename struct fanotify_{,perm_}event_info
  fanotify: open code fill_event_metadata()
  fanotify: encode file identifier for FAN_REPORT_FID
  fanotify: copy event fid info to user
  fanotify: enable FAN_REPORT_FID init flag
  fanotify: cache fsid in fsnotify_mark_connector
  vfs: add vfs_get_fsid() helper
  fanotify: use vfs_get_fsid() helper instead of vfs_statfs()
  fanotify: check FS_ISDIR flag instead of d_is_dir()
  fanotify: support events with data type FSNOTIFY_EVENT_INODE
  fanotify: add support for create/attrib/move/delete events
  fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

 fs/notify/fanotify/fanotify.c        | 228 +++++++++++++++++----
 fs/notify/fanotify/fanotify.h        | 109 ++++++++--
 fs/notify/fanotify/fanotify_user.c   | 284 +++++++++++++++++++--------
 fs/notify/fsnotify.c                 |  15 +-
 fs/notify/inotify/inotify.h          |   1 +
 fs/notify/inotify/inotify_fsnotify.c |   9 +-
 fs/notify/inotify/inotify_user.c     |   5 +-
 fs/notify/mark.c                     |  44 ++++-
 fs/notify/notification.c             |  22 +--
 fs/statfs.c                          |  14 ++
 include/linux/fanotify.h             |  33 +++-
 include/linux/fsnotify.h             |  45 ++++-
 include/linux/fsnotify_backend.h     |  70 ++++---
 include/linux/statfs.h               |   3 +
 include/uapi/linux/fanotify.h        |  29 +++
 15 files changed, 701 insertions(+), 210 deletions(-)

-- 
2.17.1

[PATCH v4 01/15] fsnotify: annotate directory entry modification events

[Amir Goldstein]

"dirent" events are referring to events that modify directory entries,
such as create,delete,rename. Those events should always be reported
on a watched directory, regardless if FS_EVENT_ON_CHILD is set
on the watch mask.

fsnotify_nameremove() and fsnotify_move() were modified to no longer
set the FS_EVENT_ON_CHILD event bit. This is a semantic change to
align with the "dirent" event definition. It has no effect on any
existing backend, because dnotify, inotify and audit always requets the
child events and fanotify does not get the delete,rename events.

The fsnotify_dirent() helper is used instead of fsnotify_parent() to
report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD
and regardless if parent has the FS_EVENT_ON_CHILD bit set.

ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and
those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD.

That means for a directory with an inotify watch and only dirent
events in the mask (i.e. create,delete,move), all children dentries
will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set.
This will allow all events that happen on children to be optimized
away in __fsnotify_parent() without the need to dereference
child->d_parent->d_inode->i_fsnotify_mask.

Since the dirent events are never repoted via __fsnotify_parent(),
this results in no change of logic, but only an optimization.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fsnotify.h         | 42 +++++++++++++++++++++++++-------
 include/linux/fsnotify_backend.h | 36 +++++++++++++++------------
 2 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 2ccb08cb5d6a..8de8f390cce2 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -17,8 +17,35 @@
 #include <linux/slab.h>
 #include <linux/bug.h>
 
+/*
+ * Notify this @dir inode about a change in the directory entry @dentry.
+ *
+ * Unlike fsnotify_parent(), the event will be reported regardless of the
+ * FS_EVENT_ON_CHILD mask on the parent inode.
+ *
+ * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent
+ * inode is used as the inode to report the event to.
+ */
+static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry,
+				  __u32 mask)
+{
+	if (!dir)
+		dir = d_inode(dentry->d_parent);
+
+	/*
+	 * This helper assumes d_parent and d_name are stable. It must be true
+	 * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about
+	 * all callers that get here from d_delete() => fsnotify_nameremove().
+	 */
+	WARN_ON(!inode_is_locked(dir));
+
+	return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
+			dentry->d_name.name, 0);
+}
+
 /* Notify this dentry's parent about a child's events. */
-static inline int fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask)
+static inline int fsnotify_parent(const struct path *path,
+				  struct dentry *dentry, __u32 mask)
 {
 	if (!dentry)
 		dentry = path->dentry;
@@ -85,8 +112,8 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 {
 	struct inode *source = moved->d_inode;
 	u32 fs_cookie = fsnotify_get_cookie();
-	__u32 old_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_FROM);
-	__u32 new_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_TO);
+	__u32 old_dir_mask = FS_MOVED_FROM;
+	__u32 new_dir_mask = FS_MOVED_TO;
 	const unsigned char *new_name = moved->d_name.name;
 
 	if (old_dir == new_dir)
@@ -136,7 +163,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
 	if (isdir)
 		mask |= FS_ISDIR;
 
-	fsnotify_parent(NULL, dentry, mask);
+	fsnotify_dirent(NULL, dentry, mask);
 }
 
 /*
@@ -155,7 +182,7 @@ static inline void fsnotify_create(struct inode *inode, struct dentry *dentry)
 {
 	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
 
-	fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
+	fsnotify_dirent(inode, dentry, FS_CREATE);
 }
 
 /*
@@ -176,12 +203,9 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct
  */
 static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
 {
-	__u32 mask = (FS_CREATE | FS_ISDIR);
-	struct inode *d_inode = dentry->d_inode;
-
 	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
 
-	fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
+	fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR);
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 7639774e7475..7f195d43efaf 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -59,27 +59,33 @@
  * dnotify and inotify. */
 #define FS_EVENT_ON_CHILD	0x08000000
 
-/* This is a list of all events that may get sent to a parernt based on fs event
- * happening to inodes inside that directory */
-#define FS_EVENTS_POSS_ON_CHILD   (FS_ACCESS | FS_MODIFY | FS_ATTRIB |\
-				   FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\
-				   FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\
-				   FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM | \
-				   FS_OPEN_EXEC | FS_OPEN_EXEC_PERM)
-
 #define FS_MOVE			(FS_MOVED_FROM | FS_MOVED_TO)
 
+/*
+ * Directory entry modification events - reported only to directory
+ * where entry is modified and not to a watching parent.
+ * The watching parent may get an FS_ATTRIB|FS_EVENT_ON_CHILD event
+ * when a directory entry inside a child subdir changes.
+ */
+#define ALL_FSNOTIFY_DIRENT_EVENTS	(FS_CREATE | FS_DELETE | FS_MOVE)
+
 #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
 				  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.
+ */
+#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)
+
 /* Events that can be reported to backends */
-#define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
-			     FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \
-			     FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \
-			     FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \
-			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
-			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME | \
-			     FS_OPEN_EXEC | FS_OPEN_EXEC_PERM)
+#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
+			     FS_EVENTS_POSS_ON_CHILD | \
+			     FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \
+			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED)
 
 /* Extra flags that may be reported with event or control handling of events */
 #define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
-- 
2.17.1

[PATCH v4 02/15] fsnotify: send all event types to super block marks

[Amir Goldstein]

So far, existence of super block marks was checked only on events with
data type FSNOTIFY_EVENT_PATH. Use the super block of the "to_tell" inode
to report the events of all event types to super block marks.

This change has no effect on current backends. Soon, this will allow
fanotify backend to receive all event types on a super block mark.

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

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index ecf09b6243d9..df06f3da166c 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -328,16 +328,15 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	     const unsigned char *file_name, u32 cookie)
 {
 	struct fsnotify_iter_info iter_info = {};
-	struct super_block *sb = NULL;
+	struct super_block *sb = to_tell->i_sb;
 	struct mount *mnt = NULL;
-	__u32 mnt_or_sb_mask = 0;
+	__u32 mnt_or_sb_mask = sb->s_fsnotify_mask;
 	int ret = 0;
 	__u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 
 	if (data_is == FSNOTIFY_EVENT_PATH) {
 		mnt = real_mount(((const struct path *)data)->mnt);
-		sb = mnt->mnt.mnt_sb;
-		mnt_or_sb_mask = mnt->mnt_fsnotify_mask | sb->s_fsnotify_mask;
+		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)
@@ -350,8 +349,8 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	 * SRCU because we have no references to any objects and do not
 	 * need SRCU to keep them "alive".
 	 */
-	if (!to_tell->i_fsnotify_marks &&
-	    (!mnt || (!mnt->mnt_fsnotify_marks && !sb->s_fsnotify_marks)))
+	if (!to_tell->i_fsnotify_marks && !sb->s_fsnotify_marks &&
+	    (!mnt || !mnt->mnt_fsnotify_marks))
 		return 0;
 	/*
 	 * if this is a modify event we may need to clear the ignored masks
@@ -366,11 +365,11 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 
 	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);
-		iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
-			fsnotify_first_mark(&sb->s_fsnotify_marks);
 	}
 
 	/*
-- 
2.17.1

[PATCH v4 03/15] fsnotify: move mask out of struct fsnotify_event

[Amir Goldstein]

Common fsnotify_event helpers have no need for the mask field.
It is only used by backend code, so move the field out of the
abstract fsnotify_event struct and into the concrete backend
event structs.

This change packs struct inotify_event_info better on 64bit
machine and will allow us to cram some more fields into
struct fanotify_event_info.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c        | 11 +++++++----
 fs/notify/fanotify/fanotify.h        |  1 +
 fs/notify/fanotify/fanotify_user.c   | 10 +++++-----
 fs/notify/inotify/inotify.h          |  1 +
 fs/notify/inotify/inotify_fsnotify.c |  9 +++++----
 fs/notify/inotify/inotify_user.c     |  5 +++--
 fs/notify/notification.c             | 22 +---------------------
 include/linux/fsnotify_backend.h     | 10 ++++++----
 8 files changed, 29 insertions(+), 40 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 3723f3d18d20..98197802bbfb 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -36,20 +36,22 @@ static bool should_merge(struct fsnotify_event *old_fsn,
 static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 {
 	struct fsnotify_event *test_event;
+	struct fanotify_event_info *new;
 
 	pr_debug("%s: list=%p event=%p\n", __func__, list, event);
+	new = FANOTIFY_E(event);
 
 	/*
 	 * Don't merge a permission event with any other event so that we know
 	 * the event structure we have created in fanotify_handle_event() is the
 	 * one we should check for permission response.
 	 */
-	if (fanotify_is_perm_event(event->mask))
+	if (fanotify_is_perm_event(new->mask))
 		return 0;
 
 	list_for_each_entry_reverse(test_event, list, list) {
 		if (should_merge(test_event, event)) {
-			test_event->mask |= event->mask;
+			FANOTIFY_E(test_event)->mask |= new->mask;
 			return 1;
 		}
 	}
@@ -173,7 +175,8 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 	if (!event)
 		goto out;
 init: __maybe_unused
-	fsnotify_init_event(&event->fse, inode, mask);
+	fsnotify_init_event(&event->fse, inode);
+	event->mask = mask;
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_TID))
 		event->pid = get_pid(task_pid(current));
 	else
@@ -280,7 +283,7 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 	event = FANOTIFY_E(fsn_event);
 	path_put(&event->path);
 	put_pid(event->pid);
-	if (fanotify_is_perm_event(fsn_event->mask)) {
+	if (fanotify_is_perm_event(event->mask)) {
 		kmem_cache_free(fanotify_perm_event_cachep,
 				FANOTIFY_PE(fsn_event));
 		return;
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index ea05b8a401e7..e630d787d4c3 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -14,6 +14,7 @@ extern struct kmem_cache *fanotify_perm_event_cachep;
  */
 struct fanotify_event_info {
 	struct fsnotify_event fse;
+	u32 mask;
 	/*
 	 * We hold ref to this path so it may be dereferenced at any point
 	 * during this object's lifetime
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index e03be5071362..7af79191d945 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -131,9 +131,9 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	metadata->metadata_len = FAN_EVENT_METADATA_LEN;
 	metadata->vers = FANOTIFY_METADATA_VERSION;
 	metadata->reserved = 0;
-	metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS;
+	metadata->mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
 	metadata->pid = pid_vnr(event->pid);
-	if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
+	if (unlikely(event->mask & FAN_Q_OVERFLOW))
 		metadata->fd = FAN_NOFD;
 	else {
 		metadata->fd = create_fd(group, event, file);
@@ -224,7 +224,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 			 fanotify_event_metadata.event_len))
 		goto out_close_fd;
 
-	if (fanotify_is_perm_event(event->mask))
+	if (fanotify_is_perm_event(FANOTIFY_E(event)->mask))
 		FANOTIFY_PE(event)->fd = fd;
 
 	if (fd != FAN_NOFD)
@@ -310,7 +310,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
 		 * Permission events get queued to wait for response.  Other
 		 * events can be destroyed now.
 		 */
-		if (!fanotify_is_perm_event(kevent->mask)) {
+		if (!fanotify_is_perm_event(FANOTIFY_E(kevent)->mask)) {
 			fsnotify_destroy_event(group, kevent);
 		} else {
 			if (ret <= 0) {
@@ -395,7 +395,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 	 */
 	while (!fsnotify_notify_queue_is_empty(group)) {
 		fsn_event = fsnotify_remove_first_event(group);
-		if (!(fsn_event->mask & FANOTIFY_PERM_EVENTS)) {
+		if (!(FANOTIFY_E(fsn_event)->mask & FANOTIFY_PERM_EVENTS)) {
 			spin_unlock(&group->notification_lock);
 			fsnotify_destroy_event(group, fsn_event);
 			spin_lock(&group->notification_lock);
diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index 7e4578d35b61..74ae60305189 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -5,6 +5,7 @@
 
 struct inotify_event_info {
 	struct fsnotify_event fse;
+	u32 mask;
 	int wd;
 	u32 sync_cookie;
 	int name_len;
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index f4184b4f3815..fe97299975f2 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -43,11 +43,11 @@ static bool event_compare(struct fsnotify_event *old_fsn,
 {
 	struct inotify_event_info *old, *new;
 
-	if (old_fsn->mask & FS_IN_IGNORED)
-		return false;
 	old = INOTIFY_E(old_fsn);
 	new = INOTIFY_E(new_fsn);
-	if ((old_fsn->mask == new_fsn->mask) &&
+	if (old->mask & FS_IN_IGNORED)
+		return false;
+	if ((old->mask == new->mask) &&
 	    (old_fsn->inode == new_fsn->inode) &&
 	    (old->name_len == new->name_len) &&
 	    (!old->name_len || !strcmp(old->name, new->name)))
@@ -114,7 +114,8 @@ int inotify_handle_event(struct fsnotify_group *group,
 	}
 
 	fsn_event = &event->fse;
-	fsnotify_init_event(fsn_event, inode, mask);
+	fsnotify_init_event(fsn_event, inode);
+	event->mask = mask;
 	event->wd = i_mark->wd;
 	event->sync_cookie = cookie;
 	event->name_len = len;
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 105576daca4a..2cec820de151 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -189,7 +189,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	 */
 	pad_name_len = round_event_name_len(fsn_event);
 	inotify_event.len = pad_name_len;
-	inotify_event.mask = inotify_mask_to_arg(fsn_event->mask);
+	inotify_event.mask = inotify_mask_to_arg(event->mask);
 	inotify_event.wd = event->wd;
 	inotify_event.cookie = event->sync_cookie;
 
@@ -634,7 +634,8 @@ static struct fsnotify_group *inotify_new_group(unsigned int max_events)
 		return ERR_PTR(-ENOMEM);
 	}
 	group->overflow_event = &oevent->fse;
-	fsnotify_init_event(group->overflow_event, NULL, FS_Q_OVERFLOW);
+	fsnotify_init_event(group->overflow_event, NULL);
+	oevent->mask = FS_Q_OVERFLOW;
 	oevent->wd = -1;
 	oevent->sync_cookie = 0;
 	oevent->name_len = 0;
diff --git a/fs/notify/notification.c b/fs/notify/notification.c
index 3c3e36745f59..027d5d5bb90e 100644
--- a/fs/notify/notification.c
+++ b/fs/notify/notification.c
@@ -71,7 +71,7 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
 			    struct fsnotify_event *event)
 {
 	/* Overflow events are per-group and we don't want to free them */
-	if (!event || event->mask == FS_Q_OVERFLOW)
+	if (!event || event == group->overflow_event)
 		return;
 	/*
 	 * If the event is still queued, we have a problem... Do an unreliable
@@ -194,23 +194,3 @@ void fsnotify_flush_notify(struct fsnotify_group *group)
 	}
 	spin_unlock(&group->notification_lock);
 }
-
-/*
- * fsnotify_create_event - Allocate a new event which will be sent to each
- * group's handle_event function if the group was interested in this
- * particular event.
- *
- * @inode the inode which is supposed to receive the event (sometimes a
- *	parent of the inode to which the event happened.
- * @mask what actually happened.
- * @data pointer to the object which was actually affected
- * @data_type flag indication if the data is a file, path, inode, nothing...
- * @name the filename, if available
- */
-void fsnotify_init_event(struct fsnotify_event *event, struct inode *inode,
-			 u32 mask)
-{
-	INIT_LIST_HEAD(&event->list);
-	event->inode = inode;
-	event->mask = mask;
-}
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 7f195d43efaf..1e4b88bd1443 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -135,7 +135,6 @@ struct fsnotify_event {
 	struct list_head list;
 	/* inode may ONLY be dereferenced during handle_event(). */
 	struct inode *inode;	/* either the inode the event happened to or its parent */
-	u32 mask;		/* the type of access, bitwise OR for FS_* event types */
 };
 
 /*
@@ -485,9 +484,12 @@ extern void fsnotify_put_mark(struct fsnotify_mark *mark);
 extern void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info);
 extern bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info);
 
-/* put here because inotify does some weird stuff when destroying watches */
-extern void fsnotify_init_event(struct fsnotify_event *event,
-				struct inode *to_tell, u32 mask);
+static inline void fsnotify_init_event(struct fsnotify_event *event,
+				       struct inode *inode)
+{
+	INIT_LIST_HEAD(&event->list);
+	event->inode = inode;
+}
 
 #else
 
-- 
2.17.1

[PATCH v4 04/15] fanotify: rename struct fanotify_{,perm_}event_info

[Amir Goldstein]

struct fanotify_event_info "inherits" from struct fsnotify_event and
therefore a more appropriate (and short) name for it is fanotify_event.
Same for struct fanotify_perm_event_info, which now "inherits" from
struct fanotify_event.

We plan to reuse the name struct fanotify_event_info for user visible
event info record format.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 98197802bbfb..d8e3b6e50844 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -19,7 +19,7 @@
 static bool should_merge(struct fsnotify_event *old_fsn,
 			 struct fsnotify_event *new_fsn)
 {
-	struct fanotify_event_info *old, *new;
+	struct fanotify_event *old, *new;
 
 	pr_debug("%s: old=%p new=%p\n", __func__, old_fsn, new_fsn);
 	old = FANOTIFY_E(old_fsn);
@@ -36,7 +36,7 @@ static bool should_merge(struct fsnotify_event *old_fsn,
 static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 {
 	struct fsnotify_event *test_event;
-	struct fanotify_event_info *new;
+	struct fanotify_event *new;
 
 	pr_debug("%s: list=%p event=%p\n", __func__, list, event);
 	new = FANOTIFY_E(event);
@@ -60,7 +60,7 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 }
 
 static int fanotify_get_response(struct fsnotify_group *group,
-				 struct fanotify_perm_event_info *event,
+				 struct fanotify_perm_event *event,
 				 struct fsnotify_iter_info *iter_info)
 {
 	int ret;
@@ -143,11 +143,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 		~marks_ignored_mask;
 }
 
-struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
+struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						 struct inode *inode, u32 mask,
 						 const struct path *path)
 {
-	struct fanotify_event_info *event = NULL;
+	struct fanotify_event *event = NULL;
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
 
 	/*
@@ -162,7 +162,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 	memalloc_use_memcg(group->memcg);
 
 	if (fanotify_is_perm_event(mask)) {
-		struct fanotify_perm_event_info *pevent;
+		struct fanotify_perm_event *pevent;
 
 		pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp);
 		if (!pevent)
@@ -200,7 +200,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 				 struct fsnotify_iter_info *iter_info)
 {
 	int ret = 0;
-	struct fanotify_event_info *event;
+	struct fanotify_event *event;
 	struct fsnotify_event *fsn_event;
 
 	BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
@@ -278,7 +278,7 @@ static void fanotify_free_group_priv(struct fsnotify_group *group)
 
 static void fanotify_free_event(struct fsnotify_event *fsn_event)
 {
-	struct fanotify_event_info *event;
+	struct fanotify_event *event;
 
 	event = FANOTIFY_E(fsn_event);
 	path_put(&event->path);
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index e630d787d4c3..898b5b2bc1c7 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -12,7 +12,7 @@ extern struct kmem_cache *fanotify_perm_event_cachep;
  * fanotify_handle_event() and freed when the information is retrieved by
  * userspace
  */
-struct fanotify_event_info {
+struct fanotify_event {
 	struct fsnotify_event fse;
 	u32 mask;
 	/*
@@ -30,16 +30,16 @@ struct fanotify_event_info {
  * group->notification_list to group->fanotify_data.access_list to wait for
  * user response.
  */
-struct fanotify_perm_event_info {
-	struct fanotify_event_info fae;
+struct fanotify_perm_event {
+	struct fanotify_event fae;
 	int response;	/* userspace answer to question */
 	int fd;		/* fd we passed to userspace for this event */
 };
 
-static inline struct fanotify_perm_event_info *
+static inline struct fanotify_perm_event *
 FANOTIFY_PE(struct fsnotify_event *fse)
 {
-	return container_of(fse, struct fanotify_perm_event_info, fae.fse);
+	return container_of(fse, struct fanotify_perm_event, fae.fse);
 }
 
 static inline bool fanotify_is_perm_event(u32 mask)
@@ -48,11 +48,11 @@ static inline bool fanotify_is_perm_event(u32 mask)
 		mask & FANOTIFY_PERM_EVENTS;
 }
 
-static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse)
+static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse)
 {
-	return container_of(fse, struct fanotify_event_info, fse);
+	return container_of(fse, struct fanotify_event, fse);
 }
 
-struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
+struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						 struct inode *inode, u32 mask,
 						 const struct path *path);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 7af79191d945..6fe703eebd9f 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -73,7 +73,7 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 }
 
 static int create_fd(struct fsnotify_group *group,
-		     struct fanotify_event_info *event,
+		     struct fanotify_event *event,
 		     struct file **file)
 {
 	int client_fd;
@@ -120,13 +120,13 @@ static int fill_event_metadata(struct fsnotify_group *group,
 			       struct file **file)
 {
 	int ret = 0;
-	struct fanotify_event_info *event;
+	struct fanotify_event *event;
 
 	pr_debug("%s: group=%p metadata=%p event=%p\n", __func__,
 		 group, metadata, fsn_event);
 
 	*file = NULL;
-	event = container_of(fsn_event, struct fanotify_event_info, fse);
+	event = container_of(fsn_event, struct fanotify_event, fse);
 	metadata->event_len = FAN_EVENT_METADATA_LEN;
 	metadata->metadata_len = FAN_EVENT_METADATA_LEN;
 	metadata->vers = FANOTIFY_METADATA_VERSION;
@@ -144,10 +144,10 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	return ret;
 }
 
-static struct fanotify_perm_event_info *dequeue_event(
+static struct fanotify_perm_event *dequeue_event(
 				struct fsnotify_group *group, int fd)
 {
-	struct fanotify_perm_event_info *event, *return_e = NULL;
+	struct fanotify_perm_event *event, *return_e = NULL;
 
 	spin_lock(&group->notification_lock);
 	list_for_each_entry(event, &group->fanotify_data.access_list,
@@ -169,7 +169,7 @@ static struct fanotify_perm_event_info *dequeue_event(
 static int process_access_response(struct fsnotify_group *group,
 				   struct fanotify_response *response_struct)
 {
-	struct fanotify_perm_event_info *event;
+	struct fanotify_perm_event *event;
 	int fd = response_struct->fd;
 	int response = response_struct->response;
 
@@ -364,7 +364,7 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t
 static int fanotify_release(struct inode *ignored, struct file *file)
 {
 	struct fsnotify_group *group = file->private_data;
-	struct fanotify_perm_event_info *event, *next;
+	struct fanotify_perm_event *event, *next;
 	struct fsnotify_event *fsn_event;
 
 	/*
@@ -682,7 +682,7 @@ 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_info *oevent;
+	struct fanotify_event *oevent;
 
 	pr_debug("%s: flags=%x event_f_flags=%x\n",
 		 __func__, flags, event_f_flags);
@@ -949,10 +949,10 @@ static int __init fanotify_user_setup(void)
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
 					 SLAB_PANIC|SLAB_ACCOUNT);
-	fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC);
+	fanotify_event_cachep = KMEM_CACHE(fanotify_event, SLAB_PANIC);
 	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) {
 		fanotify_perm_event_cachep =
-			KMEM_CACHE(fanotify_perm_event_info, SLAB_PANIC);
+			KMEM_CACHE(fanotify_perm_event, SLAB_PANIC);
 	}
 
 	return 0;
-- 
2.17.1

[PATCH v4 05/15] fanotify: open code fill_event_metadata()

[Amir Goldstein]

The helper is quite trivial and open coding it will make it easier
to implement copying event fid info to user.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 6fe703eebd9f..a953e422a020 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -114,36 +114,6 @@ static int create_fd(struct fsnotify_group *group,
 	return client_fd;
 }
 
-static int fill_event_metadata(struct fsnotify_group *group,
-			       struct fanotify_event_metadata *metadata,
-			       struct fsnotify_event *fsn_event,
-			       struct file **file)
-{
-	int ret = 0;
-	struct fanotify_event *event;
-
-	pr_debug("%s: group=%p metadata=%p event=%p\n", __func__,
-		 group, metadata, fsn_event);
-
-	*file = NULL;
-	event = container_of(fsn_event, struct fanotify_event, fse);
-	metadata->event_len = FAN_EVENT_METADATA_LEN;
-	metadata->metadata_len = FAN_EVENT_METADATA_LEN;
-	metadata->vers = FANOTIFY_METADATA_VERSION;
-	metadata->reserved = 0;
-	metadata->mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata->pid = pid_vnr(event->pid);
-	if (unlikely(event->mask & FAN_Q_OVERFLOW))
-		metadata->fd = FAN_NOFD;
-	else {
-		metadata->fd = create_fd(group, event, file);
-		if (metadata->fd < 0)
-			ret = metadata->fd;
-	}
-
-	return ret;
-}
-
 static struct fanotify_perm_event *dequeue_event(
 				struct fsnotify_group *group, int fd)
 {
@@ -205,31 +175,43 @@ static int process_access_response(struct fsnotify_group *group,
 }
 
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
-				  struct fsnotify_event *event,
+				  struct fsnotify_event *fsn_event,
 				  char __user *buf)
 {
-	struct fanotify_event_metadata fanotify_event_metadata;
+	struct fanotify_event_metadata metadata;
+	struct fanotify_event *event;
 	struct file *f;
 	int fd, ret;
 
-	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
+	pr_debug("%s: group=%p event=%p\n", __func__, group, fsn_event);
 
-	ret = fill_event_metadata(group, &fanotify_event_metadata, event, &f);
-	if (ret < 0)
-		return ret;
+	event = container_of(fsn_event, struct fanotify_event, fse);
+	metadata.event_len = FAN_EVENT_METADATA_LEN;
+	metadata.metadata_len = FAN_EVENT_METADATA_LEN;
+	metadata.vers = FANOTIFY_METADATA_VERSION;
+	metadata.reserved = 0;
+	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
+	metadata.pid = pid_vnr(event->pid);
+
+	if (unlikely(event->mask & FAN_Q_OVERFLOW)) {
+		fd = FAN_NOFD;
+	} else {
+		fd = create_fd(group, event, &f);
+		if (fd < 0)
+			return fd;
+	}
+	metadata.fd = fd;
 
-	fd = fanotify_event_metadata.fd;
 	ret = -EFAULT;
-	if (copy_to_user(buf, &fanotify_event_metadata,
-			 fanotify_event_metadata.event_len))
+	if (copy_to_user(buf, &metadata, metadata.event_len))
 		goto out_close_fd;
 
-	if (fanotify_is_perm_event(FANOTIFY_E(event)->mask))
-		FANOTIFY_PE(event)->fd = fd;
+	if (fanotify_is_perm_event(event->mask))
+		FANOTIFY_PE(fsn_event)->fd = fd;
 
 	if (fd != FAN_NOFD)
 		fd_install(fd, f);
-	return fanotify_event_metadata.event_len;
+	return metadata.event_len;
 
 out_close_fd:
 	if (fd != FAN_NOFD) {
-- 
2.17.1

[PATCH v4 06/15] fanotify: encode file identifier for FAN_REPORT_FID

[Amir Goldstein]

When user requests the flag FAN_REPORT_FID in fanotify_init(),
a unique file indetifier of the event target object will be reported
with the event.

The file identifier includes the filesystem's fsid (i.e. from statfs(2))
and an NFS file handle of the file (i.e. from name_to_handle_at(2)).

The file identifier makes holding the path reference and passing a file
descriptor to user redundant, so those are disabled in a group with
FAN_REPORT_FID.

Encode fid and store it in event for a group with FAN_REPORT_FID.
Up to 12 bytes of file handle on 32bit arch (16 bytes on 64bit arch)
are stored inline in fanotify_event struct. Larger file handles are
stored in an extennal allocated buffer.

On failure to encode fid, we print a warning and queue the event
without the fid information.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 81 ++++++++++++++++++++++++++---
 fs/notify/fanotify/fanotify.h      | 82 ++++++++++++++++++++++++++++--
 fs/notify/fanotify/fanotify_user.c |  6 +--
 include/uapi/linux/fanotify.h      |  1 +
 4 files changed, 157 insertions(+), 13 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index d8e3b6e50844..cb58c6526edb 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -13,6 +13,7 @@
 #include <linux/wait.h>
 #include <linux/audit.h>
 #include <linux/sched/mm.h>
+#include <linux/statfs.h>
 
 #include "fanotify.h"
 
@@ -25,10 +26,18 @@ static bool should_merge(struct fsnotify_event *old_fsn,
 	old = FANOTIFY_E(old_fsn);
 	new = FANOTIFY_E(new_fsn);
 
-	if (old_fsn->inode == new_fsn->inode && old->pid == new->pid &&
-	    old->path.mnt == new->path.mnt &&
-	    old->path.dentry == new->path.dentry)
-		return true;
+	if (old_fsn->inode != new_fsn->inode || old->pid != new->pid ||
+	    old->fh_type != new->fh_type || old->fh_len != new->fh_len)
+		return false;
+
+	if (fanotify_event_has_path(old)) {
+		return old->path.mnt == new->path.mnt &&
+			old->path.dentry == new->path.dentry;
+	} else if (fanotify_event_has_fid(old)) {
+		return fanotify_fid_equal(&old->fid, &new->fid, old->fh_len);
+	}
+
+	/* Do not merge events if we failed to encode fid */
 	return false;
 }
 
@@ -143,6 +152,57 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 		~marks_ignored_mask;
 }
 
+static int fanotify_encode_fid(struct fanotify_event *event,
+			       const struct path *path, gfp_t gfp)
+{
+	struct fanotify_fid *fid = &event->fid;
+	int dwords, bytes = 0;
+	struct kstatfs stat;
+	int err, type;
+
+	stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
+	fid->ext_fh = NULL;
+	dwords = 0;
+	err = -ENOENT;
+	type = exportfs_encode_fh(path->dentry, NULL, &dwords,  0);
+	if (!dwords)
+		goto out_err;
+
+	err = vfs_statfs(path, &stat);
+	if (err)
+		goto out_err;
+
+	bytes = dwords << 2;
+	if (!fanotify_inline_fh_len(bytes)) {
+		/* Treat failure to allocate fh as failure to allocate event */
+		err = -ENOMEM;
+		fid->ext_fh = kmalloc(bytes, gfp);
+		if (!fid->ext_fh)
+			goto out_err;
+	}
+
+	type = exportfs_encode_fh(path->dentry, fanotify_fid_fh(fid, bytes),
+				  &dwords,  0);
+	err = -EINVAL;
+	if (!type || type == FILEID_INVALID || bytes != dwords << 2)
+		goto out_err;
+
+	fid->fsid = stat.f_fsid;
+	event->fh_len = bytes;
+
+	return type;
+
+out_err:
+	pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
+			    stat.f_fsid.val[0], stat.f_fsid.val[1],
+			    type, bytes, err);
+	kfree(fid->ext_fh);
+	fid->ext_fh = NULL;
+	event->fh_len = 0;
+
+	return FILEID_INVALID;
+}
+
 struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						 struct inode *inode, u32 mask,
 						 const struct path *path)
@@ -181,10 +241,16 @@ init: __maybe_unused
 		event->pid = get_pid(task_pid(current));
 	else
 		event->pid = get_pid(task_tgid(current));
-	if (path) {
+	event->fh_len = 0;
+	if (path && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+		/* Report the event without a file identifier on encode error */
+		event->fh_type = fanotify_encode_fid(event, path, gfp);
+	} else if (path) {
+		event->fh_type = 0;
 		event->path = *path;
 		path_get(&event->path);
 	} else {
+		event->fh_type = FILEID_INVALID;
 		event->path.mnt = NULL;
 		event->path.dentry = NULL;
 	}
@@ -281,7 +347,10 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 	struct fanotify_event *event;
 
 	event = FANOTIFY_E(fsn_event);
-	path_put(&event->path);
+	if (fanotify_event_has_path(event))
+		path_put(&event->path);
+	else if (fanotify_event_has_ext_fh(event))
+		kfree(event->fid.ext_fh);
 	put_pid(event->pid);
 	if (fanotify_is_perm_event(event->mask)) {
 		kmem_cache_free(fanotify_perm_event_cachep,
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 898b5b2bc1c7..cddebea5ff67 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -2,11 +2,54 @@
 #include <linux/fsnotify_backend.h>
 #include <linux/path.h>
 #include <linux/slab.h>
+#include <linux/exportfs.h>
 
 extern struct kmem_cache *fanotify_mark_cache;
 extern struct kmem_cache *fanotify_event_cachep;
 extern struct kmem_cache *fanotify_perm_event_cachep;
 
+/*
+ * 3 dwords are sufficient for most local fs (64bit ino, 32bit generation).
+ * For 32bit arch, fid increases the size of fanotify_event by 12 bytes and
+ * fh_* fields increase the size of fanotify_event by another 4 bytes.
+ * For 64bit arch, fid increases the size of fanotify_fid by 8 bytes and
+ * fh_* fields are packed in a hole after mask.
+ */
+#if BITS_PER_LONG == 32
+#define FANOTIFY_INLINE_FH_LEN	(3 << 2)
+#else
+#define FANOTIFY_INLINE_FH_LEN	(4 << 2)
+#endif
+
+struct fanotify_fid {
+	__kernel_fsid_t fsid;
+	union {
+		unsigned char fh[FANOTIFY_INLINE_FH_LEN];
+		unsigned char *ext_fh;
+	};
+};
+
+static inline bool fanotify_inline_fh_len(unsigned int fh_len)
+{
+	return fh_len <= FANOTIFY_INLINE_FH_LEN;
+}
+
+static inline void *fanotify_fid_fh(struct fanotify_fid *fid,
+				    unsigned int fh_len)
+{
+	return fanotify_inline_fh_len(fh_len) ? fid->fh : fid->ext_fh;
+}
+
+static inline bool fanotify_fid_equal(struct fanotify_fid *fid1,
+				      struct fanotify_fid *fid2,
+				      unsigned int fh_len)
+{
+	return fid1->fsid.val[0] == fid2->fsid.val[0] &&
+		fid1->fsid.val[1] == fid2->fsid.val[1] &&
+		!memcmp(fanotify_fid_fh(fid1, fh_len),
+			fanotify_fid_fh(fid2, fh_len), fh_len);
+}
+
 /*
  * Structure for normal fanotify events. It gets allocated in
  * fanotify_handle_event() and freed when the information is retrieved by
@@ -16,13 +59,46 @@ struct fanotify_event {
 	struct fsnotify_event fse;
 	u32 mask;
 	/*
-	 * We hold ref to this path so it may be dereferenced at any point
-	 * during this object's lifetime
+	 * Those fields are outside fanotify_fid to pack fanotify_event nicely
+	 * on 64bit arch and to use fh_type as an indication of whether path
+	 * or fid are used in the union:
+	 * fh_type == 0 for path, > 0 for fid, FILEID_INVALID for neither.
 	 */
-	struct path path;
+	u8 fh_type;
+	u8 fh_len;
+	u16 pad;
+	union {
+		/*
+		 * We hold ref to this path so it may be dereferenced at any
+		 * point during this object's lifetime
+		 */
+		struct path path;
+		/*
+		 * With FAN_REPORT_FID, we do not hold any reference on the
+		 * victim object. Instead we store its NFS file handle and its
+		 * filesystem's fsid as a unique identifier.
+		 */
+		struct fanotify_fid fid;
+	};
 	struct pid *pid;
 };
 
+static inline bool fanotify_event_has_path(struct fanotify_event *event)
+{
+	return !event->fh_type;
+}
+
+static inline bool fanotify_event_has_fid(struct fanotify_event *event)
+{
+	return event->fh_type && event->fh_type != FILEID_INVALID;
+}
+
+static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event)
+{
+	return fanotify_event_has_fid(event) &&
+		!fanotify_inline_fh_len(event->fh_len);
+}
+
 /*
  * Structure for permission fanotify events. It gets allocated and freed in
  * fanotify_handle_event() since we wait there for user response. When the
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a953e422a020..9b9e6704f9e7 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -181,7 +181,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	struct fanotify_event_metadata metadata;
 	struct fanotify_event *event;
 	struct file *f;
-	int fd, ret;
+	int ret, fd = FAN_NOFD;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, fsn_event);
 
@@ -193,9 +193,7 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
 	metadata.pid = pid_vnr(event->pid);
 
-	if (unlikely(event->mask & FAN_Q_OVERFLOW)) {
-		fd = FAN_NOFD;
-	} else {
+	if (fanotify_event_has_path(event)) {
 		fd = create_fd(group, event, &f);
 		if (fd < 0)
 			return fd;
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 909c98fcace2..d07f3cbc2786 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -44,6 +44,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 */
 
 /* 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 v4 07/15] fanotify: copy event fid info to user

[Amir Goldstein]

If group requested FAN_REPORT_FID and event has file identifier,
copy that information to user reading the event after event metadata.

fid information is formatted as struct fanotify_event_info_fid
that includes a generic header struct fanotify_event_info_header,
so that other info types could be defined in the future using the
same header.

metadata->event_len includes the length of the fid information.

The fid information includes the filesystem's fsid (see statfs(2))
followed by an NFS file handle of the file that could be passed as
an argument to open_by_handle_at(2).

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

diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index cddebea5ff67..265fbaa2d5b7 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -99,6 +99,11 @@ static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event)
 		!fanotify_inline_fh_len(event->fh_len);
 }
 
+static inline void *fanotify_event_fh(struct fanotify_event *event)
+{
+	return fanotify_fid_fh(&event->fid, event->fh_len);
+}
+
 /*
  * Structure for permission fanotify events. It gets allocated and freed in
  * fanotify_handle_event() since we wait there for user response. When the
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 9b9e6704f9e7..032a9a225a21 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -47,6 +47,22 @@ struct kmem_cache *fanotify_mark_cache __read_mostly;
 struct kmem_cache *fanotify_event_cachep __read_mostly;
 struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 
+static int fanotify_event_info_len(struct fanotify_event *event)
+{
+	if (!fanotify_event_has_fid(event))
+		return 0;
+
+	return sizeof(struct fanotify_event_info_fid) +
+		sizeof(struct file_handle) + event->fh_len;
+}
+
+#define FANOTIFY_EVENT_ALIGN (sizeof(void *))
+
+static int fanotify_round_event_info_len(struct fanotify_event *event)
+{
+	return roundup(fanotify_event_info_len(event), FANOTIFY_EVENT_ALIGN);
+}
+
 /*
  * Get an fsnotify notification event if one exists and is small
  * enough to fit in "count". Return an error pointer if the count
@@ -57,6 +73,9 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
 static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 					    size_t count)
 {
+	size_t event_size = FAN_EVENT_METADATA_LEN;
+	struct fanotify_event *event;
+
 	assert_spin_locked(&group->notification_lock);
 
 	pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
@@ -64,11 +83,18 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 	if (fsnotify_notify_queue_is_empty(group))
 		return NULL;
 
-	if (FAN_EVENT_METADATA_LEN > count)
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+		event = FANOTIFY_E(fsnotify_peek_first_event(group));
+		event_size += fanotify_round_event_info_len(event);
+	}
+
+	if (event_size > count)
 		return ERR_PTR(-EINVAL);
 
-	/* held the notification_lock the whole time, so this is the
-	 * same event we peeked above */
+	/*
+	 * Held the notification_lock the whole time, so this is the
+	 * same event we peeked above
+	 */
 	return fsnotify_remove_first_event(group);
 }
 
@@ -174,6 +200,47 @@ static int process_access_response(struct fsnotify_group *group,
 	return 0;
 }
 
+static int copy_fid_to_user(struct fanotify_event *event, char __user *buf)
+{
+	struct fanotify_event_info_fid info;
+	struct file_handle handle;
+	size_t fh_len = event->fh_len;
+	size_t info_len = fanotify_event_info_len(event);
+	size_t len = roundup(info_len, FANOTIFY_EVENT_ALIGN);
+
+	if (!info_len)
+		return 0;
+
+	/* Copy event info fid header followed by vaiable sized file handle */
+	info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID;
+	info.hdr.len = info_len;
+	info.fsid = event->fid.fsid;
+	info_len = sizeof(info) + sizeof(struct file_handle);
+	if (copy_to_user(buf, &info, sizeof(info)))
+		return -EFAULT;
+
+	buf += sizeof(info);
+	len -= sizeof(info);
+	handle.handle_type = event->fh_type;
+	handle.handle_bytes = event->fh_len;
+	if (copy_to_user(buf, &handle, sizeof(handle)))
+		return -EFAULT;
+
+	buf += sizeof(handle);
+	len -= sizeof(handle);
+	if (copy_to_user(buf, fanotify_event_fh(event), fh_len))
+		return -EFAULT;
+
+	/* Pad with 0's */
+	buf += fh_len;
+	len -= fh_len;
+	WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);
+	if (len > 0 && clear_user(buf, len))
+		return -EFAULT;
+
+	return 0;
+}
+
 static ssize_t copy_event_to_user(struct fsnotify_group *group,
 				  struct fsnotify_event *fsn_event,
 				  char __user *buf)
@@ -197,18 +264,26 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
 		fd = create_fd(group, event, &f);
 		if (fd < 0)
 			return fd;
+	} else if (fanotify_event_has_fid(event)) {
+		metadata.event_len += fanotify_round_event_info_len(event);
 	}
 	metadata.fd = fd;
 
 	ret = -EFAULT;
-	if (copy_to_user(buf, &metadata, metadata.event_len))
+	if (copy_to_user(buf, &metadata, FAN_EVENT_METADATA_LEN))
 		goto out_close_fd;
 
 	if (fanotify_is_perm_event(event->mask))
 		FANOTIFY_PE(fsn_event)->fd = fd;
 
-	if (fd != FAN_NOFD)
+	if (fanotify_event_has_path(event)) {
 		fd_install(fd, f);
+	} else if (fanotify_event_has_fid(event)) {
+		ret = copy_fid_to_user(event, buf + FAN_EVENT_METADATA_LEN);
+		if (ret < 0)
+			return ret;
+	}
+
 	return metadata.event_len;
 
 out_close_fd:
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index d07f3cbc2786..959ae2bdc7ca 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -107,6 +107,26 @@ struct fanotify_event_metadata {
 	__s32 pid;
 };
 
+#define FAN_EVENT_INFO_TYPE_FID		1
+
+/* Variable length info record following event metadata */
+struct fanotify_event_info_header {
+	__u8 info_type;
+	__u8 pad;
+	__u16 len;
+};
+
+/* Unique file identifier info record */
+struct fanotify_event_info_fid {
+	struct fanotify_event_info_header hdr;
+	__kernel_fsid_t fsid;
+	/*
+	 * Following is an opaque struct file_handle that can be passed as
+	 * an argument to open_by_handle_at(2).
+	 */
+	unsigned char handle[0];
+};
+
 struct fanotify_response {
 	__s32 fd;
 	__u32 response;
-- 
2.17.1

[PATCH v4 08/15] fanotify: enable FAN_REPORT_FID init flag

[Amir Goldstein]

When setting up an fanotify listener, user may request to get fid
information in event instead of an open file descriptor.

The fid obtained with event on a watched object contains the file
handle returned by name_to_handle_at(2) and fsid returned by statfs(2).

When setting a mark, we need to make sure that the filesystem
supports encoding file handles with name_to_handle_at(2) and that
statfs(2) encodes a non-zero fsid.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 032a9a225a21..3b8d442e67cd 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -17,6 +17,8 @@
 #include <linux/compat.h>
 #include <linux/sched/signal.h>
 #include <linux/memcontrol.h>
+#include <linux/statfs.h>
+#include <linux/exportfs.h>
 
 #include <asm/ioctls.h>
 
@@ -850,6 +852,52 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	return fd;
 }
 
+/* Check if filesystem can encode a unique fid */
+static int fanotify_test_fid(struct path *path)
+{
+	struct kstatfs stat, root_stat;
+	struct path root = {
+		.mnt = path->mnt,
+		.dentry = path->dentry->d_sb->s_root,
+	};
+	int err;
+
+	/*
+	 * Make sure path is not in filesystem with zero fsid (e.g. tmpfs).
+	 */
+	err = vfs_statfs(path, &stat);
+	if (err)
+		return err;
+
+	if (!stat.f_fsid.val[0] && !stat.f_fsid.val[1])
+		return -ENODEV;
+
+	/*
+	 * Make sure path is not inside a filesystem subvolume (e.g. btrfs)
+	 * which uses a different fsid than sb root.
+	 */
+	err = vfs_statfs(&root, &root_stat);
+	if (err)
+		return err;
+
+	if (root_stat.f_fsid.val[0] != stat.f_fsid.val[0] ||
+	    root_stat.f_fsid.val[1] != stat.f_fsid.val[1])
+		return -EXDEV;
+
+	/*
+	 * We need to make sure that the file system supports at least
+	 * encoding a file handle so user can use name_to_handle_at() to
+	 * compare fid returned with event to the file handle of watched
+	 * objects. However, name_to_handle_at() requires that the
+	 * filesystem also supports decoding file handles.
+	 */
+	if (!path->dentry->d_sb->s_export_op ||
+	    !path->dentry->d_sb->s_export_op->fh_to_dentry)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 			    int dfd, const char  __user *pathname)
 {
@@ -935,6 +983,12 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	if (ret)
 		goto fput_and_out;
 
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+		ret = fanotify_test_fid(&path);
+		if (ret)
+			goto path_put_and_out;
+	}
+
 	/* inode held in place by reference to path; group by fget on fd */
 	if (mark_type == FAN_MARK_INODE)
 		inode = path.dentry->d_inode;
@@ -963,6 +1017,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		ret = -EINVAL;
 	}
 
+path_put_and_out:
 	path_put(&path);
 fput_and_out:
 	fdput(f);
@@ -999,7 +1054,7 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 7);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 8);
 	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 9e2142795335..f59be967f72b 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_TID | FAN_REPORT_FID | \
 				 FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
 
-- 
2.17.1

[PATCH v4 09/15] fanotify: cache fsid in fsnotify_mark_connector

[Amir Goldstein]

For FAN_REPORT_FID, we need to encode fid with fsid of the filesystem on
every event. To avoid having to call vfs_statfs() on every event to get
fsid, we store the fsid in fsnotify_mark_connector on the first time we
add a mark and on handle event we use the cached fsid.

Subsequent calls to add mark on the same object are expected to pass the
same fsid, so the call will fail on cached fsid mismatch.

If an event is reported on several mark types (inode, mount, filesystem),
all connectors should already have the same fsid, so we use the cached
fsid from the first connector.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 51 +++++++++++++++++-------
 fs/notify/fanotify/fanotify.h      |  5 ++-
 fs/notify/fanotify/fanotify_user.c | 62 ++++++++++++++++++------------
 fs/notify/mark.c                   | 44 ++++++++++++++++-----
 include/linux/fsnotify_backend.h   | 24 +++++++++---
 5 files changed, 132 insertions(+), 54 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index cb58c6526edb..a5dd8b8d92b7 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -153,14 +153,16 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 }
 
 static int fanotify_encode_fid(struct fanotify_event *event,
-			       const struct path *path, gfp_t gfp)
+			       const struct path *path, gfp_t gfp,
+			       __kernel_fsid_t *fsid)
 {
 	struct fanotify_fid *fid = &event->fid;
 	int dwords, bytes = 0;
-	struct kstatfs stat;
 	int err, type;
 
-	stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
+	if (!fsid)
+		goto out_err;
+
 	fid->ext_fh = NULL;
 	dwords = 0;
 	err = -ENOENT;
@@ -168,10 +170,6 @@ static int fanotify_encode_fid(struct fanotify_event *event,
 	if (!dwords)
 		goto out_err;
 
-	err = vfs_statfs(path, &stat);
-	if (err)
-		goto out_err;
-
 	bytes = dwords << 2;
 	if (!fanotify_inline_fh_len(bytes)) {
 		/* Treat failure to allocate fh as failure to allocate event */
@@ -187,14 +185,14 @@ static int fanotify_encode_fid(struct fanotify_event *event,
 	if (!type || type == FILEID_INVALID || bytes != dwords << 2)
 		goto out_err;
 
-	fid->fsid = stat.f_fsid;
+	fid->fsid = *fsid;
 	event->fh_len = bytes;
 
 	return type;
 
 out_err:
 	pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
-			    stat.f_fsid.val[0], stat.f_fsid.val[1],
+			    fsid ? fsid->val[0] : 0, fsid ? fsid->val[1] : 0,
 			    type, bytes, err);
 	kfree(fid->ext_fh);
 	fid->ext_fh = NULL;
@@ -204,8 +202,9 @@ static int fanotify_encode_fid(struct fanotify_event *event,
 }
 
 struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
-						 struct inode *inode, u32 mask,
-						 const struct path *path)
+					    struct inode *inode, u32 mask,
+					    const struct path *path,
+					    __kernel_fsid_t *fsid)
 {
 	struct fanotify_event *event = NULL;
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
@@ -244,7 +243,7 @@ init: __maybe_unused
 	event->fh_len = 0;
 	if (path && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
 		/* Report the event without a file identifier on encode error */
-		event->fh_type = fanotify_encode_fid(event, path, gfp);
+		event->fh_type = fanotify_encode_fid(event, path, gfp, fsid);
 	} else if (path) {
 		event->fh_type = 0;
 		event->path = *path;
@@ -259,6 +258,28 @@ init: __maybe_unused
 	return event;
 }
 
+/*
+ * Get cached fsid of the filesystem containing the object from any connector.
+ * All connectors are supposed to have the same fsid, but we do not verify that
+ * here.
+ */
+static __kernel_fsid_t *fanotify_get_fsid(struct fsnotify_iter_info *iter_info,
+					  __kernel_fsid_t *fsid)
+{
+	int type;
+
+	fsnotify_foreach_obj_type(type) {
+		if (!fsnotify_iter_should_report_type(iter_info, type))
+			continue;
+
+		*fsid = iter_info->marks[type]->connector->fsid;
+		if (!WARN_ON_ONCE(!fsid->val[0] && !fsid->val[1]))
+			return fsid;
+	}
+
+	return NULL;
+}
+
 static int fanotify_handle_event(struct fsnotify_group *group,
 				 struct inode *inode,
 				 u32 mask, const void *data, int data_type,
@@ -268,6 +289,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	int ret = 0;
 	struct fanotify_event *event;
 	struct fsnotify_event *fsn_event;
+	__kernel_fsid_t __fsid, *fsid = NULL;
 
 	BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
 	BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
@@ -300,7 +322,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 			return 0;
 	}
 
-	event = fanotify_alloc_event(group, inode, mask, data);
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID))
+		fsid = fanotify_get_fsid(iter_info, &__fsid);
+
+	event = fanotify_alloc_event(group, inode, mask, data, fsid);
 	ret = -ENOMEM;
 	if (unlikely(!event)) {
 		/*
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 265fbaa2d5b7..06b93cf227a2 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -135,5 +135,6 @@ static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse)
 }
 
 struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
-						 struct inode *inode, u32 mask,
-						 const struct path *path);
+					    struct inode *inode, u32 mask,
+					    const struct path *path,
+					    __kernel_fsid_t *fsid);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 3b8d442e67cd..b69a7c23a765 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -649,7 +649,8 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 
 static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 						   fsnotify_connp_t *connp,
-						   unsigned int type)
+						   unsigned int type,
+						   __kernel_fsid_t *fsid)
 {
 	struct fsnotify_mark *mark;
 	int ret;
@@ -662,7 +663,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 		return ERR_PTR(-ENOMEM);
 
 	fsnotify_init_mark(mark, group);
-	ret = fsnotify_add_mark_locked(mark, connp, type, 0);
+	ret = fsnotify_add_mark_locked_fsid(mark, connp, type, 0, fsid);
 	if (ret) {
 		fsnotify_put_mark(mark);
 		return ERR_PTR(ret);
@@ -674,7 +675,8 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 
 static int fanotify_add_mark(struct fsnotify_group *group,
 			     fsnotify_connp_t *connp, unsigned int type,
-			     __u32 mask, unsigned int flags)
+			     __u32 mask, unsigned int flags,
+			     __kernel_fsid_t *fsid)
 {
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
@@ -682,7 +684,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 	mutex_lock(&group->mark_mutex);
 	fsn_mark = fsnotify_find_mark(connp, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, connp, type);
+		fsn_mark = fanotify_add_new_mark(group, connp, type, fsid);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
@@ -699,23 +701,23 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 
 static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 				      struct vfsmount *mnt, __u32 mask,
-				      unsigned int flags)
+				      unsigned int flags, __kernel_fsid_t *fsid)
 {
 	return fanotify_add_mark(group, &real_mount(mnt)->mnt_fsnotify_marks,
-				 FSNOTIFY_OBJ_TYPE_VFSMOUNT, mask, flags);
+				 FSNOTIFY_OBJ_TYPE_VFSMOUNT, mask, flags, fsid);
 }
 
 static int fanotify_add_sb_mark(struct fsnotify_group *group,
-				      struct super_block *sb, __u32 mask,
-				      unsigned int flags)
+				struct super_block *sb, __u32 mask,
+				unsigned int flags, __kernel_fsid_t *fsid)
 {
 	return fanotify_add_mark(group, &sb->s_fsnotify_marks,
-				 FSNOTIFY_OBJ_TYPE_SB, mask, flags);
+				 FSNOTIFY_OBJ_TYPE_SB, mask, flags, fsid);
 }
 
 static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
-				   unsigned int flags)
+				   unsigned int flags, __kernel_fsid_t *fsid)
 {
 	pr_debug("%s: group=%p inode=%p\n", __func__, group, inode);
 
@@ -730,7 +732,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 		return 0;
 
 	return fanotify_add_mark(group, &inode->i_fsnotify_marks,
-				 FSNOTIFY_OBJ_TYPE_INODE, mask, flags);
+				 FSNOTIFY_OBJ_TYPE_INODE, mask, flags, fsid);
 }
 
 /* fanotify syscalls */
@@ -790,7 +792,7 @@ 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);
+	oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL, NULL);
 	if (unlikely(!oevent)) {
 		fd = -ENOMEM;
 		goto out_destroy_group;
@@ -853,9 +855,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 }
 
 /* Check if filesystem can encode a unique fid */
-static int fanotify_test_fid(struct path *path)
+static int fanotify_test_fid(struct path *path, struct kstatfs *stat)
 {
-	struct kstatfs stat, root_stat;
+	struct kstatfs root_stat;
 	struct path root = {
 		.mnt = path->mnt,
 		.dentry = path->dentry->d_sb->s_root,
@@ -865,11 +867,11 @@ static int fanotify_test_fid(struct path *path)
 	/*
 	 * Make sure path is not in filesystem with zero fsid (e.g. tmpfs).
 	 */
-	err = vfs_statfs(path, &stat);
+	err = vfs_statfs(path, stat);
 	if (err)
 		return err;
 
-	if (!stat.f_fsid.val[0] && !stat.f_fsid.val[1])
+	if (!stat->f_fsid.val[0] && !stat->f_fsid.val[1])
 		return -ENODEV;
 
 	/*
@@ -880,8 +882,8 @@ static int fanotify_test_fid(struct path *path)
 	if (err)
 		return err;
 
-	if (root_stat.f_fsid.val[0] != stat.f_fsid.val[0] ||
-	    root_stat.f_fsid.val[1] != stat.f_fsid.val[1])
+	if (root_stat.f_fsid.val[0] != stat->f_fsid.val[0] ||
+	    root_stat.f_fsid.val[1] != stat->f_fsid.val[1])
 		return -EXDEV;
 
 	/*
@@ -906,6 +908,8 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	struct fsnotify_group *group;
 	struct fd f;
 	struct path path;
+	struct kstatfs stat;
+	__kernel_fsid_t *fsid = NULL;
 	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
 	int ret;
@@ -984,9 +988,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		goto fput_and_out;
 
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
-		ret = fanotify_test_fid(&path);
+		ret = fanotify_test_fid(&path, &stat);
 		if (ret)
 			goto path_put_and_out;
+
+		fsid = &stat.f_fsid;
 	}
 
 	/* inode held in place by reference to path; group by fget on fd */
@@ -999,19 +1005,25 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	switch (flags & (FAN_MARK_ADD | FAN_MARK_REMOVE)) {
 	case FAN_MARK_ADD:
 		if (mark_type == FAN_MARK_MOUNT)
-			ret = fanotify_add_vfsmount_mark(group, mnt, mask, flags);
+			ret = fanotify_add_vfsmount_mark(group, mnt, mask,
+							 flags, fsid);
 		else if (mark_type == FAN_MARK_FILESYSTEM)
-			ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask, flags);
+			ret = fanotify_add_sb_mark(group, mnt->mnt_sb, mask,
+						   flags, fsid);
 		else
-			ret = fanotify_add_inode_mark(group, inode, mask, flags);
+			ret = fanotify_add_inode_mark(group, inode, mask,
+						      flags, fsid);
 		break;
 	case FAN_MARK_REMOVE:
 		if (mark_type == FAN_MARK_MOUNT)
-			ret = fanotify_remove_vfsmount_mark(group, mnt, mask, flags);
+			ret = fanotify_remove_vfsmount_mark(group, mnt, mask,
+							    flags);
 		else if (mark_type == FAN_MARK_FILESYSTEM)
-			ret = fanotify_remove_sb_mark(group, mnt->mnt_sb, mask, flags);
+			ret = fanotify_remove_sb_mark(group, mnt->mnt_sb, mask,
+						      flags);
 		else
-			ret = fanotify_remove_inode_mark(group, inode, mask, flags);
+			ret = fanotify_remove_inode_mark(group, inode, mask,
+							 flags);
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index d2dd16cb5989..9e71796dfbfb 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -82,6 +82,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/srcu.h>
+#include <linux/ratelimit.h>
 
 #include <linux/atomic.h>
 
@@ -481,7 +482,8 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
 }
 
 static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
-					       unsigned int type)
+					       unsigned int type,
+					       __kernel_fsid_t *fsid)
 {
 	struct inode *inode = NULL;
 	struct fsnotify_mark_connector *conn;
@@ -493,6 +495,11 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	INIT_HLIST_HEAD(&conn->list);
 	conn->type = type;
 	conn->obj = connp;
+	/* Cache fsid of filesystem containing the object */
+	if (fsid)
+		conn->fsid = *fsid;
+	else
+		conn->fsid.val[0] = conn->fsid.val[1] = 0;
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		inode = igrab(fsnotify_conn_inode(conn));
 	/*
@@ -544,7 +551,7 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector(
  */
 static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 				  fsnotify_connp_t *connp, unsigned int type,
-				  int allow_dups)
+				  int allow_dups, __kernel_fsid_t *fsid)
 {
 	struct fsnotify_mark *lmark, *last = NULL;
 	struct fsnotify_mark_connector *conn;
@@ -553,15 +560,33 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 
 	if (WARN_ON(!fsnotify_valid_obj_type(type)))
 		return -EINVAL;
+
+	/* Backend is expected to check for zero fsid (e.g. tmpfs) */
+	if (fsid && WARN_ON_ONCE(!fsid->val[0] && !fsid->val[1]))
+		return -ENODEV;
+
 restart:
 	spin_lock(&mark->lock);
 	conn = fsnotify_grab_connector(connp);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(connp, type);
+		err = fsnotify_attach_connector_to_object(connp, type, fsid);
 		if (err)
 			return err;
 		goto restart;
+	} else if (fsid &&
+		   (fsid->val[0] != conn->fsid.val[0] ||
+		    fsid->val[1] != conn->fsid.val[1])) {
+		/*
+		 * Backend is expected to check for non uniform fsid
+		 * (e.g. btrfs), but maybe we missed something?
+		 */
+		pr_warn_ratelimited("%s: fsid mismatch on object of type %u: %x.%x != %x.%x\n",
+				    __func__, conn->type,
+				    fsid->val[0], fsid->val[1],
+				    conn->fsid.val[0], conn->fsid.val[1]);
+		err = -EXDEV;
+		goto out_err;
 	}
 
 	/* is mark the first mark? */
@@ -604,9 +629,9 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
  * These marks may be used for the fsnotify backend to determine which
  * event types should be delivered to which group.
  */
-int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-			     fsnotify_connp_t *connp, unsigned int type,
-			     int allow_dups)
+int fsnotify_add_mark_locked_fsid(struct fsnotify_mark *mark,
+				  fsnotify_connp_t *connp, unsigned int type,
+				  int allow_dups, __kernel_fsid_t *fsid)
 {
 	struct fsnotify_group *group = mark->group;
 	int ret = 0;
@@ -627,7 +652,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 	fsnotify_get_mark(mark); /* for g_list */
 	spin_unlock(&mark->lock);
 
-	ret = fsnotify_add_mark_list(mark, connp, type, allow_dups);
+	ret = fsnotify_add_mark_list(mark, connp, type, allow_dups, fsid);
 	if (ret)
 		goto err;
 
@@ -648,13 +673,14 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 }
 
 int fsnotify_add_mark(struct fsnotify_mark *mark, fsnotify_connp_t *connp,
-		      unsigned int type, int allow_dups)
+		      unsigned int type, int allow_dups, __kernel_fsid_t *fsid)
 {
 	int ret;
 	struct fsnotify_group *group = mark->group;
 
 	mutex_lock(&group->mark_mutex);
-	ret = fsnotify_add_mark_locked(mark, connp, type, allow_dups);
+	ret = fsnotify_add_mark_locked_fsid(mark, connp, type, allow_dups,
+					    fsid);
 	mutex_unlock(&group->mark_mutex);
 	return ret;
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 1e4b88bd1443..b66c4199d629 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -293,6 +293,7 @@ typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t;
 struct fsnotify_mark_connector {
 	spinlock_t lock;
 	unsigned int type;	/* Type of object [lock] */
+	__kernel_fsid_t fsid;	/* fsid of filesystem containing object */
 	union {
 		/* Object pointer [lock] */
 		fsnotify_connp_t *obj;
@@ -433,20 +434,32 @@ extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 /* Find mark belonging to given group in the list of marks */
 extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp,
 						struct fsnotify_group *group);
+/* Get cached fsid of filesystem containing object */
+extern int fsnotify_get_conn_fsid(const struct fsnotify_mark_connector *conn,
+				  __kernel_fsid_t *fsid);
 /* attach the mark to the object */
 extern int fsnotify_add_mark(struct fsnotify_mark *mark,
 			     fsnotify_connp_t *connp, unsigned int type,
-			     int allow_dups);
-extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-				    fsnotify_connp_t *connp, unsigned int type,
-				    int allow_dups);
+			     int allow_dups, __kernel_fsid_t *fsid);
+extern int fsnotify_add_mark_locked_fsid(struct fsnotify_mark *mark,
+					 fsnotify_connp_t *connp,
+					 unsigned int type, int allow_dups,
+					 __kernel_fsid_t *fsid);
+static inline int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
+					   fsnotify_connp_t *connp,
+					   unsigned int type, int allow_dups)
+{
+	return fsnotify_add_mark_locked_fsid(mark, connp, type, allow_dups,
+					     NULL);
+}
+
 /* attach the mark to the inode */
 static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark,
 					  struct inode *inode,
 					  int allow_dups)
 {
 	return fsnotify_add_mark(mark, &inode->i_fsnotify_marks,
-				 FSNOTIFY_OBJ_TYPE_INODE, allow_dups);
+				 FSNOTIFY_OBJ_TYPE_INODE, allow_dups, NULL);
 }
 static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark,
 						 struct inode *inode,
@@ -455,6 +468,7 @@ static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark,
 	return fsnotify_add_mark_locked(mark, &inode->i_fsnotify_marks,
 					FSNOTIFY_OBJ_TYPE_INODE, allow_dups);
 }
+
 /* given a group and a mark, flag mark to be freed when all references are dropped */
 extern void fsnotify_destroy_mark(struct fsnotify_mark *mark,
 				  struct fsnotify_group *group);
-- 
2.17.1

[PATCH v4 10/15] vfs: add vfs_get_fsid() helper

[Amir Goldstein]

Wrapper around statfs() interface.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/statfs.c            | 14 ++++++++++++++
 include/linux/statfs.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/fs/statfs.c b/fs/statfs.c
index f0216629621d..eea7af6f2f22 100644
--- a/fs/statfs.c
+++ b/fs/statfs.c
@@ -67,6 +67,20 @@ static int statfs_by_dentry(struct dentry *dentry, struct kstatfs *buf)
 	return retval;
 }
 
+int vfs_get_fsid(struct dentry *dentry, __kernel_fsid_t *fsid)
+{
+	struct kstatfs st;
+	int error;
+
+	error = statfs_by_dentry(dentry, &st);
+	if (error)
+		return error;
+
+	*fsid = st.f_fsid;
+	return 0;
+}
+EXPORT_SYMBOL(vfs_get_fsid);
+
 int vfs_statfs(const struct path *path, struct kstatfs *buf)
 {
 	int error;
diff --git a/include/linux/statfs.h b/include/linux/statfs.h
index 3142e98546ac..9bc69edb8f18 100644
--- a/include/linux/statfs.h
+++ b/include/linux/statfs.h
@@ -41,4 +41,7 @@ struct kstatfs {
 #define ST_NODIRATIME	0x0800	/* do not update directory access times */
 #define ST_RELATIME	0x1000	/* update atime relative to mtime/ctime */
 
+struct dentry;
+extern int vfs_get_fsid(struct dentry *dentry, __kernel_fsid_t *fsid);
+
 #endif
-- 
2.17.1

[PATCH v4 11/15] fanotify: use vfs_get_fsid() helper instead of vfs_statfs()

[Amir Goldstein]

This is a cleanup that doesn't change any logic.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index b69a7c23a765..8ecd9db8931c 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -855,35 +855,31 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 }
 
 /* Check if filesystem can encode a unique fid */
-static int fanotify_test_fid(struct path *path, struct kstatfs *stat)
+static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
 {
-	struct kstatfs root_stat;
-	struct path root = {
-		.mnt = path->mnt,
-		.dentry = path->dentry->d_sb->s_root,
-	};
+	__kernel_fsid_t root_fsid;
 	int err;
 
 	/*
 	 * Make sure path is not in filesystem with zero fsid (e.g. tmpfs).
 	 */
-	err = vfs_statfs(path, stat);
+	err = vfs_get_fsid(path->dentry, fsid);
 	if (err)
 		return err;
 
-	if (!stat->f_fsid.val[0] && !stat->f_fsid.val[1])
+	if (!fsid->val[0] && !fsid->val[1])
 		return -ENODEV;
 
 	/*
 	 * Make sure path is not inside a filesystem subvolume (e.g. btrfs)
 	 * which uses a different fsid than sb root.
 	 */
-	err = vfs_statfs(&root, &root_stat);
+	err = vfs_get_fsid(path->dentry->d_sb->s_root, &root_fsid);
 	if (err)
 		return err;
 
-	if (root_stat.f_fsid.val[0] != stat->f_fsid.val[0] ||
-	    root_stat.f_fsid.val[1] != stat->f_fsid.val[1])
+	if (root_fsid.val[0] != fsid->val[0] ||
+	    root_fsid.val[1] != fsid->val[1])
 		return -EXDEV;
 
 	/*
@@ -908,8 +904,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	struct fsnotify_group *group;
 	struct fd f;
 	struct path path;
-	struct kstatfs stat;
-	__kernel_fsid_t *fsid = NULL;
+	__kernel_fsid_t __fsid, *fsid = NULL;
 	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
 	int ret;
@@ -988,11 +983,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		goto fput_and_out;
 
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
-		ret = fanotify_test_fid(&path, &stat);
+		ret = fanotify_test_fid(&path, &__fsid);
 		if (ret)
 			goto path_put_and_out;
 
-		fsid = &stat.f_fsid;
+		fsid = &__fsid;
 	}
 
 	/* inode held in place by reference to path; group by fget on fd */
-- 
2.17.1

[PATCH v4 12/15] fanotify: check FS_ISDIR flag instead of d_is_dir()

[Amir Goldstein]

All fsnotify hooks set the FS_ISDIR flag for events that happen
on directory victim inodes except for fsnotify_perm().

Add the missing FS_ISDIR flag in fsnotify_perm() hook and let
fanotify_group_event_mask() check the FS_ISDIR flag instead of
checking if path argument is a directory.

This is needed for fanotify support for event types that do not
carry path information.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index a5dd8b8d92b7..5a958b1d0cb6 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -144,7 +144,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 		marks_ignored_mask |= mark->ignored_mask;
 	}
 
-	if (d_is_dir(path->dentry) &&
+	if (event_mask & FS_ISDIR &&
 	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
 		return 0;
 
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 8de8f390cce2..14e1f43c38c9 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -92,6 +92,9 @@ static inline int fsnotify_perm(struct file *file, int mask)
 		fsnotify_mask = FS_ACCESS_PERM;
 	}
 
+	if (S_ISDIR(inode->i_mode))
+		fsnotify_mask |= FS_ISDIR;
+
 	return fsnotify_path(inode, path, fsnotify_mask);
 }
 
-- 
2.17.1

[PATCH v4 13/15] fanotify: support events with data type FSNOTIFY_EVENT_INODE

[Amir Goldstein]

When event data type is FSNOTIFY_EVENT_INODE, we don't have a refernece
to the mount, so we will not be able to open a file descriptor when user
reads the event. However, if the listener has enabled reporting file
identifier with the FAN_REPORT_FID init flag, we allow repoting those
events and we use an indentifier inode to encode fid.

The inode to use as indetifier when reporting fid depedns on the event.
For dirent modification events, we report the modified directory inode
and we report the "victim" inode otherwise.
For example:
FS_ATTRIB reports the child inode even if reported on a watched parent.
FS_CREATE reports the modified dir inode and not the created inode.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 5a958b1d0cb6..5f157dee2089 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -96,7 +96,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
 		 group, event, ret);
-	
+
 	return ret;
 }
 
@@ -106,9 +106,10 @@ static int fanotify_get_response(struct fsnotify_group *group,
  * been included within the event mask, but have not been explicitly
  * requested by the user, will not be present in the returned mask.
  */
-static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
-				       u32 event_mask, const void *data,
-				       int data_type)
+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)
 {
 	__u32 marks_mask = 0, marks_ignored_mask = 0;
 	const struct path *path = data;
@@ -118,14 +119,14 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 	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 we don't have enough info to send an event to userspace say no */
-	if (data_type != FSNOTIFY_EVENT_PATH)
-		return 0;
-
-	/* Sorry, fanotify only gives a damn about files and dirs */
-	if (!d_is_reg(path->dentry) &&
-	    !d_can_lookup(path->dentry))
+	if (data_type == FSNOTIFY_EVENT_PATH) {
+		/* 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 (!FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+		/* Events without path data require FAN_REPORT_FID */
 		return 0;
+	}
 
 	fsnotify_foreach_obj_type(type) {
 		if (!fsnotify_iter_should_report_type(iter_info, type))
@@ -153,7 +154,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 }
 
 static int fanotify_encode_fid(struct fanotify_event *event,
-			       const struct path *path, gfp_t gfp,
+			       struct inode *inode, gfp_t gfp,
 			       __kernel_fsid_t *fsid)
 {
 	struct fanotify_fid *fid = &event->fid;
@@ -166,7 +167,7 @@ static int fanotify_encode_fid(struct fanotify_event *event,
 	fid->ext_fh = NULL;
 	dwords = 0;
 	err = -ENOENT;
-	type = exportfs_encode_fh(path->dentry, NULL, &dwords,  0);
+	type = exportfs_encode_inode_fh(inode, NULL, &dwords,  NULL);
 	if (!dwords)
 		goto out_err;
 
@@ -179,8 +180,8 @@ static int fanotify_encode_fid(struct fanotify_event *event,
 			goto out_err;
 	}
 
-	type = exportfs_encode_fh(path->dentry, fanotify_fid_fh(fid, bytes),
-				  &dwords,  0);
+	type = exportfs_encode_inode_fh(inode, fanotify_fid_fh(fid, bytes),
+					&dwords,  NULL);
 	err = -EINVAL;
 	if (!type || type == FILEID_INVALID || bytes != dwords << 2)
 		goto out_err;
@@ -201,13 +202,34 @@ static int fanotify_encode_fid(struct fanotify_event *event,
 	return FILEID_INVALID;
 }
 
+/*
+ * The inode to use as indetifier when reporting fid depedns on the event.
+ * Report the modified directory inode on dirent modification events.
+ * Report the "victim" inode otherwise.
+ * For example:
+ * FS_ATTRIB reports the child inode even if reported on a watched parent.
+ * FS_CREATE reports the modified dir inode and not the created inode.
+ */
+static struct inode *fanotify_report_id(struct inode *to_tell, u32 event_mask,
+					const void *data, int data_type)
+{
+	if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
+		return to_tell;
+	else if (data_type == FSNOTIFY_EVENT_INODE)
+		return (struct inode *)data;
+	else if (data_type == FSNOTIFY_EVENT_PATH)
+		return d_inode(((struct path *)data)->dentry);
+	return NULL;
+}
+
 struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 					    struct inode *inode, u32 mask,
-					    const struct path *path,
+					    const void *data, int data_type,
 					    __kernel_fsid_t *fsid)
 {
 	struct fanotify_event *event = NULL;
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
+	struct inode *id = fanotify_report_id(inode, mask, data, data_type);
 
 	/*
 	 * For queues with unlimited length lost events are not expected and
@@ -241,12 +263,12 @@ init: __maybe_unused
 	else
 		event->pid = get_pid(task_tgid(current));
 	event->fh_len = 0;
-	if (path && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+	if (id && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
 		/* Report the event without a file identifier on encode error */
-		event->fh_type = fanotify_encode_fid(event, path, gfp, fsid);
-	} else if (path) {
+		event->fh_type = fanotify_encode_fid(event, id, gfp, fsid);
+	} else if (data_type == FSNOTIFY_EVENT_PATH) {
 		event->fh_type = 0;
-		event->path = *path;
+		event->path = *((struct path *)data);
 		path_get(&event->path);
 	} else {
 		event->fh_type = FILEID_INVALID;
@@ -306,7 +328,8 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 
 	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 12);
 
-	mask = fanotify_group_event_mask(iter_info, mask, data, data_type);
+	mask = fanotify_group_event_mask(group, iter_info, mask, data,
+					 data_type);
 	if (!mask)
 		return 0;
 
@@ -325,7 +348,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID))
 		fsid = fanotify_get_fsid(iter_info, &__fsid);
 
-	event = fanotify_alloc_event(group, inode, mask, data, fsid);
+	event = fanotify_alloc_event(group, inode, mask, data, data_type, fsid);
 	ret = -ENOMEM;
 	if (unlikely(!event)) {
 		/*
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 06b93cf227a2..3fd74949a26d 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -136,5 +136,5 @@ static inline struct fanotify_event *FANOTIFY_E(struct fsnotify_event *fse)
 
 struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 					    struct inode *inode, u32 mask,
-					    const struct path *path,
+					    const void *data, int data_type,
 					    __kernel_fsid_t *fsid);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8ecd9db8931c..8bbcf6157927 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -792,7 +792,8 @@ 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, NULL);
+	oevent = fanotify_alloc_event(group, NULL, FS_Q_OVERFLOW, NULL,
+				      FSNOTIFY_EVENT_NONE, NULL);
 	if (unlikely(!oevent)) {
 		fd = -ENOMEM;
 		goto out_destroy_group;
-- 
2.17.1

[PATCH v4 14/15] fanotify: add support for create/attrib/move/delete events

[Amir Goldstein]

Add support for events with data type FSNOTIFY_EVENT_INODE
(e.g. create/attrib/move/delete) for inode and filesystem mark types.

The "inode" events do not carry enough inormation (i.e. path) to
report event->fd, so we do not allow setting a mask for those events
unless group supports reporting fid.

The "inode" events are not supported on a mount mark, because they do
not carry enough inormation (i.e. path) to be filtered by mount point.

The "dirent" events (create/move/delete) report the fid of the parent
directry where events took place without specifying the filename of the
child. In the future, fanotify may get support for reporting filename
information for those events.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 5f157dee2089..89c19db4d45f 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -315,9 +315,16 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 
 	BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
 	BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
+	BUILD_BUG_ON(FAN_ATTRIB != FS_ATTRIB);
 	BUILD_BUG_ON(FAN_CLOSE_NOWRITE != FS_CLOSE_NOWRITE);
 	BUILD_BUG_ON(FAN_CLOSE_WRITE != FS_CLOSE_WRITE);
 	BUILD_BUG_ON(FAN_OPEN != FS_OPEN);
+	BUILD_BUG_ON(FAN_MOVED_TO != FS_MOVED_TO);
+	BUILD_BUG_ON(FAN_MOVED_FROM != FS_MOVED_FROM);
+	BUILD_BUG_ON(FAN_CREATE != FS_CREATE);
+	BUILD_BUG_ON(FAN_DELETE != FS_DELETE);
+	BUILD_BUG_ON(FAN_DELETE_SELF != FS_DELETE_SELF);
+	BUILD_BUG_ON(FAN_MOVE_SELF != FS_MOVE_SELF);
 	BUILD_BUG_ON(FAN_EVENT_ON_CHILD != FS_EVENT_ON_CHILD);
 	BUILD_BUG_ON(FAN_Q_OVERFLOW != FS_Q_OVERFLOW);
 	BUILD_BUG_ON(FAN_OPEN_PERM != FS_OPEN_PERM);
@@ -326,7 +333,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
 	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
 
-	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 12);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
 
 	mask = fanotify_group_event_mask(group, iter_info, mask, data,
 					 data_type);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8bbcf6157927..731f12cfaac8 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -968,6 +968,18 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	    group->priority == FS_PRIO_0)
 		goto fput_and_out;
 
+	/*
+	 * Events with data type inode do not carry enough inormation to report
+	 * event->fd, so we do not allow setting a mask for inode events unless
+	 * group supports reporting fid.
+	 * inode events are not supported on a mount mark, because they do not
+	 * carry enough inormation (i.e. path) to be filtered by mount point.
+	 */
+	if (mask & FANOTIFY_INODE_EVENTS &&
+	    (!FAN_GROUP_FLAG(group, FAN_REPORT_FID) ||
+	     mark_type == FAN_MARK_MOUNT))
+		goto fput_and_out;
+
 	if (flags & FAN_MARK_FLUSH) {
 		ret = 0;
 		if (mark_type == FAN_MARK_MOUNT)
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index f59be967f72b..e9d45387089f 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -35,10 +35,28 @@
 				 FAN_MARK_IGNORED_SURV_MODIFY | \
 				 FAN_MARK_FLUSH)
 
-/* Events that user can request to be notified on */
-#define FANOTIFY_EVENTS		(FAN_ACCESS | FAN_MODIFY | \
+/*
+ * Events that can be reported with data type FSNOTIFY_EVENT_PATH.
+ * Note that FAN_MODIFY can also be reported with data type
+ * FSNOTIFY_EVENT_INODE.
+ */
+#define FANOTIFY_PATH_EVENTS	(FAN_ACCESS | FAN_MODIFY | \
 				 FAN_CLOSE | FAN_OPEN | FAN_OPEN_EXEC)
 
+/*
+ * Directory entry modification events - reported only to directory
+ * where entry is modified and not to a watching parent.
+ */
+#define FANOTIFY_DIRENT_EVENTS	(FAN_MOVE | FAN_CREATE | FAN_DELETE)
+
+/* Events that can only be reported with data type FSNOTIFY_EVENT_INODE */
+#define FANOTIFY_INODE_EVENTS	(FANOTIFY_DIRENT_EVENTS | \
+				 FAN_ATTRIB | FAN_MOVE_SELF | FAN_DELETE_SELF)
+
+/* Events that user can request to be notified on */
+#define FANOTIFY_EVENTS		(FANOTIFY_PATH_EVENTS | \
+				 FANOTIFY_INODE_EVENTS)
+
 /* Events that require a permission response from user */
 #define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM | \
 				 FAN_OPEN_EXEC_PERM)
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 959ae2bdc7ca..b9effa6f8503 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -7,9 +7,16 @@
 /* the following events that user-space can register for */
 #define FAN_ACCESS		0x00000001	/* File was accessed */
 #define FAN_MODIFY		0x00000002	/* File was modified */
+#define FAN_ATTRIB		0x00000004	/* Metadata changed */
 #define FAN_CLOSE_WRITE		0x00000008	/* Writtable file closed */
 #define FAN_CLOSE_NOWRITE	0x00000010	/* Unwrittable file closed */
 #define FAN_OPEN		0x00000020	/* File was opened */
+#define FAN_MOVED_FROM		0x00000040	/* File was moved from X */
+#define FAN_MOVED_TO		0x00000080	/* File was moved to Y */
+#define FAN_CREATE		0x00000100	/* Subfile was created */
+#define FAN_DELETE		0x00000200	/* Subfile was deleted */
+#define FAN_DELETE_SELF		0x00000400	/* Self was deleted */
+#define FAN_MOVE_SELF		0x00000800	/* Self was moved */
 #define FAN_OPEN_EXEC		0x00001000	/* File was opened for exec */
 
 #define FAN_Q_OVERFLOW		0x00004000	/* Event queued overflowed */
@@ -24,6 +31,7 @@
 
 /* helper events */
 #define FAN_CLOSE		(FAN_CLOSE_WRITE | FAN_CLOSE_NOWRITE) /* close */
+#define FAN_MOVE		(FAN_MOVED_FROM | FAN_MOVED_TO) /* moves */
 
 /* flags used for fanotify_init() */
 #define FAN_CLOEXEC		0x00000001
-- 
2.17.1

[PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

dirent modification events (create/delete/move) do not carry the
child entry name/inode information. Instead, we report FAN_ONDIR
for mkdir/rmdir so user can differentiate them from creat/unlink.

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.

Unlike legacy fanotify events (open/access/close), dirent events
for subdir entries (mkdir/rmdir) will be reported regardless if
user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
be reported if the user asked for it.

Cc: <linux-api@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c | 31 +++++++++++++++++++++++++++++--
 include/linux/fanotify.h      |  9 ++++++---
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 89c19db4d45f..1aa23cefae5d 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -112,6 +112,7 @@ 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_EVENT_TYPES;
 	const struct path *path = data;
 	struct fsnotify_mark *mark;
 	int type;
@@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		marks_ignored_mask |= mark->ignored_mask;
 	}
 
+	test_mask = event_mask & marks_mask & ~marks_ignored_mask;
+
+	/*
+	 * dirent modification events (create/delete/move) do not carry the
+	 * child entry name/inode information. Instead, we report FAN_ONDIR
+	 * for mkdir/rmdir so user can differentiate them from creat/unlink.
+	 *
+	 * 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.
+	 */
+	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
+		/* Do not report FAN_ONDIR without an event type */
+		BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS);
+		if (!(test_mask & FANOTIFY_EVENT_TYPES))
+			return 0;
+
+		user_mask |= FAN_ONDIR;
+	}
+
+	/*
+	 * Unlike legacy fanotify events (open/access/close), dirent events
+	 * for subdir entries (mkdir/rmdir) will be reported regardless if
+	 * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
+	 * be reported if the user asked for it.
+	 */
 	if (event_mask & FS_ISDIR &&
+	    !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
 	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
 		return 0;
 
-	return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask &
-		~marks_ignored_mask;
+	return test_mask & user_mask;
 }
 
 static int fanotify_encode_fid(struct fanotify_event *event,
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index e9d45387089f..f5f86566c277 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -61,13 +61,16 @@
 #define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM | \
 				 FAN_OPEN_EXEC_PERM)
 
+/* Events types that may be reported from vfs */
+#define FANOTIFY_EVENT_TYPES	(FANOTIFY_EVENTS | \
+				 FANOTIFY_PERM_EVENTS)
+
 /* Extra flags that may be reported with event or control handling of events */
 #define FANOTIFY_EVENT_FLAGS	(FAN_EVENT_ON_CHILD | FAN_ONDIR)
 
 /* Events that may be reported to user */
-#define FANOTIFY_OUTGOING_EVENTS	(FANOTIFY_EVENTS | \
-					 FANOTIFY_PERM_EVENTS | \
-					 FAN_Q_OVERFLOW)
+#define FANOTIFY_OUTGOING_EVENTS	(FANOTIFY_EVENT_TYPES | \
+					 FAN_Q_OVERFLOW | FAN_ONDIR)
 
 #define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_OUTGOING_EVENTS | \
 					 FANOTIFY_EVENT_FLAGS)
-- 
2.17.1

Re: [PATCH v4 08/15] fanotify: enable FAN_REPORT_FID init flag

[Amir Goldstein]

On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> When setting up an fanotify listener, user may request to get fid
> information in event instead of an open file descriptor.
>
> The fid obtained with event on a watched object contains the file
> handle returned by name_to_handle_at(2) and fsid returned by statfs(2).
>
> When setting a mark, we need to make sure that the filesystem
> supports encoding file handles with name_to_handle_at(2) and that
> statfs(2) encodes a non-zero fsid.
>

Jan,

On a discussion with Matthew about tests he is writing for FAN_REPORT_TID,
the issue of permission events came up.
Since I am not aware of any specific benefit that FAN_REPORT_TID could
bring to users of permission events, I think the best course of action is to
limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF.
That would simplify tests and man page and if we ever see a use case for
anything else, we can add that in the future.

If you agree, we should add something like this to this patch:

--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -768,6 +768,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int,
flags, unsigned int, event_f_flags)
                return -EINVAL;
        }

+       if ((flags & FAN_REPORT_FID) &&
+           (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF)
+               return -EINVAL;
+
        user = get_current_user();
        if (atomic_read(&user->fanotify_listeners) >
FANOTIFY_DEFAULT_MAX_LISTENERS) {
                free_uid(user);


Thanks,
Amir.

Re: [PATCH v4 08/15] fanotify: enable FAN_REPORT_FID init flag

[Jan Kara]

On Sat 08-12-18 11:26:38, Amir Goldstein wrote:
> On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > When setting up an fanotify listener, user may request to get fid
> > information in event instead of an open file descriptor.
> >
> > The fid obtained with event on a watched object contains the file
> > handle returned by name_to_handle_at(2) and fsid returned by statfs(2).
> >
> > When setting a mark, we need to make sure that the filesystem
> > supports encoding file handles with name_to_handle_at(2) and that
> > statfs(2) encodes a non-zero fsid.
> >
> 
> Jan,
> 
> On a discussion with Matthew about tests he is writing for FAN_REPORT_TID,
> the issue of permission events came up.
> Since I am not aware of any specific benefit that FAN_REPORT_TID could
> bring to users of permission events, I think the best course of action is to
> limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF.
> That would simplify tests and man page and if we ever see a use case for
> anything else, we can add that in the future.
> 
> If you agree, we should add something like this to this patch:

Yeah, that's a good point. Agreed.

								Honza

> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -768,6 +768,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int,
> flags, unsigned int, event_f_flags)
>                 return -EINVAL;
>         }
> 
> +       if ((flags & FAN_REPORT_FID) &&
> +           (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF)
> +               return -EINVAL;
> +
>         user = get_current_user();
>         if (atomic_read(&user->fanotify_listeners) >
> FANOTIFY_DEFAULT_MAX_LISTENERS) {
>                 free_uid(user);
> 
> 
> Thanks,
> Amir.
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 08/15] fanotify: enable FAN_REPORT_FID init flag

[Matthew Bobrowski]

On Mon, Dec 10, 2018 at 05:20:38PM +0100, Jan Kara wrote:
> On Sat 08-12-18 11:26:38, Amir Goldstein wrote:
> > On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > When setting up an fanotify listener, user may request to get fid
> > > information in event instead of an open file descriptor.
> > >
> > > The fid obtained with event on a watched object contains the file
> > > handle returned by name_to_handle_at(2) and fsid returned by statfs(2).
> > >
> > > When setting a mark, we need to make sure that the filesystem
> > > supports encoding file handles with name_to_handle_at(2) and that
> > > statfs(2) encodes a non-zero fsid.
> > >
> > 
> > Jan,
> > 
> > On a discussion with Matthew about tests he is writing for FAN_REPORT_TID,
> > the issue of permission events came up.
> > Since I am not aware of any specific benefit that FAN_REPORT_TID could
> > bring to users of permission events, I think the best course of action is to
> > limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF.
> > That would simplify tests and man page and if we ever see a use case for
> > anything else, we can add that in the future.
> > 
> > If you agree, we should add something like this to this patch:
> 
> Yeah, that's a good point. Agreed.

OK, good to know. I will continue to write the FAN_REPORT_FID tests based on
Amir's fanotify_dirent branch, which contains the amendment suggested below. 

Amir, presumably we should also have a separate test that covers the expected
error result when FAN_REPORT_FID is supplied with invalid class bits? 

> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -768,6 +768,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int,
> > flags, unsigned int, event_f_flags)
> >                 return -EINVAL;
> >         }
> > 
> > +       if ((flags & FAN_REPORT_FID) &&
> > +           (flags & FANOTIFY_CLASS_BITS) != FAN_CLASS_NOTIF)
> > +               return -EINVAL;
> > +
> >         user = get_current_user();
> >         if (atomic_read(&user->fanotify_listeners) >
> > FANOTIFY_DEFAULT_MAX_LISTENERS) {
> >                 free_uid(user);

-- 
Matthew Bobrowski

Re: [PATCH v4 08/15] fanotify: enable FAN_REPORT_FID init flag

[Amir Goldstein]

On Tue, Dec 11, 2018 at 8:12 AM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> On Mon, Dec 10, 2018 at 05:20:38PM +0100, Jan Kara wrote:
> > On Sat 08-12-18 11:26:38, Amir Goldstein wrote:
> > > On Sun, Dec 2, 2018 at 1:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > When setting up an fanotify listener, user may request to get fid
> > > > information in event instead of an open file descriptor.
> > > >
> > > > The fid obtained with event on a watched object contains the file
> > > > handle returned by name_to_handle_at(2) and fsid returned by statfs(2).
> > > >
> > > > When setting a mark, we need to make sure that the filesystem
> > > > supports encoding file handles with name_to_handle_at(2) and that
> > > > statfs(2) encodes a non-zero fsid.
> > > >
> > >
> > > Jan,
> > >
> > > On a discussion with Matthew about tests he is writing for FAN_REPORT_TID,
> > > the issue of permission events came up.
> > > Since I am not aware of any specific benefit that FAN_REPORT_TID could
> > > bring to users of permission events, I think the best course of action is to
> > > limit the use of FAN_REPORT_TID to group with priority FAN_CLASS_NOTIF.
> > > That would simplify tests and man page and if we ever see a use case for
> > > anything else, we can add that in the future.
> > >
> > > If you agree, we should add something like this to this patch:
> >
> > Yeah, that's a good point. Agreed.
>
> OK, good to know. I will continue to write the FAN_REPORT_FID tests based on
> Amir's fanotify_dirent branch, which contains the amendment suggested below.
>
> Amir, presumably we should also have a separate test that covers the expected
> error result when FAN_REPORT_FID is supplied with invalid class bits?
>

Sure, that sounds good.
I also did not find any test coverage for expected error result when trying
to set permission mark bits on a FAN_CLASS_NOTIF group, so the same
test could be the home of this test case as well.
Going further to dirent events, we would also want to add validation
that dirent event cannot be set on mark for group without FAN_REPORT_FID
and cannot be set on a mount mark.

Thanks,
Amir.

Re: [PATCH v4 01/15] fsnotify: annotate directory entry modification events

[Jan Kara]

On Sun 02-12-18 13:38:12, Amir Goldstein wrote:
> "dirent" events are referring to events that modify directory entries,
> such as create,delete,rename. Those events should always be reported
> on a watched directory, regardless if FS_EVENT_ON_CHILD is set
> on the watch mask.
> 
> fsnotify_nameremove() and fsnotify_move() were modified to no longer
> set the FS_EVENT_ON_CHILD event bit. This is a semantic change to
> align with the "dirent" event definition. It has no effect on any
> existing backend, because dnotify, inotify and audit always requets the
> child events and fanotify does not get the delete,rename events.
> 
> The fsnotify_dirent() helper is used instead of fsnotify_parent() to
> report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD
> and regardless if parent has the FS_EVENT_ON_CHILD bit set.
> 
> ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and
> those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD.
> 
> That means for a directory with an inotify watch and only dirent
> events in the mask (i.e. create,delete,move), all children dentries
> will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set.
> This will allow all events that happen on children to be optimized
> away in __fsnotify_parent() without the need to dereference
> child->d_parent->d_inode->i_fsnotify_mask.
> 
> Since the dirent events are never repoted via __fsnotify_parent(),
> this results in no change of logic, but only an optimization.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/linux/fsnotify.h         | 42 +++++++++++++++++++++++++-------
>  include/linux/fsnotify_backend.h | 36 +++++++++++++++------------
>  2 files changed, 54 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 2ccb08cb5d6a..8de8f390cce2 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -17,8 +17,35 @@
>  #include <linux/slab.h>
>  #include <linux/bug.h>
>  
> +/*
> + * Notify this @dir inode about a change in the directory entry @dentry.
> + *
> + * Unlike fsnotify_parent(), the event will be reported regardless of the
> + * FS_EVENT_ON_CHILD mask on the parent inode.
> + *
> + * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent
> + * inode is used as the inode to report the event to.
> + */
> +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry,
> +				  __u32 mask)
> +{
> +	if (!dir)
> +		dir = d_inode(dentry->d_parent);

Just a small nit: This !dir handling is only used for
fsnotify_nameremove(). So why not move that d_parent logic to that single
call site? That would make the function easier to argue about.

Otherwise I like the patch.

								Honza

> +
> +	/*
> +	 * This helper assumes d_parent and d_name are stable. It must be true
> +	 * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about
> +	 * all callers that get here from d_delete() => fsnotify_nameremove().
> +	 */
> +	WARN_ON(!inode_is_locked(dir));
> +
> +	return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> +			dentry->d_name.name, 0);
> +}
> +
>  /* Notify this dentry's parent about a child's events. */
> -static inline int fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask)
> +static inline int fsnotify_parent(const struct path *path,
> +				  struct dentry *dentry, __u32 mask)
>  {
>  	if (!dentry)
>  		dentry = path->dentry;
> @@ -85,8 +112,8 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  {
>  	struct inode *source = moved->d_inode;
>  	u32 fs_cookie = fsnotify_get_cookie();
> -	__u32 old_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_FROM);
> -	__u32 new_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_TO);
> +	__u32 old_dir_mask = FS_MOVED_FROM;
> +	__u32 new_dir_mask = FS_MOVED_TO;
>  	const unsigned char *new_name = moved->d_name.name;
>  
>  	if (old_dir == new_dir)
> @@ -136,7 +163,7 @@ static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
>  	if (isdir)
>  		mask |= FS_ISDIR;
>  
> -	fsnotify_parent(NULL, dentry, mask);
> +	fsnotify_dirent(NULL, dentry, mask);
>  }
>  
>  /*
> @@ -155,7 +182,7 @@ static inline void fsnotify_create(struct inode *inode, struct dentry *dentry)
>  {
>  	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
> +	fsnotify_dirent(inode, dentry, FS_CREATE);
>  }
>  
>  /*
> @@ -176,12 +203,9 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct
>   */
>  static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
>  {
> -	__u32 mask = (FS_CREATE | FS_ISDIR);
> -	struct inode *d_inode = dentry->d_inode;
> -
>  	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
> +	fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR);
>  }
>  
>  /*
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 7639774e7475..7f195d43efaf 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -59,27 +59,33 @@
>   * dnotify and inotify. */
>  #define FS_EVENT_ON_CHILD	0x08000000
>  
> -/* This is a list of all events that may get sent to a parernt based on fs event
> - * happening to inodes inside that directory */
> -#define FS_EVENTS_POSS_ON_CHILD   (FS_ACCESS | FS_MODIFY | FS_ATTRIB |\
> -				   FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN |\
> -				   FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE |\
> -				   FS_DELETE | FS_OPEN_PERM | FS_ACCESS_PERM | \
> -				   FS_OPEN_EXEC | FS_OPEN_EXEC_PERM)
> -
>  #define FS_MOVE			(FS_MOVED_FROM | FS_MOVED_TO)
>  
> +/*
> + * Directory entry modification events - reported only to directory
> + * where entry is modified and not to a watching parent.
> + * The watching parent may get an FS_ATTRIB|FS_EVENT_ON_CHILD event
> + * when a directory entry inside a child subdir changes.
> + */
> +#define ALL_FSNOTIFY_DIRENT_EVENTS	(FS_CREATE | FS_DELETE | FS_MOVE)
> +
>  #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
>  				  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.
> + */
> +#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)
> +
>  /* Events that can be reported to backends */
> -#define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
> -			     FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \
> -			     FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \
> -			     FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \
> -			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
> -			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME | \
> -			     FS_OPEN_EXEC | FS_OPEN_EXEC_PERM)
> +#define ALL_FSNOTIFY_EVENTS (ALL_FSNOTIFY_DIRENT_EVENTS | \
> +			     FS_EVENTS_POSS_ON_CHILD | \
> +			     FS_DELETE_SELF | FS_MOVE_SELF | FS_DN_RENAME | \
> +			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED)
>  
>  /* Extra flags that may be reported with event or control handling of events */
>  #define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 06/15] fanotify: encode file identifier for FAN_REPORT_FID

[Jan Kara]

On Sun 02-12-18 13:38:17, Amir Goldstein wrote:
> When user requests the flag FAN_REPORT_FID in fanotify_init(),
> a unique file indetifier of the event target object will be reported
                ^^^ identifier

> with the event.
> 
> The file identifier includes the filesystem's fsid (i.e. from statfs(2))
> and an NFS file handle of the file (i.e. from name_to_handle_at(2)).
> 
> The file identifier makes holding the path reference and passing a file
> descriptor to user redundant, so those are disabled in a group with
> FAN_REPORT_FID.
> 
> Encode fid and store it in event for a group with FAN_REPORT_FID.
> Up to 12 bytes of file handle on 32bit arch (16 bytes on 64bit arch)
> are stored inline in fanotify_event struct. Larger file handles are
> stored in an extennal allocated buffer.
               ^^^ external

> On failure to encode fid, we print a warning and queue the event
> without the fid information.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks good. Just some nits below:

> +static int fanotify_encode_fid(struct fanotify_event *event,
> +			       const struct path *path, gfp_t gfp)
> +{
> +	struct fanotify_fid *fid = &event->fid;
> +	int dwords, bytes = 0;
> +	struct kstatfs stat;
> +	int err, type;
> +
> +	stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
> +	fid->ext_fh = NULL;
> +	dwords = 0;
> +	err = -ENOENT;
> +	type = exportfs_encode_fh(path->dentry, NULL, &dwords,  0);
> +	if (!dwords)
> +		goto out_err;
> +
> +	err = vfs_statfs(path, &stat);
> +	if (err)
> +		goto out_err;
> +
> +	bytes = dwords << 2;
> +	if (!fanotify_inline_fh_len(bytes)) {

I'd just remove this helper. Direct comparison against FANOTIFY_INLINE_FH_LEN
is clearer and I don't see a benefit of the wrapper.

> +		/* Treat failure to allocate fh as failure to allocate event */
> +		err = -ENOMEM;
> +		fid->ext_fh = kmalloc(bytes, gfp);
> +		if (!fid->ext_fh)
> +			goto out_err;
> +	}
> +
> +	type = exportfs_encode_fh(path->dentry, fanotify_fid_fh(fid, bytes),
> +				  &dwords,  0);
> +	err = -EINVAL;
> +	if (!type || type == FILEID_INVALID || bytes != dwords << 2)
> +		goto out_err;
> +
> +	fid->fsid = stat.f_fsid;
> +	event->fh_len = bytes;
> +
> +	return type;
> +
> +out_err:
> +	pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
> +			    stat.f_fsid.val[0], stat.f_fsid.val[1],
> +			    type, bytes, err);
> +	kfree(fid->ext_fh);
> +	fid->ext_fh = NULL;
> +	event->fh_len = 0;
> +
> +	return FILEID_INVALID;
> +}
> +
>  struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
>  						 struct inode *inode, u32 mask,
>  						 const struct path *path)
> @@ -181,10 +241,16 @@ init: __maybe_unused
>  		event->pid = get_pid(task_pid(current));
>  	else
>  		event->pid = get_pid(task_tgid(current));
> -	if (path) {
> +	event->fh_len = 0;
> +	if (path && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> +		/* Report the event without a file identifier on encode error */
> +		event->fh_type = fanotify_encode_fid(event, path, gfp);
> +	} else if (path) {
> +		event->fh_type = 0;

Maybe using FILEID_ROOT here and in other places like the test in
fanotify_encode_fid(), fanotify_event_has_path(), etc. would make it more
obvious that there cannot be a random clash with some real fh type? At
least I had to look it up in the exportfs code...

>  		event->path = *path;
>  		path_get(&event->path);
>  	} else {
> +		event->fh_type = FILEID_INVALID;
>  		event->path.mnt = NULL;
>  		event->path.dentry = NULL;
>  	}

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

Re: [PATCH v4 01/15] fsnotify: annotate directory entry modification events

[Amir Goldstein]

On Thu, Jan 3, 2019 at 5:41 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 02-12-18 13:38:12, Amir Goldstein wrote:
> > "dirent" events are referring to events that modify directory entries,
> > such as create,delete,rename. Those events should always be reported
> > on a watched directory, regardless if FS_EVENT_ON_CHILD is set
> > on the watch mask.
> >
> > fsnotify_nameremove() and fsnotify_move() were modified to no longer
> > set the FS_EVENT_ON_CHILD event bit. This is a semantic change to
> > align with the "dirent" event definition. It has no effect on any
> > existing backend, because dnotify, inotify and audit always requets the
> > child events and fanotify does not get the delete,rename events.
> >
> > The fsnotify_dirent() helper is used instead of fsnotify_parent() to
> > report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD
> > and regardless if parent has the FS_EVENT_ON_CHILD bit set.
> >
> > ALL_FSNOTIFY_DIRENT_EVENTS defines all the dirent event types and
> > those event types have been extracted out of FS_EVENTS_POSS_ON_CHILD.
> >
> > That means for a directory with an inotify watch and only dirent
> > events in the mask (i.e. create,delete,move), all children dentries
> > will no longer have the DCACHE_FSNOTIFY_PARENT_WATCHED flag set.
> > This will allow all events that happen on children to be optimized
> > away in __fsnotify_parent() without the need to dereference
> > child->d_parent->d_inode->i_fsnotify_mask.
> >
> > Since the dirent events are never repoted via __fsnotify_parent(),
> > this results in no change of logic, but only an optimization.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  include/linux/fsnotify.h         | 42 +++++++++++++++++++++++++-------
> >  include/linux/fsnotify_backend.h | 36 +++++++++++++++------------
> >  2 files changed, 54 insertions(+), 24 deletions(-)
> >
> > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > index 2ccb08cb5d6a..8de8f390cce2 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -17,8 +17,35 @@
> >  #include <linux/slab.h>
> >  #include <linux/bug.h>
> >
> > +/*
> > + * Notify this @dir inode about a change in the directory entry @dentry.
> > + *
> > + * Unlike fsnotify_parent(), the event will be reported regardless of the
> > + * FS_EVENT_ON_CHILD mask on the parent inode.
> > + *
> > + * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent
> > + * inode is used as the inode to report the event to.
> > + */
> > +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry,
> > +                               __u32 mask)
> > +{
> > +     if (!dir)
> > +             dir = d_inode(dentry->d_parent);
>
> Just a small nit: This !dir handling is only used for
> fsnotify_nameremove(). So why not move that d_parent logic to that single
> call site? That would make the function easier to argue about.
>

Yeh, I did that it Errata patch 16/15 (already squashed on my dev branch):
https://marc.info/?l=linux-fsdevel&m=154410829914931&w=2

> Otherwise I like the patch.
>
>                                                                 Honza
>
> > +
> > +     /*
> > +      * This helper assumes d_parent and d_name are stable. It must be true
> > +      * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about
> > +      * all callers that get here from d_delete() => fsnotify_nameremove().
> > +      */
> > +     WARN_ON(!inode_is_locked(dir));

In your review of v3 you wrote:

https://marc.info/?l=linux-fsdevel&m=154340994815488&w=2
"generally directory i_rwsem is held when unlinking and so dentry name cannot
change but then it would be good to assert that? Who knows what some weird
fs is doing..."

So I went and did the research. The result is in said 16/15 Errata patch.
Not sure you will like it though...

Thanks,
Amir.

Re: [PATCH v4 07/15] fanotify: copy event fid info to user

[Jan Kara]

On Sun 02-12-18 13:38:18, Amir Goldstein wrote:
> If group requested FAN_REPORT_FID and event has file identifier,
> copy that information to user reading the event after event metadata.
> 
> fid information is formatted as struct fanotify_event_info_fid
> that includes a generic header struct fanotify_event_info_header,
> so that other info types could be defined in the future using the
> same header.
> 
> metadata->event_len includes the length of the fid information.
> 
> The fid information includes the filesystem's fsid (see statfs(2))
> followed by an NFS file handle of the file that could be passed as
> an argument to open_by_handle_at(2).
> 
> Cc: <linux-api@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks mostly good, some smaller comments below.

> ---
>  fs/notify/fanotify/fanotify.h      |  5 ++
>  fs/notify/fanotify/fanotify_user.c | 85 ++++++++++++++++++++++++++++--
>  include/uapi/linux/fanotify.h      | 20 +++++++
>  3 files changed, 105 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index cddebea5ff67..265fbaa2d5b7 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -99,6 +99,11 @@ static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event)
>  		!fanotify_inline_fh_len(event->fh_len);
>  }
>  
> +static inline void *fanotify_event_fh(struct fanotify_event *event)
> +{
> +	return fanotify_fid_fh(&event->fid, event->fh_len);
> +}
> +
>  /*
>   * Structure for permission fanotify events. It gets allocated and freed in
>   * fanotify_handle_event() since we wait there for user response. When the
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 9b9e6704f9e7..032a9a225a21 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -47,6 +47,22 @@ struct kmem_cache *fanotify_mark_cache __read_mostly;
>  struct kmem_cache *fanotify_event_cachep __read_mostly;
>  struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
>  
> +static int fanotify_event_info_len(struct fanotify_event *event)
> +{
> +	if (!fanotify_event_has_fid(event))
> +		return 0;
> +
> +	return sizeof(struct fanotify_event_info_fid) +
> +		sizeof(struct file_handle) + event->fh_len;
> +}
> +
> +#define FANOTIFY_EVENT_ALIGN (sizeof(void *))

Please no. This will be different for 32-bit vs 64-bit and that is just
asking for trouble with applications that start depending on particular
alignment in weird ways. Just make this 4 and be done with it. The info
header is 4 bytes so any additional info will get 4-byte alignment at best
anyway.

> +
> +static int fanotify_round_event_info_len(struct fanotify_event *event)
> +{
> +	return roundup(fanotify_event_info_len(event), FANOTIFY_EVENT_ALIGN);
> +}
> +
>  /*
>   * Get an fsnotify notification event if one exists and is small
>   * enough to fit in "count". Return an error pointer if the count
> @@ -57,6 +73,9 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
>  static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
>  					    size_t count)
>  {
> +	size_t event_size = FAN_EVENT_METADATA_LEN;
> +	struct fanotify_event *event;
> +
>  	assert_spin_locked(&group->notification_lock);
>  
>  	pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
> @@ -64,11 +83,18 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
>  	if (fsnotify_notify_queue_is_empty(group))
>  		return NULL;
>  
> -	if (FAN_EVENT_METADATA_LEN > count)
> +	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> +		event = FANOTIFY_E(fsnotify_peek_first_event(group));
> +		event_size += fanotify_round_event_info_len(event);
> +	}
> +
> +	if (event_size > count)
>  		return ERR_PTR(-EINVAL);
>  
> -	/* held the notification_lock the whole time, so this is the
> -	 * same event we peeked above */
> +	/*
> +	 * Held the notification_lock the whole time, so this is the
> +	 * same event we peeked above
> +	 */
>  	return fsnotify_remove_first_event(group);
>  }
>  
> @@ -174,6 +200,47 @@ static int process_access_response(struct fsnotify_group *group,
>  	return 0;
>  }
>  
> +static int copy_fid_to_user(struct fanotify_event *event, char __user *buf)
> +{
> +	struct fanotify_event_info_fid info;
> +	struct file_handle handle;
> +	size_t fh_len = event->fh_len;
> +	size_t info_len = fanotify_event_info_len(event);
> +	size_t len = roundup(info_len, FANOTIFY_EVENT_ALIGN);
> +
> +	if (!info_len)
> +		return 0;
> +
> +	/* Copy event info fid header followed by vaiable sized file handle */
> +	info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID;
> +	info.hdr.len = info_len;

I'm somewhat undecided whether the info_len should include the padding or
not. Including it would make code somewhat simpler (in particular userspace
would not have to be aware of FANOTIFY_EVENT_ALIGN - which you currently
don't propagate BTW), also fanotify_event_info_len() could just include the
padding and thus reduce the possibility we forget about it in some place.
OTOH not including it could save us from having to specify length of some
variable length array in some future event info type (e.g. here we would
not have to pass handle length if we didn't want to pass opaque
file_handle).

Currently I'm leaning more towards including it, what are your thoughts on
this?

> +	info.fsid = event->fid.fsid;
> +	info_len = sizeof(info) + sizeof(struct file_handle);

The value set here does not appear to be used anywhere?

> +	if (copy_to_user(buf, &info, sizeof(info)))
> +		return -EFAULT;

You can leak 1 byte of uninitialized stack contents to userspace here
(info.hdr.pad). I'd suggest you use empty initializers for info and handle
to make sure we cannot leak anything and the compiler will hopefully
eliminate the unnecessary zeroing.

> +
> +	buf += sizeof(info);
> +	len -= sizeof(info);
> +	handle.handle_type = event->fh_type;
> +	handle.handle_bytes = event->fh_len;

Use local variable fh_len here?

> +	if (copy_to_user(buf, &handle, sizeof(handle)))
> +		return -EFAULT;
> +
> +	buf += sizeof(handle);
> +	len -= sizeof(handle);
> +	if (copy_to_user(buf, fanotify_event_fh(event), fh_len))
> +		return -EFAULT;
> +
> +	/* Pad with 0's */
> +	buf += fh_len;
> +	len -= fh_len;
> +	WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN);
> +	if (len > 0 && clear_user(buf, len))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +

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

Re: [PATCH v4 07/15] fanotify: copy event fid info to user

[Amir Goldstein]

On Thu, Jan 3, 2019 at 7:13 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 02-12-18 13:38:18, Amir Goldstein wrote:
> > If group requested FAN_REPORT_FID and event has file identifier,
> > copy that information to user reading the event after event metadata.
> >
> > fid information is formatted as struct fanotify_event_info_fid
> > that includes a generic header struct fanotify_event_info_header,
> > so that other info types could be defined in the future using the
> > same header.
> >
> > metadata->event_len includes the length of the fid information.
> >
> > The fid information includes the filesystem's fsid (see statfs(2))
> > followed by an NFS file handle of the file that could be passed as
> > an argument to open_by_handle_at(2).
> >
> > Cc: <linux-api@vger.kernel.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> The patch looks mostly good, some smaller comments below.
>
> > ---
> >  fs/notify/fanotify/fanotify.h      |  5 ++
> >  fs/notify/fanotify/fanotify_user.c | 85 ++++++++++++++++++++++++++++--
> >  include/uapi/linux/fanotify.h      | 20 +++++++
> >  3 files changed, 105 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index cddebea5ff67..265fbaa2d5b7 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -99,6 +99,11 @@ static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event)
> >               !fanotify_inline_fh_len(event->fh_len);
> >  }
> >
> > +static inline void *fanotify_event_fh(struct fanotify_event *event)
> > +{
> > +     return fanotify_fid_fh(&event->fid, event->fh_len);
> > +}
> > +
> >  /*
> >   * Structure for permission fanotify events. It gets allocated and freed in
> >   * fanotify_handle_event() since we wait there for user response. When the
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 9b9e6704f9e7..032a9a225a21 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -47,6 +47,22 @@ struct kmem_cache *fanotify_mark_cache __read_mostly;
> >  struct kmem_cache *fanotify_event_cachep __read_mostly;
> >  struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
> >
> > +static int fanotify_event_info_len(struct fanotify_event *event)
> > +{
> > +     if (!fanotify_event_has_fid(event))
> > +             return 0;
> > +
> > +     return sizeof(struct fanotify_event_info_fid) +
> > +             sizeof(struct file_handle) + event->fh_len;
> > +}
> > +
> > +#define FANOTIFY_EVENT_ALIGN (sizeof(void *))
>
> Please no. This will be different for 32-bit vs 64-bit and that is just
> asking for trouble with applications that start depending on particular
> alignment in weird ways. Just make this 4 and be done with it. The info
> header is 4 bytes so any additional info will get 4-byte alignment at best
> anyway.
>

ok.

> > +
> > +static int fanotify_round_event_info_len(struct fanotify_event *event)
> > +{
> > +     return roundup(fanotify_event_info_len(event), FANOTIFY_EVENT_ALIGN);
> > +}
> > +
> >  /*
> >   * Get an fsnotify notification event if one exists and is small
> >   * enough to fit in "count". Return an error pointer if the count
> > @@ -57,6 +73,9 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
> >  static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
> >                                           size_t count)
> >  {
> > +     size_t event_size = FAN_EVENT_METADATA_LEN;
> > +     struct fanotify_event *event;
> > +
> >       assert_spin_locked(&group->notification_lock);
> >
> >       pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
> > @@ -64,11 +83,18 @@ static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
> >       if (fsnotify_notify_queue_is_empty(group))
> >               return NULL;
> >
> > -     if (FAN_EVENT_METADATA_LEN > count)
> > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> > +             event = FANOTIFY_E(fsnotify_peek_first_event(group));
> > +             event_size += fanotify_round_event_info_len(event);
> > +     }
> > +
> > +     if (event_size > count)
> >               return ERR_PTR(-EINVAL);
> >
> > -     /* held the notification_lock the whole time, so this is the
> > -      * same event we peeked above */
> > +     /*
> > +      * Held the notification_lock the whole time, so this is the
> > +      * same event we peeked above
> > +      */
> >       return fsnotify_remove_first_event(group);
> >  }
> >
> > @@ -174,6 +200,47 @@ static int process_access_response(struct fsnotify_group *group,
> >       return 0;
> >  }
> >
> > +static int copy_fid_to_user(struct fanotify_event *event, char __user *buf)
> > +{
> > +     struct fanotify_event_info_fid info;
> > +     struct file_handle handle;
> > +     size_t fh_len = event->fh_len;
> > +     size_t info_len = fanotify_event_info_len(event);
> > +     size_t len = roundup(info_len, FANOTIFY_EVENT_ALIGN);
> > +
> > +     if (!info_len)
> > +             return 0;
> > +
> > +     /* Copy event info fid header followed by vaiable sized file handle */
> > +     info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID;
> > +     info.hdr.len = info_len;
>
> I'm somewhat undecided whether the info_len should include the padding or
> not. Including it would make code somewhat simpler (in particular userspace
> would not have to be aware of FANOTIFY_EVENT_ALIGN - which you currently
> don't propagate BTW), also fanotify_event_info_len() could just include the
> padding and thus reduce the possibility we forget about it in some place.
> OTOH not including it could save us from having to specify length of some
> variable length array in some future event info type (e.g. here we would
> not have to pass handle length if we didn't want to pass opaque
> file_handle).
>
> Currently I'm leaning more towards including it, what are your thoughts on
> this?
>

Arguments in favor of include padding in len out number and out weight
the alternative, so I'll go with that.

> > +     info.fsid = event->fid.fsid;
> > +     info_len = sizeof(info) + sizeof(struct file_handle);
>
> The value set here does not appear to be used anywhere?
>
> > +     if (copy_to_user(buf, &info, sizeof(info)))
> > +             return -EFAULT;
>
> You can leak 1 byte of uninitialized stack contents to userspace here
> (info.hdr.pad). I'd suggest you use empty initializers for info and handle
> to make sure we cannot leak anything and the compiler will hopefully
> eliminate the unnecessary zeroing.
>
> > +
> > +     buf += sizeof(info);
> > +     len -= sizeof(info);
> > +     handle.handle_type = event->fh_type;
> > +     handle.handle_bytes = event->fh_len;
>
> Use local variable fh_len here?
>

Sure.

Thanks,
Amir.

Re: [PATCH v4 09/15] fanotify: cache fsid in fsnotify_mark_connector

[Jan Kara]

On Sun 02-12-18 13:38:20, Amir Goldstein wrote:
> @@ -268,6 +289,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>  	int ret = 0;
>  	struct fanotify_event *event;
>  	struct fsnotify_event *fsn_event;
> +	__kernel_fsid_t __fsid, *fsid = NULL;
>  
>  	BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
>  	BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
> @@ -300,7 +322,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>  			return 0;
>  	}
>  
> -	event = fanotify_alloc_event(group, inode, mask, data);
> +	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID))
> +		fsid = fanotify_get_fsid(iter_info, &__fsid);
> +
> +	event = fanotify_alloc_event(group, inode, mask, data, fsid);

This looks wrong? fsid is never assigned anything != NULL? How could have
this worked?

> @@ -853,9 +855,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  }
>  
>  /* Check if filesystem can encode a unique fid */
> -static int fanotify_test_fid(struct path *path)
> +static int fanotify_test_fid(struct path *path, struct kstatfs *stat)
>  {
> -	struct kstatfs stat, root_stat;
> +	struct kstatfs root_stat;

Any reason why you don't just return fsid from fanotify_test_fid() instead
of full kstatfs structure? The extra copy of 8 bytes does not really hurt
us...

> @@ -553,15 +560,33 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
>  
>  	if (WARN_ON(!fsnotify_valid_obj_type(type)))
>  		return -EINVAL;
> +
> +	/* Backend is expected to check for zero fsid (e.g. tmpfs) */
> +	if (fsid && WARN_ON_ONCE(!fsid->val[0] && !fsid->val[1]))
> +		return -ENODEV;
> +
>  restart:
>  	spin_lock(&mark->lock);
>  	conn = fsnotify_grab_connector(connp);
>  	if (!conn) {
>  		spin_unlock(&mark->lock);
> -		err = fsnotify_attach_connector_to_object(connp, type);
> +		err = fsnotify_attach_connector_to_object(connp, type, fsid);
>  		if (err)
>  			return err;
>  		goto restart;
> +	} else if (fsid &&
> +		   (fsid->val[0] != conn->fsid.val[0] ||
> +		    fsid->val[1] != conn->fsid.val[1])) {

I think you need to allow transition from 0 to something != 0 here.
Otherwise you'll get this warning if someone attaches say inotify mark to
an inode and then fanotify mark for FAN_REPORT_FID group.

> +		/*
> +		 * Backend is expected to check for non uniform fsid
> +		 * (e.g. btrfs), but maybe we missed something?
> +		 */
> +		pr_warn_ratelimited("%s: fsid mismatch on object of type %u: %x.%x != %x.%x\n",
> +				    __func__, conn->type,
> +				    fsid->val[0], fsid->val[1],
> +				    conn->fsid.val[0], conn->fsid.val[1]);
> +		err = -EXDEV;
> +		goto out_err;
>  	}
>  
>  	/* is mark the first mark? */

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

Re: [PATCH v4 09/15] fanotify: cache fsid in fsnotify_mark_connector

[Jan Kara]

On Fri 04-01-19 10:38:39, Jan Kara wrote:
> On Sun 02-12-18 13:38:20, Amir Goldstein wrote:
> > @@ -268,6 +289,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
> >  	int ret = 0;
> >  	struct fanotify_event *event;
> >  	struct fsnotify_event *fsn_event;
> > +	__kernel_fsid_t __fsid, *fsid = NULL;
> >  
> >  	BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
> >  	BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
> > @@ -300,7 +322,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
> >  			return 0;
> >  	}
> >  
> > -	event = fanotify_alloc_event(group, inode, mask, data);
> > +	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID))
> > +		fsid = fanotify_get_fsid(iter_info, &__fsid);
> > +
> > +	event = fanotify_alloc_event(group, inode, mask, data, fsid);
> 
> This looks wrong? fsid is never assigned anything != NULL? How could have
> this worked?

Sorry. I understand now. But the calling convention is somewhat unusual.
Actually, since fsid is just 8 bytes anyway, why not just return is as
value? I.e. something like:

static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info*iter_info)
{
...
}

You won't save any copying on 64-bit archs and I've checked and GCC will
optimize this to a standard register return convention anyway.

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

Re: [PATCH v4 11/15] fanotify: use vfs_get_fsid() helper instead of vfs_statfs()

[Jan Kara]

On Sun 02-12-18 13:38:22, Amir Goldstein wrote:
> This is a cleanup that doesn't change any logic.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify_user.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index b69a7c23a765..8ecd9db8931c 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -855,35 +855,31 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  }
>  
>  /* Check if filesystem can encode a unique fid */
> -static int fanotify_test_fid(struct path *path, struct kstatfs *stat)
> +static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid)
>  {

Ah OK, here you do what I've suggested. I guess no need to redo this then.

								Honza

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

Re: [PATCH v4 13/15] fanotify: support events with data type FSNOTIFY_EVENT_INODE

[Jan Kara]

On Sun 02-12-18 13:38:24, Amir Goldstein wrote:
> When event data type is FSNOTIFY_EVENT_INODE, we don't have a refernece
> to the mount, so we will not be able to open a file descriptor when user
> reads the event. However, if the listener has enabled reporting file
> identifier with the FAN_REPORT_FID init flag, we allow repoting those
							 ^^^ reporting

> events and we use an indentifier inode to encode fid.
			^^ identifier

> The inode to use as indetifier when reporting fid depedns on the event.
			^^ identifier			^^ depends

> For dirent modification events, we report the modified directory inode
> and we report the "victim" inode otherwise.
> For example:
> FS_ATTRIB reports the child inode even if reported on a watched parent.
> FS_CREATE reports the modified dir inode and not the created inode.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
...
> @@ -201,13 +202,34 @@ static int fanotify_encode_fid(struct fanotify_event *event,
>  	return FILEID_INVALID;
>  }
>  
> +/*
> + * The inode to use as indetifier when reporting fid depedns on the event.
			  ^^ identifier			^^ depends

> + * Report the modified directory inode on dirent modification events.
> + * Report the "victim" inode otherwise.
> + * For example:
> + * FS_ATTRIB reports the child inode even if reported on a watched parent.
> + * FS_CREATE reports the modified dir inode and not the created inode.
> + */
> +static struct inode *fanotify_report_id(struct inode *to_tell, u32 event_mask,
> +					const void *data, int data_type)
> +{
> +	if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
> +		return to_tell;
> +	else if (data_type == FSNOTIFY_EVENT_INODE)
> +		return (struct inode *)data;
> +	else if (data_type == FSNOTIFY_EVENT_PATH)
> +		return d_inode(((struct path *)data)->dentry);
> +	return NULL;
> +}
> +

Maybe call this function fanotify_fid_inode()?

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

Re: [PATCH v4 14/15] fanotify: add support for create/attrib/move/delete events

[Jan Kara]

Just some language nits below...

On Sun 02-12-18 13:38:25, Amir Goldstein wrote:
> Add support for events with data type FSNOTIFY_EVENT_INODE
> (e.g. create/attrib/move/delete) for inode and filesystem mark types.
> 
> The "inode" events do not carry enough inormation (i.e. path) to
					 ^^ information

> report event->fd, so we do not allow setting a mask for those events
> unless group supports reporting fid.
> 
> The "inode" events are not supported on a mount mark, because they do
> not carry enough inormation (i.e. path) to be filtered by mount point.
		   ^^ information

> The "dirent" events (create/move/delete) report the fid of the parent
> directry where events took place without specifying the filename of the
  ^^ directory

> @@ -326,7 +333,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>  	BUILD_BUG_ON(FAN_OPEN_EXEC != FS_OPEN_EXEC);
>  	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
>  
> -	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 12);
> +	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
>  
>  	mask = fanotify_group_event_mask(group, iter_info, mask, data,
>  					 data_type);
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 8bbcf6157927..731f12cfaac8 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -968,6 +968,18 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  	    group->priority == FS_PRIO_0)
>  		goto fput_and_out;
>  
> +	/*
> +	 * Events with data type inode do not carry enough inormation to report
							   ^^ information

> +	 * event->fd, so we do not allow setting a mask for inode events unless
> +	 * group supports reporting fid.
> +	 * inode events are not supported on a mount mark, because they do not
> +	 * carry enough inormation (i.e. path) to be filtered by mount point.
			^^ information

> +	 */
> +	if (mask & FANOTIFY_INODE_EVENTS &&
> +	    (!FAN_GROUP_FLAG(group, FAN_REPORT_FID) ||
> +	     mark_type == FAN_MARK_MOUNT))
> +		goto fput_and_out;
> +
>  	if (flags & FAN_MARK_FLUSH) {
>  		ret = 0;
>  		if (mark_type == FAN_MARK_MOUNT)

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

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Jan Kara]

On Sun 02-12-18 13:38:26, Amir Goldstein wrote:
> dirent modification events (create/delete/move) do not carry the
> child entry name/inode information. Instead, we report FAN_ONDIR
> for mkdir/rmdir so user can differentiate them from creat/unlink.
> 
> 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.
> 
> Unlike legacy fanotify events (open/access/close), dirent events
> for subdir entries (mkdir/rmdir) will be reported regardless if
> user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> be reported if the user asked for it.
> 
> Cc: <linux-api@vger.kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Some comments below.

> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 89c19db4d45f..1aa23cefae5d 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -112,6 +112,7 @@ 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_EVENT_TYPES;
>  	const struct path *path = data;
>  	struct fsnotify_mark *mark;
>  	int type;
> @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  		marks_ignored_mask |= mark->ignored_mask;
>  	}
>  
> +	test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> +
> +	/*
> +	 * dirent modification events (create/delete/move) do not carry the
> +	 * child entry name/inode information. Instead, we report FAN_ONDIR
> +	 * for mkdir/rmdir so user can differentiate them from creat/unlink.
> +	 *
> +	 * 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.
> +	 */
> +	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> +		/* Do not report FAN_ONDIR without an event type */
> +		BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS);
> +		if (!(test_mask & FANOTIFY_EVENT_TYPES))
> +			return 0;
> +
> +		user_mask |= FAN_ONDIR;
> +	}
> +
> +	/*
> +	 * Unlike legacy fanotify events (open/access/close), dirent events
> +	 * for subdir entries (mkdir/rmdir) will be reported regardless if
> +	 * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> +	 * be reported if the user asked for it.
> +	 */
>  	if (event_mask & FS_ISDIR &&
> +	    !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&

I disagree with this. It just seems inconsistent for dirent events for
directories to get reported without FAN_ONDIR. I understand there's not
great use for not reporting directory dirent events but it's not like
adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
makes the API more consistent. You could possibly remind the reader in the
manpage that FAN_ONDIR is required to get all dirent events.

>  	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
>  		return 0;
>  
> -	return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask &
> -		~marks_ignored_mask;
> +	return test_mask & user_mask;

The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
I guess. E.g. when implementing recursive watching of a directory. Or what
is your intended usecase? It should be said explicitely in the changelog.

>  static int fanotify_encode_fid(struct fanotify_event *event,
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index e9d45387089f..f5f86566c277 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -61,13 +61,16 @@
>  #define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM | \
>  				 FAN_OPEN_EXEC_PERM)
>  
> +/* Events types that may be reported from vfs */
> +#define FANOTIFY_EVENT_TYPES	(FANOTIFY_EVENTS | \
> +				 FANOTIFY_PERM_EVENTS)
> +
>  /* Extra flags that may be reported with event or control handling of events */
>  #define FANOTIFY_EVENT_FLAGS	(FAN_EVENT_ON_CHILD | FAN_ONDIR)
>  
>  /* Events that may be reported to user */
> -#define FANOTIFY_OUTGOING_EVENTS	(FANOTIFY_EVENTS | \
> -					 FANOTIFY_PERM_EVENTS | \
> -					 FAN_Q_OVERFLOW)
> +#define FANOTIFY_OUTGOING_EVENTS	(FANOTIFY_EVENT_TYPES | \
> +					 FAN_Q_OVERFLOW | FAN_ONDIR)
>  
>  #define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_OUTGOING_EVENTS | \
>  					 FANOTIFY_EVENT_FLAGS)

I don't like this renaming. FAN_ONDIR essentially becomes the same type of
thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
defines as is...

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

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Jan Kara]

Hi,

On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> This is the 4th revision of patch series to add support for filesystem
> change monitoring to fanotify.
> It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> patches.
> The complete work is available on fanotify_dirent branch [1] on my tree.
> 
> The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> dirent modification events is demonstrated with a prototype of global
> filesystem monitor based on inotify-tools [2].
> 
> In your review of v3 patched you only got as far as patch v3 9/13.
> Because this patch marks the end of the FAN_REPORT_FID sub series,
> I found it best to re-post the entire series with the changes you
> requested thus far. For convenience of review, I pushed branches
> fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> reviewed so far and its re-worked version.
> 
> One thing that we discussed and I did NOT do is move struct file_handle
> to uapi headers. This got complicated due to existing definitions in
> glibc header files and I realized we could do without it.
> 
> I have added the vfs_get_fsid() helper as you requested, but since it
> wasn't required by the patch set, I added it as two new cleanup patches
> at the end of the FAN_REPORT_FID series, so you will be able to stage
> the feature with or without the VFS change.

So overall the series looks very good. I've had only some smaller comments
/ disagreements. So once we settle those please resend the series and I'll
pick it up to my tree.

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

Re: [PATCH v4 13/15] fanotify: support events with data type FSNOTIFY_EVENT_INODE

[Amir Goldstein]

On Fri, Jan 4, 2019 at 12:17 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 02-12-18 13:38:24, Amir Goldstein wrote:
> > When event data type is FSNOTIFY_EVENT_INODE, we don't have a refernece
> > to the mount, so we will not be able to open a file descriptor when user
> > reads the event. However, if the listener has enabled reporting file
> > identifier with the FAN_REPORT_FID init flag, we allow repoting those
>                                                          ^^^ reporting
>
> > events and we use an indentifier inode to encode fid.
>                         ^^ identifier
>
> > The inode to use as indetifier when reporting fid depedns on the event.
>                         ^^ identifier                   ^^ depends
>
> > For dirent modification events, we report the modified directory inode
> > and we report the "victim" inode otherwise.
> > For example:
> > FS_ATTRIB reports the child inode even if reported on a watched parent.
> > FS_CREATE reports the modified dir inode and not the created inode.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> > @@ -201,13 +202,34 @@ static int fanotify_encode_fid(struct fanotify_event *event,
> >       return FILEID_INVALID;
> >  }
> >
> > +/*
> > + * The inode to use as indetifier when reporting fid depedns on the event.
>                           ^^ identifier                 ^^ depends
>
> > + * Report the modified directory inode on dirent modification events.
> > + * Report the "victim" inode otherwise.
> > + * For example:
> > + * FS_ATTRIB reports the child inode even if reported on a watched parent.
> > + * FS_CREATE reports the modified dir inode and not the created inode.
> > + */
> > +static struct inode *fanotify_report_id(struct inode *to_tell, u32 event_mask,
> > +                                     const void *data, int data_type)
> > +{
> > +     if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
> > +             return to_tell;
> > +     else if (data_type == FSNOTIFY_EVENT_INODE)
> > +             return (struct inode *)data;
> > +     else if (data_type == FSNOTIFY_EVENT_PATH)
> > +             return d_inode(((struct path *)data)->dentry);
> > +     return NULL;
> > +}
> > +
>
> Maybe call this function fanotify_fid_inode()?

Sounds good.

Thanks,
Amir.

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 02-12-18 13:38:26, Amir Goldstein wrote:
> > dirent modification events (create/delete/move) do not carry the
> > child entry name/inode information. Instead, we report FAN_ONDIR
> > for mkdir/rmdir so user can differentiate them from creat/unlink.
> >
> > 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.
> >
> > Unlike legacy fanotify events (open/access/close), dirent events
> > for subdir entries (mkdir/rmdir) will be reported regardless if
> > user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > be reported if the user asked for it.
> >
> > Cc: <linux-api@vger.kernel.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Some comments below.
>
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 89c19db4d45f..1aa23cefae5d 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -112,6 +112,7 @@ 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_EVENT_TYPES;
> >       const struct path *path = data;
> >       struct fsnotify_mark *mark;
> >       int type;
> > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >               marks_ignored_mask |= mark->ignored_mask;
> >       }
> >
> > +     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > +
> > +     /*
> > +      * dirent modification events (create/delete/move) do not carry the
> > +      * child entry name/inode information. Instead, we report FAN_ONDIR
> > +      * for mkdir/rmdir so user can differentiate them from creat/unlink.
> > +      *
> > +      * 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.
> > +      */
> > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> > +             /* Do not report FAN_ONDIR without an event type */
> > +             BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS);
> > +             if (!(test_mask & FANOTIFY_EVENT_TYPES))
> > +                     return 0;
> > +
> > +             user_mask |= FAN_ONDIR;
> > +     }
> > +
> > +     /*
> > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > +      * be reported if the user asked for it.
> > +      */
> >       if (event_mask & FS_ISDIR &&
> > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
>
> I disagree with this. It just seems inconsistent for dirent events for
> directories to get reported without FAN_ONDIR. I understand there's not
> great use for not reporting directory dirent events but it's not like
> adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> makes the API more consistent. You could possibly remind the reader in the
> manpage that FAN_ONDIR is required to get all dirent events.

I see your point.
I have no problem with requiring FAN_ONDIR for mkdir events.
I believe the strongest argument should be which way is easier
to document/understand.

Matthew, if you agree that it looks easier to document Jan's proposal,
please go a head with this and we will see how man page looks like
before making the final decision.

Anyway, I will change the logic accordingly for v5 patch set.


>
> >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> >               return 0;
> >
> > -     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask &
> > -             ~marks_ignored_mask;
> > +     return test_mask & user_mask;
>
> The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> I guess. E.g. when implementing recursive watching of a directory. Or what
> is your intended usecase? It should be said explicitely in the changelog.

Recursive watch of directory tree is certainly a use case that could benefit
from "mkdir" events. I will add that to commit message.

>
> >  static int fanotify_encode_fid(struct fanotify_event *event,
> > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > index e9d45387089f..f5f86566c277 100644
> > --- a/include/linux/fanotify.h
> > +++ b/include/linux/fanotify.h
> > @@ -61,13 +61,16 @@
> >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> >                                FAN_OPEN_EXEC_PERM)
> >
> > +/* Events types that may be reported from vfs */
> > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > +                              FANOTIFY_PERM_EVENTS)
> > +
> >  /* Extra flags that may be reported with event or control handling of events */
> >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> >
> >  /* Events that may be reported to user */
> > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > -                                      FANOTIFY_PERM_EVENTS | \
> > -                                      FAN_Q_OVERFLOW)
> > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> >
> >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> >                                        FANOTIFY_EVENT_FLAGS)
>
> I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> defines as is...
>

Sorry. I don't understand what you mean.
FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
Do you not like the new group definition FANOTIFY_EVENT_TYPES?

Please explain.

Thanks,
Amir.

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Jan Kara]

On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > I guess. E.g. when implementing recursive watching of a directory. Or what
> > is your intended usecase? It should be said explicitely in the changelog.
> 
> Recursive watch of directory tree is certainly a use case that could benefit
> from "mkdir" events. I will add that to commit message.

Hum, so it does not seem like you had any particular usecase in mind? :)
Before complicating the interface with reporting FAN_ONDIR in some cases
I'd prefer we considered whether the usecases are worth it. So let me start
that: Reporting FAN_ONDIR can distinguish between file/directory
creation/deletion. That can save userspace some rescans of the changed
directory if it is interested only in subdirectory creation / deletion (if
the application is interested only in file changes it just does not set
FAN_ONDIR and that's it).

Another motivation is the compatibility with inotify and it's IN_ISDIR flag
I guess.

Hum, OK, I guess the complication is worth it.

> > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > index e9d45387089f..f5f86566c277 100644
> > > --- a/include/linux/fanotify.h
> > > +++ b/include/linux/fanotify.h
> > > @@ -61,13 +61,16 @@
> > >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > >                                FAN_OPEN_EXEC_PERM)
> > >
> > > +/* Events types that may be reported from vfs */
> > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > +                              FANOTIFY_PERM_EVENTS)
> > > +
> > >  /* Extra flags that may be reported with event or control handling of events */
> > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > >
> > >  /* Events that may be reported to user */
> > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > -                                      FANOTIFY_PERM_EVENTS | \
> > > -                                      FAN_Q_OVERFLOW)
> > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > >
> > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > >                                        FANOTIFY_EVENT_FLAGS)
> >
> > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > defines as is...
> >
> 
> Sorry. I don't understand what you mean.
> FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> Do you not like the new group definition FANOTIFY_EVENT_TYPES?

Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported
to userspace. I don't like the FANOTIFY_EVENT_TYPES name and
FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR
there. So how about renaming FANOTIFY_OUTGOING_EVENTS to
FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your
FANOTIFY_EVENT_TYPES is?

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

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Jan Kara]

On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > >               marks_ignored_mask |= mark->ignored_mask;
> > >       }
> > >
> > > +     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > > +
> > > +     /*
> > > +      * dirent modification events (create/delete/move) do not carry the
> > > +      * child entry name/inode information. Instead, we report FAN_ONDIR
> > > +      * for mkdir/rmdir so user can differentiate them from creat/unlink.
> > > +      *
> > > +      * 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.
> > > +      */
> > > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> > > +             /* Do not report FAN_ONDIR without an event type */
> > > +             BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS);
> > > +             if (!(test_mask & FANOTIFY_EVENT_TYPES))
> > > +                     return 0;
> > > +
> > > +             user_mask |= FAN_ONDIR;
> > > +     }
> > > +
> > > +     /*
> > > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > +      * be reported if the user asked for it.
> > > +      */
> > >       if (event_mask & FS_ISDIR &&
> > > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
> >
> > I disagree with this. It just seems inconsistent for dirent events for
> > directories to get reported without FAN_ONDIR. I understand there's not
> > great use for not reporting directory dirent events but it's not like
> > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> > makes the API more consistent. You could possibly remind the reader in the
> > manpage that FAN_ONDIR is required to get all dirent events.
> 
> I see your point.
> I have no problem with requiring FAN_ONDIR for mkdir events.
> I believe the strongest argument should be which way is easier
> to document/understand.
> 
> Matthew, if you agree that it looks easier to document Jan's proposal,
> please go a head with this and we will see how man page looks like
> before making the final decision.

Agreed.

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

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > > I guess. E.g. when implementing recursive watching of a directory. Or what
> > > is your intended usecase? It should be said explicitely in the changelog.
> >
> > Recursive watch of directory tree is certainly a use case that could benefit
> > from "mkdir" events. I will add that to commit message.
>
> Hum, so it does not seem like you had any particular usecase in mind? :)

Our application does have special handling (scanning) for mkdir events.

> Before complicating the interface with reporting FAN_ONDIR in some cases
> I'd prefer we considered whether the usecases are worth it. So let me start
> that: Reporting FAN_ONDIR can distinguish between file/directory
> creation/deletion. That can save userspace some rescans of the changed
> directory if it is interested only in subdirectory creation / deletion (if
> the application is interested only in file changes it just does not set
> FAN_ONDIR and that's it).
>
> Another motivation is the compatibility with inotify and it's IN_ISDIR flag
> I guess.
>
> Hum, OK, I guess the complication is worth it.
>

Let's see.

The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID
is set, is something that needs to be clarified.

The fact that reported FID refers to the modified directory in dirent events
but FAN_ONDIR flag refers to the created/removed/renamed child is
another thing that needs to be clarified.

Sigh! "things that need to be clarified" are things we need to avoid.
Therefore, I am going to make another suggestion.

How about if we introduced a new flag FAN_DIRENT_ISDIR?
Like IN_ISDIR, it is an out-only flag that cannot be requested.
As its name suggests, it is only applicable to dirent events, so will always
be set when a sub directory entry is created/renamed/removed.

FAN_ONDIR has no effect on dirent events, because FAN_ONDIR
refers to the "victim" inode and the "victim" is the modified directory.
If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user
wants to get notified when *directories* are modified and specifying
FAN_ONDIR explicitly is not needed.

How about that?

> > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > > index e9d45387089f..f5f86566c277 100644
> > > > --- a/include/linux/fanotify.h
> > > > +++ b/include/linux/fanotify.h
> > > > @@ -61,13 +61,16 @@
> > > >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > > >                                FAN_OPEN_EXEC_PERM)
> > > >
> > > > +/* Events types that may be reported from vfs */
> > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > > +                              FANOTIFY_PERM_EVENTS)
> > > > +
> > > >  /* Extra flags that may be reported with event or control handling of events */
> > > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > > >
> > > >  /* Events that may be reported to user */
> > > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > > -                                      FANOTIFY_PERM_EVENTS | \
> > > > -                                      FAN_Q_OVERFLOW)
> > > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > > >
> > > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > > >                                        FANOTIFY_EVENT_FLAGS)
> > >
> > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > > defines as is...
> > >
> >
> > Sorry. I don't understand what you mean.
> > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> > Do you not like the new group definition FANOTIFY_EVENT_TYPES?
>
> Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported
> to userspace. I don't like the FANOTIFY_EVENT_TYPES name and
> FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR
> there. So how about renaming FANOTIFY_OUTGOING_EVENTS to
> FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your
> FANOTIFY_EVENT_TYPES is?
>

Still a bit confusing to me.
I think that with FAN_DIRENT_ISDIR being a different flag than FAN_ONDIR
I can do without the FANOTIFY_EVENT_TYPES group.
something like this:

#define FANOTIFY_EVENT_IN_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
#define FANOTIFY_EVENT_OUT_FLAGS (FAN_Q_OVERFLOW | FAN_DIRENT_ISDIR)
#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
                                      FANOTIFY_PERM_EVENTS | \
                                      FANOTIFY_EVENT_OUT_FLAGS)

...
       test_mask = event_mask & marks_mask & ~marks_ignored_mask;

        if ((event_mask & FS_ISDIR) &&
           (test_mask & ALL_FSNOTIFY_DIRENT_EVENTS))
              test_mask |= FAN_DIRENT_ISDIR;

        return test_mask & FANOTIFY_OUTGOING_EVENTS;

Thanks,
Amir.

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Matthew Bobrowski]

On Fri, Jan 04, 2019 at 01:42:33PM +0200, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Sun 02-12-18 13:38:26, Amir Goldstein wrote:
> > > dirent modification events (create/delete/move) do not carry the
> > > child entry name/inode information. Instead, we report FAN_ONDIR
> > > for mkdir/rmdir so user can differentiate them from creat/unlink.
> > >
> > > 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.
> > >
> > > Unlike legacy fanotify events (open/access/close), dirent events
> > > for subdir entries (mkdir/rmdir) will be reported regardless if
> > > user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > be reported if the user asked for it.
> > >
> > > Cc: <linux-api@vger.kernel.org>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Some comments below.
> >
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index 89c19db4d45f..1aa23cefae5d 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -112,6 +112,7 @@ 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_EVENT_TYPES;
> > >       const struct path *path = data;
> > >       struct fsnotify_mark *mark;
> > >       int type;
> > > @@ -145,12 +146,38 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > >               marks_ignored_mask |= mark->ignored_mask;
> > >       }
> > >
> > > +     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > > +
> > > +     /*
> > > +      * dirent modification events (create/delete/move) do not carry the
> > > +      * child entry name/inode information. Instead, we report FAN_ONDIR
> > > +      * for mkdir/rmdir so user can differentiate them from creat/unlink.
> > > +      *
> > > +      * 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.
> > > +      */
> > > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> > > +             /* Do not report FAN_ONDIR without an event type */
> > > +             BUILD_BUG_ON(FANOTIFY_EVENT_TYPES & FANOTIFY_EVENT_FLAGS);
> > > +             if (!(test_mask & FANOTIFY_EVENT_TYPES))
> > > +                     return 0;
> > > +
> > > +             user_mask |= FAN_ONDIR;
> > > +     }
> > > +
> > > +     /*
> > > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > +      * be reported if the user asked for it.
> > > +      */
> > >       if (event_mask & FS_ISDIR &&
> > > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
> >
> > I disagree with this. It just seems inconsistent for dirent events for
> > directories to get reported without FAN_ONDIR. I understand there's not
> > great use for not reporting directory dirent events but it's not like
> > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> > makes the API more consistent. You could possibly remind the reader in the
> > manpage that FAN_ONDIR is required to get all dirent events.
> 
> I see your point.
> I have no problem with requiring FAN_ONDIR for mkdir events.
> I believe the strongest argument should be which way is easier
> to document/understand.
> 
> Matthew, if you agree that it looks easier to document Jan's proposal,
> please go a head with this and we will see how man page looks like
> before making the final decision.

To be fair, for the sake of clarity and consistency with the existing API I do
believe it would make it easier for the API consumer to comprehend what Jan has
suggested. Simple, in order to receive any events of type dirent, one must
supply FAN_ONDIR as part of their mark mask. 
 
> >
> > >           !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
> > >               return 0;
> > >
> > > -     return event_mask & FANOTIFY_OUTGOING_EVENTS & marks_mask &
> > > -             ~marks_ignored_mask;
> > > +     return test_mask & user_mask;
> >
> > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > I guess. E.g. when implementing recursive watching of a directory. Or what
> > is your intended usecase? It should be said explicitely in the changelog.
> 
> Recursive watch of directory tree is certainly a use case that could benefit
> from "mkdir" events. I will add that to commit message.
> 
> >
> > >  static int fanotify_encode_fid(struct fanotify_event *event,
> > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > index e9d45387089f..f5f86566c277 100644
> > > --- a/include/linux/fanotify.h
> > > +++ b/include/linux/fanotify.h
> > > @@ -61,13 +61,16 @@
> > >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > >                                FAN_OPEN_EXEC_PERM)
> > >
> > > +/* Events types that may be reported from vfs */
> > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > +                              FANOTIFY_PERM_EVENTS)
> > > +
> > >  /* Extra flags that may be reported with event or control handling of events */
> > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > >
> > >  /* Events that may be reported to user */
> > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > -                                      FANOTIFY_PERM_EVENTS | \
> > > -                                      FAN_Q_OVERFLOW)
> > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > >
> > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > >                                        FANOTIFY_EVENT_FLAGS)
> >
> > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > defines as is...
> >
> 
> Sorry. I don't understand what you mean.
> FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> Do you not like the new group definition FANOTIFY_EVENT_TYPES?
> 
> Please explain.

-- 
Matthew Bobrowski

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Matthew Bobrowski]

On Fri, Jan 04, 2019 at 02:39:06PM +0200, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> > > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > > > I guess. E.g. when implementing recursive watching of a directory. Or what
> > > > is your intended usecase? It should be said explicitely in the changelog.
> > >
> > > Recursive watch of directory tree is certainly a use case that could benefit
> > > from "mkdir" events. I will add that to commit message.
> >
> > Hum, so it does not seem like you had any particular usecase in mind? :)
> 
> Our application does have special handling (scanning) for mkdir events.
> 
> > Before complicating the interface with reporting FAN_ONDIR in some cases
> > I'd prefer we considered whether the usecases are worth it. So let me start
> > that: Reporting FAN_ONDIR can distinguish between file/directory
> > creation/deletion. That can save userspace some rescans of the changed
> > directory if it is interested only in subdirectory creation / deletion (if
> > the application is interested only in file changes it just does not set
> > FAN_ONDIR and that's it).
> >
> > Another motivation is the compatibility with inotify and it's IN_ISDIR flag
> > I guess.
> >
> > Hum, OK, I guess the complication is worth it.
> >
> 
> Let's see.
> 
> The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID
> is set, is something that needs to be clarified.
> 
> The fact that reported FID refers to the modified directory in dirent events
> but FAN_ONDIR flag refers to the created/removed/renamed child is
> another thing that needs to be clarified.
> 
> Sigh! "things that need to be clarified" are things we need to avoid.
> Therefore, I am going to make another suggestion.
> 
> How about if we introduced a new flag FAN_DIRENT_ISDIR?
> Like IN_ISDIR, it is an out-only flag that cannot be requested.
> As its name suggests, it is only applicable to dirent events, so will always
> be set when a sub directory entry is created/renamed/removed.
> 
> FAN_ONDIR has no effect on dirent events, because FAN_ONDIR
> refers to the "victim" inode and the "victim" is the modified directory.
> If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user
> wants to get notified when *directories* are modified and specifying
> FAN_ONDIR explicitly is not needed.
> 
> How about that?

I think this could work.

However to me, and if I'm understanding this correctly, it looks like we're
going down the route of adding "bonus" flags to particular events, which I
thought was something that we wanted to try and avoid in future versions of the
API? This would be so that users don't get any sudden surprises when processing
their events. It would also go against the notion of only receiving events that
were explicitly requested for by the calling process, which was something that
we discussed and implemented in a previous patch series?
 
> > > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > > > index e9d45387089f..f5f86566c277 100644
> > > > > --- a/include/linux/fanotify.h
> > > > > +++ b/include/linux/fanotify.h
> > > > > @@ -61,13 +61,16 @@
> > > > >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > > > >                                FAN_OPEN_EXEC_PERM)
> > > > >
> > > > > +/* Events types that may be reported from vfs */
> > > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > > > +                              FANOTIFY_PERM_EVENTS)
> > > > > +
> > > > >  /* Extra flags that may be reported with event or control handling of events */
> > > > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > > > >
> > > > >  /* Events that may be reported to user */
> > > > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > > > -                                      FANOTIFY_PERM_EVENTS | \
> > > > > -                                      FAN_Q_OVERFLOW)
> > > > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > > > >
> > > > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > > > >                                        FANOTIFY_EVENT_FLAGS)
> > > >
> > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > > > defines as is...
> > > >
> > >
> > > Sorry. I don't understand what you mean.
> > > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> > > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> > > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> > > Do you not like the new group definition FANOTIFY_EVENT_TYPES?
> >
> > Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported
> > to userspace. I don't like the FANOTIFY_EVENT_TYPES name and
> > FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR
> > there. So how about renaming FANOTIFY_OUTGOING_EVENTS to
> > FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your
> > FANOTIFY_EVENT_TYPES is?
> >
> 
> Still a bit confusing to me.
> I think that with FAN_DIRENT_ISDIR being a different flag than FAN_ONDIR
> I can do without the FANOTIFY_EVENT_TYPES group.
> something like this:
> 
> #define FANOTIFY_EVENT_IN_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> #define FANOTIFY_EVENT_OUT_FLAGS (FAN_Q_OVERFLOW | FAN_DIRENT_ISDIR)
> #define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
>                                       FANOTIFY_PERM_EVENTS | \
>                                       FANOTIFY_EVENT_OUT_FLAGS)
> 
> ...
>        test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> 
>         if ((event_mask & FS_ISDIR) &&
>            (test_mask & ALL_FSNOTIFY_DIRENT_EVENTS))
>               test_mask |= FAN_DIRENT_ISDIR;
> 
>         return test_mask & FANOTIFY_OUTGOING_EVENTS;

-- 
Matthew Bobrowski

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

...
> > > > +     /*
> > > > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > > > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > > > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > > +      * be reported if the user asked for it.
> > > > +      */
> > > >       if (event_mask & FS_ISDIR &&
> > > > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
> > >
> > > I disagree with this. It just seems inconsistent for dirent events for
> > > directories to get reported without FAN_ONDIR. I understand there's not
> > > great use for not reporting directory dirent events but it's not like
> > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> > > makes the API more consistent. You could possibly remind the reader in the
> > > manpage that FAN_ONDIR is required to get all dirent events.
> >
> > I see your point.
> > I have no problem with requiring FAN_ONDIR for mkdir events.
> > I believe the strongest argument should be which way is easier
> > to document/understand.
> >
> > Matthew, if you agree that it looks easier to document Jan's proposal,
> > please go a head with this and we will see how man page looks like
> > before making the final decision.
>
> To be fair, for the sake of clarity and consistency with the existing API I do
> believe it would make it easier for the API consumer to comprehend what Jan has
> suggested. Simple, in order to receive any events of type dirent, one must
> supply FAN_ONDIR as part of their mark mask.
>

But that was not the suggestion.

The debate is whether or not user needs to specify (for example)
 FAN_ONDIR|FAN_CREATE in order to get mkdir events.

The three of us understanding FAN_ONDIR intuitively different is what makes
me unease.

The purpose of my alternative suggestion was to dis-disambiguate which inode
each flag refers to.

It should be clear that FAN_DIRENT_ISDIR does not refer to the modified
directry but to the created/deleted/renamed subdir.
We will avoid making a change of behavior making FAN_ONDIR an out flag.

Thanks,
Amir.

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

On Sat, Jan 5, 2019 at 2:34 AM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> On Fri, Jan 04, 2019 at 02:39:06PM +0200, Amir Goldstein wrote:
> > On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> > > > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > > > > I guess. E.g. when implementing recursive watching of a directory. Or what
> > > > > is your intended usecase? It should be said explicitely in the changelog.
> > > >
> > > > Recursive watch of directory tree is certainly a use case that could benefit
> > > > from "mkdir" events. I will add that to commit message.
> > >
> > > Hum, so it does not seem like you had any particular usecase in mind? :)
> >
> > Our application does have special handling (scanning) for mkdir events.
> >
> > > Before complicating the interface with reporting FAN_ONDIR in some cases
> > > I'd prefer we considered whether the usecases are worth it. So let me start
> > > that: Reporting FAN_ONDIR can distinguish between file/directory
> > > creation/deletion. That can save userspace some rescans of the changed
> > > directory if it is interested only in subdirectory creation / deletion (if
> > > the application is interested only in file changes it just does not set
> > > FAN_ONDIR and that's it).
> > >
> > > Another motivation is the compatibility with inotify and it's IN_ISDIR flag
> > > I guess.
> > >
> > > Hum, OK, I guess the complication is worth it.
> > >
> >
> > Let's see.
> >
> > The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID
> > is set, is something that needs to be clarified.
> >
> > The fact that reported FID refers to the modified directory in dirent events
> > but FAN_ONDIR flag refers to the created/removed/renamed child is
> > another thing that needs to be clarified.
> >
> > Sigh! "things that need to be clarified" are things we need to avoid.
> > Therefore, I am going to make another suggestion.
> >
> > How about if we introduced a new flag FAN_DIRENT_ISDIR?
> > Like IN_ISDIR, it is an out-only flag that cannot be requested.
> > As its name suggests, it is only applicable to dirent events, so will always
> > be set when a sub directory entry is created/renamed/removed.
> >
> > FAN_ONDIR has no effect on dirent events, because FAN_ONDIR
> > refers to the "victim" inode and the "victim" is the modified directory.
> > If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user
> > wants to get notified when *directories* are modified and specifying
> > FAN_ONDIR explicitly is not needed.
> >
> > How about that?
>
> I think this could work.
>
> However to me, and if I'm understanding this correctly, it looks like we're
> going down the route of adding "bonus" flags to particular events, which I
> thought was something that we wanted to try and avoid in future versions of the
> API? This would be so that users don't get any sudden surprises when processing
> their events. It would also go against the notion of only receiving events that
> were explicitly requested for by the calling process, which was something that
> we discussed and implemented in a previous patch series?
>

This is a bit different case than FAN_OPEN_EXEC, because I wasn't suggesting
to let the user opt-in/out of mkdir events.
There is nothing wrong with "out event flags" as they do exist in inotify.

If we want to take FAN_OPEN_EXEC as an example, we could define
completely new set of events FAN_SUBDIR_{CREATE,DELETE,MOVE}
but IMO that is not needed and not consistent with what users are accustomed
for from inotify. We don't need to stick by inotify always, but if we don't
have a good reason to differentiate from it, we should avoid that IMO.

There is another option that does not sound very alluring to me, but I'll write
it anyway for your consideration:

-#define FAN_ONDIR              0x40000000      /* event occurred against dir */
+#define FAN_ISDIR              0x20000000      /* event occurred against dir */
+#define FAN_ONDIR              0x40000000      /* interested in
events against dir */

 #define FAN_EVENT_ON_CHILD     0x08000000      /* interested in child events */

FAN_ONDIR remains in-only flag and keeps the same meaning.
FAN_ISDIR is an in/out flag for all events (not just dirent events)
and preserves the same meaning as it has in inotify (refers to dirent
OR to victim
depending on the event type).

Technically, FAN_ISDIR implies FAN_ONDIR, but *also* carries the
information in out event, like in inotify.

Food for though...

Thanks,
Amir.

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Matthew Bobrowski]

On Sat, Jan 05, 2019 at 09:59:42AM +0200, Amir Goldstein wrote:
> ...
> > > > > +     /*
> > > > > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > > > > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > > > > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > > > +      * be reported if the user asked for it.
> > > > > +      */
> > > > >       if (event_mask & FS_ISDIR &&
> > > > > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
> > > >
> > > > I disagree with this. It just seems inconsistent for dirent events for
> > > > directories to get reported without FAN_ONDIR. I understand there's not
> > > > great use for not reporting directory dirent events but it's not like
> > > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> > > > makes the API more consistent. You could possibly remind the reader in the
> > > > manpage that FAN_ONDIR is required to get all dirent events.
> > >
> > > I see your point.
> > > I have no problem with requiring FAN_ONDIR for mkdir events.
> > > I believe the strongest argument should be which way is easier
> > > to document/understand.
> > >
> > > Matthew, if you agree that it looks easier to document Jan's proposal,
> > > please go a head with this and we will see how man page looks like
> > > before making the final decision.
> >
> > To be fair, for the sake of clarity and consistency with the existing API I do
> > believe it would make it easier for the API consumer to comprehend what Jan has
> > suggested. Simple, in order to receive any events of type dirent, one must
> > supply FAN_ONDIR as part of their mark mask.
> >
> 
> But that was not the suggestion.
> 
> The debate is whether or not user needs to specify (for example)
>  FAN_ONDIR|FAN_CREATE in order to get mkdir events.

And I'm agreeing with the fact that I think this ^ i.e. FAN_ONDIR | FAN_CREATE
is the way to go moving forward. However, there is still a small part of me
that thinks doing it this way seems a little weird and solely supplying
FAN_CREATE for example should be sufficient in order to get these type of
dirent events. I don't know why, but for whatever reason I have a feeling of
uncertainty about this.
 
> The three of us understanding FAN_ONDIR intuitively different is what makes
> me unease.
> 
> The purpose of my alternative suggestion was to dis-disambiguate which inode
> each flag refers to.
> 
> It should be clear that FAN_DIRENT_ISDIR does not refer to the modified
> directry but to the created/deleted/renamed subdir.
> We will avoid making a change of behavior making FAN_ONDIR an out flag.

Yeah, so maybe using FAN_DIRENT_ISDIR is indeed the solution. I don't really
have any objections with it at this particular point. Let's see whether Jan has
looked at it from a different perspective and can share his opinion.

-- 
Matthew Bobrowski

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

On Sat, Jan 5, 2019 at 11:49 AM Matthew Bobrowski
<mbobrowski@mbobrowski.org> wrote:
>
> On Sat, Jan 05, 2019 at 09:59:42AM +0200, Amir Goldstein wrote:
> > ...
> > > > > > +     /*
> > > > > > +      * Unlike legacy fanotify events (open/access/close), dirent events
> > > > > > +      * for subdir entries (mkdir/rmdir) will be reported regardless if
> > > > > > +      * user requested FAN_ONDIR, but the FAN_ONDIR flag itself will only
> > > > > > +      * be reported if the user asked for it.
> > > > > > +      */
> > > > > >       if (event_mask & FS_ISDIR &&
> > > > > > +         !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
> > > > >
> > > > > I disagree with this. It just seems inconsistent for dirent events for
> > > > > directories to get reported without FAN_ONDIR. I understand there's not
> > > > > great use for not reporting directory dirent events but it's not like
> > > > > adding FAN_ONDIR to the mark mask is that big deal for userspace. And it
> > > > > makes the API more consistent. You could possibly remind the reader in the
> > > > > manpage that FAN_ONDIR is required to get all dirent events.
> > > >
> > > > I see your point.
> > > > I have no problem with requiring FAN_ONDIR for mkdir events.
> > > > I believe the strongest argument should be which way is easier
> > > > to document/understand.
> > > >
> > > > Matthew, if you agree that it looks easier to document Jan's proposal,
> > > > please go a head with this and we will see how man page looks like
> > > > before making the final decision.
> > >
> > > To be fair, for the sake of clarity and consistency with the existing API I do
> > > believe it would make it easier for the API consumer to comprehend what Jan has
> > > suggested. Simple, in order to receive any events of type dirent, one must
> > > supply FAN_ONDIR as part of their mark mask.
> > >
> >
> > But that was not the suggestion.
> >
> > The debate is whether or not user needs to specify (for example)
> >  FAN_ONDIR|FAN_CREATE in order to get mkdir events.
>
> And I'm agreeing with the fact that I think this ^ i.e. FAN_ONDIR | FAN_CREATE
> is the way to go moving forward. However, there is still a small part of me
> that thinks doing it this way seems a little weird and solely supplying
> FAN_CREATE for example should be sufficient in order to get these type of
> dirent events. I don't know why, but for whatever reason I have a feeling of
> uncertainty about this.
>

I think that is sufficient consensus for requiring FAN_ONDIR to
get mkdir/rmdir events, so this is what I will do.
As Jan wrote, it's quite easy to document this weirdness and ease of
documentation is the important thing.

> > The three of us understanding FAN_ONDIR intuitively different is what makes
> > me unease.
> >
> > The purpose of my alternative suggestion was to dis-disambiguate which inode
> > each flag refers to.
> >
> > It should be clear that FAN_DIRENT_ISDIR does not refer to the modified
> > directry but to the created/deleted/renamed subdir.
> > We will avoid making a change of behavior making FAN_ONDIR an out flag.
>
> Yeah, so maybe using FAN_DIRENT_ISDIR is indeed the solution. I don't really
> have any objections with it at this particular point. Let's see whether Jan has
> looked at it from a different perspective and can share his opinion.
>

Nah. It's going to complicated rather than clarify IMO.
Just need to document that FAN_ONDIR is reported only with FAN_REPORT_FID.
If that is too weird, we can propose a new explicit init flag FAN_REPORT_ONDIR.
Do you guys thing that is necessary?

Thanks,
Amir.

Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Amir Goldstein]

> > > > +/* Events types that may be reported from vfs */
> > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > > +                              FANOTIFY_PERM_EVENTS)
> > > > +
> > > >  /* Extra flags that may be reported with event or control handling of events */
> > > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > > >
> > > >  /* Events that may be reported to user */
> > > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > > -                                      FANOTIFY_PERM_EVENTS | \
> > > > -                                      FAN_Q_OVERFLOW)
> > > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > > >
> > > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > > >                                        FANOTIFY_EVENT_FLAGS)
> > >
> > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > > defines as is...
> > >
> >
> > Sorry. I don't understand what you mean.
> > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> > Do you not like the new group definition FANOTIFY_EVENT_TYPES?
>
> Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported
> to userspace. I don't like the FANOTIFY_EVENT_TYPES name and
> FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR
> there. So how about renaming FANOTIFY_OUTGOING_EVENTS to
> FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your
> FANOTIFY_EVENT_TYPES is?
>

Jan,

I decided to avert this specific bikeshed and my updated version does
not use any
new defined.

Thanks,
Amir.

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi,
>
> On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > This is the 4th revision of patch series to add support for filesystem
> > change monitoring to fanotify.
> > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > patches.
> > The complete work is available on fanotify_dirent branch [1] on my tree.
> >
> > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > dirent modification events is demonstrated with a prototype of global
> > filesystem monitor based on inotify-tools [2].
> >
> > In your review of v3 patched you only got as far as patch v3 9/13.
> > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > I found it best to re-post the entire series with the changes you
> > requested thus far. For convenience of review, I pushed branches
> > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > reviewed so far and its re-worked version.
> >
> > One thing that we discussed and I did NOT do is move struct file_handle
> > to uapi headers. This got complicated due to existing definitions in
> > glibc header files and I realized we could do without it.
> >
> > I have added the vfs_get_fsid() helper as you requested, but since it
> > wasn't required by the patch set, I added it as two new cleanup patches
> > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > the feature with or without the VFS change.
>
> So overall the series looks very good. I've had only some smaller comments
> / disagreements. So once we settle those please resend the series and I'll
> pick it up to my tree.
>

Jan,

I addressed all your comments, rebased on v5.0-rc1 and pushed to:
https://github.com/amir73il/linux/commits/fanotify_dirent

When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
events (as you suggested) and FAN_ONDIR is reported as output flag
for FAN_REPORT_FID group for all events that occur on directories
and never reported to non FAN_REPORT_FID group.

Thanks,
Amir.

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Jan Kara]

On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > This is the 4th revision of patch series to add support for filesystem
> > > change monitoring to fanotify.
> > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > patches.
> > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > >
> > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > dirent modification events is demonstrated with a prototype of global
> > > filesystem monitor based on inotify-tools [2].
> > >
> > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > I found it best to re-post the entire series with the changes you
> > > requested thus far. For convenience of review, I pushed branches
> > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > reviewed so far and its re-worked version.
> > >
> > > One thing that we discussed and I did NOT do is move struct file_handle
> > > to uapi headers. This got complicated due to existing definitions in
> > > glibc header files and I realized we could do without it.
> > >
> > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > wasn't required by the patch set, I added it as two new cleanup patches
> > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > the feature with or without the VFS change.
> >
> > So overall the series looks very good. I've had only some smaller comments
> > / disagreements. So once we settle those please resend the series and I'll
> > pick it up to my tree.
> >
> 
> Jan,
> 
> I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> https://github.com/amir73il/linux/commits/fanotify_dirent
> 
> When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> events (as you suggested) and FAN_ONDIR is reported as output flag
> for FAN_REPORT_FID group for all events that occur on directories
> and never reported to non FAN_REPORT_FID group.

Agreed. But thinking about it a bit more and looking at your patch on
Github I think your solution doesn't quite work in presence of event
merging since an event with FAN_ONDIR can get merged with event without
FAN_ONDIR, can't it? That seems to be generally a problem with
should_merge() for direntry events that it doesn't take ISDIR flag into
account...

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

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > This is the 4th revision of patch series to add support for filesystem
> > > > change monitoring to fanotify.
> > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > patches.
> > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > >
> > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > dirent modification events is demonstrated with a prototype of global
> > > > filesystem monitor based on inotify-tools [2].
> > > >
> > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > I found it best to re-post the entire series with the changes you
> > > > requested thus far. For convenience of review, I pushed branches
> > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > reviewed so far and its re-worked version.
> > > >
> > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > to uapi headers. This got complicated due to existing definitions in
> > > > glibc header files and I realized we could do without it.
> > > >
> > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > the feature with or without the VFS change.
> > >
> > > So overall the series looks very good. I've had only some smaller comments
> > > / disagreements. So once we settle those please resend the series and I'll
> > > pick it up to my tree.
> > >
> >
> > Jan,
> >
> > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > https://github.com/amir73il/linux/commits/fanotify_dirent
> >
> > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > events (as you suggested) and FAN_ONDIR is reported as output flag
> > for FAN_REPORT_FID group for all events that occur on directories
> > and never reported to non FAN_REPORT_FID group.
>
> Agreed. But thinking about it a bit more and looking at your patch on
> Github I think your solution doesn't quite work in presence of event
> merging since an event with FAN_ONDIR can get merged with event without
> FAN_ONDIR, can't it? That seems to be generally a problem with
> should_merge() for direntry events that it doesn't take ISDIR flag into
> account...
>

Right... good catch! will fix should_merge().
Do you want me to re-post the series or just push to Github?

Thanks,
Amir.

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > > This is the 4th revision of patch series to add support for filesystem
> > > > > change monitoring to fanotify.
> > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > > patches.
> > > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > > >
> > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > > dirent modification events is demonstrated with a prototype of global
> > > > > filesystem monitor based on inotify-tools [2].
> > > > >
> > > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > > I found it best to re-post the entire series with the changes you
> > > > > requested thus far. For convenience of review, I pushed branches
> > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > > reviewed so far and its re-worked version.
> > > > >
> > > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > > to uapi headers. This got complicated due to existing definitions in
> > > > > glibc header files and I realized we could do without it.
> > > > >
> > > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > > the feature with or without the VFS change.
> > > >
> > > > So overall the series looks very good. I've had only some smaller comments
> > > > / disagreements. So once we settle those please resend the series and I'll
> > > > pick it up to my tree.
> > > >
> > >
> > > Jan,
> > >
> > > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > > https://github.com/amir73il/linux/commits/fanotify_dirent
> > >
> > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > > events (as you suggested) and FAN_ONDIR is reported as output flag
> > > for FAN_REPORT_FID group for all events that occur on directories
> > > and never reported to non FAN_REPORT_FID group.
> >
> > Agreed. But thinking about it a bit more and looking at your patch on
> > Github I think your solution doesn't quite work in presence of event
> > merging since an event with FAN_ONDIR can get merged with event without
> > FAN_ONDIR, can't it? That seems to be generally a problem with
> > should_merge() for direntry events that it doesn't take ISDIR flag into
> > account...
> >
>
> Right... good catch! will fix should_merge().
> Do you want me to re-post the series or just push to Github?
>

In the mean while, I pushed the fix to branch fanotify_dirent.
A branch with the same name in LTP tree has a WIP dirent events
test that includes the test for expected merge behavior.
Matthew intends to work this test into shape.

I did notice another wrinkle.
There is a group of events that this patch set adds support to
that are not dirent events, namely: DELETE_SELF, MOVE_SELF
and ATTRIB.

The first two never carry the ISDIR flag (in inotify as well).
ATTRIB does usually carry the ISDIR flag, except for the odd case
of fsnotify_link_count(target) being called on rename over an empty
directory.

If we add FS_ISDIR in those hooks, we risk breaking inotify apps.
If we don't do anything, we will get inconsistent behavior w.r.t
FAN_ONDIR (especially relevant for XXX_SELF).

The only sane solution I can think of is something along:

static u32 fanotify_group_event_mask(...
...
        if (data_type == FSNOTIFY_EVENT_INODE &&
            !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
            S_ISDIR(((struct inode *)data)->i_mode)
            event_mask |= FS_ISDIR;

Thoughts?

Thanks,
Amir.

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Jan Kara]

On Wed 09-01-19 17:34:36, Amir Goldstein wrote:
> On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > > This is the 4th revision of patch series to add support for filesystem
> > > > > change monitoring to fanotify.
> > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > > patches.
> > > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > > >
> > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > > dirent modification events is demonstrated with a prototype of global
> > > > > filesystem monitor based on inotify-tools [2].
> > > > >
> > > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > > I found it best to re-post the entire series with the changes you
> > > > > requested thus far. For convenience of review, I pushed branches
> > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > > reviewed so far and its re-worked version.
> > > > >
> > > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > > to uapi headers. This got complicated due to existing definitions in
> > > > > glibc header files and I realized we could do without it.
> > > > >
> > > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > > the feature with or without the VFS change.
> > > >
> > > > So overall the series looks very good. I've had only some smaller comments
> > > > / disagreements. So once we settle those please resend the series and I'll
> > > > pick it up to my tree.
> > > >
> > >
> > > Jan,
> > >
> > > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > > https://github.com/amir73il/linux/commits/fanotify_dirent
> > >
> > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > > events (as you suggested) and FAN_ONDIR is reported as output flag
> > > for FAN_REPORT_FID group for all events that occur on directories
> > > and never reported to non FAN_REPORT_FID group.
> >
> > Agreed. But thinking about it a bit more and looking at your patch on
> > Github I think your solution doesn't quite work in presence of event
> > merging since an event with FAN_ONDIR can get merged with event without
> > FAN_ONDIR, can't it? That seems to be generally a problem with
> > should_merge() for direntry events that it doesn't take ISDIR flag into
> > account...
> >
> 
> Right... good catch! will fix should_merge().
> Do you want me to re-post the series or just push to Github?

Please post the series now since there were quite some changes (although
smaller ones) since the last posting. I'll go through it and push it to my
tree. Do we already have LTP tests for the new functionality? I'd like to
do some testing before pushing things out...

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

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Jan Kara]

On Thu 10-01-19 09:49:24, Amir Goldstein wrote:
> On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > > > This is the 4th revision of patch series to add support for filesystem
> > > > > > change monitoring to fanotify.
> > > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > > > patches.
> > > > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > > > >
> > > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > > > dirent modification events is demonstrated with a prototype of global
> > > > > > filesystem monitor based on inotify-tools [2].
> > > > > >
> > > > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > > > I found it best to re-post the entire series with the changes you
> > > > > > requested thus far. For convenience of review, I pushed branches
> > > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > > > reviewed so far and its re-worked version.
> > > > > >
> > > > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > > > to uapi headers. This got complicated due to existing definitions in
> > > > > > glibc header files and I realized we could do without it.
> > > > > >
> > > > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > > > the feature with or without the VFS change.
> > > > >
> > > > > So overall the series looks very good. I've had only some smaller comments
> > > > > / disagreements. So once we settle those please resend the series and I'll
> > > > > pick it up to my tree.
> > > > >
> > > >
> > > > Jan,
> > > >
> > > > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > > > https://github.com/amir73il/linux/commits/fanotify_dirent
> > > >
> > > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > > > events (as you suggested) and FAN_ONDIR is reported as output flag
> > > > for FAN_REPORT_FID group for all events that occur on directories
> > > > and never reported to non FAN_REPORT_FID group.
> > >
> > > Agreed. But thinking about it a bit more and looking at your patch on
> > > Github I think your solution doesn't quite work in presence of event
> > > merging since an event with FAN_ONDIR can get merged with event without
> > > FAN_ONDIR, can't it? That seems to be generally a problem with
> > > should_merge() for direntry events that it doesn't take ISDIR flag into
> > > account...
> > >
> >
> > Right... good catch! will fix should_merge().
> > Do you want me to re-post the series or just push to Github?
> >
> 
> In the mean while, I pushed the fix to branch fanotify_dirent.
> A branch with the same name in LTP tree has a WIP dirent events
> test that includes the test for expected merge behavior.
> Matthew intends to work this test into shape.

OK, thanks!

> I did notice another wrinkle.
> There is a group of events that this patch set adds support to
> that are not dirent events, namely: DELETE_SELF, MOVE_SELF
> and ATTRIB.
> 
> The first two never carry the ISDIR flag (in inotify as well).
> ATTRIB does usually carry the ISDIR flag, except for the odd case
> of fsnotify_link_count(target) being called on rename over an empty
> directory.
> 
> If we add FS_ISDIR in those hooks, we risk breaking inotify apps.
> If we don't do anything, we will get inconsistent behavior w.r.t
> FAN_ONDIR (especially relevant for XXX_SELF).

I see. So how about adding ISDIR flag properly to all events and just mask
it out in inotify_handle_event() for bug-to-bug compatibility? I think we
can start sending ISDIR flag properly for fsnotify_link_count() from because
that is unlikely to surprise anybody given other ATTRIB events can have it.
Because the fact that ISDIR flag is missing looks like a bug to me.

Generally the ISDIR flag handling might deserve a larger cleanup but I
don't want to mix it into your series as that's big enough as is.

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

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

On Thu, Jan 10, 2019 at 11:22 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 10-01-19 09:49:24, Amir Goldstein wrote:
> > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > > > > This is the 4th revision of patch series to add support for filesystem
> > > > > > > change monitoring to fanotify.
> > > > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > > > > patches.
> > > > > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > > > > >
> > > > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > > > > dirent modification events is demonstrated with a prototype of global
> > > > > > > filesystem monitor based on inotify-tools [2].
> > > > > > >
> > > > > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > > > > I found it best to re-post the entire series with the changes you
> > > > > > > requested thus far. For convenience of review, I pushed branches
> > > > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > > > > reviewed so far and its re-worked version.
> > > > > > >
> > > > > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > > > > to uapi headers. This got complicated due to existing definitions in
> > > > > > > glibc header files and I realized we could do without it.
> > > > > > >
> > > > > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > > > > the feature with or without the VFS change.
> > > > > >
> > > > > > So overall the series looks very good. I've had only some smaller comments
> > > > > > / disagreements. So once we settle those please resend the series and I'll
> > > > > > pick it up to my tree.
> > > > > >
> > > > >
> > > > > Jan,
> > > > >
> > > > > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > > > > https://github.com/amir73il/linux/commits/fanotify_dirent
> > > > >
> > > > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > > > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > > > > events (as you suggested) and FAN_ONDIR is reported as output flag
> > > > > for FAN_REPORT_FID group for all events that occur on directories
> > > > > and never reported to non FAN_REPORT_FID group.
> > > >
> > > > Agreed. But thinking about it a bit more and looking at your patch on
> > > > Github I think your solution doesn't quite work in presence of event
> > > > merging since an event with FAN_ONDIR can get merged with event without
> > > > FAN_ONDIR, can't it? That seems to be generally a problem with
> > > > should_merge() for direntry events that it doesn't take ISDIR flag into
> > > > account...
> > > >
> > >
> > > Right... good catch! will fix should_merge().
> > > Do you want me to re-post the series or just push to Github?
> > >
> >
> > In the mean while, I pushed the fix to branch fanotify_dirent.
> > A branch with the same name in LTP tree has a WIP dirent events
> > test that includes the test for expected merge behavior.
> > Matthew intends to work this test into shape.
>
> OK, thanks!
>
> > I did notice another wrinkle.
> > There is a group of events that this patch set adds support to
> > that are not dirent events, namely: DELETE_SELF, MOVE_SELF
> > and ATTRIB.
> >
> > The first two never carry the ISDIR flag (in inotify as well).
> > ATTRIB does usually carry the ISDIR flag, except for the odd case
> > of fsnotify_link_count(target) being called on rename over an empty
> > directory.
> >
> > If we add FS_ISDIR in those hooks, we risk breaking inotify apps.
> > If we don't do anything, we will get inconsistent behavior w.r.t
> > FAN_ONDIR (especially relevant for XXX_SELF).
>
> I see. So how about adding ISDIR flag properly to all events and just mask
> it out in inotify_handle_event() for bug-to-bug compatibility? I think we
> can start sending ISDIR flag properly for fsnotify_link_count() from because
> that is unlikely to surprise anybody given other ATTRIB events can have it.
> Because the fact that ISDIR flag is missing looks like a bug to me.
>

Agreed. I will work this into the series and post v5.

TBH, I think that the ATTRIB event just before renaming over an empty
dir is grossly uninteresting. Not to mention that there is no such event
on rmdir() nor on unlink(), even if nlink is decremented to non zero.
So I would rather if we just removed the fsnotify_link_count(target)
call altogether or add the missing calls, but I won't do that now.

Thanks,
Amir.

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

> Please post the series now since there were quite some changes (although
> smaller ones) since the last posting. I'll go through it and push it to my
> tree. Do we already have LTP tests for the new functionality? I'd like to
> do some testing before pushing things out...

Yes there are LTP tests you can use.

I'll use this opportunity to dump the content of my LTP dev branch
and recap the WIP (mainly by Matthew) on fanotify LTP tests:

* 9eb01c6b4 - (HEAD -> fanotify_dirent, github/fanotify_dirent)
syscalls/fanotify102: new test for dirent events
* 2218e949c - (github/fanotify_fid, fanotify_fid) syscalls/fanotify14:
new test to validate FAN_REPORT_FID interface return values
* b7ad5c4fe - syscalls/fanotify13: new test to verify FAN_REPORT_FID
functionality
* 10abf45c1 - syscalls/fanotify01: add FAN_REPORT_FID test cases
* 9185a0aa1 - (github/fanotify_exec, fanotify_exec)
syscalls/fanotify10: increase test coverage to support FAN_OPEN_EXEC
mask
* cd7ab20d5 - syscalls/fanotify12: add new test to support new
FAN_OPEN_EXEC event mask
* 56ba122c0 - syscalls/fanotify03: add FAN_OPEN_EXEC_PERM tcase support
* f596b4a5d - syscalls/fanotify03: included execve() to
generate_events() to increase test coverage
* f23d990ad - syscalls/fanotify03: defined additional tcase members to
support more tcase control
* 7846bbbed - syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM
permission events

fanotify_exec branch has Matthew's tests for FAN_OPEN_EXEC.
Following request from LTP maintainer, he will post them around v5.0-rc3 time.

fanotify_fid branch has 2 new FAN_REPORT_FID tests by Matthew,
which verify the functionality of reporting fid.

fanotify_dirent branch has 1 WIP test to verify new events functionality.
its temporary name fanotify102 goes after the test inotify02 which verifies
similar functionality with inotify.

Matthew is going to work this test into shape and add verification that
the reported fid in dirent events (and _SELF events) is correct.

Thanks,
Amir.

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Jan Kara]

On Thu 10-01-19 11:50:30, Amir Goldstein wrote:
> On Thu, Jan 10, 2019 at 11:22 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 10-01-19 09:49:24, Amir Goldstein wrote:
> > > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > > > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > > > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > > > > > This is the 4th revision of patch series to add support for filesystem
> > > > > > > > change monitoring to fanotify.
> > > > > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > > > > > patches.
> > > > > > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > > > > > >
> > > > > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > > > > > dirent modification events is demonstrated with a prototype of global
> > > > > > > > filesystem monitor based on inotify-tools [2].
> > > > > > > >
> > > > > > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > > > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > > > > > I found it best to re-post the entire series with the changes you
> > > > > > > > requested thus far. For convenience of review, I pushed branches
> > > > > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > > > > > reviewed so far and its re-worked version.
> > > > > > > >
> > > > > > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > > > > > to uapi headers. This got complicated due to existing definitions in
> > > > > > > > glibc header files and I realized we could do without it.
> > > > > > > >
> > > > > > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > > > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > > > > > the feature with or without the VFS change.
> > > > > > >
> > > > > > > So overall the series looks very good. I've had only some smaller comments
> > > > > > > / disagreements. So once we settle those please resend the series and I'll
> > > > > > > pick it up to my tree.
> > > > > > >
> > > > > >
> > > > > > Jan,
> > > > > >
> > > > > > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > > > > > https://github.com/amir73il/linux/commits/fanotify_dirent
> > > > > >
> > > > > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > > > > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > > > > > events (as you suggested) and FAN_ONDIR is reported as output flag
> > > > > > for FAN_REPORT_FID group for all events that occur on directories
> > > > > > and never reported to non FAN_REPORT_FID group.
> > > > >
> > > > > Agreed. But thinking about it a bit more and looking at your patch on
> > > > > Github I think your solution doesn't quite work in presence of event
> > > > > merging since an event with FAN_ONDIR can get merged with event without
> > > > > FAN_ONDIR, can't it? That seems to be generally a problem with
> > > > > should_merge() for direntry events that it doesn't take ISDIR flag into
> > > > > account...
> > > > >
> > > >
> > > > Right... good catch! will fix should_merge().
> > > > Do you want me to re-post the series or just push to Github?
> > > >
> > >
> > > In the mean while, I pushed the fix to branch fanotify_dirent.
> > > A branch with the same name in LTP tree has a WIP dirent events
> > > test that includes the test for expected merge behavior.
> > > Matthew intends to work this test into shape.
> >
> > OK, thanks!
> >
> > > I did notice another wrinkle.
> > > There is a group of events that this patch set adds support to
> > > that are not dirent events, namely: DELETE_SELF, MOVE_SELF
> > > and ATTRIB.
> > >
> > > The first two never carry the ISDIR flag (in inotify as well).
> > > ATTRIB does usually carry the ISDIR flag, except for the odd case
> > > of fsnotify_link_count(target) being called on rename over an empty
> > > directory.
> > >
> > > If we add FS_ISDIR in those hooks, we risk breaking inotify apps.
> > > If we don't do anything, we will get inconsistent behavior w.r.t
> > > FAN_ONDIR (especially relevant for XXX_SELF).
> >
> > I see. So how about adding ISDIR flag properly to all events and just mask
> > it out in inotify_handle_event() for bug-to-bug compatibility? I think we
> > can start sending ISDIR flag properly for fsnotify_link_count() from because
> > that is unlikely to surprise anybody given other ATTRIB events can have it.
> > Because the fact that ISDIR flag is missing looks like a bug to me.
> >
> 
> Agreed. I will work this into the series and post v5.
> 
> TBH, I think that the ATTRIB event just before renaming over an empty
> dir is grossly uninteresting. Not to mention that there is no such event
> on rmdir() nor on unlink(), even if nlink is decremented to non zero.

But you won't get other event telling you that some dir entry got actually
overwritten by the rename, will you? Note that the inode need not get
deleted if it has other links to it. Actually sending fsnotify_nameremove()
would look like a more sensible choice but that ship has already sailed...

> So I would rather if we just removed the fsnotify_link_count(target)
> call altogether or add the missing calls, but I won't do that now.

I would not remove existing calls - possibility of breaking compatibility
just because we don't find the calls very useful simply is not worth it.
Adding new calls is possible but let's leave that for later once things
settle down a bit.

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

Re: [PATCH v4 00/15] fanotify: add support for more event types

[Amir Goldstein]

> > TBH, I think that the ATTRIB event just before renaming over an empty
> > dir is grossly uninteresting. Not to mention that there is no such event
> > on rmdir() nor on unlink(), even if nlink is decremented to non zero.
>
> But you won't get other event telling you that some dir entry got actually
> overwritten by the rename, will you? Note that the inode need not get
> deleted if it has other links to it. Actually sending fsnotify_nameremove()
> would look like a more sensible choice but that ship has already sailed...
>

You get a MOVED_TO event on the modified dir.
So long as we do not report dirent filenames, that is just as informative as
getting ATTRIB event on the modified dir (MOVED_TO has greater chance
to imply overwrite than ATTRIB does).

The inode of an overwritten empty directory will produce DELETE_SELF
event, because empty directory cannot have links to it.

I am not saying ATTRIB event for decremented nlink on non dir doesn't
make sense. I am saying ATTRIB|ISDIR event in that case doesn't make
a whole lot of sense. Anyway I am not changing this now.

Thanks,
Amir.