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

[Jan Kara]

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()...

>         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.

> +       }
>         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!

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR