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

On Wed, Aug 19, 2026 at 1:30 AM NeilBrown <[email protected]> wrote:
>
> 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);

I don't think skipping update_children is correct when called
from fcntl_dirnotify().

>  }
>
>  /*
> 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);
>  }
>

I think I prefer the straight forward semantics in the attached patch.

Thanks,
Amir.

From 73bbfcd1b21a0fc723999aee36af662f36ba4dd2 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <[email protected]>
Date: Wed, 19 Aug 2026 10:56:40 +0200
Subject: [PATCH] fsnotify: update children flags only when adding mask bits

Since we only ever set the PARENT_WATCHED flag from
fsnotify_recalc_mask(), there is never a need to update children flags
when removing mark mask bits only when adding them.

Fixes: 172e422ffea20 ("fsnotify: clear PARENT_WATCHED flags lazily")
Signed-off-by: Amir Goldstein <[email protected]>
---
 fs/notify/dnotify/dnotify.c        | 10 +++++-----
 fs/notify/fanotify/fanotify_user.c |  4 ++--
 fs/notify/inotify/inotify_user.c   |  2 +-
 fs/notify/mark.c                   | 19 ++++++++++---------
 include/linux/fsnotify_backend.h   |  8 ++++++--
 5 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 9fb73bafd41d2..060ea9a455322 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -59,7 +59,7 @@ struct dnotify_mark {
  * it calls the fsnotify function so it can update the set of all events relevant
  * to this inode.
  */
-static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
+static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark, bool add)
 {
 	__u32 new_mask = 0;
 	struct dnotify_struct *dn;
@@ -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(fsn_mark->connector, add);
 }
 
 /*
@@ -116,7 +116,7 @@ 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);
+			dnotify_recalc_inode_mask(inode_mark, false);
 		}
 	}
 
@@ -174,7 +174,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 		if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
-			dnotify_recalc_inode_mask(fsn_mark);
+			dnotify_recalc_inode_mask(fsn_mark, false);
 			break;
 		}
 		prev = &dn->dn_next;
@@ -377,7 +377,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned int arg)
 	else if (error == -EEXIST)
 		error = 0;
 
-	dnotify_recalc_inode_mask(fsn_mark);
+	dnotify_recalc_inode_mask(fsn_mark, true);
 out:
 	spin_unlock(&fsn_mark->lock);
 
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a32c6634d5927..700e353c425a6 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1264,7 +1264,7 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
 	removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
 						 umask, &destroy_mark);
 	if (removed & fsnotify_conn_mask(fsn_mark->connector))
-		fsnotify_recalc_mask(fsn_mark->connector);
+		fsnotify_recalc_mask(fsn_mark->connector, false);
 	if (destroy_mark)
 		fsnotify_detach_mark(fsn_mark);
 	fsnotify_group_unlock(group);
@@ -1538,7 +1538,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 
 	recalc = fanotify_mark_add_to_mask(fsn_mark, mask, fan_flags);
 	if (recalc)
-		fsnotify_recalc_mask(fsn_mark->connector);
+		fsnotify_recalc_mask(fsn_mark->connector, true);
 
 out:
 	fsnotify_group_unlock(group);
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5f19c24ec187f..6d84c5116749d 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -562,7 +562,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
 	fsn_mark->flags |= inotify_arg_to_flags(arg);
 	spin_unlock(&fsn_mark->lock);
 
-	fsnotify_recalc_mask(fsn_mark->connector);
+	fsnotify_recalc_mask(fsn_mark->connector, true);
 
 	/* return the wd */
 	ret = i_mark->wd;
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index b2640d836a712..731e430f2f3c6 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -317,20 +317,21 @@ static void fsnotify_conn_set_children_dentry_flags(
 }
 
 /*
- * 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.
+ * Calculate mask of events for a list of marks.
+ * If @update_children is true, update children dentry flags if needed.
+ * 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.
  */
-void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
+			  bool update_children)
 {
-	bool update_children;
 
 	if (!conn)
 		return;
 
 	spin_lock(&conn->lock);
-	update_children = !fsnotify_conn_watches_children(conn);
+	update_children &= !fsnotify_conn_watches_children(conn);
 	fsnotify_recalc_mask_set_iref(conn);
 	update_children &= fsnotify_conn_watches_children(conn);
 	spin_unlock(&conn->lock);
@@ -368,7 +369,7 @@ void fsnotify_modify_mark_mask(struct fsnotify_mark *mark, u32 set, u32 clear)
 	spin_unlock(&mark->lock);
 
 	if (recalc)
-		fsnotify_recalc_mask(mark->connector);
+		fsnotify_recalc_mask(mark->connector, !!set);
 }
 EXPORT_SYMBOL_GPL(fsnotify_modify_mark_mask);
 
@@ -998,7 +999,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 	if (ret)
 		goto err;
 
-	fsnotify_recalc_mask(mark->connector);
+	fsnotify_recalc_mask(mark->connector, true);
 
 	return ret;
 err:
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 618eed4d6d724..8263287e162bf 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -885,8 +885,12 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 
 /* Get mask of events for a list of marks */
 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);
+/*
+ * Calculate mask of events for a list of marks and update children dentry
+ * flag is needed.
+ */
+extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
+				 bool update_children);
 extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 			       struct fsnotify_group *group);
 /* Find mark belonging to given group in the list of marks */
-- 
2.55.0