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

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

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR