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

[Amir Goldstein]

On Fri, Aug 28, 2026 at 11:05 AM Jan Kara <jack@xxxxxxx> wrote:
>
> Hi!
>
> On Fri 28-08-26 13:24:19, Daehyeon Ko wrote:
> > You are right that an fd-held dentry cannot satisfy the count == 1 check. I
> > was imprecise in the report: the fd and the final unlink refer to two
> > different hard-link dentries for the same inode.
> >
> > The setup is:
> >
> >   1. create A/x and link it as B/y;
> >   2. open B/y, add the old-group inode mark through that fd, then unlink
> >      B/y, leaving i_nlink == 1 while the fd retains the B/y dentry;
> >   3. race these two calls:
> >
> >        unlink("A/x")
> >
> >        fanotify_mark(racer_group, FAN_MARK_ADD, FAN_MODIFY,
> >                      victim_fd, NULL)
> >
> >      where victim_fd still refers to the already unlinked B/y dentry.
> >
> > So the A/x dentry checked by d_delete() is not held by the fd and can have
> > d_lockref.count == 1. Its unlink drops the inode's last link and reaches
> > dentry_unlink_inode() -> fsnotify_inoderemove(). At the same time,
> > fanotify_find_path() gets the same inode through the fd-held B/y dentry and
> > adds the racer mark.
>
> Thanks for explanation of the reproducer. Now it makes sense. The
> conclusion from this is that fsnotify_inoderemove() can indeed be called
> for still fully alive open inodes which should be (think of permission /
> pre-content events) still generating events. That could be actually
> breaking some assumptions of userspace although I don't think it's a
> serious issue in practice because users of permission / pre-content watches
> tend to use filesystem / mount watches.
>
> Also I think we could generate FS_DELETE_SELF twice for the inode - once it
> will we generated as a result of d_delete("A/x"), and finish_dput() ->
> dentry_kill() -> dentry_unlink_inode() will generate it for the second time
> on the last close of the open through "B/y". That should be mostly harmless
> but still it's an oddity to keep in mind and preferably avoid if we can.
>

I don't know. This sounds like a fundamental behavior change.
In current upstream FAN_DELETE_SELF is a dying breath of an
inode and no more events on this inode are expected IIUC.

Applications could be relying on this.
In fact I have an application that almost relies on this, but IIRC
it does one last open_by_handle_at() to make sure before calling
the inode dead. Other applications could be less strict.

Changing behavior so events could happen after FAN_DELETE_SELF
sounds risky.

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

One way to think about it is that if we do the re-attachable marks,
we would need to store ino/st_dev and maybe fid in the mark and
then show_fdinfo wont need to get the inode reference at all.

So in a way, allowing an orphan mark on the connector is a step in
the direction of allowing re-attachable marks on the connector.

Maybe...

Thanks,
Amir.