[PATCH bpf-next v3 1/4] libfs: move shmem_exchange to simple_rename_exchange

[PATCH bpf-next v3 1/4] libfs: move shmem_exchange to simple_rename_exchange

[Lorenz Bauer]

Move shmem_exchange and make it available to other callers.

Suggested-by: <mszeredi@redhat.com>
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 fs/libfs.c         | 24 ++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 mm/shmem.c         | 24 +-----------------------
 3 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 51b4de3b3447..1cf144dc9ed2 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -448,6 +448,30 @@ int simple_rmdir(struct inode *dir, struct dentry *dentry)
 }
 EXPORT_SYMBOL(simple_rmdir);
 
+int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
+			   struct inode *new_dir, struct dentry *new_dentry)
+{
+	bool old_is_dir = d_is_dir(old_dentry);
+	bool new_is_dir = d_is_dir(new_dentry);
+
+	if (old_dir != new_dir && old_is_dir != new_is_dir) {
+		if (old_is_dir) {
+			drop_nlink(old_dir);
+			inc_nlink(new_dir);
+		} else {
+			drop_nlink(new_dir);
+			inc_nlink(old_dir);
+		}
+	}
+	old_dir->i_ctime = old_dir->i_mtime =
+	new_dir->i_ctime = new_dir->i_mtime =
+	d_inode(old_dentry)->i_ctime =
+	d_inode(new_dentry)->i_ctime = current_time(old_dir);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(simple_rename_exchange);
+
 int simple_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 		  struct dentry *old_dentry, struct inode *new_dir,
 		  struct dentry *new_dentry, unsigned int flags)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e7a633353fd2..333b8af405ce 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3383,6 +3383,8 @@ extern int simple_open(struct inode *inode, struct file *file);
 extern int simple_link(struct dentry *, struct inode *, struct dentry *);
 extern int simple_unlink(struct inode *, struct dentry *);
 extern int simple_rmdir(struct inode *, struct dentry *);
+extern int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
+				  struct inode *new_dir, struct dentry *new_dentry);
 extern int simple_rename(struct user_namespace *, struct inode *,
 			 struct dentry *, struct inode *, struct dentry *,
 			 unsigned int);
diff --git a/mm/shmem.c b/mm/shmem.c
index b5860f4a2738..a18dde3d3092 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2945,28 +2945,6 @@ static int shmem_rmdir(struct inode *dir, struct dentry *dentry)
 	return shmem_unlink(dir, dentry);
 }
 
-static int shmem_exchange(struct inode *old_dir, struct dentry *old_dentry, struct inode *new_dir, struct dentry *new_dentry)
-{
-	bool old_is_dir = d_is_dir(old_dentry);
-	bool new_is_dir = d_is_dir(new_dentry);
-
-	if (old_dir != new_dir && old_is_dir != new_is_dir) {
-		if (old_is_dir) {
-			drop_nlink(old_dir);
-			inc_nlink(new_dir);
-		} else {
-			drop_nlink(new_dir);
-			inc_nlink(old_dir);
-		}
-	}
-	old_dir->i_ctime = old_dir->i_mtime =
-	new_dir->i_ctime = new_dir->i_mtime =
-	d_inode(old_dentry)->i_ctime =
-	d_inode(new_dentry)->i_ctime = current_time(old_dir);
-
-	return 0;
-}
-
 static int shmem_whiteout(struct user_namespace *mnt_userns,
 			  struct inode *old_dir, struct dentry *old_dentry)
 {
@@ -3012,7 +2990,7 @@ static int shmem_rename2(struct user_namespace *mnt_userns,
 		return -EINVAL;
 
 	if (flags & RENAME_EXCHANGE)
-		return shmem_exchange(old_dir, old_dentry, new_dir, new_dentry);
+		return simple_rename_exchange(old_dir, old_dentry, new_dir, new_dentry);
 
 	if (!simple_empty(new_dentry))
 		return -ENOTEMPTY;
-- 
2.32.0

Re: [PATCH bpf-next v3 1/4] libfs: move shmem_exchange to simple_rename_exchange

[Daniel Borkmann]

On 10/28/21 11:47 AM, Lorenz Bauer wrote:
> Move shmem_exchange and make it available to other callers.
> 
> Suggested-by: <mszeredi@redhat.com>

nit: Should say proper name, but we can fix it up while applying.

Miklos, does the below look good to you? Would be good to have an ACK from fs
folks before applying, please take a look if you have a chance. Thanks!

> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>   fs/libfs.c         | 24 ++++++++++++++++++++++++
>   include/linux/fs.h |  2 ++
>   mm/shmem.c         | 24 +-----------------------
>   3 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 51b4de3b3447..1cf144dc9ed2 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -448,6 +448,30 @@ int simple_rmdir(struct inode *dir, struct dentry *dentry)
>   }
>   EXPORT_SYMBOL(simple_rmdir);
>   
> +int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> +			   struct inode *new_dir, struct dentry *new_dentry)
> +{
> +	bool old_is_dir = d_is_dir(old_dentry);
> +	bool new_is_dir = d_is_dir(new_dentry);
> +
> +	if (old_dir != new_dir && old_is_dir != new_is_dir) {
> +		if (old_is_dir) {
> +			drop_nlink(old_dir);
> +			inc_nlink(new_dir);
> +		} else {
> +			drop_nlink(new_dir);
> +			inc_nlink(old_dir);
> +		}
> +	}
> +	old_dir->i_ctime = old_dir->i_mtime =
> +	new_dir->i_ctime = new_dir->i_mtime =
> +	d_inode(old_dentry)->i_ctime =
> +	d_inode(new_dentry)->i_ctime = current_time(old_dir);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(simple_rename_exchange);
> +
>   int simple_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
>   		  struct dentry *old_dentry, struct inode *new_dir,
>   		  struct dentry *new_dentry, unsigned int flags)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e7a633353fd2..333b8af405ce 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3383,6 +3383,8 @@ extern int simple_open(struct inode *inode, struct file *file);
>   extern int simple_link(struct dentry *, struct inode *, struct dentry *);
>   extern int simple_unlink(struct inode *, struct dentry *);
>   extern int simple_rmdir(struct inode *, struct dentry *);
> +extern int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> +				  struct inode *new_dir, struct dentry *new_dentry);
>   extern int simple_rename(struct user_namespace *, struct inode *,
>   			 struct dentry *, struct inode *, struct dentry *,
>   			 unsigned int);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b5860f4a2738..a18dde3d3092 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2945,28 +2945,6 @@ static int shmem_rmdir(struct inode *dir, struct dentry *dentry)
>   	return shmem_unlink(dir, dentry);
>   }
>   
> -static int shmem_exchange(struct inode *old_dir, struct dentry *old_dentry, struct inode *new_dir, struct dentry *new_dentry)
> -{
> -	bool old_is_dir = d_is_dir(old_dentry);
> -	bool new_is_dir = d_is_dir(new_dentry);
> -
> -	if (old_dir != new_dir && old_is_dir != new_is_dir) {
> -		if (old_is_dir) {
> -			drop_nlink(old_dir);
> -			inc_nlink(new_dir);
> -		} else {
> -			drop_nlink(new_dir);
> -			inc_nlink(old_dir);
> -		}
> -	}
> -	old_dir->i_ctime = old_dir->i_mtime =
> -	new_dir->i_ctime = new_dir->i_mtime =
> -	d_inode(old_dentry)->i_ctime =
> -	d_inode(new_dentry)->i_ctime = current_time(old_dir);
> -
> -	return 0;
> -}
> -
>   static int shmem_whiteout(struct user_namespace *mnt_userns,
>   			  struct inode *old_dir, struct dentry *old_dentry)
>   {
> @@ -3012,7 +2990,7 @@ static int shmem_rename2(struct user_namespace *mnt_userns,
>   		return -EINVAL;
>   
>   	if (flags & RENAME_EXCHANGE)
> -		return shmem_exchange(old_dir, old_dentry, new_dir, new_dentry);
> +		return simple_rename_exchange(old_dir, old_dentry, new_dir, new_dentry);
>   
>   	if (!simple_empty(new_dentry))
>   		return -ENOTEMPTY;
> 

Re: [PATCH bpf-next v3 1/4] libfs: move shmem_exchange to simple_rename_exchange

[Miklos Szeredi]

On Thu, 28 Oct 2021 at 11:48, Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Move shmem_exchange and make it available to other callers.
>
> Suggested-by: <mszeredi@redhat.com>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>

Acked-by: Miklos Szeredi <mszeredi@redhat.com>

[PATCH bpf-next v3 2/4] libfs: support RENAME_EXCHANGE in simple_rename()

[Lorenz Bauer]

Allow atomic exchange via RENAME_EXCHANGE when using simple_rename.
This affects binderfs, ramfs, hubetlbfs and bpffs.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 fs/libfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 1cf144dc9ed2..ba7438ab9371 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -479,9 +479,12 @@ int simple_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 	struct inode *inode = d_inode(old_dentry);
 	int they_are_dirs = d_is_dir(old_dentry);
 
-	if (flags & ~RENAME_NOREPLACE)
+	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
 		return -EINVAL;
 
+	if (flags & RENAME_EXCHANGE)
+		return simple_rename_exchange(old_dir, old_dentry, new_dir, new_dentry);
+
 	if (!simple_empty(new_dentry))
 		return -ENOTEMPTY;
 
-- 
2.32.0

Re: [PATCH bpf-next v3 2/4] libfs: support RENAME_EXCHANGE in simple_rename()

[Miklos Szeredi]

On Thu, 28 Oct 2021 at 11:48, Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Allow atomic exchange via RENAME_EXCHANGE when using simple_rename.
> This affects binderfs, ramfs, hubetlbfs and bpffs.

Ramfs and hugetlbfs are generic enough; those seem safe.

Binderfs: I have no idea what this does; binderfs_rename() should
probably error out on RENAME_EXCHANGE for now, or an explicit ack from
the maintainers.

Bpffs is your baby...

Thanks,
Miklos

Re: [PATCH bpf-next v3 2/4] libfs: support RENAME_EXCHANGE in simple_rename()

[Daniel Borkmann]

On 11/2/21 10:25 AM, Miklos Szeredi wrote:
> On Thu, 28 Oct 2021 at 11:48, Lorenz Bauer <lmb@cloudflare.com> wrote:
>>
>> Allow atomic exchange via RENAME_EXCHANGE when using simple_rename.
>> This affects binderfs, ramfs, hubetlbfs and bpffs.
> 
> Ramfs and hugetlbfs are generic enough; those seem safe.
> 
> Binderfs: I have no idea what this does; binderfs_rename() should
> probably error out on RENAME_EXCHANGE for now, or an explicit ack from
> the maintainers.

Thanks for the review, Miklos! Adding Christian to Cc wrt binderfs ... full context
for all patches: https://lore.kernel.org/bpf/20211028094724.59043-1-lmb@cloudflare.com/

> Bpffs is your baby...
> 
> Thanks,
> Miklos
> 

Re: [PATCH bpf-next v3 2/4] libfs: support RENAME_EXCHANGE in simple_rename()

[Christian Brauner]

On Tue, Nov 02, 2021 at 11:11:02AM +0100, Daniel Borkmann wrote:
> On 11/2/21 10:25 AM, Miklos Szeredi wrote:
> > On Thu, 28 Oct 2021 at 11:48, Lorenz Bauer <lmb@cloudflare.com> wrote:
> > > 
> > > Allow atomic exchange via RENAME_EXCHANGE when using simple_rename.
> > > This affects binderfs, ramfs, hubetlbfs and bpffs.
> > 
> > Ramfs and hugetlbfs are generic enough; those seem safe.
> > 
> > Binderfs: I have no idea what this does; binderfs_rename() should

Fwiw, allows dynamic creation and removal of Android binder ipc
devices. Each mount is a separate instance and it's mountable inside
unprivileged containers. Since Android 12 default how binder devices are
managed. Also makes it possibe to run Android in unprivileged
containers.

> > probably error out on RENAME_EXCHANGE for now, or an explicit ack from
> > the maintainers.
> 
> Thanks for the review, Miklos! Adding Christian to Cc wrt binderfs ... full context
> for all patches: https://lore.kernel.org/bpf/20211028094724.59043-1-lmb@cloudflare.com/

Yep, I saw that. Seems good.

> probably error out on RENAME_EXCHANGE for now, or an explicit ack from
> the maintainers.

I don't think there is any issue in allowing binderfs to support this.
Binderfs files are always device nodes. Allowing them to be atomically
renamed shouldn't be a problem. So:

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Christian

[PATCH bpf-next v3 3/4] selftests: bpf: convert test_bpffs to ASSERT macros

[Lorenz Bauer]

Remove usage of deprecated CHECK macros.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../selftests/bpf/prog_tests/test_bpffs.c     | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpffs.c b/tools/testing/selftests/bpf/prog_tests/test_bpffs.c
index 172c999e523c..533e3f3a459a 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_bpffs.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_bpffs.c
@@ -29,43 +29,43 @@ static int read_iter(char *file)
 
 static int fn(void)
 {
-	int err, duration = 0;
+	int err;
 
 	err = unshare(CLONE_NEWNS);
-	if (CHECK(err, "unshare", "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "unshare"))
 		goto out;
 
 	err = mount("", "/", "", MS_REC | MS_PRIVATE, NULL);
-	if (CHECK(err, "mount /", "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "mount /"))
 		goto out;
 
 	err = umount(TDIR);
-	if (CHECK(err, "umount " TDIR, "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "umount " TDIR))
 		goto out;
 
 	err = mount("none", TDIR, "tmpfs", 0, NULL);
-	if (CHECK(err, "mount", "mount root failed: %d\n", errno))
+	if (!ASSERT_OK(err, "mount tmpfs"))
 		goto out;
 
 	err = mkdir(TDIR "/fs1", 0777);
-	if (CHECK(err, "mkdir "TDIR"/fs1", "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "mkdir " TDIR "/fs1"))
 		goto out;
 	err = mkdir(TDIR "/fs2", 0777);
-	if (CHECK(err, "mkdir "TDIR"/fs2", "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "mkdir " TDIR "/fs2"))
 		goto out;
 
 	err = mount("bpf", TDIR "/fs1", "bpf", 0, NULL);
-	if (CHECK(err, "mount bpffs "TDIR"/fs1", "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "mount bpffs " TDIR "/fs1"))
 		goto out;
 	err = mount("bpf", TDIR "/fs2", "bpf", 0, NULL);
-	if (CHECK(err, "mount bpffs " TDIR "/fs2", "failed: %d\n", errno))
+	if (!ASSERT_OK(err, "mount bpffs " TDIR "/fs2"))
 		goto out;
 
 	err = read_iter(TDIR "/fs1/maps.debug");
-	if (CHECK(err, "reading " TDIR "/fs1/maps.debug", "failed\n"))
+	if (!ASSERT_OK(err, "reading " TDIR "/fs1/maps.debug"))
 		goto out;
 	err = read_iter(TDIR "/fs2/progs.debug");
-	if (CHECK(err, "reading " TDIR "/fs2/progs.debug", "failed\n"))
+	if (!ASSERT_OK(err, "reading " TDIR "/fs2/progs.debug"))
 		goto out;
 out:
 	umount(TDIR "/fs1");
-- 
2.32.0

[PATCH bpf-next v3 4/4] selftests: bpf: test RENAME_EXCHANGE and RENAME_NOREPLACE on bpffs

[Lorenz Bauer]

Add tests to exercise the behaviour of RENAME_EXCHANGE and RENAME_NOREPLACE
on bpffs. The former checks that after an exchange the inode of two
directories has changed. The latter checks that the source still exists
after a failed rename.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../selftests/bpf/prog_tests/test_bpffs.c     | 65 ++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpffs.c b/tools/testing/selftests/bpf/prog_tests/test_bpffs.c
index 533e3f3a459a..d29ebfeef9c5 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_bpffs.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_bpffs.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2020 Facebook */
 #define _GNU_SOURCE
+#include <stdio.h>
 #include <sched.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
@@ -29,7 +30,8 @@ static int read_iter(char *file)
 
 static int fn(void)
 {
-	int err;
+	struct stat a, b, c;
+	int err, map;
 
 	err = unshare(CLONE_NEWNS);
 	if (!ASSERT_OK(err, "unshare"))
@@ -67,6 +69,67 @@ static int fn(void)
 	err = read_iter(TDIR "/fs2/progs.debug");
 	if (!ASSERT_OK(err, "reading " TDIR "/fs2/progs.debug"))
 		goto out;
+
+	err = mkdir(TDIR "/fs1/a", 0777);
+	if (!ASSERT_OK(err, "creating " TDIR "/fs1/a"))
+		goto out;
+	err = mkdir(TDIR "/fs1/a/1", 0777);
+	if (!ASSERT_OK(err, "creating " TDIR "/fs1/a/1"))
+		goto out;
+	err = mkdir(TDIR "/fs1/b", 0777);
+	if (!ASSERT_OK(err, "creating " TDIR "/fs1/b"))
+		goto out;
+
+	map = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 4, 1, 0);
+	if (!ASSERT_GT(map, 0, "create_map(ARRAY)"))
+		goto out;
+	err = bpf_obj_pin(map, TDIR "/fs1/c");
+	if (!ASSERT_OK(err, "pin map"))
+		goto out;
+	close(map);
+
+	/* Check that RENAME_EXCHANGE works for directories. */
+	err = stat(TDIR "/fs1/a", &a);
+	if (!ASSERT_OK(err, "stat(" TDIR "/fs1/a)"))
+		goto out;
+	err = renameat2(0, TDIR "/fs1/a", 0, TDIR "/fs1/b", RENAME_EXCHANGE);
+	if (!ASSERT_OK(err, "renameat2(/fs1/a, /fs1/b, RENAME_EXCHANGE)"))
+		goto out;
+	err = stat(TDIR "/fs1/b", &b);
+	if (!ASSERT_OK(err, "stat(" TDIR "/fs1/b)"))
+		goto out;
+	if (!ASSERT_EQ(a.st_ino, b.st_ino, "b should have a's inode"))
+		goto out;
+	err = access(TDIR "/fs1/b/1", F_OK);
+	if (!ASSERT_OK(err, "access(" TDIR "/fs1/b/1)"))
+		goto out;
+
+	/* Check that RENAME_EXCHANGE works for mixed file types. */
+	err = stat(TDIR "/fs1/c", &c);
+	if (!ASSERT_OK(err, "stat(" TDIR "/fs1/map)"))
+		goto out;
+	err = renameat2(0, TDIR "/fs1/c", 0, TDIR "/fs1/b", RENAME_EXCHANGE);
+	if (!ASSERT_OK(err, "renameat2(/fs1/c, /fs1/b, RENAME_EXCHANGE)"))
+		goto out;
+	err = stat(TDIR "/fs1/b", &b);
+	if (!ASSERT_OK(err, "stat(" TDIR "/fs1/b)"))
+		goto out;
+	if (!ASSERT_EQ(c.st_ino, b.st_ino, "b should have c's inode"))
+		goto out;
+	err = access(TDIR "/fs1/c/1", F_OK);
+	if (!ASSERT_OK(err, "access(" TDIR "/fs1/c/1)"))
+		goto out;
+
+	/* Check that RENAME_NOREPLACE works. */
+	err = renameat2(0, TDIR "/fs1/b", 0, TDIR "/fs1/a", RENAME_NOREPLACE);
+	if (!ASSERT_ERR(err, "renameat2(RENAME_NOREPLACE)")) {
+		err = -EINVAL;
+		goto out;
+	}
+	err = access(TDIR "/fs1/b", F_OK);
+	if (!ASSERT_OK(err, "access(" TDIR "/fs1/b)"))
+		goto out;
+
 out:
 	umount(TDIR "/fs1");
 	umount(TDIR "/fs2");
-- 
2.32.0