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

[Amir Goldstein]

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.

Thanks,
Amir.

--- 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;
        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);
+       }
        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);