[PATCH v2 0/3] Fix shmem_rename2 directory offset calculation

[PATCH v2 0/3] Fix shmem_rename2 directory offset calculation

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

The existing code in shmem_rename2() allocates a fresh directory
offset value when renaming over an existing destination entry. User
space does not expect this behavior. In particular, applications
that rename while walking a directory can loop indefinitely because
they never reach the end of the directory.

The only test that is problematic at the moment is generic/449,
which live-locks (interruptibly). I don't have a baseline yet, so
I can't say whether the fix introduces this behavior or pre-dates
the shmem conversion to simple_offset.


--
Changes since v1:
- Patches reorganized for easier review and backport
- Passes git regression and fstests (with scratch device)
- Dropped the API clean-up patch for now

Chuck Lever (3):
  libfs: Fix simple_offset_rename_exchange()
  libfs: Add simple_offset_rename() API
  shmem: Fix shmem_rename2()

 fs/libfs.c         | 55 +++++++++++++++++++++++++++++++++++++++++-----
 include/linux/fs.h |  2 ++
 mm/shmem.c         |  3 +--
 3 files changed, 52 insertions(+), 8 deletions(-)


base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
-- 
2.44.0

[PATCH v2 1/3] libfs: Fix simple_offset_rename_exchange()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

User space expects the replacement (old) directory entry to have
the same directory offset after the rename.

Suggested-by: Christian Brauner <brauner@kernel.org>
Fixes: a2e459555c5f ("shmem: stable directory offsets")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/libfs.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 3a6f2cb364f8..ab61fae92cde 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -295,6 +295,18 @@ int simple_offset_add(struct offset_ctx *octx, struct dentry *dentry)
 	return 0;
 }
 
+static int simple_offset_replace(struct offset_ctx *octx, struct dentry *dentry,
+				 long offset)
+{
+	int ret;
+
+	ret = mtree_store(&octx->mt, offset, dentry, GFP_KERNEL);
+	if (ret)
+		return ret;
+	offset_set(dentry, offset);
+	return 0;
+}
+
 /**
  * simple_offset_remove - Remove an entry to a directory's offset map
  * @octx: directory offset ctx to be updated
@@ -352,6 +364,9 @@ int simple_offset_empty(struct dentry *dentry)
  * @new_dir: destination parent
  * @new_dentry: destination dentry
  *
+ * This API preserves the directory offset values. Caller provides
+ * appropriate serialization.
+ *
  * Returns zero on success. Otherwise a negative errno is returned and the
  * rename is rolled back.
  */
@@ -369,11 +384,11 @@ int simple_offset_rename_exchange(struct inode *old_dir,
 	simple_offset_remove(old_ctx, old_dentry);
 	simple_offset_remove(new_ctx, new_dentry);
 
-	ret = simple_offset_add(new_ctx, old_dentry);
+	ret = simple_offset_replace(new_ctx, old_dentry, new_index);
 	if (ret)
 		goto out_restore;
 
-	ret = simple_offset_add(old_ctx, new_dentry);
+	ret = simple_offset_replace(old_ctx, new_dentry, old_index);
 	if (ret) {
 		simple_offset_remove(new_ctx, old_dentry);
 		goto out_restore;
@@ -388,10 +403,8 @@ int simple_offset_rename_exchange(struct inode *old_dir,
 	return 0;
 
 out_restore:
-	offset_set(old_dentry, old_index);
-	mtree_store(&old_ctx->mt, old_index, old_dentry, GFP_KERNEL);
-	offset_set(new_dentry, new_index);
-	mtree_store(&new_ctx->mt, new_index, new_dentry, GFP_KERNEL);
+	(void)simple_offset_replace(old_ctx, old_dentry, old_index);
+	(void)simple_offset_replace(new_ctx, new_dentry, new_index);
 	return ret;
 }
 
-- 
2.44.0

[PATCH v2 2/3] libfs: Add simple_offset_rename() API

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

I'm about to fix a tmpfs rename bug that requires the use of
internal simple_offset helpers that are not available in mm/shmem.c

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/libfs.c         | 21 +++++++++++++++++++++
 include/linux/fs.h |  2 ++
 mm/shmem.c         |  3 +--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index ab61fae92cde..c392a6edd393 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -357,6 +357,27 @@ int simple_offset_empty(struct dentry *dentry)
 	return ret;
 }
 
+/**
+ * simple_offset_rename - handle directory offsets for rename
+ * @old_dir: parent directory of source entry
+ * @old_dentry: dentry of source entry
+ * @new_dir: parent_directory of destination entry
+ * @new_dentry: dentry of destination
+ *
+ * Caller provides appropriate serialization.
+ *
+ * Returns zero on success, a negative errno value on failure.
+ */
+int simple_offset_rename(struct inode *old_dir, struct dentry *old_dentry,
+			 struct inode *new_dir, struct dentry *new_dentry)
+{
+	struct offset_ctx *old_ctx = old_dir->i_op->get_offset_ctx(old_dir);
+	struct offset_ctx *new_ctx = new_dir->i_op->get_offset_ctx(new_dir);
+
+	simple_offset_remove(old_ctx, old_dentry);
+	return simple_offset_add(new_ctx, old_dentry);
+}
+
 /**
  * simple_offset_rename_exchange - exchange rename with directory offsets
  * @old_dir: parent of dentry being moved
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8dfd53b52744..b09f14132110 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3340,6 +3340,8 @@ void simple_offset_init(struct offset_ctx *octx);
 int simple_offset_add(struct offset_ctx *octx, struct dentry *dentry);
 void simple_offset_remove(struct offset_ctx *octx, struct dentry *dentry);
 int simple_offset_empty(struct dentry *dentry);
+int simple_offset_rename(struct inode *old_dir, struct dentry *old_dentry,
+			 struct inode *new_dir, struct dentry *new_dentry);
 int simple_offset_rename_exchange(struct inode *old_dir,
 				  struct dentry *old_dentry,
 				  struct inode *new_dir,
diff --git a/mm/shmem.c b/mm/shmem.c
index 0aad0d9a621b..c0fb65223963 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3473,8 +3473,7 @@ static int shmem_rename2(struct mnt_idmap *idmap,
 			return error;
 	}
 
-	simple_offset_remove(shmem_get_offset_ctx(old_dir), old_dentry);
-	error = simple_offset_add(shmem_get_offset_ctx(new_dir), old_dentry);
+	error = simple_offset_rename(old_dir, old_dentry, new_dir, new_dentry);
 	if (error)
 		return error;
 
-- 
2.44.0

[PATCH v2 3/3] shmem: Fix shmem_rename2()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

When renaming onto an existing directory entry, user space expects
the replacement entry to have the same directory offset as the
original one.

Link: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15966
Fixes: a2e459555c5f ("shmem: stable directory offsets")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/libfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/libfs.c b/fs/libfs.c
index c392a6edd393..b635ee5adbcc 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -366,6 +366,9 @@ int simple_offset_empty(struct dentry *dentry)
  *
  * Caller provides appropriate serialization.
  *
+ * User space expects the directory offset value of the replaced
+ * (new) directory entry to be unchanged after a rename.
+ *
  * Returns zero on success, a negative errno value on failure.
  */
 int simple_offset_rename(struct inode *old_dir, struct dentry *old_dentry,
@@ -373,8 +376,14 @@ int simple_offset_rename(struct inode *old_dir, struct dentry *old_dentry,
 {
 	struct offset_ctx *old_ctx = old_dir->i_op->get_offset_ctx(old_dir);
 	struct offset_ctx *new_ctx = new_dir->i_op->get_offset_ctx(new_dir);
+	long new_offset = dentry2offset(new_dentry);
 
 	simple_offset_remove(old_ctx, old_dentry);
+
+	if (new_offset) {
+		offset_set(new_dentry, 0);
+		return simple_offset_replace(new_ctx, old_dentry, new_offset);
+	}
 	return simple_offset_add(new_ctx, old_dentry);
 }
 
-- 
2.44.0

Re: [PATCH v2 0/3] Fix shmem_rename2 directory offset calculation

[Chuck Lever]

On Mon, Apr 15, 2024 at 11:20:53AM -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The existing code in shmem_rename2() allocates a fresh directory
> offset value when renaming over an existing destination entry. User
> space does not expect this behavior. In particular, applications
> that rename while walking a directory can loop indefinitely because
> they never reach the end of the directory.
> 
> The only test that is problematic at the moment is generic/449,
> which live-locks (interruptibly). I don't have a baseline yet, so
> I can't say whether the fix introduces this behavior or pre-dates
> the shmem conversion to simple_offset.

v6.5 exhibits the same behavior, so this fix did not introduce this
issue. IMO these patches are ready.


> --
> Changes since v1:
> - Patches reorganized for easier review and backport
> - Passes git regression and fstests (with scratch device)
> - Dropped the API clean-up patch for now
> 
> Chuck Lever (3):
>   libfs: Fix simple_offset_rename_exchange()
>   libfs: Add simple_offset_rename() API
>   shmem: Fix shmem_rename2()
> 
>  fs/libfs.c         | 55 +++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/fs.h |  2 ++
>  mm/shmem.c         |  3 +--
>  3 files changed, 52 insertions(+), 8 deletions(-)
> 
> 
> base-commit: fec50db7033ea478773b159e0e2efb135270e3b7
> -- 
> 2.44.0

-- 
Chuck Lever

Re: [PATCH v2 0/3] Fix shmem_rename2 directory offset calculation

[Christian Brauner]

On Mon, 15 Apr 2024 11:20:53 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> The existing code in shmem_rename2() allocates a fresh directory
> offset value when renaming over an existing destination entry. User
> space does not expect this behavior. In particular, applications
> that rename while walking a directory can loop indefinitely because
> they never reach the end of the directory.
> 
> [...]

Thanks for fixing this!

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/3] libfs: Fix simple_offset_rename_exchange()
      https://git.kernel.org/vfs/vfs/c/23cdd0eed3f1
[2/3] libfs: Add simple_offset_rename() API
      https://git.kernel.org/vfs/vfs/c/5a1a25be995e
[3/3] shmem: Fix shmem_rename2()
      https://git.kernel.org/vfs/vfs/c/ad191eb6d694