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

[Jan Kara]

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.

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