Re: [PATCH v4 16/15] fsnotify: relax WARN_ON() in fsnotify_nameremove()

[Jan Kara <jack@suse.cz>]

On Thu 06-12-18 16:58:03, Amir Goldstein wrote:
> After commit "fsnotify: annotate directory entry modification events"
> fsnotify_nameremove() requires that dentry->d_name and dentry->d_parent
> are stable.
> 
> fsnotify_nameremove() is usually called from d_delete() from filesystem
> ->unlink(), ->rmdir(), ->rename() ops under parent vfs inode lock, so
> said commit adds a WARN_ON() if parent inode is not locked.
> fsnotify_nameremove() is also called from nfs_complete_sillyrename(),
> also under parent vfs inode lock.
> 
> While the requirement of stable d_name and d_parent seems to be true
> for all in-tree callers, the WARN_ON() was not true is some cases.
> 
> The following functions call d_delete() from pseudo filesystems
> without parent inode lock, but those filesystems have no rename()
> op, so this is not a concern: ffs_epfiles_destroy(),
> rpc_gssd_dummy_depopulate(), devpts_pty_kill(), efivarfs_file_write(),
> __ns_get_path(). In the last case, the dentry is a pseudo dentry
> that should not generate any event.
> 
> The following functions call d_delete() from clustered filesystems
> without parent inode lock, but those filesystems use clustered locks
> to synchronize d_delete() with d_move():
> ceph_fill_trace(), ocfs2_dentry_convert_worker().
> 
> Relax the WARN_ON() to accommodate for these two special cases:
> - fs with no dir rename() op
> - fs that does d_move() instead of vfs
> 
> Move the WARN_ON() into fsnotify_nameremove() where it matters.
> 
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Jan,
> 
> I have completed my research about d_delete() callers and my conclusions
> are summarized in this patch.
> 
> Should you decide to accept the result, you would probably want to
> squash this with patch 1.
> 
> I am guessing you would feel more cozy with dget_parent() and
> take_dentry_name_snapshot(), but let's see if we can get an ack on
> my conclusions from either Al or Miklos first.

Thanks for looking into this! I was pondering on this for a while and I
don't see a problem in your analysis. But as you've guessed I don't think
the saved dget_parent() and dentry name snapshot is really worth the
hassle. So I'd rather make fsnotify_nameremove() careful and use
those...

								Honza

> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 14e1f43c38c9..d20c92c3cdeb 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -22,23 +22,10 @@
>   *
>   * Unlike fsnotify_parent(), the event will be reported regardless of the
>   * FS_EVENT_ON_CHILD mask on the parent inode.
> - *
> - * When called with NULL @dir (from fsnotify_nameremove()), the dentry parent
> - * inode is used as the inode to report the event to.
>   */
>  static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry,
>  				  __u32 mask)
>  {
> -	if (!dir)
> -		dir = d_inode(dentry->d_parent);
> -
> -	/*
> -	 * This helper assumes d_parent and d_name are stable. It must be true
> -	 * when called from fsnotify_create()/fsnotify_mkdir(). Less sure about
> -	 * all callers that get here from d_delete() => fsnotify_nameremove().
> -	 */
> -	WARN_ON(!inode_is_locked(dir));
> -
>  	return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
>  			dentry->d_name.name, 0);
>  }
> @@ -158,15 +145,46 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
>  
>  /*
>   * fsnotify_nameremove - a filename was removed from a directory
> + *
> + * Requires that d_parent and d_name are stable.
> + *
> + * Called from d_delete() and nfs_complete_sillyrename().
> + * The latter is called from nfs client ->unlink() ->rmdir() ->rename()
> + * under parent vfs inode lock.
> + *
> + * Most filesystems call d_delete() from ->unlink() ->rmdir() ->rename()
> + * ops under parent vfs inode lock.
> + *
> + * Some pseudo filesystems call d_delete() without parent inode lock.
> + * Those filesystems have no ->rename() op and they do not call
> + * d_move() directly, so d_parent and d_name are stable by definition.
> + * Examples: devpts, efivarfs, rpc_pipefs, functionfs.
> + *
> + * Last, there are the clustered filesystems that call d_delete() on
> + * remote nodes, not under vfs parent inode lock, but they use cluster
> + * distributed locks on local and remote nodes. Those filesystems call
> + * d_delete() under their cluster lock. Examples:
> + * - in ceph_fill_trace() under CEPH_MDS_R_PARENT_LOCKED
> + * - in ocfs2_dentry_convert_worker() under ocfs2_dentry_lock
> + * But those filesystems also call d_move() under the same cluster lock
> + * (i.e. FS_RENAME_DOES_D_MOVE), so d_parent and d_name are also stable.
>   */
>  static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
>  {
> +	struct inode *dir = d_inode(dentry->d_parent);
>  	__u32 mask = FS_DELETE;
>  
> +	/* d_delete() of pseudo inode? (e.g. __ns_get_path() playing tricks) */
> +	if (IS_ROOT(dentry))
> +		return;
> +
> +	WARN_ON_ONCE(!inode_is_locked(dir) && dir->i_op->rename &&
> +		     !(dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE));
> +
>  	if (isdir)
>  		mask |= FS_ISDIR;
>  
> -	fsnotify_dirent(NULL, dentry, mask);
> +	fsnotify_dirent(dir, dentry, mask);
>  }
>  
>  /*
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR