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

Hi Honza,

The last line can affect the result, but it only narrows the race.

send_to_group() can clear mark->ignore_mask without mark->lock. Suppose a
mark has FS_MODIFY in its normal mask and FS_ACCESS in a non-surviving
ignore mask. After old_mask is read, FS_MODIFY clears the ignore mask and
a concurrent recalculation scans the mark without FS_ACCESS. The add then
puts FS_ACCESS in the normal mask, making new_mask equal to old_mask. If
the scan publishes before the aggregate check, the last line catches it.
If it publishes afterward, the check sees the old aggregate, skips, and
the scan then publishes a mask without FS_ACCESS.

A raw mark->mask comparison catches this case but misses FAN_MARK_IGNORE
updates. The inotify old/new check has the same structural problem: its
replace path temporarily sets mark->mask to zero, which a concurrent scan
can observe. If the final mask equals the old mask, the updater skips and
the scan can publish an aggregate without that mark's bits.

I will therefore make recalculation unconditional after every successful
fanotify add/update and every update of an existing inotify watch. The
fanotify remove path can remain unchanged because it only reduces the
calculated mask; inotify removal has no analogous conditional skip.

I will send a v2 implementing this.

Thanks,
Youngjae Kwon

2026년 7월 31일 (금) 오후 9:27, Jan Kara <[email protected]>님이 작성:
>
> On Fri 31-07-26 00:22:41, Youngjae Kwon 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 request recalculation when an existing mark's calculated mask
> > actually changes. For fanotify, compare fsnotify_calc_mask() before and
> > after normal-mask, ignore-mask, and flag updates. Keep the cached aggregate
> > check for unchanged updates, and keep the flag helper's inode-reference
> > handling.
> >
> > 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]>
>
> Looks mostly good, just one question below:
>
> >       spin_lock(&fsn_mark->lock);
> > +     old_mask = fsnotify_calc_mask(fsn_mark);
> >       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);
> > +     recalc = fanotify_mark_update_flags(fsn_mark, fan_flags);
> > +     new_mask = fsnotify_calc_mask(fsn_mark);
> > +     recalc |= old_mask != new_mask;
> > +     recalc |= new_mask & ~fsnotify_conn_mask(fsn_mark->connector);
>
> How could this last line ever change anything? If old_mask == new_mask,
> then it would look like a bug that some bit from the new_mask (and thus
> old_mask) isn't already included in the aggregate connector mask?
>
> I can fix this up on commit but I want to understand why you've kept the
> line here just in case I'm missing something...
>
>                                                                 Honza
>
> >       spin_unlock(&fsn_mark->lock);
> >
> >       return recalc;
> > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> > index ed37491c1..c44f2daa1 100644
> > --- a/fs/notify/inotify/inotify_user.c
> > +++ b/fs/notify/inotify/inotify_user.c
> > @@ -565,17 +565,8 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
> >       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);
> > -
> > -     }
> > +     if (old_mask != new_mask)
> > +             fsnotify_recalc_mask(fsn_mark->connector);
> >
> >       /* return the wd */
> >       ret = i_mark->wd;
> > --
> > 2.50.1
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
>
>