Re: [PATCH v1 03/12] fsnotify: don't hold a spin_lock across fsnotify_recalc_mask() calls.
From: Amir Goldstein
Date: Wed Aug 19 2026 - 05:12:29 EST
On Wed, Aug 19, 2026 at 1:30 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> On Mon, 17 Aug 2026, Amir Goldstein wrote:
> > On Mon, Aug 17, 2026 at 1:03 PM Jan Kara <jack@xxxxxxx> 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 <neilb@xxxxxxxxxxx> 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 <amir73il@xxxxxxxxx>
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 <amir73il@xxxxxxxxx>
---
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