[PATCH 0/2] New fanotify API for ignoring events

[PATCH 0/2] New fanotify API for ignoring events

[Amir Goldstein]

Hi Jan,

As we discussed [1], here is the implementation of the new
FAN_MARK_IGNORE API, to try and sort the historic mess of
FAN_MARK_IGNORED_MASK.

It is worth mentioning that the new API enables the functionality of
watching events ONLY on directories (by ignoring events on non-dir).

LTP tests [2] and man page draft [3] are ready as well.

I am going on vacation in two weeks time, so wanted to
send these out early.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20220428123824.ssq72ovqg2nao5f4@quack3.lan/
[2] https://github.com/amir73il/ltp/commits/fan_mark_ignore
[3] https://github.com/amir73il/man-pages/commits/fan_mark_ignore

Amir Goldstein (2):
  fanotify: prepare for setting event flags in ignore mask
  fanotify: introduce FAN_MARK_IGNORE

 fs/notify/fanotify/fanotify.c      | 17 +++++----
 fs/notify/fanotify/fanotify.h      |  2 ++
 fs/notify/fanotify/fanotify_user.c | 55 +++++++++++++++++++++---------
 fs/notify/fdinfo.c                 |  6 ++--
 fs/notify/fsnotify.c               | 21 +++++++-----
 include/linux/fanotify.h           |  5 ++-
 include/linux/fsnotify_backend.h   | 46 +++++++++++++++++++++----
 include/uapi/linux/fanotify.h      |  2 ++
 8 files changed, 113 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Amir Goldstein]

Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
ignore mask is always implicitly applied to events on directories.

Define a mark flag that replaces this legacy behavior with logic of
applying the ignore mask according to event flags in ignore mask.

Implement the new logic to prepare for supporting an ignore mask that
ignores events on children and ignore mask that does not ignore events
on directories.

To emphasize the change in terminology, also rename ignored_mask mark
member to ignore_mask and use accessor to get only ignored events or
events and flags.

This change in terminology finally aligns with the "ignore mask"
language in man pages and in most of the comments.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 17 +++++++----
 fs/notify/fanotify/fanotify_user.c | 21 ++++++++------
 fs/notify/fdinfo.c                 |  6 ++--
 fs/notify/fsnotify.c               | 21 ++++++++------
 include/linux/fsnotify_backend.h   | 46 ++++++++++++++++++++++++++----
 5 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 4f897e109547..a641e597b11c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -295,12 +295,13 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 				     const void *data, int data_type,
 				     struct inode *dir)
 {
-	__u32 marks_mask = 0, marks_ignored_mask = 0;
+	__u32 marks_mask = 0, marks_ignore_mask = 0;
 	__u32 test_mask, user_mask = FANOTIFY_OUTGOING_EVENTS |
 				     FANOTIFY_EVENT_FLAGS;
 	const struct path *path = fsnotify_data_path(data, data_type);
 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
 	struct fsnotify_mark *mark;
+	bool ondir = event_mask & FAN_ONDIR;
 	int type;
 
 	pr_debug("%s: report_mask=%x mask=%x data=%p data_type=%d\n",
@@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 			return 0;
 	} else if (!(fid_mode & FAN_REPORT_FID)) {
 		/* Do we have a directory inode to report? */
-		if (!dir && !(event_mask & FS_ISDIR))
+		if (!dir && !ondir)
 			return 0;
 	}
 
 	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
-		/* Apply ignore mask regardless of mark's ISDIR flag */
-		marks_ignored_mask |= mark->ignored_mask;
+		/*
+		 * Apply ignore mask depending on whether FAN_ONDIR flag in
+		 * ignore mask should be checked to ignore events on dirs.
+		 */
+		if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
+			marks_ignore_mask |= mark->ignore_mask;
 
 		/*
 		 * If the event is on dir and this mark doesn't care about
 		 * events on dir, don't send it!
 		 */
-		if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
+		if (ondir && !(mark->mask & FAN_ONDIR))
 			continue;
 
 		marks_mask |= mark->mask;
@@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		*match_mask |= 1U << type;
 	}
 
-	test_mask = event_mask & marks_mask & ~marks_ignored_mask;
+	test_mask = event_mask & marks_mask & ~marks_ignore_mask;
 
 	/*
 	 * For dirent modification events (create/delete/move) that do not carry
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c2255b440df9..b718df84bd56 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1012,7 +1012,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
 		fsn_mark->mask &= ~mask;
 	} else {
-		fsn_mark->ignored_mask &= ~mask;
+		fsn_mark->ignore_mask &= ~mask;
 	}
 	newmask = fsnotify_calc_mask(fsn_mark);
 	/*
@@ -1021,7 +1021,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 	 * changes to the mask.
 	 * Destroy mark when only umask bits remain.
 	 */
-	*destroy = !((fsn_mark->mask | fsn_mark->ignored_mask) & ~umask);
+	*destroy = !((fsn_mark->mask | fsn_mark->ignore_mask) & ~umask);
 	spin_unlock(&fsn_mark->lock);
 
 	return oldmask & ~newmask;
@@ -1090,7 +1090,7 @@ static bool fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
 	/*
 	 * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to
 	 * the removal of the FS_MODIFY bit in calculated mask if it was set
-	 * because of an ignored mask that is now going to survive FS_MODIFY.
+	 * because of an ignore mask that is now going to survive FS_MODIFY.
 	 */
 	if ((fan_flags & FAN_MARK_IGNORED_MASK) &&
 	    (fan_flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
@@ -1123,7 +1123,7 @@ static bool fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 	if (!(fan_flags & FAN_MARK_IGNORED_MASK))
 		fsn_mark->mask |= mask;
 	else
-		fsn_mark->ignored_mask |= mask;
+		fsn_mark->ignore_mask |= mask;
 
 	recalc = fsnotify_calc_mask(fsn_mark) &
 		~fsnotify_conn_mask(fsn_mark->connector);
@@ -1261,7 +1261,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 
 	/*
 	 * If some other task has this inode open for write we should not add
-	 * an ignored mark, unless that ignored mark is supposed to survive
+	 * an ignore mask, unless that ignore mask is supposed to survive
 	 * modification changes anyway.
 	 */
 	if ((flags & FAN_MARK_IGNORED_MASK) &&
@@ -1540,7 +1540,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	__kernel_fsid_t __fsid, *fsid = NULL;
 	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
-	bool ignored = flags & FAN_MARK_IGNORED_MASK;
+	bool ignore = flags & FAN_MARK_IGNORED_MASK;
 	unsigned int obj_type, fid_mode;
 	u32 umask = 0;
 	int ret;
@@ -1589,8 +1589,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	if (mask & ~valid_mask)
 		return -EINVAL;
 
-	/* Event flags (ONDIR, ON_CHILD) are meaningless in ignored mask */
-	if (ignored)
+	/*
+	 * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
+	 * FAN_MARK_IGNORED_MASK.
+	 */
+	if (ignore)
 		mask &= ~FANOTIFY_EVENT_FLAGS;
 
 	f = fdget(fanotify_fd);
@@ -1717,7 +1720,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		 * events with parent/name info for non-directory.
 		 */
 		if ((fid_mode & FAN_REPORT_DIR_FID) &&
-		    (flags & FAN_MARK_ADD) && !ignored)
+		    (flags & FAN_MARK_ADD) && !ignore)
 			mask |= FAN_EVENT_ON_CHILD;
 	}
 
diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 59fb40abe33d..55081ae3a6ec 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -113,7 +113,7 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 			return;
 		seq_printf(m, "fanotify ino:%lx sdev:%x mflags:%x mask:%x ignored_mask:%x ",
 			   inode->i_ino, inode->i_sb->s_dev,
-			   mflags, mark->mask, mark->ignored_mask);
+			   mflags, mark->mask, mark->ignore_mask);
 		show_mark_fhandle(m, inode);
 		seq_putc(m, '\n');
 		iput(inode);
@@ -121,12 +121,12 @@ static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		struct mount *mnt = fsnotify_conn_mount(mark->connector);
 
 		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
-			   mnt->mnt_id, mflags, mark->mask, mark->ignored_mask);
+			   mnt->mnt_id, mflags, mark->mask, mark->ignore_mask);
 	} else if (mark->connector->type == FSNOTIFY_OBJ_TYPE_SB) {
 		struct super_block *sb = fsnotify_conn_sb(mark->connector);
 
 		seq_printf(m, "fanotify sdev:%x mflags:%x mask:%x ignored_mask:%x\n",
-			   sb->s_dev, mflags, mark->mask, mark->ignored_mask);
+			   sb->s_dev, mflags, mark->mask, mark->ignore_mask);
 	}
 }
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 0b3e74935cb4..b5b61f94cec1 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -324,7 +324,7 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 	struct fsnotify_group *group = NULL;
 	__u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 	__u32 marks_mask = 0;
-	__u32 marks_ignored_mask = 0;
+	__u32 marks_ignore_mask = 0;
 	struct fsnotify_mark *mark;
 	int type;
 
@@ -336,7 +336,7 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 		fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
 			if (!(mark->flags &
 			      FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
-				mark->ignored_mask = 0;
+				mark->ignore_mask = 0;
 		}
 	}
 
@@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
 		group = mark->group;
 		marks_mask |= mark->mask;
-		marks_ignored_mask |= mark->ignored_mask;
+		if (!(mask & FS_ISDIR) ||
+		    (fsnotify_ignore_mask(mark) & FS_ISDIR))
+			marks_ignore_mask |= mark->ignore_mask;
 	}
 
-	pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
-		 __func__, group, mask, marks_mask, marks_ignored_mask,
+	pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
+		 __func__, group, mask, marks_mask, marks_ignore_mask,
 		 data, data_type, dir, cookie);
 
-	if (!(test_mask & marks_mask & ~marks_ignored_mask))
+	if (!(test_mask & marks_mask & ~marks_ignore_mask))
 		return 0;
 
 	if (group->ops->handle_event) {
@@ -423,7 +425,8 @@ static bool fsnotify_iter_select_report_types(
 			 * But is *this mark* watching children?
 			 */
 			if (type == FSNOTIFY_ITER_TYPE_PARENT &&
-			    !(mark->mask & FS_EVENT_ON_CHILD))
+			    !(mark->mask & FS_EVENT_ON_CHILD) &&
+			    !(fsnotify_ignore_mask(mark) & FS_EVENT_ON_CHILD))
 				continue;
 
 			fsnotify_iter_set_report_type(iter_info, type);
@@ -532,8 +535,8 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 
 
 	/*
-	 * If this is a modify event we may need to clear some ignored masks.
-	 * In that case, the object with ignored masks will have the FS_MODIFY
+	 * If this is a modify event we may need to clear some ignore masks.
+	 * In that case, the object with ignore masks will have the FS_MODIFY
 	 * event in its mask.
 	 * Otherwise, return if none of the marks care about this type of event.
 	 */
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9560734759fa..894e1b1c18bd 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -518,8 +518,8 @@ struct fsnotify_mark {
 	struct hlist_node obj_list;
 	/* Head of list of marks for an object [mark ref] */
 	struct fsnotify_mark_connector *connector;
-	/* Events types to ignore [mark->lock, group->mark_mutex] */
-	__u32 ignored_mask;
+	/* Events types and flags to ignore [mark->lock, group->mark_mutex] */
+	__u32 ignore_mask;
 	/* General fsnotify mark flags */
 #define FSNOTIFY_MARK_FLAG_ALIVE		0x0001
 #define FSNOTIFY_MARK_FLAG_ATTACHED		0x0002
@@ -529,6 +529,7 @@ struct fsnotify_mark {
 	/* fanotify mark flags */
 #define FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY	0x0100
 #define FSNOTIFY_MARK_FLAG_NO_IREF		0x0200
+#define FSNOTIFY_MARK_FLAG_IGNORE_FLAGS		0x0400
 	unsigned int flags;		/* flags [mark->lock] */
 };
 
@@ -655,15 +656,48 @@ extern void fsnotify_remove_queued_event(struct fsnotify_group *group,
 
 /* functions used to manipulate the marks attached to inodes */
 
-/* Get mask for calculating object interest taking ignored mask into account */
+/*
+ * Canonical "ignore mask" including event flags.
+ * Note the subtle semantic difference from the legacy term "ignored_mask" -
+ * ignored_mask traditionally only meant which events should be ignored,
+ * while ignore_mask also includes flags regarding the type of objects on
+ * which events should be ignored.
+ */
+static inline __u32 fsnotify_ignore_mask(struct fsnotify_mark *mark)
+{
+	__u32 ignore_mask = mark->ignore_mask;
+
+	/* The event flags in ignore mask take effect */
+	if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORE_FLAGS)
+		return ignore_mask;
+
+	/*
+	 * Legacy behavior:
+	 * - Always ignore events on dir
+	 * - Ignore events on child if parent is watching children
+	 */
+	ignore_mask |= FS_ISDIR;
+	ignore_mask &= ~FS_EVENT_ON_CHILD;
+	ignore_mask |= mark->mask & FS_EVENT_ON_CHILD;
+	return ignore_mask;
+}
+
+/* Legacy ignored_mask - only event types to ignore */
+static inline __u32 fsnotify_ignored_events(struct fsnotify_mark *mark)
+{
+	return mark->ignore_mask & ALL_FSNOTIFY_EVENTS;
+}
+
+/* Get mask for calculating object interest taking ignore mask into account */
 static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 {
 	__u32 mask = mark->mask;
+	__u32 ignored_events = fsnotify_ignored_events(mark);
 
-	if (!mark->ignored_mask)
+	if (!ignored_events)
 		return mask;
 
-	/* Interest in FS_MODIFY may be needed for clearing ignored mask */
+	/* Interest in FS_MODIFY may be needed for clearing ignore mask */
 	if (!(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
 		mask |= FS_MODIFY;
 
@@ -671,7 +705,7 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 	 * If mark is interested in ignoring events on children, the object must
 	 * show interest in those events for fsnotify_parent() to notice it.
 	 */
-	return mask | (mark->ignored_mask & ALL_FSNOTIFY_EVENTS);
+	return mask | mark->ignore_mask;
 }
 
 /* Get mask of events for a list of marks */
-- 
2.25.1

[PATCH 2/2] fanotify: introduce FAN_MARK_IGNORE

[Amir Goldstein]

This flag is a new way to configure ignore mask which allows adding and
removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask.

The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on
directories and would ignore events on children depending on whether
the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask.

FAN_MARK_IGNORE can be used to ignore events on children without setting
FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on
directories unconditionally, only when FAN_ONDIR is set in ignore mask.

The new behavior is sticky.  After calling fanotify_mark() with
FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
will update the ignore mask, but will not change the event flags in
ignore mask nor how these flags are treated.

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

diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 80e0ec95b113..c54d0b404373 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -499,6 +499,8 @@ static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark)
 		mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
 	if (mark->flags & FSNOTIFY_MARK_FLAG_NO_IREF)
 		mflags |= FAN_MARK_EVICTABLE;
+	if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORE_FLAGS)
+		mflags |= FAN_MARK_IGNORE;
 
 	return mflags;
 }
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index b718df84bd56..07876d6c1dbc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1009,7 +1009,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 	mask &= ~umask;
 	spin_lock(&fsn_mark->lock);
 	oldmask = fsnotify_calc_mask(fsn_mark);
-	if (!(flags & FAN_MARK_IGNORED_MASK)) {
+	if (!(flags & FANOTIFY_MARK_IGNORE_BITS)) {
 		fsn_mark->mask &= ~mask;
 	} else {
 		fsn_mark->ignore_mask &= ~mask;
@@ -1085,15 +1085,24 @@ static bool fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
 				       unsigned int fan_flags)
 {
 	bool want_iref = !(fan_flags & FAN_MARK_EVICTABLE);
+	unsigned int ignore = fan_flags & FANOTIFY_MARK_IGNORE_BITS;
 	bool recalc = false;
 
+	/*
+	 * When using FAN_MARK_IGNORE for the first time, mark starts using
+	 * independent event flags in ignore mask.  After that, updating the
+	 * ignore mask with FAN_MARK_IGNORED_MASK works, but does not change
+	 * the flags nor the behavior.
+	 */
+	if (ignore == FAN_MARK_IGNORE)
+		fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORE_FLAGS;
+
 	/*
 	 * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to
 	 * the removal of the FS_MODIFY bit in calculated mask if it was set
 	 * because of an ignore mask that is now going to survive FS_MODIFY.
 	 */
-	if ((fan_flags & FAN_MARK_IGNORED_MASK) &&
-	    (fan_flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
+	if (ignore && (fan_flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
 	    !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) {
 		fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
 		if (!(fsn_mark->mask & FS_MODIFY))
@@ -1120,7 +1129,7 @@ static bool fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 	bool recalc;
 
 	spin_lock(&fsn_mark->lock);
-	if (!(fan_flags & FAN_MARK_IGNORED_MASK))
+	if (!(fan_flags & FANOTIFY_MARK_IGNORE_BITS))
 		fsn_mark->mask |= mask;
 	else
 		fsn_mark->ignore_mask |= mask;
@@ -1220,7 +1229,8 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 	 * Error events are pre-allocated per group, only if strictly
 	 * needed (i.e. FAN_FS_ERROR was requested).
 	 */
-	if (!(fan_flags & FAN_MARK_IGNORED_MASK) && (mask & FAN_FS_ERROR)) {
+	if (!(fan_flags & FANOTIFY_MARK_IGNORE_BITS) &&
+	    (mask & FAN_FS_ERROR)) {
 		ret = fanotify_group_init_error_pool(group);
 		if (ret)
 			goto out;
@@ -1264,7 +1274,7 @@ static int fanotify_add_inode_mark(struct fsnotify_group *group,
 	 * an ignore mask, unless that ignore mask is supposed to survive
 	 * modification changes anyway.
 	 */
-	if ((flags & FAN_MARK_IGNORED_MASK) &&
+	if ((flags & FANOTIFY_MARK_IGNORE_BITS) &&
 	    !(flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
 	    inode_is_open_for_write(inode))
 		return 0;
@@ -1540,7 +1550,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	__kernel_fsid_t __fsid, *fsid = NULL;
 	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
-	bool ignore = flags & FAN_MARK_IGNORED_MASK;
+	unsigned int ignore = flags & FANOTIFY_MARK_IGNORE_BITS;
 	unsigned int obj_type, fid_mode;
 	u32 umask = 0;
 	int ret;
@@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 
 	/*
 	 * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
-	 * FAN_MARK_IGNORED_MASK.
+	 * FAN_MARK_IGNORED_MASK.  They can be updated in ignore mask with
+	 * FAN_MARK_IGNORE and then they do take effect.
 	 */
-	if (ignore)
+	switch (ignore) {
+	case 0:
+	case FAN_MARK_IGNORE:
+		break;
+	case FAN_MARK_IGNORED_MASK:
 		mask &= ~FANOTIFY_EVENT_FLAGS;
+		umask = FANOTIFY_EVENT_FLAGS;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	f = fdget(fanotify_fd);
 	if (unlikely(!f.file))
@@ -1803,7 +1823,7 @@ static int __init fanotify_user_setup(void)
 
 	BUILD_BUG_ON(FANOTIFY_INIT_FLAGS & FANOTIFY_INTERNAL_GROUP_FLAGS);
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 12);
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 10);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 11);
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
 					 SLAB_PANIC|SLAB_ACCOUNT);
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index edc28555814c..f32893942fd7 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -59,12 +59,15 @@
 #define FANOTIFY_MARK_TYPE_BITS	(FAN_MARK_INODE | FAN_MARK_MOUNT | \
 				 FAN_MARK_FILESYSTEM)
 
+#define FANOTIFY_MARK_IGNORE_BITS (FAN_MARK_IGNORED_MASK | \
+				   FAN_MARK_IGNORE)
+
 #define FANOTIFY_MARK_FLAGS	(FANOTIFY_MARK_TYPE_BITS | \
+				 FANOTIFY_MARK_IGNORE_BITS | \
 				 FAN_MARK_ADD | \
 				 FAN_MARK_REMOVE | \
 				 FAN_MARK_DONT_FOLLOW | \
 				 FAN_MARK_ONLYDIR | \
-				 FAN_MARK_IGNORED_MASK | \
 				 FAN_MARK_IGNORED_SURV_MODIFY | \
 				 FAN_MARK_EVICTABLE | \
 				 FAN_MARK_FLUSH)
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index f1f89132d60e..3efe417f2282 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -83,6 +83,8 @@
 #define FAN_MARK_FLUSH		0x00000080
 /* FAN_MARK_FILESYSTEM is	0x00000100 */
 #define FAN_MARK_EVICTABLE	0x00000200
+/* This bit is mutually exclusive with FAN_MARK_IGNORED_MASK bit */
+#define FAN_MARK_IGNORE		0x00000400
 
 /* These are NOT bitwise flags.  Both bits can be used togther.  */
 #define FAN_MARK_INODE		0x00000000
-- 
2.25.1

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Jan Kara]

On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> ignore mask is always implicitly applied to events on directories.
> 
> Define a mark flag that replaces this legacy behavior with logic of
> applying the ignore mask according to event flags in ignore mask.
> 
> Implement the new logic to prepare for supporting an ignore mask that
> ignores events on children and ignore mask that does not ignore events
> on directories.
> 
> To emphasize the change in terminology, also rename ignored_mask mark
> member to ignore_mask and use accessor to get only ignored events or
> events and flags.
> 
> This change in terminology finally aligns with the "ignore mask"
> language in man pages and in most of the comments.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks mostly good to me. Just one question / suggestion: You are
introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events().
So shouldn't we be using these helpers as much as possible throughout the
code? Because in several places I had to check the code around whether
using mark->ignore_mask directly is actually fine. In particular:

> @@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  			return 0;
>  	} else if (!(fid_mode & FAN_REPORT_FID)) {
>  		/* Do we have a directory inode to report? */
> -		if (!dir && !(event_mask & FS_ISDIR))
> +		if (!dir && !ondir)
>  			return 0;
>  	}
>  
>  	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> -		/* Apply ignore mask regardless of mark's ISDIR flag */
> -		marks_ignored_mask |= mark->ignored_mask;
> +		/*
> +		 * Apply ignore mask depending on whether FAN_ONDIR flag in
> +		 * ignore mask should be checked to ignore events on dirs.
> +		 */
> +		if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
> +			marks_ignore_mask |= mark->ignore_mask;
>  
>  		/*
>  		 * If the event is on dir and this mark doesn't care about
>  		 * events on dir, don't send it!
>  		 */
> -		if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
> +		if (ondir && !(mark->mask & FAN_ONDIR))
>  			continue;
>  
>  		marks_mask |= mark->mask;

So for example here I'm wondering whether a helper should not be used...

> @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  		*match_mask |= 1U << type;
>  	}
>  
> -	test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> +	test_mask = event_mask & marks_mask & ~marks_ignore_mask;

Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
marks_ignore_mask it can result in clearing this flag in the returned
'mask' which is likely not what we want if there are some events left
unignored in the 'mask'?
  
> @@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
>  	fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
>  		group = mark->group;
>  		marks_mask |= mark->mask;
> -		marks_ignored_mask |= mark->ignored_mask;
> +		if (!(mask & FS_ISDIR) ||
> +		    (fsnotify_ignore_mask(mark) & FS_ISDIR))
> +			marks_ignore_mask |= mark->ignore_mask;
>  	}
>  
> -	pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> -		 __func__, group, mask, marks_mask, marks_ignored_mask,
> +	pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> +		 __func__, group, mask, marks_mask, marks_ignore_mask,
>  		 data, data_type, dir, cookie);
>  
> -	if (!(test_mask & marks_mask & ~marks_ignored_mask))
> +	if (!(test_mask & marks_mask & ~marks_ignore_mask))
>  		return 0;

And I'm wondering about similar things here...

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

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Jan Kara]

On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> ignore mask is always implicitly applied to events on directories.
> 
> Define a mark flag that replaces this legacy behavior with logic of
> applying the ignore mask according to event flags in ignore mask.
> 
> Implement the new logic to prepare for supporting an ignore mask that
> ignores events on children and ignore mask that does not ignore events
> on directories.
> 
> To emphasize the change in terminology, also rename ignored_mask mark
> member to ignore_mask and use accessor to get only ignored events or
> events and flags.
> 
> This change in terminology finally aligns with the "ignore mask"
> language in man pages and in most of the comments.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

..

> @@ -423,7 +425,8 @@ static bool fsnotify_iter_select_report_types(
>  			 * But is *this mark* watching children?
>  			 */
>  			if (type == FSNOTIFY_ITER_TYPE_PARENT &&
> -			    !(mark->mask & FS_EVENT_ON_CHILD))
> +			    !(mark->mask & FS_EVENT_ON_CHILD) &&
> +			    !(fsnotify_ignore_mask(mark) & FS_EVENT_ON_CHILD))
>  				continue;

So now we have in ->report_mask the FSNOTIFY_ITER_TYPE_PARENT if either
->mask or ->ignore_mask have FS_EVENT_ON_CHILD set. But I see nothing that
would stop us from applying say ->mask to the set of events we are
interested in if FS_EVENT_ON_CHILD is set only in ->ignore_mask? And
there's the same problem in the other direction as well. Am I missing
something?

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

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Amir Goldstein]

On Wed, Jun 22, 2022 at 7:00 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > ignore mask is always implicitly applied to events on directories.
> >
> > Define a mark flag that replaces this legacy behavior with logic of
> > applying the ignore mask according to event flags in ignore mask.
> >
> > Implement the new logic to prepare for supporting an ignore mask that
> > ignores events on children and ignore mask that does not ignore events
> > on directories.
> >
> > To emphasize the change in terminology, also rename ignored_mask mark
> > member to ignore_mask and use accessor to get only ignored events or
> > events and flags.
> >
> > This change in terminology finally aligns with the "ignore mask"
> > language in man pages and in most of the comments.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> ..
>
> > @@ -423,7 +425,8 @@ static bool fsnotify_iter_select_report_types(
> >                        * But is *this mark* watching children?
> >                        */
> >                       if (type == FSNOTIFY_ITER_TYPE_PARENT &&
> > -                         !(mark->mask & FS_EVENT_ON_CHILD))
> > +                         !(mark->mask & FS_EVENT_ON_CHILD) &&
> > +                         !(fsnotify_ignore_mask(mark) & FS_EVENT_ON_CHILD))
> >                               continue;
>
> So now we have in ->report_mask the FSNOTIFY_ITER_TYPE_PARENT if either
> ->mask or ->ignore_mask have FS_EVENT_ON_CHILD set. But I see nothing that
> would stop us from applying say ->mask to the set of events we are
> interested in if FS_EVENT_ON_CHILD is set only in ->ignore_mask? And

I think I spent some time thinking about this and came to a conclusion that
1. It is hard to get all the cases right
2. It is a micro optimization

The implication is that the user can set an ignore mask of an object, get no
events but still cause performance penalty. Right?
So just don't do that...

It is easier to maintain the code with the rule that an "interest" in the object
is either positive (I want this event) or negative (I don't want this event).
If the user has no interest, the user should set nothing in the mark.

Do you agree with me that the added complexity would not be worth it?

> there's the same problem in the other direction as well. Am I missing

Other direction?

Thanks,
Amir.

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Amir Goldstein]

On Wed, Jun 22, 2022 at 6:52 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > ignore mask is always implicitly applied to events on directories.
> >
> > Define a mark flag that replaces this legacy behavior with logic of
> > applying the ignore mask according to event flags in ignore mask.
> >
> > Implement the new logic to prepare for supporting an ignore mask that
> > ignores events on children and ignore mask that does not ignore events
> > on directories.
> >
> > To emphasize the change in terminology, also rename ignored_mask mark
> > member to ignore_mask and use accessor to get only ignored events or
> > events and flags.
> >
> > This change in terminology finally aligns with the "ignore mask"
> > language in man pages and in most of the comments.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks mostly good to me. Just one question / suggestion: You are
> introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events().
> So shouldn't we be using these helpers as much as possible throughout the
> code? Because in several places I had to check the code around whether
> using mark->ignore_mask directly is actually fine. In particular:
>
> > @@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >                       return 0;
> >       } else if (!(fid_mode & FAN_REPORT_FID)) {
> >               /* Do we have a directory inode to report? */
> > -             if (!dir && !(event_mask & FS_ISDIR))
> > +             if (!dir && !ondir)
> >                       return 0;
> >       }
> >
> >       fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> > -             /* Apply ignore mask regardless of mark's ISDIR flag */
> > -             marks_ignored_mask |= mark->ignored_mask;
> > +             /*
> > +              * Apply ignore mask depending on whether FAN_ONDIR flag in
> > +              * ignore mask should be checked to ignore events on dirs.
> > +              */
> > +             if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
> > +                     marks_ignore_mask |= mark->ignore_mask;
> >
> >               /*
> >                * If the event is on dir and this mark doesn't care about
> >                * events on dir, don't send it!
> >                */
> > -             if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
> > +             if (ondir && !(mark->mask & FAN_ONDIR))
> >                       continue;
> >
> >               marks_mask |= mark->mask;
>
> So for example here I'm wondering whether a helper should not be used...
>
> > @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >               *match_mask |= 1U << type;
> >       }
> >
> > -     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > +     test_mask = event_mask & marks_mask & ~marks_ignore_mask;
>
> Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
> marks_ignore_mask it can result in clearing this flag in the returned
> 'mask' which is likely not what we want if there are some events left
> unignored in the 'mask'?
>
> > @@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
> >       fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> >               group = mark->group;
> >               marks_mask |= mark->mask;
> > -             marks_ignored_mask |= mark->ignored_mask;
> > +             if (!(mask & FS_ISDIR) ||
> > +                 (fsnotify_ignore_mask(mark) & FS_ISDIR))
> > +                     marks_ignore_mask |= mark->ignore_mask;
> >       }
> >
> > -     pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > -              __func__, group, mask, marks_mask, marks_ignored_mask,
> > +     pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > +              __func__, group, mask, marks_mask, marks_ignore_mask,
> >                data, data_type, dir, cookie);
> >
> > -     if (!(test_mask & marks_mask & ~marks_ignored_mask))
> > +     if (!(test_mask & marks_mask & ~marks_ignore_mask))
> >               return 0;
>
> And I'm wondering about similar things here...
>

I can't remember if I left those cases on purpose.
I will check if it makes sense to use a macro here.

Amir.

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Jan Kara]

On Wed 22-06-22 21:28:23, Amir Goldstein wrote:
> On Wed, Jun 22, 2022 at 7:00 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > > ignore mask is always implicitly applied to events on directories.
> > >
> > > Define a mark flag that replaces this legacy behavior with logic of
> > > applying the ignore mask according to event flags in ignore mask.
> > >
> > > Implement the new logic to prepare for supporting an ignore mask that
> > > ignores events on children and ignore mask that does not ignore events
> > > on directories.
> > >
> > > To emphasize the change in terminology, also rename ignored_mask mark
> > > member to ignore_mask and use accessor to get only ignored events or
> > > events and flags.
> > >
> > > This change in terminology finally aligns with the "ignore mask"
> > > language in man pages and in most of the comments.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > ..
> >
> > > @@ -423,7 +425,8 @@ static bool fsnotify_iter_select_report_types(
> > >                        * But is *this mark* watching children?
> > >                        */
> > >                       if (type == FSNOTIFY_ITER_TYPE_PARENT &&
> > > -                         !(mark->mask & FS_EVENT_ON_CHILD))
> > > +                         !(mark->mask & FS_EVENT_ON_CHILD) &&
> > > +                         !(fsnotify_ignore_mask(mark) & FS_EVENT_ON_CHILD))
> > >                               continue;
> >
> > So now we have in ->report_mask the FSNOTIFY_ITER_TYPE_PARENT if either
> > ->mask or ->ignore_mask have FS_EVENT_ON_CHILD set. But I see nothing that
> > would stop us from applying say ->mask to the set of events we are
> > interested in if FS_EVENT_ON_CHILD is set only in ->ignore_mask? And
> 
> I think I spent some time thinking about this and came to a conclusion that
> 1. It is hard to get all the cases right
> 2. It is a micro optimization
> 
> The implication is that the user can set an ignore mask of an object, get no
> events but still cause performance penalty. Right?
> So just don't do that...

So I was more afraid that this actually results in generating events we
should not generate. For example consider dir 'd' with mask FS_OPEN and
ignore_mask FS_MODIFY | FS_EVENT_ON_CHILD. Now open("d/file") happens so
FS_OPEN is generated for d/file. We select FSNOTIFY_ITER_TYPE_PARENT in the
->report_mask because of the ignore_mask on 'd' and pass the iter to
fanotify_handle_event(). There fanotify_group_event_mask() will include
FS_OPEN to marks_mask and conclude event should be reported. But there's no
mark that should result in reporting this...

The problem is that with the introduction of FSNOTIFY_ITER_TYPE_PARENT we
started to rely on that type being set only when the event on child should
be reported to parent and now you break that AFAICT.

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

Re: [PATCH 2/2] fanotify: introduce FAN_MARK_IGNORE

[Jan Kara]

On Mon 20-06-22 16:45:51, Amir Goldstein wrote:
> This flag is a new way to configure ignore mask which allows adding and
> removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask.
> 
> The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on
> directories and would ignore events on children depending on whether
> the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask.
> 
> FAN_MARK_IGNORE can be used to ignore events on children without setting
> FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on
> directories unconditionally, only when FAN_ONDIR is set in ignore mask.
> 
> The new behavior is sticky.  After calling fanotify_mark() with
> FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
> will update the ignore mask, but will not change the event flags in
> ignore mask nor how these flags are treated.

IMHO this stickyness is not very obvious. Wouldn't it be less error-prone
for users to say that once FAN_MARK_IGNORE is used for a mark, all
subsequent modifications of ignore mask have to use FAN_MARK_IGNORE? I mean
if some program bothers with FAN_MARK_IGNORE, I'd expect it to use it for
all its calls as otherwise the mixup is kind of difficult to reason
about...

Also it follows the behavior we have picked for FAN_MARK_EVICTABLE AFAIR
but that's not really important to me.

> @@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  
>  	/*
>  	 * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
> -	 * FAN_MARK_IGNORED_MASK.
> +	 * FAN_MARK_IGNORED_MASK.  They can be updated in ignore mask with
> +	 * FAN_MARK_IGNORE and then they do take effect.
>  	 */
> -	if (ignore)
> +	switch (ignore) {
> +	case 0:
> +	case FAN_MARK_IGNORE:
> +		break;
> +	case FAN_MARK_IGNORED_MASK:
>  		mask &= ~FANOTIFY_EVENT_FLAGS;
> +		umask = FANOTIFY_EVENT_FLAGS;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

I think this would be easier to follow as two ifs:

	/* We don't allow FAN_MARK_IGNORE & FAN_MARK_IGNORED_MASK together */
	if (ignore == FAN_MARK_IGNORE | FAN_MARK_IGNORED_MASK)
		return -EINVAL;
	/*
	 * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
	 * FAN_MARK_IGNORED_MASK.
	 */
	if (ignore == FAN_MARK_IGNORED_MASK) {
  		mask &= ~FANOTIFY_EVENT_FLAGS;
		umask = FANOTIFY_EVENT_FLAGS;
	}

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

Re: [PATCH 2/2] fanotify: introduce FAN_MARK_IGNORE

[Amir Goldstein]

On Thu, Jun 23, 2022 at 1:14 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 20-06-22 16:45:51, Amir Goldstein wrote:
> > This flag is a new way to configure ignore mask which allows adding and
> > removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask.
> >
> > The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on
> > directories and would ignore events on children depending on whether
> > the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask.
> >
> > FAN_MARK_IGNORE can be used to ignore events on children without setting
> > FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on
> > directories unconditionally, only when FAN_ONDIR is set in ignore mask.
> >
> > The new behavior is sticky.  After calling fanotify_mark() with
> > FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
> > will update the ignore mask, but will not change the event flags in
> > ignore mask nor how these flags are treated.
>
> IMHO this stickyness is not very obvious. Wouldn't it be less error-prone
> for users to say that once FAN_MARK_IGNORE is used for a mark, all
> subsequent modifications of ignore mask have to use FAN_MARK_IGNORE? I mean
> if some program bothers with FAN_MARK_IGNORE, I'd expect it to use it for
> all its calls as otherwise the mixup is kind of difficult to reason
> about...

I like that.

>
> Also it follows the behavior we have picked for FAN_MARK_EVICTABLE AFAIR
> but that's not really important to me.

It's kind of the opposite in the case of FAN_MARK_EVICTABLE.
FAN_MARK_EVICTABLE can be "upgraded" no non-evictable
but not the other way around.
And also with FAN_MARK_EVICTABLE we do not deprecate the
old API...

See man page draft:
https://github.com/amir73il/man-pages/commit/58851140bbc08b9ab9c7edd8830f37cf883d8d2a#diff-7a4387558a34e18ed6fb13d31933b2e4506496f8b3dd55df700f62b258e6f004R165

>
> > @@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> >
> >       /*
> >        * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
> > -      * FAN_MARK_IGNORED_MASK.
> > +      * FAN_MARK_IGNORED_MASK.  They can be updated in ignore mask with
> > +      * FAN_MARK_IGNORE and then they do take effect.
> >        */
> > -     if (ignore)
> > +     switch (ignore) {
> > +     case 0:
> > +     case FAN_MARK_IGNORE:
> > +             break;
> > +     case FAN_MARK_IGNORED_MASK:
> >               mask &= ~FANOTIFY_EVENT_FLAGS;
> > +             umask = FANOTIFY_EVENT_FLAGS;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> I think this would be easier to follow as two ifs:
>
>         /* We don't allow FAN_MARK_IGNORE & FAN_MARK_IGNORED_MASK together */
>         if (ignore == FAN_MARK_IGNORE | FAN_MARK_IGNORED_MASK)
>                 return -EINVAL;
>         /*
>          * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
>          * FAN_MARK_IGNORED_MASK.
>          */
>         if (ignore == FAN_MARK_IGNORED_MASK) {
>                 mask &= ~FANOTIFY_EVENT_FLAGS;
>                 umask = FANOTIFY_EVENT_FLAGS;
>         }
>

Yeh that looks better.

Thanks,
Amir.

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Amir Goldstein]

On Thu, Jun 23, 2022 at 12:49 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 22-06-22 21:28:23, Amir Goldstein wrote:
> > On Wed, Jun 22, 2022 at 7:00 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > > > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > > > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > > > ignore mask is always implicitly applied to events on directories.
> > > >
> > > > Define a mark flag that replaces this legacy behavior with logic of
> > > > applying the ignore mask according to event flags in ignore mask.
> > > >
> > > > Implement the new logic to prepare for supporting an ignore mask that
> > > > ignores events on children and ignore mask that does not ignore events
> > > > on directories.
> > > >
> > > > To emphasize the change in terminology, also rename ignored_mask mark
> > > > member to ignore_mask and use accessor to get only ignored events or
> > > > events and flags.
> > > >
> > > > This change in terminology finally aligns with the "ignore mask"
> > > > language in man pages and in most of the comments.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > >
> > > ..
> > >
> > > > @@ -423,7 +425,8 @@ static bool fsnotify_iter_select_report_types(
> > > >                        * But is *this mark* watching children?
> > > >                        */
> > > >                       if (type == FSNOTIFY_ITER_TYPE_PARENT &&
> > > > -                         !(mark->mask & FS_EVENT_ON_CHILD))
> > > > +                         !(mark->mask & FS_EVENT_ON_CHILD) &&
> > > > +                         !(fsnotify_ignore_mask(mark) & FS_EVENT_ON_CHILD))
> > > >                               continue;
> > >
> > > So now we have in ->report_mask the FSNOTIFY_ITER_TYPE_PARENT if either
> > > ->mask or ->ignore_mask have FS_EVENT_ON_CHILD set. But I see nothing that
> > > would stop us from applying say ->mask to the set of events we are
> > > interested in if FS_EVENT_ON_CHILD is set only in ->ignore_mask? And
> >
> > I think I spent some time thinking about this and came to a conclusion that
> > 1. It is hard to get all the cases right
> > 2. It is a micro optimization
> >
> > The implication is that the user can set an ignore mask of an object, get no
> > events but still cause performance penalty. Right?
> > So just don't do that...
>
> So I was more afraid that this actually results in generating events we
> should not generate. For example consider dir 'd' with mask FS_OPEN and
> ignore_mask FS_MODIFY | FS_EVENT_ON_CHILD. Now open("d/file") happens so
> FS_OPEN is generated for d/file. We select FSNOTIFY_ITER_TYPE_PARENT in the
> ->report_mask because of the ignore_mask on 'd' and pass the iter to
> fanotify_handle_event(). There fanotify_group_event_mask() will include
> FS_OPEN to marks_mask and conclude event should be reported. But there's no
> mark that should result in reporting this...
>

I see... I think the FS_EVENT_ON_CHILD needs to be added to send_to_group()
just like it knows about FS_ISDIR. Will need to look into it and see which tests
we are missing.

> The problem is that with the introduction of FSNOTIFY_ITER_TYPE_PARENT we
> started to rely on that type being set only when the event on child should
> be reported to parent and now you break that AFAICT.
>

The idea behind FSNOTIFY_ITER_TYPE_PARENT is that the event handlers
have all the information to make the right decision based on mask/ignored_mask
and flags. I guess the implementation is incorrect though.

Well spotted!

Thanks,
Amir.

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Amir Goldstein]

On Wed, Jun 22, 2022 at 6:52 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > ignore mask is always implicitly applied to events on directories.
> >
> > Define a mark flag that replaces this legacy behavior with logic of
> > applying the ignore mask according to event flags in ignore mask.
> >
> > Implement the new logic to prepare for supporting an ignore mask that
> > ignores events on children and ignore mask that does not ignore events
> > on directories.
> >
> > To emphasize the change in terminology, also rename ignored_mask mark
> > member to ignore_mask and use accessor to get only ignored events or
> > events and flags.
> >
> > This change in terminology finally aligns with the "ignore mask"
> > language in man pages and in most of the comments.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks mostly good to me. Just one question / suggestion: You are
> introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events().
> So shouldn't we be using these helpers as much as possible throughout the
> code? Because in several places I had to check the code around whether
> using mark->ignore_mask directly is actually fine. In particular:

I looked at the code and the only two cases I found were the two cases
that you pointed out that needed to use fsnotify_ignored_events().

>
> > @@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >                       return 0;
> >       } else if (!(fid_mode & FAN_REPORT_FID)) {
> >               /* Do we have a directory inode to report? */
> > -             if (!dir && !(event_mask & FS_ISDIR))
> > +             if (!dir && !ondir)
> >                       return 0;
> >       }
> >
> >       fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> > -             /* Apply ignore mask regardless of mark's ISDIR flag */
> > -             marks_ignored_mask |= mark->ignored_mask;
> > +             /*
> > +              * Apply ignore mask depending on whether FAN_ONDIR flag in
> > +              * ignore mask should be checked to ignore events on dirs.
> > +              */
> > +             if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
> > +                     marks_ignore_mask |= mark->ignore_mask;
> >
> >               /*
> >                * If the event is on dir and this mark doesn't care about
> >                * events on dir, don't send it!
> >                */
> > -             if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
> > +             if (ondir && !(mark->mask & FAN_ONDIR))
> >                       continue;
> >
> >               marks_mask |= mark->mask;
>
> So for example here I'm wondering whether a helper should not be used...

fixed.

>
> > @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >               *match_mask |= 1U << type;
> >       }
> >
> > -     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > +     test_mask = event_mask & marks_mask & ~marks_ignore_mask;
>
> Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
> marks_ignore_mask it can result in clearing this flag in the returned
> 'mask' which is likely not what we want if there are some events left
> unignored in the 'mask'?

You are right.
This can end up clearing FAN_ONDIR and then we won't report it.
However, take a look at this:

commit 0badfa029e5fd6d5462adb767937319335637c83
Author: Amir Goldstein <amir73il@gmail.com>
Date:   Thu Jul 16 11:42:09 2020 +0300

    fanotify: generalize the handling of extra event flags

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

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

What the hell did I mean by "We will have use for this indication later on"?
fanotify_alloc_event() does not look at the FAN_EVENT_ON_CHILD flag.
I think I had the idea that events reported in a group with FAN_REPORT_NAME
on an inode mark should not report its parent fid+name to be compatible with
inotify behavior and I think you shot this idea down, but it is only a guess.

>
> > @@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
> >       fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> >               group = mark->group;
> >               marks_mask |= mark->mask;
> > -             marks_ignored_mask |= mark->ignored_mask;
> > +             if (!(mask & FS_ISDIR) ||
> > +                 (fsnotify_ignore_mask(mark) & FS_ISDIR))
> > +                     marks_ignore_mask |= mark->ignore_mask;
> >       }
> >
> > -     pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > -              __func__, group, mask, marks_mask, marks_ignored_mask,
> > +     pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > +              __func__, group, mask, marks_mask, marks_ignore_mask,
> >                data, data_type, dir, cookie);
> >
> > -     if (!(test_mask & marks_mask & ~marks_ignored_mask))
> > +     if (!(test_mask & marks_mask & ~marks_ignore_mask))
> >               return 0;
>
> And I'm wondering about similar things here...

Fixed here too, but here there is no meaning to the event flags,
because test_mask masks them out.

Thanks,
Amir.

Re: [PATCH 1/2] fanotify: prepare for setting event flags in ignore mask

[Jan Kara]

On Fri 24-06-22 14:32:24, Amir Goldstein wrote:
> On Wed, Jun 22, 2022 at 6:52 PM Jan Kara <jack@suse.cz> wrote:
> > > @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > >               *match_mask |= 1U << type;
> > >       }
> > >
> > > -     test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > > +     test_mask = event_mask & marks_mask & ~marks_ignore_mask;
> >
> > Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
> > marks_ignore_mask it can result in clearing this flag in the returned
> > 'mask' which is likely not what we want if there are some events left
> > unignored in the 'mask'?
> 
> You are right.
> This can end up clearing FAN_ONDIR and then we won't report it.
> However, take a look at this:
> 
> commit 0badfa029e5fd6d5462adb767937319335637c83
> Author: Amir Goldstein <amir73il@gmail.com>
> Date:   Thu Jul 16 11:42:09 2020 +0300
> 
>     fanotify: generalize the handling of extra event flags
> 
>     In fanotify_group_event_mask() there is logic in place to make sure we
>     are not going to handle an event with no type and just FAN_ONDIR flag.
>     Generalize this logic to any FANOTIFY_EVENT_FLAGS.
> 
>     There is only one more flag in this group at the moment -
>     FAN_EVENT_ON_CHILD. We never report it to user, but we do pass it in to
>     fanotify_alloc_event() when group is reporting fid as indication that
>     event happened on child. We will have use for this indication later on.
> 
> What the hell did I mean by "We will have use for this indication later on"?

Heh, I was wondering about exactly that sentence when I was writing my
review comments and looking where the code came from :). I didn't find a
good explanation...

> fanotify_alloc_event() does not look at the FAN_EVENT_ON_CHILD flag.
> I think I had the idea that events reported in a group with FAN_REPORT_NAME
> on an inode mark should not report its parent fid+name to be compatible with
> inotify behavior and I think you shot this idea down, but it is only a guess.

Yeah, maybe.

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