| Message ID | 20181206145803.32228-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 >
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); } /*
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, Amir. include/linux/fsnotify.h | 46 ++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 14 deletions(-)