[PATCH v2] fsnotify: Fix stale object mask after concurrent mark updates

When a mark gets a new event bit, fanotify and inotify avoid recalculating
the object mask if the cached aggregate already contains that bit. This is
racy with a recalculation triggered by a concurrent update to another mark
on the same connector.

The concurrent scan can read the mark before the new bit is added, while
the updater reads the old aggregate before that scan publishes its result.
The updater then skips recalculation and the scan publishes a mask without
the bit, leaving the object mask stale after both updates complete.

This can be reproduced with two fanotify groups watching the same inode:
one thread removes FAN_MODIFY from one existing mark while another thread
adds FAN_MODIFY to the other mark. After both fanotify_mark() calls return,
writes can fail to produce FAN_MODIFY for the group whose mark now contains
the bit. This was reproduced on an unmodified v6.12.95 kernel. The
equivalent inotify interleaving loses IN_MODIFY events.

Always recalculate the connector mask after successfully updating an
existing fanotify mark or inotify watch. In fanotify, FS_MODIFY handling
may clear ignore_mask concurrently without taking mark->lock, so
calculated mask snapshots are not reliable. In inotify, replacing a watch
temporarily sets mark->mask to zero, so a concurrent scan can observe zero
even when the old and final masks are equal. Mark updates are expected to
be infrequent, so avoid these racy optimizations.

Link: https://lore.kernel.org/all/CACwKKmCZdiZDoFuYm6LZhQ=XvHPk0fNKH=X3LmoXMqakYqJaNw@mail.gmail.com/
Fixes: 63c882a05416 ("inotify: reimplement inotify using fsnotify")
Fixes: 912ee3946c5e ("fanotify: do not call fanotify_update_object_mask in fanotify_add_mark")
Cc: [email protected] # needs adjustments for <= 7.0
Suggested-by: Jan Kara <[email protected]>
Signed-off-by: Youngjae Kwon <[email protected]>
---
Changes in v2:
- Always recalculate after a successful fanotify mark add/update or update of
  an existing inotify watch.
- Drop the fanotify calculated-mask and cached-aggregate checks; neither can
  close the race with concurrent ignore-mask clearing.
- Make fanotify_mark_update_flags() return void; its recalculation
  bookkeeping and the now-redundant first-time guard are no longer needed.
- Drop the inotify old/new mask check because the replace path can expose a
  temporary zero mask even when the old and final masks are equal.

I will send backports for stable kernels through 7.0.y after this lands.

 fs/notify/fanotify/fanotify_user.c | 35 ++++++++----------------------
 fs/notify/inotify/inotify_user.c   | 15 +------------
 2 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 9ee373ff5..a3648cec5 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1276,12 +1276,11 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
 	return 0;
 }
 
-static bool fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
+static void 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
@@ -1293,20 +1292,15 @@ static bool fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
 		fsn_mark->flags |= FSNOTIFY_MARK_FLAG_HAS_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.
+	 * Setting FAN_MARK_IGNORED_SURV_MODIFY may remove FS_MODIFY from the
+	 * calculated mask if it was included only to clear the ignore mask.
 	 */
-	if (ignore && (fan_flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
-	    !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) {
+	if (ignore && (fan_flags & FAN_MARK_IGNORED_SURV_MODIFY))
 		fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
-		if (!(fsn_mark->mask & FS_MODIFY))
-			recalc = true;
-	}
 
 	if (fsn_mark->connector->type != FSNOTIFY_OBJ_TYPE_INODE ||
 	    want_iref == !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_NO_IREF))
-		return recalc;
+		return;
 
 	/*
 	 * NO_IREF may be removed from a mark, but not added.
@@ -1314,28 +1308,19 @@ static bool fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark,
 	 */
 	WARN_ON_ONCE(!want_iref);
 	fsn_mark->flags &= ~FSNOTIFY_MARK_FLAG_NO_IREF;
-
-	return true;
 }
 
-static bool fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
+static void fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 				      __u32 mask, unsigned int fan_flags)
 {
-	bool recalc;
-
 	spin_lock(&fsn_mark->lock);
 	if (!(fan_flags & FANOTIFY_MARK_IGNORE_BITS))
 		fsn_mark->mask |= mask;
 	else
 		fsn_mark->ignore_mask |= mask;
 
-	recalc = fsnotify_calc_mask(fsn_mark) &
-		~fsnotify_conn_mask(fsn_mark->connector);
-
-	recalc |= fanotify_mark_update_flags(fsn_mark, fan_flags);
+	fanotify_mark_update_flags(fsn_mark, fan_flags);
 	spin_unlock(&fsn_mark->lock);
-
-	return recalc;
 }
 
 struct fan_fsid {
@@ -1502,7 +1487,6 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			     struct fan_fsid *fsid)
 {
 	struct fsnotify_mark *fsn_mark;
-	bool recalc;
 	int ret = 0;
 
 	fsnotify_group_lock(group);
@@ -1534,9 +1518,8 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			goto out;
 	}
 
-	recalc = fanotify_mark_add_to_mask(fsn_mark, mask, fan_flags);
-	if (recalc)
-		fsnotify_recalc_mask(fsn_mark->connector);
+	fanotify_mark_add_to_mask(fsn_mark, mask, fan_flags);
+	fsnotify_recalc_mask(fsn_mark->connector);
 
 out:
 	fsnotify_group_unlock(group);
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index ed37491c1..5f19c24ec 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -539,7 +539,6 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 {
 	struct fsnotify_mark *fsn_mark;
 	struct inotify_inode_mark *i_mark;
-	__u32 old_mask, new_mask;
 	int replace = !(arg & IN_MASK_ADD);
 	int create = (arg & IN_MASK_CREATE);
 	int ret;
@@ -555,27 +554,15 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 	i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
 
 	spin_lock(&fsn_mark->lock);
-	old_mask = fsn_mark->mask;
 	if (replace) {
 		fsn_mark->mask = 0;
 		fsn_mark->flags &= ~INOTIFY_MARK_FLAGS;
 	}
 	fsn_mark->mask |= inotify_arg_to_mask(inode, arg);
 	fsn_mark->flags |= inotify_arg_to_flags(arg);
-	new_mask = fsn_mark->mask;
 	spin_unlock(&fsn_mark->lock);
 
-	if (old_mask != new_mask) {
-		/* more bits in old than in new? */
-		int dropped = (old_mask & ~new_mask);
-		/* more bits in this fsn_mark than the inode's mask? */
-		int do_inode = (new_mask & ~READ_ONCE(inode->i_fsnotify_mask));
-
-		/* update the inode with this new fsn_mark */
-		if (dropped || do_inode)
-			fsnotify_recalc_mask(fsn_mark->connector);
-
-	}
+	fsnotify_recalc_mask(fsn_mark->connector);
 
 	/* return the wd */
 	ret = i_mark->wd;
-- 
2.50.1