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

[Jan Kara]

On Fri 28-08-26 12:18:52, Amir Goldstein wrote:
> On Fri, Aug 28, 2026 at 11:05 AM Jan Kara <jack@xxxxxxx> wrote:
> > 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.

I was probably imprecise with my wording. What I meant is that *currently*
when you have a hardlinked inode, you open and unlink through B/y and then
the unlink though A/x races with close of the B/y file, then you could get
two FAN_DELETE_SELF events if the race works out right. The only change of
behavior I was planning was avoiding this race if possible :).
 
> > 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.

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