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

On Mon, 17 Aug 2026, Amir Goldstein wrote:
> 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.

I had thought something along these lines too.  I don't think we need to
drop all of fsnotify_recalc_mask(), only the
fsnotify_conn_set_children_dentry_flags() part that walks the d_children
list.  And that only happens when we add to the mask, not when bits are
cleared.  So maybe the fsnotify_conn_set_children_dentry_flags() simply
never happens in this context so it doesn't need fixing.

I would prefer to make that clear from the code with a patch like the
following.

Note that I'm not (yet) convinced by the locking argument, but maybe we
don't need to pursue it.

Thanks,
NeilBrown

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 9fb73bafd41d..be66d4142563 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -75,7 +75,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 		return;
 	fsn_mark->mask = new_mask;
 
-	fsnotify_recalc_mask(fsn_mark->connector);
+	fsnotify_recalc_mask_inatomic(fsn_mark->connector);
 }
 
 /*
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index b2640d836a71..7ba79828b07d 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -322,7 +322,8 @@ static void fsnotify_conn_set_children_dentry_flags(
  * this by holding a mark->lock or mark->group->mark_mutex for a mark on this
  * list.
  */
-void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+static void do_fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
+				    bool in_atomic)
 {
 	bool update_children;
 
@@ -339,10 +340,20 @@ void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	 * When parent stops watching, we clear false positive PARENT_WATCHED
 	 * flags lazily in __fsnotify_parent().
 	 */
-	if (update_children)
+	if (update_children && !WARN_ON(in_atomic))
 		fsnotify_conn_set_children_dentry_flags(conn);
 }
 
+void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+{
+	do_fsnotify_recalc_mask(conn, false);
+}
+
+void fsnotify_recalc_mask_inatomic(struct fsnotify_mark_connector *conn)
+{
+	do_fsnotify_recalc_mask(conn, true);
+}
+
 /**
  * fsnotify_modify_mark_mask - set and/or clear flags in a mark's mask
  * @mark: mark to be modified
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 618eed4d6d72..3e0fdd639d18 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -887,6 +887,7 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 extern __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn);
 /* Calculate mask of events for a list of marks */
 extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn);
+extern void fsnotify_recalc_mask_inatomic(struct fsnotify_mark_connector *conn);
 extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 			       struct fsnotify_group *group);
 /* Find mark belonging to given group in the list of marks */