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

On Fri, Jul 31, 2026 at 6:41 PM ­권영재 / 학생 / 전기·정보공학부
<[email protected]> wrote:
>
> 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.

I think you should use raw mark->mask comparison in the normal add case.
That should be enough to prevent races from NOT sending a due event.

You can do unconditional recalc on update of ignore_mask, which is a more
rare use case.

> 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

That seems like an unnecessary overkill.
It feels wrong to me to kill the optimization of the common use case
for esoteric corner use cases.

> and every update of an existing inotify watch. The

Not that I think that update of inotify watch mask is worth optimizing,
but seems that the temporary set to zero can easily be fixed:

if (replace) {
    fsn_mark->mask = inotify_arg_to_mask(inode, arg);
    fsn_mark->flags = (fsn_mark->flags & ~INOTIFY_MARK_FLAGS)
                    | inotify_arg_to_flags(arg);
} else {
    fsn_mark->mask |= inotify_arg_to_mask(inode, arg);
    fsn_mark->flags |= inotify_arg_to_flags(arg);
}

Perhaps there is still a data race here and not worth the trouble
of discussing it at all..

Thanks,
Amir.

> 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
> >
> >