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

[Amir Goldstein <amir73il@gmail.com>]

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(-)

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