Re: [PATCH v1 03/12] fsnotify: don't hold a spin_lock across fsnotify_recalc_mask() calls.

On Mon, Aug 17, 2026 at 1:03 PM Jan Kara <[email protected]> wrote:
>
> On Sat 15-08-26 10:28:41, NeilBrown wrote:
> > On Tue, 11 Aug 2026, NeilBrown wrote:
> > > On Tue, 11 Aug 2026, Miklos Szeredi wrote:
> > > > On Mon, 3 Aug 2026 at 03:39, NeilBrown <[email protected]> wrote:
> > > >
> > > > > @@ -116,11 +117,15 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
> > > > >                 else {
> > > > >                         *prev = dn->dn_next;
> > > > >                         kmem_cache_free(dnotify_struct_cache, dn);
> > > > > -                       dnotify_recalc_inode_mask(inode_mark);
> > > > > +                       need_recalc = true;
> > > > >                 }
> > > > >         }
> > > > >
> > > > > +       if (need_recalc)
> > > > > +               need_recalc = dnotify_recalc_inode_mask(inode_mark);
> > > > >         spin_unlock(&inode_mark->lock);
> > > > > +       if (need_recalc)
> > > > > +               fsnotify_recalc_mask(inode_mark->connector);
> > > >
> > > > Is the fsnotify_group_lock() held in this case?   I don't see it.
> > >
> > > It isn't held.  Doesn't it need to be...
> > > It seems to protect marks, so maybe it does.
> > >
> > > srcu seems to be used to protect this section, so maybe we can rely on
> > > that.
> >
> > I dug into this some more, and we do rely on srcu, but don't need the
> > extra code below.
> > inode_mark->lock doesn't protect inode_mark->connector, so moving the
> > dereference out of the lock has no effect.
> > srcu_read_lock is taken before we get the ref to the mark, so the mark
> > and the connector cannot disappear underneath us.
> > A race could result in inode_mark->connector reading as NULL, but
> > fsnotify_recalc_mask() checks for NULL, so there is no risk for harm.
> >
> > Thanks for encouraging me to dig into this.
>
> Sorry for not replying earlier but I was on vacation. The lifetime rules
> around marks & connectors are subtle so we have to be really careful and
> dnotify with its single shot marks is peculiar which makes things even
> harder. fsnotify_recalc_mask() has a comment about locking in front of it:
>
> /*
>  * Calculate mask of events for a list of marks. The caller must make sure
>  * connector and connector->obj cannot disappear under us.  Callers achieve
>  * this by holding a mark->lock or mark->group->mark_mutex for a mark on this
>  * list.
>  */
>
> and you very obviously start violating these rules with your changes. Now I
> admit I've forgotten all the details why I did it like this so let me
> reconstruct it :).
>
> Connector stays alive as long as there's any mark in its list. Both marks
> and connectors are protected by the srcu. Mark also has
> FSNOTIFY_MARK_FLAG_ATTACHED flag which is set iff the connector->obj is
> pointing to valid inode/mount/... FSNOTIFY_MARK_FLAG_ATTACHED changes only
> under mark_mutex so that's why mark_mutex is stabilizing the connector (and
> also connector->obj). This is what is used by most places calling
> fsnotify_recalc_mask(). But dnotify needs to mess with notification mark
> mask from event handling and there we cannot take mark_mutex due to lock
> ordering constraints. That's where the mark->lock rule comes into play
> because mark->lock also needs to be acquired to clear
> FSNOTIFY_MARK_FLAG_ATTACHED. That being said this dnotify use of
> fsnotify_recalc_mask() still looks somewhat racy because
> dnotify_handle_event() can get called after FSNOTIFY_MARK_FLAG_ATTACHED is
> cleared.
>
> Anyway if you move fsnotify_recalc_mask() call outside of mark->lock, you
> seem to make the race with dnotify clearing the mark from
> fcntl_dirnotify() easier to hit. Now in the notification path the inode
> itself is guaranteed to stay alive and the rest is protected by the SRCU so
> there's no direct UAF. But fsnotify_recalc_mask() simply isn't prepared for
> the connector changing under it due to object getting detached and so we
> could end up doing weird things like NULL ptr derefs or similar stuff.
>
> So this call to fsnotify_recalc_mask() from dnotify needs a more careful
> handling. Which is sad because I doubt anybody still uses dnotify...

Maybe this is the way out.

dnotify_recalc_inode_mask() can only remove bits from i_fsnotify_mask,
so it is an optimization.

If we just remove fsnotify_recalc_mask() call from
dnotify_recalc_inode_mask(), then i_fsnotify_mask will be updated when
dnotify_mask gets removed eventually.

If nobody uses dnotify, noone is likely to notice the loss of the optimization.

Thanks,
Amir.