Re: [BUG] fanotify: destroy/add race leaves a mark on a detached connector

[Amir Goldstein]

On Tue, Sep 1, 2026 at 6:57 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Mon 31-08-26 16:54:55, Amir Goldstein wrote:
> > On Mon, Aug 31, 2026 at 1:39 PM Jan Kara <jack@xxxxxxx> wrote:
> > >
> > > On Sat 29-08-26 17:04:19, Amir Goldstein wrote:
> > > > On Fri, Aug 28, 2026 at 7:36 PM Jan Kara <jack@xxxxxxx> wrote:
> > > > > > > Now there's the question about the best fix for the problem. I've realized
> > > > > > > we cannot just remove the call to fsnotify_inode_delete() from the place
> > > > > > > where fsnotify_inoderemove() is called (and rely on the call from
> > > > > > > __destroy_inode()) because the connector holds inode reference and so
> > > > > > > __destroy_inode() would never get called. Hrm, this is a hairy issue and so
> > > > > > > far I don't see a good place to reliably trigger detaching of connector
> > > > > > > from the inode. One reliable place would be iput() but that's really ugly
> > > > > > > and would slow down every iput() user. The cleanest solution I can
> > > > > > > currently see is to stop holding inode reference from a connector as I've
> > > > > > > proposed in the past. Then we can reliably perform the cleanup in
> > > > > > > __destroy_inode(). But that will be a rather non-trivial undertaking. Amir,
> > > > > > > do you see some other option?
> > > > > >
> > > > > > What about the option I proposed to live with the orphan marks
> > > > > > Is it so bad? I mean it's confusing and removing the assertion could
> > > > > > hide real bugs/races, but maybe it's worth not complicating the code
> > > > > > until we move forward to re-attachable marks.
> > > > >
> > > > > I'm not so much worried about the case when placing of the mark races with
> > > > > unlink. There I agree we can somehow paper over the problem for now. What
> > > > > I'm more worried about is the case where someone places a precontent /
> > > > > permission mark on the inode and a malicious user does hardlink + open +
> > > > > unlink trick and after unlinking the original link to the inode, he has an
> > > > > open fd to the inode stripped of any notification marks which I'd say
> > > > > violates the promise of precontent / permission marks.
> > > >
> > > > I see. so we need a point where there are no more hardlinks and no more
> > > > dentry aliases to the inode.
> > > >
> > > > How about we check for this condition?
> > > >
> > > > --- a/fs/dcache.c
> > > > +++ b/fs/dcache.c
> > > > @@ -471,7 +471,7 @@ static void dentry_unlink_inode(struct dentry * dentry)
> > > >         raw_write_seqcount_end(&dentry->d_seq);
> > > >         spin_unlock(&dentry->d_lock);
> > > >         spin_unlock(&inode->i_lock);
> > > > -       if (!inode->i_nlink)
> > > > +       if (!inode->i_nlink && hlist_empty(&inode->i_dentry))
> > > >                 fsnotify_inoderemove(inode);
> > >
> > > This looks like an intriguing idea. But I think it still has a hole. Until
> > > the last inode reference is put, open_by_handle() can still get you a new
> > > dentry and an open file pointing to that inode.
> >
> > Maybe we can close this hole, because whether open_by_handle()
> > should or should not succeed in this case is not well defined,
> > We can make it defined.
> > Something like this?
> > NFS should have a sillyrenamed link to avoid undefined behavior
> > around unlinked open files.
>
> Hum, right. That could work.
>
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -459,6 +459,7 @@ static void dentry_unlink_inode(struct dentry * dentry)
> >         __releases(dentry->d_inode->i_lock)
> >  {
> >         struct inode *inode = dentry->d_inode;
> > +       bool dead;
> >
> >         raw_write_seqcount_begin(&dentry->d_seq);
> >         __d_clear_type_and_inode(dentry);
> > @@ -469,9 +470,12 @@ static void dentry_unlink_inode(struct dentry * dentry)
> >          */
> >         dentry->waiters = NULL;
> >         raw_write_seqcount_end(&dentry->d_seq);
> > +       dead = !inode->i_nlink && hlist_empty(&inode->i_dentry);
> > +       if (dead)
> > +               inode->i_state |= I_ZOMBIE;
>
> Do we really need the I_ZOMBIE state? I think we can just check i_nlink and
> hlist_empty() directly in __d_obtain_alias(), cannot we? Once this
> condition is true, it cannot change until we go through
> __d_obtain_alias()...
>

OK I added convenience helper inode_is_dead()
names are debatable...

> >         spin_unlock(&dentry->d_lock);
> >         spin_unlock(&inode->i_lock);
> > -       if (!inode->i_nlink)
> > +       if (dead)
> >                 fsnotify_inoderemove(inode);
> >         if (dentry->d_op && dentry->d_op->d_iput)
> >                 dentry->d_op->d_iput(dentry, inode);
> > @@ -2244,6 +2248,12 @@ static struct dentry *__d_obtain_alias(struct
> > inode *inode, bool disconnected)
> >
> >         security_d_instantiate(new, inode);
> >         spin_lock(&inode->i_lock);
> > +       if (inode->i_state & I_ZOMBIE) {
> > +               spin_unlock(&inode->i_lock);
> > +               dput(new);
> > +               iput(inode);
> > +               return ERR_PTR(-ESTALE);
>
> I think a failure return after security_d_instantiate() could be unexpected
> so we need to do this check before calling it.

You mean before and after.

>
> > +       }
> >         res = __d_find_any_alias(inode); /* recheck under lock */
> >         if (likely(!res)) { /* still no alias, attach a disconnected dentry */
> >                 unsigned add_flags = d_flags_for_inode(inode);
>
> Thanks for looking into this!
>

See attached patch (again helper named debatable).

Daehyeon, care to test this patch with your reproducer?

Thanks,
Amir.
From 2d3d1938373bfcb27545a58fff1600f2f2ccdd31 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Tue, 1 Sep 2026 23:40:43 +0200
Subject: [PATCH] fs: make sure to call fsnotify_removeinode() once when inode
 is dead

Daehyeon reported a race wherein fsnotify_removeinode() can be called
when inode still have a live alias.  This can lead to several undesired
outcomes.

Check that inode is really "dead", meaning no nlink and no more aliases
before calling fsnotify_removeinode() for final cleanup of inode marks.

Also do not allow open_by_handle() to add new aliases to a "dead" inode.

Reported-by: Daehyeon Ko <4ncienth@xxxxxxxxx>
Closes: https://lore.kernel.org/linux-fsdevel/20260827045007.3831259-1-4ncienth@xxxxxxxxx/
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
 fs/dcache.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 3e9af9de70746..a76260092e26a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -450,6 +450,12 @@ static void dentry_free(struct dentry *dentry)
 		call_rcu(&dentry->d_rcu, __d_free);
 }
 
+/* Inode may be kept alive by a positive nlink or by a dentry alias */
+static bool inode_is_dead(struct inode *inode)
+{
+	return !inode->i_nlink && hlist_empty(&inode->i_dentry);
+}
+
 /*
  * Release the dentry's inode, using the filesystem
  * d_iput() operation if defined.
@@ -459,6 +465,7 @@ static void dentry_unlink_inode(struct dentry * dentry)
 	__releases(dentry->d_inode->i_lock)
 {
 	struct inode *inode = dentry->d_inode;
+	bool dead;
 
 	raw_write_seqcount_begin(&dentry->d_seq);
 	__d_clear_type_and_inode(dentry);
@@ -469,9 +476,10 @@ static void dentry_unlink_inode(struct dentry * dentry)
 	 */
 	dentry->waiters = NULL;
 	raw_write_seqcount_end(&dentry->d_seq);
+	dead = inode_is_dead(inode);
 	spin_unlock(&dentry->d_lock);
 	spin_unlock(&inode->i_lock);
-	if (!inode->i_nlink)
+	if (dead)
 		fsnotify_inoderemove(inode);
 	if (dentry->d_op && dentry->d_op->d_iput)
 		dentry->d_op->d_iput(dentry, inode);
@@ -1160,6 +1168,18 @@ static struct dentry * __d_find_any_alias(struct inode *inode)
 	return NULL;
 }
 
+/*
+ * Same as __d_find_any_alias(), but returns ERR_PTR(-ESTALE) if the inode is
+ * dead and should not accept any new aliases.
+ */
+static struct dentry * __d_find_alias_of_live_inode(struct inode *inode)
+{
+	struct dentry *alias = __d_find_any_alias(inode);
+	if (!alias && inode_is_dead(inode))
+		return ERR_PTR(-ESTALE);
+	return alias;
+}
+
 /**
  * d_find_any_alias - find any alias for a given inode
  * @inode: inode to find an alias for
@@ -2232,7 +2252,9 @@ static struct dentry *__d_obtain_alias(struct inode *inode, bool disconnected)
 
 	sb = inode->i_sb;
 
-	res = d_find_any_alias(inode); /* existing alias? */
+	spin_lock(&inode->i_lock);
+	res = __d_find_alias_of_live_inode(inode); /* existing alias? */
+	spin_unlock(&inode->i_lock);
 	if (res)
 		goto out;
 
@@ -2244,7 +2266,7 @@ static struct dentry *__d_obtain_alias(struct inode *inode, bool disconnected)
 
 	security_d_instantiate(new, inode);
 	spin_lock(&inode->i_lock);
-	res = __d_find_any_alias(inode); /* recheck under lock */
+	res = __d_find_alias_of_live_inode(inode); /* recheck under lock */
 	if (likely(!res)) { /* still no alias, attach a disconnected dentry */
 		unsigned add_flags = d_flags_for_inode(inode);
 
-- 
2.55.0