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

On Sat, Aug 1, 2026 at 8:00 AM Youngjae Kwon <[email protected]> wrote:
>
> 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

Please don't use this big hammer.
See comments on v1.

Thanks,
Amir.

> 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