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

[Jan Kara]

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.

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?

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