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@xxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> 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@xxxxxxxx>
SUSE Labs, CR
