[PATCH v2 3/4] kernfs: don't lose IN_DELETE_SELF when decoding a file handle

[PATCH v2 3/4] kernfs: don't lose IN_DELETE_SELF when decoding a file handle

[Shakeel Butt]

__kernfs_remove() clears i_nlink on a node's inodes and finds them with
ilookup().  A decode that has pinned the node but not yet hashed its
inode is invisible to that pass:

  CPU0                                    CPU1
  open_by_handle_at()
    kernfs_find_and_get_node_by_id()
      pins the node, still active
                                          rmdir()
                                            marks the subtree removing
                                            ilookup() finds no inode
    kernfs_get_inode()
      hashes an inode with i_nlink 1

Nothing fixes it later: kernfs_refresh_inode() never touches i_nlink for
a file and skips it for a directory being removed.  The inode keeps the
1 it was born with, and dentry_unlink_inode() sends IN_DELETE_SELF only
at 0, so a watcher never learns the node went away.

The other callers of kernfs_get_inode() are safe: those in fs/kernfs
hold kernfs_rwsem, and cgroup_may_write() is covered by cgroup_mutex,
which cgroup_destroy_locked() holds across kernfs_remove().
__kernfs_fh_to_dentry() has held nothing since exportfs support was
added.

Take kernfs_rwsem for reading, as ->get_parent already does, and cover
the lookup as well as kernfs_get_inode().  __kernfs_remove() deactivates
the whole subtree under the write lock, so under the read lock either
the lookup refuses the node, or the inode is hashed before the ilookup()
pass runs.  The same holds for ->fh_to_parent, since a node cannot be
active while an ancestor is being removed.

Reproduced with a 300ms delay between the lookup and kernfs_get_inode(),
decoding a handle for a file in a cgroup directory while another task
rmdir()s it: st_nlink is 1 without this patch and 0 with it.

->get_parent still has a window of its own.  It takes the same lock but
has no active check, so reconnect_path() can build an inode for an
ancestor that is already gone.  That needs the active test rather than a
lock, and changes what ->get_parent returns, so it is left to the series
that reworks these paths.

Fixes: eea5d2bb34ba ("kernfs: Send IN_DELETE_SELF and IN_IGNORED")
Cc: stable@vger.kernel.org
Acked-by: Tejun Heo <tj@kernel.org>
Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/mount.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index f183a96778b9..c15ba6357162 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -124,22 +124,32 @@ static struct dentry *__kernfs_fh_to_dentry(struct super_block *sb,
 		return NULL;
 	}
 
-	kn = kernfs_find_and_get_node_by_id(info->root, id);
-	if (!kn)
-		return ERR_PTR(-ESTALE);
+	/*
+	 * Hold kernfs_rwsem across the lookup as well as kernfs_get_inode().
+	 * __kernfs_remove() deactivates the subtree and clears i_nlink on its
+	 * inodes under the write lock, so under the read lock either
+	 * kernfs_find_and_get_node_by_id() refuses the node, or the inode is
+	 * in the inode hash before the ilookup() pass goes looking for it.
+	 */
+	scoped_guard(rwsem_read, &info->root->kernfs_rwsem) {
+		kn = kernfs_find_and_get_node_by_id(info->root, id);
+		if (!kn)
+			return ERR_PTR(-ESTALE);
 
-	if (get_parent) {
-		struct kernfs_node *parent;
+		if (get_parent) {
+			struct kernfs_node *parent;
 
-		parent = kernfs_get_parent(kn);
+			parent = kernfs_get_parent(kn);
+			kernfs_put(kn);
+			kn = parent;
+			if (!kn)
+				return ERR_PTR(-ESTALE);
+		}
+
+		inode = kernfs_get_inode(sb, kn);
 		kernfs_put(kn);
-		kn = parent;
-		if (!kn)
-			return ERR_PTR(-ESTALE);
 	}
 
-	inode = kernfs_get_inode(sb, kn);
-	kernfs_put(kn);
 	return d_obtain_alias(inode);
 }
 
-- 
2.53.0-Meta