[PATCH 0/7] Report more information in fanotify dirent events

[PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

Jan,

This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
from 3 months ago.

With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
I figured we could get an early (re)start of the discussion on
FAN_REPORT_TARGET_FID towards 5.17.

The added information in dirent events solves problems for my use case -
It helps getting the following information in a race free manner:
1. fid of a created directory on mkdir
2. from/to path information on rename of non-dir

I realize those are two different API traits, but they are close enough
so I preferred not to clutter the REPORT flags space any further than it
already is. The single added flag FAN_REPORT_TARGET_FID adds:
1. child fid info to CREATE/DELETE/MOVED_* events
2. new parent+name info to MOVED_FROM event

Instead of going the "inotify way" and trying to join the MOVED_FROM/
MOVED_TO events using a cookie, I chose to incorporate the new
parent+name intomation only in the MOVED_FROM event.
I made this choice for several reasons:
1. Availability of the moved dentry in the hook and event data
2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
   DFID_NAME info to statat(2) the object as we suggested

I chose to reduce testing complexity and require all other FID
flags with FAN_REPORT_TARGET_FID and there is a convenience
macro FAN_REPORT_ALL_FIDS that application can use.
This restriction could be relaxed in the future if we have a good reason
to do so.

Since the POC branch 3 months ago, I dropped the 'sub_type' field of
the info header, because it did not add much value IMO.

Patches [2] and LTP test [3] are available on my github.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjYDDk00VPdWtRB1_tf+gCoPFgSQ9O0p0fGaW_JiFUUKA@mail.gmail.com/
[2] https://github.com/amir73il/linux/commits/fanotify_target_fid
[3] https://github.com/amir73il/ltp/commits/fanotify_target_fid

Amir Goldstein (7):
  fsnotify: pass dentry instead of inode data for move events
  fanotify: introduce group flag FAN_REPORT_TARGET_FID
  fanotify: use macros to get the offset to fanotify_info buffer
  fanotify: support secondary dir fh and name in fanotify_info
  fanotify: record new parent and name in MOVED_FROM event
  fanotify: report new parent and name in MOVED_FROM event
  fanotify: enable the FAN_REPORT_TARGET_FID flag

 fs/notify/fanotify/fanotify.c      | 108 ++++++++++++++++++++++-----
 fs/notify/fanotify/fanotify.h      | 113 +++++++++++++++++++++++++----
 fs/notify/fanotify/fanotify_user.c |  43 +++++++++--
 include/linux/fanotify.h           |   2 +-
 include/linux/fsnotify.h           |   7 +-
 include/uapi/linux/fanotify.h      |   5 ++
 6 files changed, 235 insertions(+), 43 deletions(-)

-- 
2.33.1

[PATCH 1/7] fsnotify: pass dentry instead of inode data for move events

[Amir Goldstein]

This is needed for reporting the new parent/name with MOVED_FROM
events.

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

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 787545e87eeb..ae7501c80d05 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -140,7 +140,6 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 				 int isdir, struct inode *target,
 				 struct dentry *moved)
 {
-	struct inode *source = moved->d_inode;
 	u32 fs_cookie = fsnotify_get_cookie();
 	__u32 old_dir_mask = FS_MOVED_FROM;
 	__u32 new_dir_mask = FS_MOVED_TO;
@@ -154,14 +153,14 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
 		new_dir_mask |= FS_ISDIR;
 	}
 
-	fsnotify_name(old_dir_mask, source, FSNOTIFY_EVENT_INODE,
+	fsnotify_name(old_dir_mask, moved, FSNOTIFY_EVENT_DENTRY,
 		      old_dir, old_name, fs_cookie);
-	fsnotify_name(new_dir_mask, source, FSNOTIFY_EVENT_INODE,
+	fsnotify_name(new_dir_mask, moved, FSNOTIFY_EVENT_DENTRY,
 		      new_dir, new_name, fs_cookie);
 
 	if (target)
 		fsnotify_link_count(target);
-	fsnotify_inode(source, FS_MOVE_SELF);
+	fsnotify_inode(d_inode(moved), FS_MOVE_SELF);
 	audit_inode_child(new_dir, moved, AUDIT_TYPE_CHILD_CREATE);
 }
 
-- 
2.33.1

[PATCH 2/7] fanotify: introduce group flag FAN_REPORT_TARGET_FID

[Amir Goldstein]

FAN_REPORT_FID is ambiguous in that it reports the fid of the child for
some events and the fid of the parent for create/delete/move events.

The new FAN_REPORT_TARGET_FID flag is an implicit request to report
the fid of the target object of the operation (a.k.a the child inode)
also in create/delete/move events in addition to the fid of the parent
and the name of the child.

To reduce the test matrix for uninteresting use cases, the new
FAN_REPORT_TARGET_FID flag requires both FAN_REPORT_NAME and
FAN_REPORT_FID.  The convenience macro FAN_REPORT_ALL_FIDS combines
FAN_REPORT_TARGET_FID with all the required flags.

The new flag is not yet enabled in this change.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index b6091775aa6e..9b1641ecfe97 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -458,17 +458,41 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 }
 
 /*
- * The inode to use as identifier when reporting fid depends on the event.
- * Report the modified directory inode on dirent modification events.
- * Report the "victim" inode otherwise.
+ * FAN_REPORT_FID is ambiguous in that it reports the fid of the child for
+ * some events and the fid of the parent for create/delete/move events.
+ *
+ * With the FAN_REPORT_TARGET_FID flag, the fid of the child is reported
+ * also in create/delete/move events in addition to the fid of the parent
+ * and the name of the child.
+ */
+static inline bool fanotify_report_child_fid(unsigned int fid_mode, u32 mask)
+{
+	if (mask & ALL_FSNOTIFY_DIRENT_EVENTS)
+		return (fid_mode & FAN_REPORT_TARGET_FID);
+
+	return (fid_mode & FAN_REPORT_FID) && !(mask & FAN_ONDIR);
+}
+
+/*
+ * The inode to use as identifier when reporting fid depends on the event
+ * and the group flags.
+ *
+ * With the group flag FAN_REPORT_TARGET_FID, always report the child fid.
+ *
+ * Without the group flag FAN_REPORT_TARGET_FID, report the modified directory
+ * fid on dirent events and the child fid 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.
+ * FS_ATTRIB reports the child fid even if reported on a watched parent.
+ * FS_CREATE reports the modified dir fid without FAN_REPORT_TARGET_FID.
+ *       and reports the created child fid with FAN_REPORT_TARGET_FID.
  */
 static struct inode *fanotify_fid_inode(u32 event_mask, const void *data,
-					int data_type, struct inode *dir)
+					int data_type, struct inode *dir,
+					unsigned int fid_mode)
 {
-	if (event_mask & ALL_FSNOTIFY_DIRENT_EVENTS)
+	if ((event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
+	    !(fid_mode & FAN_REPORT_TARGET_FID))
 		return dir;
 
 	return fsnotify_data_inode(data, data_type);
@@ -647,10 +671,11 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 {
 	struct fanotify_event *event = NULL;
 	gfp_t gfp = GFP_KERNEL_ACCOUNT;
-	struct inode *id = fanotify_fid_inode(mask, data, data_type, dir);
+	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
+	struct inode *id = fanotify_fid_inode(mask, data, data_type, dir,
+					      fid_mode);
 	struct inode *dirid = fanotify_dfid_inode(mask, data, data_type, dir);
 	const struct path *path = fsnotify_data_path(data, data_type);
-	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct mem_cgroup *old_memcg;
 	struct inode *child = NULL;
 	bool name_event = false;
@@ -660,11 +685,10 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 
 	if ((fid_mode & FAN_REPORT_DIR_FID) && dirid) {
 		/*
-		 * With both flags FAN_REPORT_DIR_FID and FAN_REPORT_FID, we
-		 * report the child fid for events reported on a non-dir child
+		 * For certain events and group flags, report the child fid
 		 * in addition to reporting the parent fid and maybe child name.
 		 */
-		if ((fid_mode & FAN_REPORT_FID) && id != dirid && !ondir)
+		if (fanotify_report_child_fid(fid_mode, mask) && id != dirid)
 			child = id;
 
 		id = dirid;
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 559bc1e9926d..6ec26b124041 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1275,6 +1275,15 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	if ((fid_mode & FAN_REPORT_NAME) && !(fid_mode & FAN_REPORT_DIR_FID))
 		return -EINVAL;
 
+	/*
+	 * FAN_REPORT_TARGET_FID requires FAN_REPORT_NAME and FAN_REPORT_FID
+	 * and is used as an indication to report both dir and child fid on all
+	 * dirent events.
+	 */
+	if ((fid_mode & FAN_REPORT_TARGET_FID) &&
+	    (!(fid_mode & FAN_REPORT_NAME) || !(fid_mode & FAN_REPORT_FID)))
+		return -EINVAL;
+
 	f_flags = O_RDWR | FMODE_NONOTIFY;
 	if (flags & FAN_CLOEXEC)
 		f_flags |= O_CLOEXEC;
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index bd1932c2074d..f9202ce31b0d 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -57,9 +57,13 @@
 #define FAN_REPORT_FID		0x00000200	/* Report unique file id */
 #define FAN_REPORT_DIR_FID	0x00000400	/* Report unique directory id */
 #define FAN_REPORT_NAME		0x00000800	/* Report events with name */
+#define FAN_REPORT_TARGET_FID	0x00001000	/* Report dirent target id  */
 
 /* Convenience macro - FAN_REPORT_NAME requires FAN_REPORT_DIR_FID */
 #define FAN_REPORT_DFID_NAME	(FAN_REPORT_DIR_FID | FAN_REPORT_NAME)
+/* Convenience macro - FAN_REPORT_TARGET_FID requires all other FID flags */
+#define FAN_REPORT_ALL_FIDS	(FAN_REPORT_DFID_NAME | FAN_REPORT_FID | \
+				 FAN_REPORT_TARGET_FID)
 
 /* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
-- 
2.33.1

[PATCH 3/7] fanotify: use macros to get the offset to fanotify_info buffer

[Amir Goldstein]

The fanotify_info buffer contains up to two file handles and a name.
Use macros to simplify the code that access the different items within
the buffer.

Add assertions to verify that stored fh len and name len do not overflow
the u8 stored value in fanotify_info header.

Remove the unused fanotify_info_len() helper.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 9b1641ecfe97..4a812411ae5b 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -411,7 +411,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	 * be zero in that case if encoding fh len failed.
 	 */
 	err = -ENOENT;
-	if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4))
+	if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4) || fh_len > MAX_HANDLE_SZ)
 		goto out_err;
 
 	/* No external buffer in a variable size allocated fh */
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index d25f500bf7e7..dd23ba659e76 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -49,6 +49,22 @@ struct fanotify_info {
 	 * (optional) file_fh starts at buf[dir_fh_totlen]
 	 * name starts at buf[dir_fh_totlen + file_fh_totlen]
 	 */
+#define FANOTIFY_DIR_FH_SIZE(info)	((info)->dir_fh_totlen)
+#define FANOTIFY_FILE_FH_SIZE(info)	((info)->file_fh_totlen)
+#define FANOTIFY_NAME_SIZE(info)	((info)->name_len + 1)
+
+#define FANOTIFY_DIR_FH_OFFSET(info)	0
+#define FANOTIFY_FILE_FH_OFFSET(info) \
+	(FANOTIFY_DIR_FH_OFFSET(info) + FANOTIFY_DIR_FH_SIZE(info))
+#define FANOTIFY_NAME_OFFSET(info) \
+	(FANOTIFY_FILE_FH_OFFSET(info) + FANOTIFY_FILE_FH_SIZE(info))
+
+#define FANOTIFY_DIR_FH_BUF(info) \
+	((info)->buf + FANOTIFY_DIR_FH_OFFSET(info))
+#define FANOTIFY_FILE_FH_BUF(info) \
+	((info)->buf + FANOTIFY_FILE_FH_OFFSET(info))
+#define FANOTIFY_NAME_BUF(info) \
+	((info)->buf + FANOTIFY_NAME_OFFSET(info))
 } __aligned(4);
 
 static inline bool fanotify_fh_has_ext_buf(struct fanotify_fh *fh)
@@ -87,7 +103,7 @@ static inline struct fanotify_fh *fanotify_info_dir_fh(struct fanotify_info *inf
 {
 	BUILD_BUG_ON(offsetof(struct fanotify_info, buf) % 4);
 
-	return (struct fanotify_fh *)info->buf;
+	return (struct fanotify_fh *)FANOTIFY_DIR_FH_BUF(info);
 }
 
 static inline int fanotify_info_file_fh_len(struct fanotify_info *info)
@@ -101,32 +117,35 @@ static inline int fanotify_info_file_fh_len(struct fanotify_info *info)
 
 static inline struct fanotify_fh *fanotify_info_file_fh(struct fanotify_info *info)
 {
-	return (struct fanotify_fh *)(info->buf + info->dir_fh_totlen);
+	return (struct fanotify_fh *)FANOTIFY_FILE_FH_BUF(info);
 }
 
-static inline const char *fanotify_info_name(struct fanotify_info *info)
+static inline char *fanotify_info_name(struct fanotify_info *info)
 {
-	return info->buf + info->dir_fh_totlen + info->file_fh_totlen;
+	if (!info->name_len)
+		return NULL;
+
+	return FANOTIFY_NAME_BUF(info);
 }
 
 static inline void fanotify_info_init(struct fanotify_info *info)
 {
+	BUILD_BUG_ON(FANOTIFY_FH_HDR_LEN + MAX_HANDLE_SZ > U8_MAX);
+	BUILD_BUG_ON(NAME_MAX > U8_MAX);
+
 	info->dir_fh_totlen = 0;
 	info->file_fh_totlen = 0;
 	info->name_len = 0;
 }
 
-static inline unsigned int fanotify_info_len(struct fanotify_info *info)
-{
-	return info->dir_fh_totlen + info->file_fh_totlen + info->name_len;
-}
-
 static inline void fanotify_info_copy_name(struct fanotify_info *info,
 					   const struct qstr *name)
 {
+	if (WARN_ON_ONCE(name->len > NAME_MAX))
+		return;
+
 	info->name_len = name->len;
-	strcpy(info->buf + info->dir_fh_totlen + info->file_fh_totlen,
-	       name->name);
+	strcpy(fanotify_info_name(info), name->name);
 }
 
 /*
-- 
2.33.1

[PATCH 4/7] fanotify: support secondary dir fh and name in fanotify_info

[Amir Goldstein]

Allow storing a secondary dir fh and name tupple in fanotify_info.
The secondary dir fh and name can only be stored after storing the
primary dir fh and name.

This will be used to store the new parent and name information in
MOVED_FROM event.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 4a812411ae5b..795bedcb6f9b 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -76,8 +76,10 @@ static bool fanotify_info_equal(struct fanotify_info *info1,
 				struct fanotify_info *info2)
 {
 	if (info1->dir_fh_totlen != info2->dir_fh_totlen ||
+	    info1->dir2_fh_totlen != info2->dir2_fh_totlen ||
 	    info1->file_fh_totlen != info2->file_fh_totlen ||
-	    info1->name_len != info2->name_len)
+	    info1->name_len != info2->name_len ||
+	    info1->name2_len != info2->name2_len)
 		return false;
 
 	if (info1->dir_fh_totlen &&
@@ -85,14 +87,24 @@ static bool fanotify_info_equal(struct fanotify_info *info1,
 			       fanotify_info_dir_fh(info2)))
 		return false;
 
+	if (info1->dir2_fh_totlen &&
+	    !fanotify_fh_equal(fanotify_info_dir2_fh(info1),
+			       fanotify_info_dir2_fh(info2)))
+		return false;
+
 	if (info1->file_fh_totlen &&
 	    !fanotify_fh_equal(fanotify_info_file_fh(info1),
 			       fanotify_info_file_fh(info2)))
 		return false;
 
-	return !info1->name_len ||
-		!memcmp(fanotify_info_name(info1), fanotify_info_name(info2),
-			info1->name_len);
+	if (info1->name_len &&
+	    memcmp(fanotify_info_name(info1), fanotify_info_name(info2),
+		   info1->name_len))
+		return false;
+
+	return !info1->name2_len ||
+		!memcmp(fanotify_info_name2(info1), fanotify_info_name2(info2),
+			info1->name2_len);
 }
 
 static bool fanotify_name_event_equal(struct fanotify_name_event *fne1,
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index dd23ba659e76..0864e7efe23c 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -40,31 +40,45 @@ struct fanotify_fh {
 struct fanotify_info {
 	/* size of dir_fh/file_fh including fanotify_fh hdr size */
 	u8 dir_fh_totlen;
+	u8 dir2_fh_totlen;
 	u8 file_fh_totlen;
 	u8 name_len;
-	u8 pad;
+	u8 name2_len;
+	u8 pad[3];
 	unsigned char buf[];
 	/*
 	 * (struct fanotify_fh) dir_fh starts at buf[0]
-	 * (optional) file_fh starts at buf[dir_fh_totlen]
-	 * name starts at buf[dir_fh_totlen + file_fh_totlen]
+	 * (optional) dir2_fh starts at buf[dir_fh_totlen]
+	 * (optional) file_fh starts at buf[dir_fh_totlen + dir2_fh_totlen]
+	 * name starts at buf[dir_fh_totlen + dir2_fh_totlen + file_fh_totlen]
+	 * ...
 	 */
 #define FANOTIFY_DIR_FH_SIZE(info)	((info)->dir_fh_totlen)
+#define FANOTIFY_DIR2_FH_SIZE(info)	((info)->dir2_fh_totlen)
 #define FANOTIFY_FILE_FH_SIZE(info)	((info)->file_fh_totlen)
 #define FANOTIFY_NAME_SIZE(info)	((info)->name_len + 1)
+#define FANOTIFY_NAME2_SIZE(info)	((info)->name2_len + 1)
 
 #define FANOTIFY_DIR_FH_OFFSET(info)	0
-#define FANOTIFY_FILE_FH_OFFSET(info) \
+#define FANOTIFY_DIR2_FH_OFFSET(info) \
 	(FANOTIFY_DIR_FH_OFFSET(info) + FANOTIFY_DIR_FH_SIZE(info))
+#define FANOTIFY_FILE_FH_OFFSET(info) \
+	(FANOTIFY_DIR2_FH_OFFSET(info) + FANOTIFY_DIR2_FH_SIZE(info))
 #define FANOTIFY_NAME_OFFSET(info) \
 	(FANOTIFY_FILE_FH_OFFSET(info) + FANOTIFY_FILE_FH_SIZE(info))
+#define FANOTIFY_NAME2_OFFSET(info) \
+	(FANOTIFY_NAME_OFFSET(info) + FANOTIFY_NAME_SIZE(info))
 
 #define FANOTIFY_DIR_FH_BUF(info) \
 	((info)->buf + FANOTIFY_DIR_FH_OFFSET(info))
+#define FANOTIFY_DIR2_FH_BUF(info) \
+	((info)->buf + FANOTIFY_DIR2_FH_OFFSET(info))
 #define FANOTIFY_FILE_FH_BUF(info) \
 	((info)->buf + FANOTIFY_FILE_FH_OFFSET(info))
 #define FANOTIFY_NAME_BUF(info) \
 	((info)->buf + FANOTIFY_NAME_OFFSET(info))
+#define FANOTIFY_NAME2_BUF(info) \
+	((info)->buf + FANOTIFY_NAME2_OFFSET(info))
 } __aligned(4);
 
 static inline bool fanotify_fh_has_ext_buf(struct fanotify_fh *fh)
@@ -106,6 +120,20 @@ static inline struct fanotify_fh *fanotify_info_dir_fh(struct fanotify_info *inf
 	return (struct fanotify_fh *)FANOTIFY_DIR_FH_BUF(info);
 }
 
+static inline int fanotify_info_dir2_fh_len(struct fanotify_info *info)
+{
+	if (!info->dir2_fh_totlen ||
+	    WARN_ON_ONCE(info->dir2_fh_totlen < FANOTIFY_FH_HDR_LEN))
+		return 0;
+
+	return info->dir2_fh_totlen - FANOTIFY_FH_HDR_LEN;
+}
+
+static inline struct fanotify_fh *fanotify_info_dir2_fh(struct fanotify_info *info)
+{
+	return (struct fanotify_fh *)FANOTIFY_DIR2_FH_BUF(info);
+}
+
 static inline int fanotify_info_file_fh_len(struct fanotify_info *info)
 {
 	if (!info->file_fh_totlen ||
@@ -128,14 +156,24 @@ static inline char *fanotify_info_name(struct fanotify_info *info)
 	return FANOTIFY_NAME_BUF(info);
 }
 
+static inline char *fanotify_info_name2(struct fanotify_info *info)
+{
+	if (!info->name_len || !info->name2_len)
+		return NULL;
+
+	return FANOTIFY_NAME2_BUF(info);
+}
+
 static inline void fanotify_info_init(struct fanotify_info *info)
 {
 	BUILD_BUG_ON(FANOTIFY_FH_HDR_LEN + MAX_HANDLE_SZ > U8_MAX);
 	BUILD_BUG_ON(NAME_MAX > U8_MAX);
 
 	info->dir_fh_totlen = 0;
+	info->dir2_fh_totlen = 0;
 	info->file_fh_totlen = 0;
 	info->name_len = 0;
+	info->name2_len = 0;
 }
 
 static inline void fanotify_info_copy_name(struct fanotify_info *info,
@@ -148,6 +186,17 @@ static inline void fanotify_info_copy_name(struct fanotify_info *info,
 	strcpy(fanotify_info_name(info), name->name);
 }
 
+static inline void fanotify_info_copy_name2(struct fanotify_info *info,
+					    const struct qstr *name)
+{
+	if (WARN_ON_ONCE(name->len > NAME_MAX) ||
+	    WARN_ON_ONCE(!info->name_len))
+		return;
+
+	info->name2_len = name->len;
+	strcpy(fanotify_info_name2(info), name->name);
+}
+
 /*
  * Common structure for fanotify events. Concrete structs are allocated in
  * fanotify_handle_event() and freed when the information is retrieved by
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 6ec26b124041..d973f36676a9 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -332,11 +332,10 @@ static int process_access_response(struct fsnotify_group *group,
 static size_t copy_error_info_to_user(struct fanotify_event *event,
 				      char __user *buf, int count)
 {
-	struct fanotify_event_info_error info;
+	struct fanotify_event_info_error info = { };
 	struct fanotify_error_event *fee = FANOTIFY_EE(event);
 
 	info.hdr.info_type = FAN_EVENT_INFO_TYPE_ERROR;
-	info.hdr.pad = 0;
 	info.hdr.len = FANOTIFY_ERROR_INFO_LEN;
 
 	if (WARN_ON(count < info.hdr.len))
-- 
2.33.1

[PATCH 5/7] fanotify: record new parent and name in MOVED_FROM event

[Amir Goldstein]

In the special case of MOVED_FROM event, if we are recording the child
fid due to FAN_REPORT_TARGET_FID init flag, we also record the new
parent and name.

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

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 795bedcb6f9b..d1adcb3437c5 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -592,21 +592,30 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 							__kernel_fsid_t *fsid,
 							const struct qstr *name,
 							struct inode *child,
+							struct dentry *moved,
 							unsigned int *hash,
 							gfp_t gfp)
 {
 	struct fanotify_name_event *fne;
 	struct fanotify_info *info;
 	struct fanotify_fh *dfh, *ffh;
+	struct inode *dir2 = moved ? d_inode(moved->d_parent) : NULL;
+	const struct qstr *name2 = moved ? &moved->d_name : NULL;
 	unsigned int dir_fh_len = fanotify_encode_fh_len(id);
+	unsigned int dir2_fh_len = fanotify_encode_fh_len(dir2);
 	unsigned int child_fh_len = fanotify_encode_fh_len(child);
 	unsigned int size;
 
 	size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len;
+	if (dir2_fh_len)
+		size += FANOTIFY_FH_HDR_LEN + dir2_fh_len;
 	if (child_fh_len)
 		size += FANOTIFY_FH_HDR_LEN + child_fh_len;
-	if (name)
+	if (name) {
 		size += name->len + 1;
+		if (name2)
+			size += name2->len + 1;
+	}
 	fne = kmalloc(size, gfp);
 	if (!fne)
 		return NULL;
@@ -618,6 +627,11 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 	fanotify_info_init(info);
 	dfh = fanotify_info_dir_fh(info);
 	info->dir_fh_totlen = fanotify_encode_fh(dfh, id, dir_fh_len, hash, 0);
+	if (dir2_fh_len) {
+		dfh = fanotify_info_dir2_fh(info);
+		info->dir2_fh_totlen = fanotify_encode_fh(dfh, dir2,
+							  dir2_fh_len, hash, 0);
+	}
 	if (child_fh_len) {
 		ffh = fanotify_info_file_fh(info);
 		info->file_fh_totlen = fanotify_encode_fh(ffh, child,
@@ -628,12 +642,26 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
 
 		fanotify_info_copy_name(info, name);
 		*hash ^= full_name_hash((void *)salt, name->name, name->len);
+
+		/* name2 can only be stored after valid name1 */
+		if (name2) {
+			salt = name2->len;
+			fanotify_info_copy_name2(info, name2);
+			*hash ^= full_name_hash((void *)salt, name2->name,
+						name2->len);
+		}
 	}
 
 	pr_debug("%s: ino=%lu size=%u dir_fh_len=%u child_fh_len=%u name_len=%u name='%.*s'\n",
 		 __func__, id->i_ino, size, dir_fh_len, child_fh_len,
 		 info->name_len, info->name_len, fanotify_info_name(info));
 
+	if (dir2_fh_len) {
+		pr_debug("%s: dir2_fh_len=%u name2_len=%u name2='%.*s'\n",
+			 __func__, dir2_fh_len, info->name2_len,
+			 info->name2_len, fanotify_info_name2(info));
+	}
+
 	return &fne->fae;
 }
 
@@ -689,6 +717,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 	struct inode *dirid = fanotify_dfid_inode(mask, data, data_type, dir);
 	const struct path *path = fsnotify_data_path(data, data_type);
 	struct mem_cgroup *old_memcg;
+	struct dentry *moved = NULL;
 	struct inode *child = NULL;
 	bool name_event = false;
 	unsigned int hash = 0;
@@ -699,9 +728,14 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 		/*
 		 * For certain events and group flags, report the child fid
 		 * in addition to reporting the parent fid and maybe child name.
+		 * In the special case of MOVED_FROM event, if we are reporting
+		 * the child fid we are also reporting the new parent and name.
 		 */
-		if (fanotify_report_child_fid(fid_mode, mask) && id != dirid)
+		if (fanotify_report_child_fid(fid_mode, mask) && id != dirid) {
 			child = id;
+			if (mask & FAN_MOVED_FROM)
+				moved = fsnotify_data_dentry(data, data_type);
+		}
 
 		id = dirid;
 
@@ -747,7 +781,7 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
 						   data_type, &hash);
 	} else if (name_event && (file_name || child)) {
 		event = fanotify_alloc_name_event(id, fsid, file_name, child,
-						  &hash, gfp);
+						  moved, &hash, gfp);
 	} else if (fid_mode) {
 		event = fanotify_alloc_fid_event(id, fsid, &hash, gfp);
 	} else {
-- 
2.33.1

[PATCH 6/7] fanotify: report new parent and name in MOVED_FROM event

[Amir Goldstein]

In the special case of MOVED_FROM event, if we are reporting the child
fid due to FAN_REPORT_TARGET_FID init flag, we also report the new
parent and name.

The new parent and name are reported using a new info record of type
FAN_EVENT_INFO_TYPE_DFID_NAME2 that follows the info record of type
FAN_EVENT_INFO_TYPE_DFID_NAME with the old parent and name.

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

diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 0864e7efe23c..26d471aab054 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -339,6 +339,13 @@ static inline int fanotify_event_dir_fh_len(struct fanotify_event *event)
 	return info ? fanotify_info_dir_fh_len(info) : 0;
 }
 
+static inline int fanotify_event_dir2_fh_len(struct fanotify_event *event)
+{
+	struct fanotify_info *info = fanotify_event_info(event);
+
+	return info ? fanotify_info_dir2_fh_len(info) : 0;
+}
+
 static inline bool fanotify_event_has_object_fh(struct fanotify_event *event)
 {
 	/* For error events, even zeroed fh are reported. */
@@ -352,6 +359,16 @@ static inline bool fanotify_event_has_dir_fh(struct fanotify_event *event)
 	return fanotify_event_dir_fh_len(event) > 0;
 }
 
+/* For MOVED_FROM event with FAN_REPORT_TARGET_FID */
+static inline bool fanotify_event_has_two_names(struct fanotify_event *event)
+{
+	struct fanotify_info *info = fanotify_event_info(event);
+
+	return info && info->name_len && info->name2_len &&
+		fanotify_info_dir_fh_len(info) > 0 &&
+		fanotify_info_dir2_fh_len(info) > 0;
+}
+
 struct fanotify_path_event {
 	struct fanotify_event fae;
 	struct path path;
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index d973f36676a9..d6420e10740d 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -134,7 +134,7 @@ static size_t fanotify_event_len(unsigned int info_mode,
 {
 	size_t event_len = FAN_EVENT_METADATA_LEN;
 	struct fanotify_info *info;
-	int dir_fh_len;
+	int dir_fh_len, dir2_fh_len;
 	int fh_len;
 	int dot_len = 0;
 
@@ -149,6 +149,11 @@ static size_t fanotify_event_len(unsigned int info_mode,
 	if (fanotify_event_has_dir_fh(event)) {
 		dir_fh_len = fanotify_event_dir_fh_len(event);
 		event_len += fanotify_fid_info_len(dir_fh_len, info->name_len);
+		if (fanotify_event_has_two_names(event)) {
+			dir2_fh_len = fanotify_event_dir2_fh_len(event);
+			event_len += fanotify_fid_info_len(dir2_fh_len,
+							   info->name2_len);
+		}
 	} else if ((info_mode & FAN_REPORT_NAME) &&
 		   (event->mask & FAN_ONDIR)) {
 		/*
@@ -379,6 +384,7 @@ static int copy_fid_info_to_user(__kernel_fsid_t *fsid, struct fanotify_fh *fh,
 			return -EFAULT;
 		break;
 	case FAN_EVENT_INFO_TYPE_DFID_NAME:
+	case FAN_EVENT_INFO_TYPE_DFID_NAME2:
 		if (WARN_ON_ONCE(!name || !name_len))
 			return -EFAULT;
 		break;
@@ -478,7 +484,10 @@ static int copy_info_records_to_user(struct fanotify_event *event,
 	unsigned int pidfd_mode = info_mode & FAN_REPORT_PIDFD;
 
 	/*
-	 * Event info records order is as follows: dir fid + name, child fid.
+	 * Event info records order is as follows:
+	 * 1. dir fid + name
+	 * 2. (optional) new dir fid + new name
+	 * 3. (optional) child fid
 	 */
 	if (fanotify_event_has_dir_fh(event)) {
 		info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
@@ -496,6 +505,22 @@ static int copy_info_records_to_user(struct fanotify_event *event,
 		total_bytes += ret;
 	}
 
+	/* New dir fid + name can only be reported after old dir fid + name */
+	if (info_type && fanotify_event_has_two_names(event)) {
+		info_type = FAN_EVENT_INFO_TYPE_DFID_NAME2;
+		ret = copy_fid_info_to_user(fanotify_event_fsid(event),
+					    fanotify_info_dir2_fh(info),
+					    info_type,
+					    fanotify_info_name2(info),
+					    info->name2_len, buf, count);
+		if (ret < 0)
+			return ret;
+
+		buf += ret;
+		count -= ret;
+		total_bytes += ret;
+	}
+
 	if (fanotify_event_has_object_fh(event)) {
 		const char *dot = NULL;
 		int dot_len = 0;
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index f9202ce31b0d..1ac31a912cea 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -131,6 +131,7 @@ struct fanotify_event_metadata {
 #define FAN_EVENT_INFO_TYPE_DFID	3
 #define FAN_EVENT_INFO_TYPE_PIDFD	4
 #define FAN_EVENT_INFO_TYPE_ERROR	5
+#define FAN_EVENT_INFO_TYPE_DFID_NAME2	6 /* For FAN_MOVED_FROM */
 
 /* Variable length info record following event metadata */
 struct fanotify_event_info_header {
-- 
2.33.1

[PATCH 7/7] fanotify: enable the FAN_REPORT_TARGET_FID flag

[Amir Goldstein]

That will result in reporting of self inode fid in dirent events
and destination dir+name in MOVED_FROM event.

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

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index d6420e10740d..b62218d4aea2 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1700,7 +1700,7 @@ static int __init fanotify_user_setup(void)
 				     FANOTIFY_DEFAULT_MAX_USER_MARKS);
 
 	BUILD_BUG_ON(FANOTIFY_INIT_FLAGS & FANOTIFY_INTERNAL_GROUP_FLAGS);
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 11);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 12);
 	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 616af2ea20f3..25c1894510a0 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -25,7 +25,7 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */
 
 #define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FANOTIFY_PERM_CLASSES)
 
-#define FANOTIFY_FID_BITS	(FAN_REPORT_FID | FAN_REPORT_DFID_NAME)
+#define FANOTIFY_FID_BITS	(FAN_REPORT_ALL_FIDS | FAN_REPORT_NAME)
 
 #define FANOTIFY_INFO_MODES	(FANOTIFY_FID_BITS | FAN_REPORT_PIDFD)
 
-- 
2.33.1

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Jan,
>
> This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> from 3 months ago.
>
> With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> I figured we could get an early (re)start of the discussion on
> FAN_REPORT_TARGET_FID towards 5.17.
>
> The added information in dirent events solves problems for my use case -
> It helps getting the following information in a race free manner:
> 1. fid of a created directory on mkdir
> 2. from/to path information on rename of non-dir
>
> I realize those are two different API traits, but they are close enough
> so I preferred not to clutter the REPORT flags space any further than it
> already is. The single added flag FAN_REPORT_TARGET_FID adds:
> 1. child fid info to CREATE/DELETE/MOVED_* events
> 2. new parent+name info to MOVED_FROM event
>
> Instead of going the "inotify way" and trying to join the MOVED_FROM/
> MOVED_TO events using a cookie, I chose to incorporate the new
> parent+name intomation only in the MOVED_FROM event.
> I made this choice for several reasons:
> 1. Availability of the moved dentry in the hook and event data
> 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
>    DFID_NAME info to statat(2) the object as we suggested
>
> I chose to reduce testing complexity and require all other FID
> flags with FAN_REPORT_TARGET_FID and there is a convenience
> macro FAN_REPORT_ALL_FIDS that application can use.

Self comment - Don't use ALL_ for macro names in uapi...
There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...

BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
not because I object to FAN_RENAME, just because it was simpler to implement
the MOVED_FROM alternative, so I thought I'll start with this proposal
and see how
it goes.

Thanks,
Amir.

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Jan Kara]

Hi Amir!

On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
> On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> > from 3 months ago.
> >
> > With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> > I figured we could get an early (re)start of the discussion on
> > FAN_REPORT_TARGET_FID towards 5.17.
> >
> > The added information in dirent events solves problems for my use case -
> > It helps getting the following information in a race free manner:
> > 1. fid of a created directory on mkdir
> > 2. from/to path information on rename of non-dir
> >
> > I realize those are two different API traits, but they are close enough
> > so I preferred not to clutter the REPORT flags space any further than it
> > already is. The single added flag FAN_REPORT_TARGET_FID adds:
> > 1. child fid info to CREATE/DELETE/MOVED_* events
> > 2. new parent+name info to MOVED_FROM event
> >
> > Instead of going the "inotify way" and trying to join the MOVED_FROM/
> > MOVED_TO events using a cookie, I chose to incorporate the new
> > parent+name intomation only in the MOVED_FROM event.
> > I made this choice for several reasons:
> > 1. Availability of the moved dentry in the hook and event data
> > 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> > 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
> >    DFID_NAME info to statat(2) the object as we suggested
> >
> > I chose to reduce testing complexity and require all other FID
> > flags with FAN_REPORT_TARGET_FID and there is a convenience
> > macro FAN_REPORT_ALL_FIDS that application can use.
> 
> Self comment - Don't use ALL_ for macro names in uapi...
> There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...

Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
another FID flag later ;)

> BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> not because I object to FAN_RENAME, just because it was simpler to implement
> the MOVED_FROM alternative, so I thought I'll start with this proposal
> and see how
> it goes.

I've read through all the patches and I didn't find anything wrong.
Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
fsnotify_name() once more with FS_RENAME event and we'd gate addition of
second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
lean a bit more towards that.

								Honza

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

Re: [PATCH 5/7] fanotify: record new parent and name in MOVED_FROM event

[Jan Kara]

On Fri 29-10-21 14:40:26, Amir Goldstein wrote:
> In the special case of MOVED_FROM event, if we are recording the child
> fid due to FAN_REPORT_TARGET_FID init flag, we also record the new
> parent and name.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
...
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 795bedcb6f9b..d1adcb3437c5 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -592,21 +592,30 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
>  							__kernel_fsid_t *fsid,
>  							const struct qstr *name,
>  							struct inode *child,
> +							struct dentry *moved,
>  							unsigned int *hash,
>  							gfp_t gfp)
>  {
>  	struct fanotify_name_event *fne;
>  	struct fanotify_info *info;
>  	struct fanotify_fh *dfh, *ffh;
> +	struct inode *dir2 = moved ? d_inode(moved->d_parent) : NULL;

I think we need to be more careful here (like dget_parent()?). Fsnotify is
called after everything is unlocked after rename so dentry can be changing
under us, cannot it? Also does that mean that we could actually get a wrong
parent here if the dentry is renamed immediately after we unlock things and
before we report fsnotify event?

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

Re: [PATCH 5/7] fanotify: record new parent and name in MOVED_FROM event

[Amir Goldstein]

On Fri, Nov 12, 2021 at 6:48 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 29-10-21 14:40:26, Amir Goldstein wrote:
> > In the special case of MOVED_FROM event, if we are recording the child
> > fid due to FAN_REPORT_TARGET_FID init flag, we also record the new
> > parent and name.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 795bedcb6f9b..d1adcb3437c5 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -592,21 +592,30 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
> >                                                       __kernel_fsid_t *fsid,
> >                                                       const struct qstr *name,
> >                                                       struct inode *child,
> > +                                                     struct dentry *moved,
> >                                                       unsigned int *hash,
> >                                                       gfp_t gfp)
> >  {
> >       struct fanotify_name_event *fne;
> >       struct fanotify_info *info;
> >       struct fanotify_fh *dfh, *ffh;
> > +     struct inode *dir2 = moved ? d_inode(moved->d_parent) : NULL;
>
> I think we need to be more careful here (like dget_parent()?). Fsnotify is
> called after everything is unlocked after rename so dentry can be changing
> under us, cannot it? Also does that mean that we could actually get a wrong
> parent here if the dentry is renamed immediately after we unlock things and
> before we report fsnotify event?

fsnotify_move() is called inside lock_rename() (both old_dir and new_dir locks),
which are held by the caller of vfs_rename(), and prevent d_parent/d_name
changes to child dentries, so moved->d_parent is stable here.
You are probably confusing with inode_unlock(target), which is the
child inode lock.

d_parent/d_name are also stable for fsnotify_create/link/unlink/mkdir/rmdir
per the vfs locking rules for those operations. In all those cases, the parent
dir lock is held for vfs helper callers.
This is why we can use dentry->d_name (and moved->d_name) in all those
fsnotify hooks.

Thanks,
Amir.

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

On Fri, Nov 12, 2021 at 6:39 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi Amir!
>
> On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
> > On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> > > from 3 months ago.
> > >
> > > With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> > > I figured we could get an early (re)start of the discussion on
> > > FAN_REPORT_TARGET_FID towards 5.17.
> > >
> > > The added information in dirent events solves problems for my use case -
> > > It helps getting the following information in a race free manner:
> > > 1. fid of a created directory on mkdir
> > > 2. from/to path information on rename of non-dir
> > >
> > > I realize those are two different API traits, but they are close enough
> > > so I preferred not to clutter the REPORT flags space any further than it
> > > already is. The single added flag FAN_REPORT_TARGET_FID adds:
> > > 1. child fid info to CREATE/DELETE/MOVED_* events
> > > 2. new parent+name info to MOVED_FROM event
> > >
> > > Instead of going the "inotify way" and trying to join the MOVED_FROM/
> > > MOVED_TO events using a cookie, I chose to incorporate the new
> > > parent+name intomation only in the MOVED_FROM event.
> > > I made this choice for several reasons:
> > > 1. Availability of the moved dentry in the hook and event data
> > > 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> > > 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
> > >    DFID_NAME info to statat(2) the object as we suggested
> > >
> > > I chose to reduce testing complexity and require all other FID
> > > flags with FAN_REPORT_TARGET_FID and there is a convenience
> > > macro FAN_REPORT_ALL_FIDS that application can use.
> >
> > Self comment - Don't use ALL_ for macro names in uapi...
> > There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...
>
> Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
> another FID flag later ;)
>
> > BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> > not because I object to FAN_RENAME, just because it was simpler to implement
> > the MOVED_FROM alternative, so I thought I'll start with this proposal
> > and see how
> > it goes.
>
> I've read through all the patches and I didn't find anything wrong.
> Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
> fsnotify_name() once more with FS_RENAME event and we'd gate addition of
> second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
> FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
> current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
> lean a bit more towards that.

I grew to like FAN_RENAME better myself as well.
To make sure we are talking about the same thing:
1. FAN_RENAME always reports 2*(dirfid+name)
2. FAN_REPORT_TARGET_FID adds optional child fid record to
    CREATE/DELETE/RENAME/MOVED_TO/FROM

Thanks,
Amir.

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Nov 12, 2021 at 6:39 PM Jan Kara <jack@suse.cz> wrote:
> >
> > Hi Amir!
> >
> > On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
> > > On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> > > > from 3 months ago.
> > > >
> > > > With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> > > > I figured we could get an early (re)start of the discussion on
> > > > FAN_REPORT_TARGET_FID towards 5.17.
> > > >
> > > > The added information in dirent events solves problems for my use case -
> > > > It helps getting the following information in a race free manner:
> > > > 1. fid of a created directory on mkdir
> > > > 2. from/to path information on rename of non-dir
> > > >
> > > > I realize those are two different API traits, but they are close enough
> > > > so I preferred not to clutter the REPORT flags space any further than it
> > > > already is. The single added flag FAN_REPORT_TARGET_FID adds:
> > > > 1. child fid info to CREATE/DELETE/MOVED_* events
> > > > 2. new parent+name info to MOVED_FROM event
> > > >
> > > > Instead of going the "inotify way" and trying to join the MOVED_FROM/
> > > > MOVED_TO events using a cookie, I chose to incorporate the new
> > > > parent+name intomation only in the MOVED_FROM event.
> > > > I made this choice for several reasons:
> > > > 1. Availability of the moved dentry in the hook and event data
> > > > 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> > > > 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
> > > >    DFID_NAME info to statat(2) the object as we suggested
> > > >
> > > > I chose to reduce testing complexity and require all other FID
> > > > flags with FAN_REPORT_TARGET_FID and there is a convenience
> > > > macro FAN_REPORT_ALL_FIDS that application can use.
> > >
> > > Self comment - Don't use ALL_ for macro names in uapi...
> > > There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...
> >
> > Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
> > another FID flag later ;)
> >
> > > BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> > > not because I object to FAN_RENAME, just because it was simpler to implement
> > > the MOVED_FROM alternative, so I thought I'll start with this proposal
> > > and see how
> > > it goes.
> >
> > I've read through all the patches and I didn't find anything wrong.
> > Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
> > fsnotify_name() once more with FS_RENAME event and we'd gate addition of
> > second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
> > FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
> > current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
> > lean a bit more towards that.
>
> I grew to like FAN_RENAME better myself as well.
> To make sure we are talking about the same thing:
> 1. FAN_RENAME always reports 2*(dirfid+name)
> 2. FAN_REPORT_TARGET_FID adds optional child fid record to
>     CREATE/DELETE/RENAME/MOVED_TO/FROM
>

Err, I tried the FAN_RENAME approach and hit a semantic issue:
Users can watch a directory inode and get only MOVED_FROM
when entries are moved from this directory. Same for MOVED_TO.
How would FAN_RENAME behave when setting FAN_RENAME on a
directory inode? Should listeners get events on files renamed in and out of that
directory?

I see several options:
1. Go back to FAN_MOVED_FROM as in this patch set, where semantics are clear
2. Report FAN_RENAME if either old or new dir is watched (or mount/sb)
3. Report FAN_RENAME only if both old and new dirs are watched (or mount/sb)

For option 2, we may need to hide information records, For example, because an
unprivileged listener may not have access to old or new directory.

A variant of option 3, is that FAN_RENAME will be an event mask flag
that can be added to FAN_MOVE events, to request that if both FROM/TO events
are going to be reported, then a single joint event will be reported
instead, e.g:

#define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO)
#define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN)

Instead of generating an extra FS_RENAME event in fsnotify_move(),
fsnotify() will search for matching marks on the moved->d_parent->d_inode
of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT
mark iterator type and then fanotify_group_event_mask() will be able
to tell if the
event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint
FAN_RENAME.

If a group has the FAN_RENAME mask on the new parent dir, then
FS_MOVED_TO events can be dropped, because the event was already
reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM
event.

Am I over complicating this?
Do you have a better and clearer semantics to propose?

Thanks,
Amir.

Re: [PATCH 5/7] fanotify: record new parent and name in MOVED_FROM event

[Jan Kara]

On Sat 13-11-21 11:40:39, Amir Goldstein wrote:
> On Fri, Nov 12, 2021 at 6:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 29-10-21 14:40:26, Amir Goldstein wrote:
> > > In the special case of MOVED_FROM event, if we are recording the child
> > > fid due to FAN_REPORT_TARGET_FID init flag, we also record the new
> > > parent and name.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ...
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index 795bedcb6f9b..d1adcb3437c5 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -592,21 +592,30 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id,
> > >                                                       __kernel_fsid_t *fsid,
> > >                                                       const struct qstr *name,
> > >                                                       struct inode *child,
> > > +                                                     struct dentry *moved,
> > >                                                       unsigned int *hash,
> > >                                                       gfp_t gfp)
> > >  {
> > >       struct fanotify_name_event *fne;
> > >       struct fanotify_info *info;
> > >       struct fanotify_fh *dfh, *ffh;
> > > +     struct inode *dir2 = moved ? d_inode(moved->d_parent) : NULL;
> >
> > I think we need to be more careful here (like dget_parent()?). Fsnotify is
> > called after everything is unlocked after rename so dentry can be changing
> > under us, cannot it? Also does that mean that we could actually get a wrong
> > parent here if the dentry is renamed immediately after we unlock things and
> > before we report fsnotify event?
> 
> fsnotify_move() is called inside lock_rename() (both old_dir and new_dir locks),
> which are held by the caller of vfs_rename(), and prevent d_parent/d_name
> changes to child dentries, so moved->d_parent is stable here.
> You are probably confusing with inode_unlock(target), which is the
> child inode lock.
> 
> d_parent/d_name are also stable for fsnotify_create/link/unlink/mkdir/rmdir
> per the vfs locking rules for those operations. In all those cases, the parent
> dir lock is held for vfs helper callers.
> This is why we can use dentry->d_name (and moved->d_name) in all those
> fsnotify hooks.

Bah, you're right. I got confused by the locking of source and target
inside vfs_rename() but those are not parent directories but rather "files"
being renamed from / two. Sorry for the noise.

								Honza

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

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Jan Kara]

On Sat 13-11-21 21:31:25, Amir Goldstein wrote:
> On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Nov 12, 2021 at 6:39 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Hi Amir!
> > >
> > > On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
> > > > On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> > > > > from 3 months ago.
> > > > >
> > > > > With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> > > > > I figured we could get an early (re)start of the discussion on
> > > > > FAN_REPORT_TARGET_FID towards 5.17.
> > > > >
> > > > > The added information in dirent events solves problems for my use case -
> > > > > It helps getting the following information in a race free manner:
> > > > > 1. fid of a created directory on mkdir
> > > > > 2. from/to path information on rename of non-dir
> > > > >
> > > > > I realize those are two different API traits, but they are close enough
> > > > > so I preferred not to clutter the REPORT flags space any further than it
> > > > > already is. The single added flag FAN_REPORT_TARGET_FID adds:
> > > > > 1. child fid info to CREATE/DELETE/MOVED_* events
> > > > > 2. new parent+name info to MOVED_FROM event
> > > > >
> > > > > Instead of going the "inotify way" and trying to join the MOVED_FROM/
> > > > > MOVED_TO events using a cookie, I chose to incorporate the new
> > > > > parent+name intomation only in the MOVED_FROM event.
> > > > > I made this choice for several reasons:
> > > > > 1. Availability of the moved dentry in the hook and event data
> > > > > 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> > > > > 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
> > > > >    DFID_NAME info to statat(2) the object as we suggested
> > > > >
> > > > > I chose to reduce testing complexity and require all other FID
> > > > > flags with FAN_REPORT_TARGET_FID and there is a convenience
> > > > > macro FAN_REPORT_ALL_FIDS that application can use.
> > > >
> > > > Self comment - Don't use ALL_ for macro names in uapi...
> > > > There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...
> > >
> > > Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
> > > another FID flag later ;)
> > >
> > > > BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> > > > not because I object to FAN_RENAME, just because it was simpler to implement
> > > > the MOVED_FROM alternative, so I thought I'll start with this proposal
> > > > and see how
> > > > it goes.
> > >
> > > I've read through all the patches and I didn't find anything wrong.
> > > Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
> > > fsnotify_name() once more with FS_RENAME event and we'd gate addition of
> > > second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
> > > FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
> > > current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
> > > lean a bit more towards that.
> >
> > I grew to like FAN_RENAME better myself as well.
> > To make sure we are talking about the same thing:
> > 1. FAN_RENAME always reports 2*(dirfid+name)
> > 2. FAN_REPORT_TARGET_FID adds optional child fid record to
> >     CREATE/DELETE/RENAME/MOVED_TO/FROM
> >

Correct, that's what I meant.

> Err, I tried the FAN_RENAME approach and hit a semantic issue:
> Users can watch a directory inode and get only MOVED_FROM
> when entries are moved from this directory. Same for MOVED_TO.
> How would FAN_RENAME behave when setting FAN_RENAME on a directory inode?
> Should listeners get events on files renamed in and out of that
> directory?
> 
> I see several options:
> 1. Go back to FAN_MOVED_FROM as in this patch set, where semantics are clear

Well, semantics are clear but in principle user does not have access to
target dir either so the permission problems are the same as with option 2,
aren't they?

> 2. Report FAN_RENAME if either old or new dir is watched (or mount/sb)
> 3. Report FAN_RENAME only if both old and new dirs are watched (or mount/sb)
> 
> For option 2, we may need to hide information records, For example,
> because an unprivileged listener may not have access to old or new
> directory.

Good spotting. That can indeed be a problem.

> A variant of option 3, is that FAN_RENAME will be an event mask flag
> that can be added to FAN_MOVE events, to request that if both FROM/TO events
> are going to be reported, then a single joint event will be reported
> instead, e.g:
> 
> #define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO)
> #define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN)
> 
> Instead of generating an extra FS_RENAME event in fsnotify_move(),
> fsnotify() will search for matching marks on the moved->d_parent->d_inode
> of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT
> mark iterator type and then fanotify_group_event_mask() will be able
> to tell if the
> event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint
> FAN_RENAME.
> 
> If a group has the FAN_RENAME mask on the new parent dir, then
> FS_MOVED_TO events can be dropped, because the event was already
> reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM
> event.
> 
> Am I over complicating this?
> Do you have a better and clearer semantics to propose?

So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO &
FAN_MOVED_FROM. It would be generated whenever source or target is tagged
with FAN_RENAME, source info is provided if source is tagged, target info
is provided when target is tagged (both are provides when both are tagged).
So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed
merging. This looks like a clean enough and simple to explain API. Sure it
duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity
of the API outweights the duplication. Basically FAN_MOVED_FROM &
FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although
I don't think we want to do it for compatibility reasons.

Implementation-wise we have couple of options. Currently the simplest I can
see is that fsnotify() would iterate marks on both source & target dirs
(like we already do for inode & parent) when it handles FS_RENAME event. In
fanotify_handle_event() we will decide which info to report with FAN_RENAME
event based on which marks in iter_info have FS_RENAME set (luckily mount
marks are out of question for rename events so it will be relatively
simple). What do you think?

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

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

> > > > > BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> > > > > not because I object to FAN_RENAME, just because it was simpler to implement
> > > > > the MOVED_FROM alternative, so I thought I'll start with this proposal
> > > > > and see how
> > > > > it goes.
> > > >
> > > > I've read through all the patches and I didn't find anything wrong.
> > > > Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
> > > > fsnotify_name() once more with FS_RENAME event and we'd gate addition of
> > > > second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
> > > > FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
> > > > current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
> > > > lean a bit more towards that.
> > >
> > > I grew to like FAN_RENAME better myself as well.
> > > To make sure we are talking about the same thing:
> > > 1. FAN_RENAME always reports 2*(dirfid+name)
> > > 2. FAN_REPORT_TARGET_FID adds optional child fid record to
> > >     CREATE/DELETE/RENAME/MOVED_TO/FROM
> > >
>
> Correct, that's what I meant.
>
> > Err, I tried the FAN_RENAME approach and hit a semantic issue:
> > Users can watch a directory inode and get only MOVED_FROM
> > when entries are moved from this directory. Same for MOVED_TO.
> > How would FAN_RENAME behave when setting FAN_RENAME on a directory inode?
> > Should listeners get events on files renamed in and out of that
> > directory?
> >
> > I see several options:
> > 1. Go back to FAN_MOVED_FROM as in this patch set, where semantics are clear
>
> Well, semantics are clear but in principle user does not have access to
> target dir either so the permission problems are the same as with option 2,
> aren't they?

Correct.

>
> > 2. Report FAN_RENAME if either old or new dir is watched (or mount/sb)
> > 3. Report FAN_RENAME only if both old and new dirs are watched (or mount/sb)
> >
> > For option 2, we may need to hide information records, For example,
> > because an unprivileged listener may not have access to old or new
> > directory.
>
> Good spotting. That can indeed be a problem.
>
> > A variant of option 3, is that FAN_RENAME will be an event mask flag
> > that can be added to FAN_MOVE events, to request that if both FROM/TO events
> > are going to be reported, then a single joint event will be reported
> > instead, e.g:
> >
> > #define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO)
> > #define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN)
> >
> > Instead of generating an extra FS_RENAME event in fsnotify_move(),
> > fsnotify() will search for matching marks on the moved->d_parent->d_inode
> > of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT
> > mark iterator type and then fanotify_group_event_mask() will be able
> > to tell if the
> > event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint
> > FAN_RENAME.
> >
> > If a group has the FAN_RENAME mask on the new parent dir, then
> > FS_MOVED_TO events can be dropped, because the event was already
> > reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM
> > event.
> >
> > Am I over complicating this?
> > Do you have a better and clearer semantics to propose?
>
> So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO &
> FAN_MOVED_FROM. It would be generated whenever source or target is tagged
> with FAN_RENAME, source info is provided if source is tagged, target info
> is provided when target is tagged (both are provides when both are tagged).
> So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed
> merging. This looks like a clean enough and simple to explain API. Sure it
> duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity
> of the API outweights the duplication. Basically FAN_MOVED_FROM &
> FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although
> I don't think we want to do it for compatibility reasons.

Well, not only for compatibility.
The ability to request events for files moved into directory ~/inbox/ and files
moved out of directory ~/outbox/ cannot be expressed with FAN_RENAME
alone...

>
> Implementation-wise we have couple of options. Currently the simplest I can
> see is that fsnotify() would iterate marks on both source & target dirs
> (like we already do for inode & parent) when it handles FS_RENAME event. In

Yes. I already have a WIP branch (fan_reanme) using
FSNOTIFY_OBJ_TYPE_PARENT for the target dir mark.

Heads up: I intend to repurpose FS_DN_RENAME, by sending FS_RENAME
to ->handle_inode_event() backends only if (parent_mark == inode_mark).
Duplicating FS_MOVED_FROM I can cope with, but wasting 3 flags for
the same event is too much for me to bare ;-)

> fanotify_handle_event() we will decide which info to report with FAN_RENAME
> event based on which marks in iter_info have FS_RENAME set (luckily mount
> marks are out of question for rename events so it will be relatively
> simple). What do you think?

I like it. However,
If FAN_RENAME can have any combination of old,new,old+new info
we cannot get any with a single new into type
FAN_EVENT_INFO_TYPE_DFID_NAME2

(as in this posting)

We can go with:
#define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME   6
#define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME  7
#define FAN_EVENT_INFO_TYPE_OLD_DFID               8
#define FAN_EVENT_INFO_TYPE_NEW_DFID              9

Or we can go with:
/* Sub-types common to all three fid info types */
#define FAN_EVENT_INFO_FID_OF_OLD_DIR     1
#define FAN_EVENT_INFO_FID_OF_NEW_DIR    2

struct fanotify_event_info_header {
       __u8 info_type;
       __u8 sub_type;
       __u16 len;
};

(as in my wip branch fanotify_fid_of)

We could also have FAN_RENAME require FAN_REPORT_NAME
that would limit the number of info types, but I cannot find a good
justification for this requirement.

Any preference?

Thanks,
Amir.

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Jan Kara]

On Mon 15-11-21 14:22:49, Amir Goldstein wrote:
> > > A variant of option 3, is that FAN_RENAME will be an event mask flag
> > > that can be added to FAN_MOVE events, to request that if both FROM/TO events
> > > are going to be reported, then a single joint event will be reported
> > > instead, e.g:
> > >
> > > #define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO)
> > > #define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN)
> > >
> > > Instead of generating an extra FS_RENAME event in fsnotify_move(),
> > > fsnotify() will search for matching marks on the moved->d_parent->d_inode
> > > of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT
> > > mark iterator type and then fanotify_group_event_mask() will be able
> > > to tell if the
> > > event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint
> > > FAN_RENAME.
> > >
> > > If a group has the FAN_RENAME mask on the new parent dir, then
> > > FS_MOVED_TO events can be dropped, because the event was already
> > > reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM
> > > event.
> > >
> > > Am I over complicating this?
> > > Do you have a better and clearer semantics to propose?
> >
> > So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO &
> > FAN_MOVED_FROM. It would be generated whenever source or target is tagged
> > with FAN_RENAME, source info is provided if source is tagged, target info
> > is provided when target is tagged (both are provides when both are tagged).
> > So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed
> > merging. This looks like a clean enough and simple to explain API. Sure it
> > duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity
> > of the API outweights the duplication. Basically FAN_MOVED_FROM &
> > FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although
> > I don't think we want to do it for compatibility reasons.
> 
> Well, not only for compatibility.
> The ability to request events for files moved into directory ~/inbox/ and files
> moved out of directory ~/outbox/ cannot be expressed with FAN_RENAME
> alone...

If you ask for FAN_RENAME on ~/inbox, you can then filter out the "move
out" events based on the information coming with the event to userspace.
But I agree it requires more work in userspace to simulate FAN_MOVED_FROM.

> > Implementation-wise we have couple of options. Currently the simplest I can
> > see is that fsnotify() would iterate marks on both source & target dirs
> > (like we already do for inode & parent) when it handles FS_RENAME event. In
> 
> Yes. I already have a WIP branch (fan_reanme) using
> FSNOTIFY_OBJ_TYPE_PARENT for the target dir mark.
> 
> Heads up: I intend to repurpose FS_DN_RENAME, by sending FS_RENAME
> to ->handle_inode_event() backends only if (parent_mark == inode_mark).
> Duplicating FS_MOVED_FROM I can cope with, but wasting 3 flags for
> the same event is too much for me to bare ;-)

:)

> > fanotify_handle_event() we will decide which info to report with FAN_RENAME
> > event based on which marks in iter_info have FS_RENAME set (luckily mount
> > marks are out of question for rename events so it will be relatively
> > simple). What do you think?
> 
> I like it. However,
> If FAN_RENAME can have any combination of old,new,old+new info
> we cannot get any with a single new into type
> FAN_EVENT_INFO_TYPE_DFID_NAME2
> 
> (as in this posting)

We could define only DFID2 and DFID_NAME2 but I agree it would be somewhat
weird to have DFID_NAME2 in an event and not DFID_NAME.

> We can go with:
> #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME   6
> #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME  7
> #define FAN_EVENT_INFO_TYPE_OLD_DFID               8
> #define FAN_EVENT_INFO_TYPE_NEW_DFID              9
> 
> Or we can go with:
> /* Sub-types common to all three fid info types */
> #define FAN_EVENT_INFO_FID_OF_OLD_DIR     1
> #define FAN_EVENT_INFO_FID_OF_NEW_DIR    2
> 
> struct fanotify_event_info_header {
>        __u8 info_type;
>        __u8 sub_type;
>        __u16 len;
> };
> 
> (as in my wip branch fanotify_fid_of)

When we went the way of having different types for FID and DFID, I'd
continue with OLD_DFID_NAME, NEW_DFID_NAME, ... and keep the padding byte
free for now (just in case there's some extension which would urgently need
it).
 
> We could also have FAN_RENAME require FAN_REPORT_NAME
> that would limit the number of info types, but I cannot find a good
> justification for this requirement.

Yeah, I would not force that.

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

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

> > I like it. However,
> > If FAN_RENAME can have any combination of old,new,old+new info
> > we cannot get any with a single new into type
> > FAN_EVENT_INFO_TYPE_DFID_NAME2
> >
> > (as in this posting)
>
> We could define only DFID2 and DFID_NAME2 but I agree it would be somewhat
> weird to have DFID_NAME2 in an event and not DFID_NAME.
>
> > We can go with:
> > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME   6
> > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME  7
> > #define FAN_EVENT_INFO_TYPE_OLD_DFID               8
> > #define FAN_EVENT_INFO_TYPE_NEW_DFID              9
> >
> > Or we can go with:
> > /* Sub-types common to all three fid info types */
> > #define FAN_EVENT_INFO_FID_OF_OLD_DIR     1
> > #define FAN_EVENT_INFO_FID_OF_NEW_DIR    2
> >
> > struct fanotify_event_info_header {
> >        __u8 info_type;
> >        __u8 sub_type;
> >        __u16 len;
> > };
> >
> > (as in my wip branch fanotify_fid_of)
>
> When we went the way of having different types for FID and DFID, I'd
> continue with OLD_DFID_NAME, NEW_DFID_NAME, ... and keep the padding byte
> free for now (just in case there's some extension which would urgently need
> it).
>
> > We could also have FAN_RENAME require FAN_REPORT_NAME
> > that would limit the number of info types, but I cannot find a good
> > justification for this requirement.
>
> Yeah, I would not force that.
>

On second thought and after trying to write a mental man page
and realizing how ugly it gets, I feel strongly in favor of requiring
FAN_REPORT_NAME for the FAN_RENAME event.

My arguments are:
1. What is the benefit of FAN_RENAME without names?
    Is the knowledge that *something* was moved from dir A to dir B
    that important that it qualifies for the extra man page noise and
    application developer headache?
2. My declared motivation for this patch set was to close the last (?)
    functional gap between inotify and fanotify, that is, being able to
    reliably join MOVED_FROM and MOVED_TO events.
    Requiring FAN_REPORT_NAME still meets that goal.
3. In this patch set, FAN_REPORT_NAME is required (for now) for
    FAN_REPORT_TARGET_FID to reduce implementation and test
    matrix complexity (you did not object, so I wasn't planning on
    changing this requirement).
    The same argument holds for FAN_RENAME

So let's say this - we can add support for OLD_DFID, NEW_DFID types
later if we think they may serve a purpose, but at this time, I see no
reason to complicate the UAPI anymore than it already is and I would
rather implement only:

/* Info types for FAN_RENAME */
#define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME       10
/* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID    11 */
#define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME       12
/* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID    13 */

Do you agree?

Thanks,
Amir.

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Jan Kara]

On Tue 16-11-21 08:59:29, Amir Goldstein wrote:
> > > I like it. However,
> > > If FAN_RENAME can have any combination of old,new,old+new info
> > > we cannot get any with a single new into type
> > > FAN_EVENT_INFO_TYPE_DFID_NAME2
> > >
> > > (as in this posting)
> >
> > We could define only DFID2 and DFID_NAME2 but I agree it would be somewhat
> > weird to have DFID_NAME2 in an event and not DFID_NAME.
> >
> > > We can go with:
> > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME   6
> > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME  7
> > > #define FAN_EVENT_INFO_TYPE_OLD_DFID               8
> > > #define FAN_EVENT_INFO_TYPE_NEW_DFID              9
> > >
> > > Or we can go with:
> > > /* Sub-types common to all three fid info types */
> > > #define FAN_EVENT_INFO_FID_OF_OLD_DIR     1
> > > #define FAN_EVENT_INFO_FID_OF_NEW_DIR    2
> > >
> > > struct fanotify_event_info_header {
> > >        __u8 info_type;
> > >        __u8 sub_type;
> > >        __u16 len;
> > > };
> > >
> > > (as in my wip branch fanotify_fid_of)
> >
> > When we went the way of having different types for FID and DFID, I'd
> > continue with OLD_DFID_NAME, NEW_DFID_NAME, ... and keep the padding byte
> > free for now (just in case there's some extension which would urgently need
> > it).
> >
> > > We could also have FAN_RENAME require FAN_REPORT_NAME
> > > that would limit the number of info types, but I cannot find a good
> > > justification for this requirement.
> >
> > Yeah, I would not force that.
> >
> 
> On second thought and after trying to write a mental man page
> and realizing how ugly it gets, I feel strongly in favor of requiring
> FAN_REPORT_NAME for the FAN_RENAME event.
> 
> My arguments are:
> 1. What is the benefit of FAN_RENAME without names?
>     Is the knowledge that *something* was moved from dir A to dir B
>     that important that it qualifies for the extra man page noise and
>     application developer headache?
> 2. My declared motivation for this patch set was to close the last (?)
>     functional gap between inotify and fanotify, that is, being able to
>     reliably join MOVED_FROM and MOVED_TO events.
>     Requiring FAN_REPORT_NAME still meets that goal.
> 3. In this patch set, FAN_REPORT_NAME is required (for now) for
>     FAN_REPORT_TARGET_FID to reduce implementation and test
>     matrix complexity (you did not object, so I wasn't planning on
>     changing this requirement).
>     The same argument holds for FAN_RENAME
> 
> So let's say this - we can add support for OLD_DFID, NEW_DFID types
> later if we think they may serve a purpose, but at this time, I see no
> reason to complicate the UAPI anymore than it already is and I would
> rather implement only:
> 
> /* Info types for FAN_RENAME */
> #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME       10
> /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID    11 */
> #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME       12
> /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID    13 */
> 
> Do you agree?

I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited
so I'm OK with not implementing that at least for now.

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

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

> > So let's say this - we can add support for OLD_DFID, NEW_DFID types
> > later if we think they may serve a purpose, but at this time, I see no
> > reason to complicate the UAPI anymore than it already is and I would
> > rather implement only:
> >
> > /* Info types for FAN_RENAME */
> > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME       10
> > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID    11 */
> > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME       12
> > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID    13 */
> >
> > Do you agree?
>
> I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited
> so I'm OK with not implementing that at least for now.

OK. The patches are staged at [1], but I ran into one more UAPI question
that I wanted to run by you before posting the patches.

The question may be best described by the last commit on the tests branch [2]:

    syscalls/fanotify16: Test FAN_RENAME with ignored mask

    When a file is moved between two directories and only one of them is
    watching for FAN_RENAME events, the FAN_RENAME event will include only
    the information about the entry in the watched directory.

    When one of the directories is watching FAN_RENAME, but the other is
    ignoring FAN_RENAME events, the FAN_RENAME event will not be reported
    at all.

    This is not the same behavior as MOVED_FROM/TO events. User cannot
    request to ignore MOVED_FROM events according to destination directory
    nor MOVED_TO events according to source directory.

I chose this behavior because I found it to be useful and consistent with
other behaviors involving ignored masks. Maybe "chose" is a strong word
here - I did not do anything to implement this specific behavior - it is just
how the code in fanotify_group_event_mask() works for merging masks
and ignored masks of different marks.

Let me know if you approve of those ignored FAN_RENAME semantics
and I will post the patches for review.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fan_rename
[2] https://github.com/amir73il/ltp/commits/fan_rename

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Jan Kara]

On Thu 18-11-21 14:47:18, Amir Goldstein wrote:
> > > So let's say this - we can add support for OLD_DFID, NEW_DFID types
> > > later if we think they may serve a purpose, but at this time, I see no
> > > reason to complicate the UAPI anymore than it already is and I would
> > > rather implement only:
> > >
> > > /* Info types for FAN_RENAME */
> > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME       10
> > > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID    11 */
> > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME       12
> > > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID    13 */
> > >
> > > Do you agree?
> >
> > I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited
> > so I'm OK with not implementing that at least for now.
> 
> OK. The patches are staged at [1], but I ran into one more UAPI question
> that I wanted to run by you before posting the patches.
> 
> The question may be best described by the last commit on the tests branch [2]:
> 
>     syscalls/fanotify16: Test FAN_RENAME with ignored mask
> 
>     When a file is moved between two directories and only one of them is
>     watching for FAN_RENAME events, the FAN_RENAME event will include only
>     the information about the entry in the watched directory.
> 
>     When one of the directories is watching FAN_RENAME, but the other is
>     ignoring FAN_RENAME events, the FAN_RENAME event will not be reported
>     at all.
> 
>     This is not the same behavior as MOVED_FROM/TO events. User cannot
>     request to ignore MOVED_FROM events according to destination directory
>     nor MOVED_TO events according to source directory.
> 
> I chose this behavior because I found it to be useful and consistent with
> other behaviors involving ignored masks. Maybe "chose" is a strong word
> here - I did not do anything to implement this specific behavior - it is just
> how the code in fanotify_group_event_mask() works for merging masks
> and ignored masks of different marks.
> 
> Let me know if you approve of those ignored FAN_RENAME semantics
> and I will post the patches for review.

Yeah, I guess ignore masks with FAN_RENAME are going to be a bit
non-intuitive either way and what you suggest makes sense. I suppose when
SB has FAN_RENAME mark and either source or target have FAN_RENAME in the
ignore mask, nothing will get reported as well, do it? If we are consistent
like this, I guess fine by me.

								Honza

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

Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein]

On Thu, Nov 18, 2021 at 6:29 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 18-11-21 14:47:18, Amir Goldstein wrote:
> > > > So let's say this - we can add support for OLD_DFID, NEW_DFID types
> > > > later if we think they may serve a purpose, but at this time, I see no
> > > > reason to complicate the UAPI anymore than it already is and I would
> > > > rather implement only:
> > > >
> > > > /* Info types for FAN_RENAME */
> > > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME       10
> > > > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID    11 */
> > > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME       12
> > > > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID    13 */
> > > >
> > > > Do you agree?
> > >
> > > I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited
> > > so I'm OK with not implementing that at least for now.
> >
> > OK. The patches are staged at [1], but I ran into one more UAPI question
> > that I wanted to run by you before posting the patches.
> >
> > The question may be best described by the last commit on the tests branch [2]:
> >
> >     syscalls/fanotify16: Test FAN_RENAME with ignored mask
> >
> >     When a file is moved between two directories and only one of them is
> >     watching for FAN_RENAME events, the FAN_RENAME event will include only
> >     the information about the entry in the watched directory.
> >
> >     When one of the directories is watching FAN_RENAME, but the other is
> >     ignoring FAN_RENAME events, the FAN_RENAME event will not be reported
> >     at all.
> >
> >     This is not the same behavior as MOVED_FROM/TO events. User cannot
> >     request to ignore MOVED_FROM events according to destination directory
> >     nor MOVED_TO events according to source directory.
> >
> > I chose this behavior because I found it to be useful and consistent with
> > other behaviors involving ignored masks. Maybe "chose" is a strong word
> > here - I did not do anything to implement this specific behavior - it is just
> > how the code in fanotify_group_event_mask() works for merging masks
> > and ignored masks of different marks.
> >
> > Let me know if you approve of those ignored FAN_RENAME semantics
> > and I will post the patches for review.
>
> Yeah, I guess ignore masks with FAN_RENAME are going to be a bit
> non-intuitive either way and what you suggest makes sense. I suppose when
> SB has FAN_RENAME mark and either source or target have FAN_RENAME in the
> ignore mask, nothing will get reported as well, do it? If we are consistent
> like this, I guess fine by me.

Yes, that is correct, because the join of combined masks and combined
ignored_masks is done at the very end, if an event is in ignored mask of
any related mark, it will cause the drop of the event.

I will post patches soon.

Thanks,
Amir.