[PATCH] fs: fix acl translation

[PATCH] fs: fix acl translation

[Christian Brauner]

Last cycle we extended the idmapped mounts infrastructure to support idmapped
mounts of idmapped filesystems (No such filesystem yet exist.).
Since then, the meaning of an idmapped mount is a mount whose idmapping is
different from the filesystems idmapping.

While doing that work we missed to adapt the acl translation helpers. They
still assume that checking for the identity mapping is enough. But they need to
use the no_idmapping() helper instead.

Note, POSIX ACLs are always translated right at the userspace-kernel boundary
using the caller's current idmapping and the initial idmapping. The order
depends on whether we're coming from or going to userspace. The filesystem's
idmapping doesn't matter at the border.

Consequently, if a non-idmapped mount is passed we need to make sure to always
pass the initial idmapping as the mount's idmapping and not the filesystem
idmapping. Since it's irrelevant here it would yield invalid ids and prevent
setting acls for filesystems that are mountable in a userns and support posix
acls (tmpfs and fuse).

I verified the regression reported in [1] and verified that this patch fixes
it. A regression test will be added to xfstests in parallel.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=215849 [1]
Fixes: bd303368b776 ("fs: support mapped mounts of mapped filesystems")
Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org> # 5.17
Cc: <regressions@lists.linux.dev>
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---

Hey Linus,

Last cycle we introduced a regression that got reported to me right over Easter
on Bugzilla. It is only relevant for 5.17 and current mainline. Could you
please apply this regression fix?

Thanks!
Christian
---
 fs/posix_acl.c                  | 10 ++++++++++
 fs/xattr.c                      |  6 ++++--
 include/linux/posix_acl_xattr.h |  4 ++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 80acb6885cf9..962d32468eb4 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -759,9 +759,14 @@ static void posix_acl_fix_xattr_userns(
 }
 
 void posix_acl_fix_xattr_from_user(struct user_namespace *mnt_userns,
+				   struct inode *inode,
 				   void *value, size_t size)
 {
 	struct user_namespace *user_ns = current_user_ns();
+
+	/* Leave ids untouched on non-idmapped mounts. */
+	if (no_idmapping(mnt_userns, i_user_ns(inode)))
+		mnt_userns = &init_user_ns;
 	if ((user_ns == &init_user_ns) && (mnt_userns == &init_user_ns))
 		return;
 	posix_acl_fix_xattr_userns(&init_user_ns, user_ns, mnt_userns, value,
@@ -769,9 +774,14 @@ void posix_acl_fix_xattr_from_user(struct user_namespace *mnt_userns,
 }
 
 void posix_acl_fix_xattr_to_user(struct user_namespace *mnt_userns,
+				 struct inode *inode,
 				 void *value, size_t size)
 {
 	struct user_namespace *user_ns = current_user_ns();
+
+	/* Leave ids untouched on non-idmapped mounts. */
+	if (no_idmapping(mnt_userns, i_user_ns(inode)))
+		mnt_userns = &init_user_ns;
 	if ((user_ns == &init_user_ns) && (mnt_userns == &init_user_ns))
 		return;
 	posix_acl_fix_xattr_userns(user_ns, &init_user_ns, mnt_userns, value,
diff --git a/fs/xattr.c b/fs/xattr.c
index 5c8c5175b385..998045165916 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -569,7 +569,8 @@ setxattr(struct user_namespace *mnt_userns, struct dentry *d,
 		}
 		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
 		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
-			posix_acl_fix_xattr_from_user(mnt_userns, kvalue, size);
+			posix_acl_fix_xattr_from_user(mnt_userns, d_inode(d),
+						      kvalue, size);
 	}
 
 	error = vfs_setxattr(mnt_userns, d, kname, kvalue, size, flags);
@@ -667,7 +668,8 @@ getxattr(struct user_namespace *mnt_userns, struct dentry *d,
 	if (error > 0) {
 		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
 		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
-			posix_acl_fix_xattr_to_user(mnt_userns, kvalue, error);
+			posix_acl_fix_xattr_to_user(mnt_userns, d_inode(d),
+						    kvalue, error);
 		if (size && copy_to_user(value, kvalue, error))
 			error = -EFAULT;
 	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
index 060e8d203181..1766e1de6956 100644
--- a/include/linux/posix_acl_xattr.h
+++ b/include/linux/posix_acl_xattr.h
@@ -34,15 +34,19 @@ posix_acl_xattr_count(size_t size)
 
 #ifdef CONFIG_FS_POSIX_ACL
 void posix_acl_fix_xattr_from_user(struct user_namespace *mnt_userns,
+				   struct inode *inode,
 				   void *value, size_t size);
 void posix_acl_fix_xattr_to_user(struct user_namespace *mnt_userns,
+				   struct inode *inode,
 				 void *value, size_t size);
 #else
 static inline void posix_acl_fix_xattr_from_user(struct user_namespace *mnt_userns,
+						 struct inode *inode,
 						 void *value, size_t size)
 {
 }
 static inline void posix_acl_fix_xattr_to_user(struct user_namespace *mnt_userns,
+					       struct inode *inode,
 					       void *value, size_t size)
 {
 }

base-commit: b2d229d4ddb17db541098b83524d901257e93845
-- 
2.32.0

[PATCH] generic: add test for tmpfs POSIX ACLs

[Christian Brauner]

Add a regression test for commit 705191b03d50 ("fs: fix acl translation").
This tests whether setting POSIX ACLs on a tmpfs mounted in a
non-initial user and mount namespace works as expected.

Note, once again the idmapped mount testsuite is grossly misnamed at
this point. It has morphed into a full-blown generic vfs feature
testsuite.

Cc: Eryu Guan <guaneryu@gmail.com>
Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Zorro Lang <zlang@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
Hey,

As promised yesterday in
https://lore.kernel.org/linux-fsdevel/20220419131423.2367795-1-brauner@kernel.org
this adds a regression test to xfstests.

Thanks!
Christian
---
 src/idmapped-mounts/idmapped-mounts.c | 140 +++++++++++++++++++++++++-
 tests/generic/683                     |  32 ++++++
 tests/generic/683.out                 |   2 +
 3 files changed, 173 insertions(+), 1 deletion(-)
 create mode 100755 tests/generic/683
 create mode 100644 tests/generic/683.out

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 4cf6c3bb..7a0aeb3d 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -13791,6 +13791,128 @@ out:
 	return fret;
 }
 
+/**
+ * setxattr_fix_705191b03d50 - test for commit 705191b03d50 ("fs: fix acl translation").
+ */
+static int setxattr_fix_705191b03d50(void)
+{
+	int fret = -1;
+	int fd_userns = -EBADF;
+	int ret;
+	uid_t user1_uid;
+	gid_t user1_gid;
+	pid_t pid;
+	struct list idmap;
+	struct list *it_cur, *it_next;
+
+	list_init(&idmap);
+
+	if (!lookup_ids(USER1, &user1_uid, &user1_gid)) {
+		log_stderr("failure: lookup_user");
+		goto out;
+	}
+
+	log_debug("Found " USER1 " with uid(%d) and gid(%d)", user1_uid, user1_gid);
+
+	if (mkdirat(t_dir1_fd, DIR1, 0777)) {
+		log_stderr("failure: mkdirat");
+		goto out;
+	}
+
+	if (chown_r(t_mnt_fd, T_DIR1, user1_uid, user1_gid)) {
+		log_stderr("failure: chown_r");
+		goto out;
+	}
+
+	print_r(t_mnt_fd, T_DIR1);
+
+	/* u:0:user1_uid:1 */
+	ret = add_map_entry(&idmap, user1_uid, 0, 1, ID_TYPE_UID);
+	if (ret) {
+		log_stderr("failure: add_map_entry");
+		goto out;
+	}
+
+	/* g:0:user1_gid:1 */
+	ret = add_map_entry(&idmap, user1_gid, 0, 1, ID_TYPE_GID);
+	if (ret) {
+		log_stderr("failure: add_map_entry");
+		goto out;
+	}
+
+	/* u:100:10000:100 */
+	ret = add_map_entry(&idmap, 10000, 100, 100, ID_TYPE_UID);
+	if (ret) {
+		log_stderr("failure: add_map_entry");
+		goto out;
+	}
+
+	/* g:100:10000:100 */
+	ret = add_map_entry(&idmap, 10000, 100, 100, ID_TYPE_GID);
+	if (ret) {
+		log_stderr("failure: add_map_entry");
+		goto out;
+	}
+
+	fd_userns = get_userns_fd_from_idmap(&idmap);
+	if (fd_userns < 0) {
+		log_stderr("failure: get_userns_fd");
+		goto out;
+	}
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		if (!switch_userns(fd_userns, 0, 0, false))
+			die("failure: switch_userns");
+
+		/* create separate mount namespace */
+		if (unshare(CLONE_NEWNS))
+			die("failure: create new mount namespace");
+
+		/* turn off mount propagation */
+		if (sys_mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0))
+			die("failure: turn mount propagation off");
+
+		snprintf(t_buf, sizeof(t_buf), "%s/%s/%s", t_mountpoint, T_DIR1, DIR1);
+
+		if (sys_mount("none", t_buf, "tmpfs", 0, "mode=0755"))
+			die("failure: mount");
+
+		snprintf(t_buf, sizeof(t_buf), "%s/%s/%s/%s", t_mountpoint, T_DIR1, DIR1, DIR3);
+		if (mkdir(t_buf, 0700))
+			die("failure: mkdir");
+
+		snprintf(t_buf, sizeof(t_buf), "setfacl -m u:100:rwx %s/%s/%s/%s", t_mountpoint, T_DIR1, DIR1, DIR3);
+		if (system(t_buf))
+			die("failure: system");
+
+		snprintf(t_buf, sizeof(t_buf), "getfacl -n -p %s/%s/%s/%s | grep -q user:100:rwx", t_mountpoint, T_DIR1, DIR1, DIR3);
+		if (system(t_buf))
+			die("failure: system");
+
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(fd_userns);
+
+	list_for_each_safe(it_cur, &idmap, it_next) {
+		list_del(it_cur);
+		free(it_cur->elem);
+		free(it_cur);
+	}
+
+	return fret;
+}
+
 static void usage(void)
 {
 	fprintf(stderr, "Description:\n");
@@ -13809,6 +13931,7 @@ static void usage(void)
 	fprintf(stderr, "--test-nested-userns                Run nested userns idmapped mount testsuite\n");
 	fprintf(stderr, "--test-btrfs                        Run btrfs specific idmapped mount testsuite\n");
 	fprintf(stderr, "--test-setattr-fix-968219708108     Run setattr regression tests\n");
+	fprintf(stderr, "--test-setxattr-fix-705191b03d50    Run setxattr regression tests\n");
 
 	_exit(EXIT_SUCCESS);
 }
@@ -13826,6 +13949,7 @@ static const struct option longopts[] = {
 	{"test-nested-userns",			no_argument,		0,	'n'},
 	{"test-btrfs",				no_argument,		0,	'b'},
 	{"test-setattr-fix-968219708108",	no_argument,		0,	'i'},
+	{"test-setxattr-fix-705191b03d50",	no_argument,		0,	'j'},
 	{NULL,					0,			0,	  0},
 };
 
@@ -13923,6 +14047,11 @@ struct t_idmapped_mounts t_setattr_fix_968219708108[] = {
 	{ setattr_fix_968219708108,					true,	"test that setattr works correctly",								},
 };
 
+/* Test for commit 705191b03d50 ("fs: fix acl translation"). */
+struct t_idmapped_mounts t_setxattr_fix_705191b03d50[] = {
+	{ setxattr_fix_705191b03d50,					false,	"test that setxattr works correctly for userns mountable filesystems",				},
+};
+
 static bool run_test(struct t_idmapped_mounts suite[], size_t suite_size)
 {
 	int i;
@@ -14000,7 +14129,8 @@ int main(int argc, char *argv[])
 	int index = 0;
 	bool supported = false, test_btrfs = false, test_core = false,
 	     test_fscaps_regression = false, test_nested_userns = false,
-	     test_setattr_fix_968219708108 = false;
+	     test_setattr_fix_968219708108 = false,
+	     test_setxattr_fix_705191b03d50 = false;
 
 	while ((ret = getopt_long_only(argc, argv, "", longopts, &index)) != -1) {
 		switch (ret) {
@@ -14037,6 +14167,9 @@ int main(int argc, char *argv[])
 		case 'i':
 			test_setattr_fix_968219708108 = true;
 			break;
+		case 'j':
+			test_setxattr_fix_705191b03d50 = true;
+			break;
 		case 'h':
 			/* fallthrough */
 		default:
@@ -14106,6 +14239,11 @@ int main(int argc, char *argv[])
 		      ARRAY_SIZE(t_setattr_fix_968219708108)))
 		goto out;
 
+	if (test_setxattr_fix_705191b03d50 &&
+	    !run_test(t_setxattr_fix_705191b03d50,
+		      ARRAY_SIZE(t_setxattr_fix_705191b03d50)))
+		goto out;
+
 	fret = EXIT_SUCCESS;
 
 out:
diff --git a/tests/generic/683 b/tests/generic/683
new file mode 100755
index 00000000..397548ed
--- /dev/null
+++ b/tests/generic/683
@@ -0,0 +1,32 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Christian Brauner (Microsoft).  All Rights Reserved.
+#
+# FS QA Test No. 683
+#
+# Test that setting POSIX ACLs in userns-mountable filesystems works.
+#
+# Regression test for commit:
+#
+# 705191b03d50 ("fs: fix acl translation")
+#
+. ./common/preamble
+_begin_fstest auto quick perms
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+_supported_fs generic
+_require_test
+_require_user fsgqa
+_require_group fsgqa
+
+echo "Silence is golden"
+
+$here/src/idmapped-mounts/idmapped-mounts --test-setxattr-fix-705191b03d50 \
+	--device "$TEST_DEV" --mount "$TEST_DIR" --fstype "$FSTYP"
+
+status=$?
+exit
diff --git a/tests/generic/683.out b/tests/generic/683.out
new file mode 100644
index 00000000..7f2a2ace
--- /dev/null
+++ b/tests/generic/683.out
@@ -0,0 +1,2 @@
+QA output created by 683
+Silence is golden

base-commit: fbc6486be09c93a68d3863ebf7e3ed851fc4721c
-- 
2.32.0

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Zorro Lang]

On Wed, Apr 20, 2022 at 07:52:22PM +0200, Christian Brauner wrote:
> Add a regression test for commit 705191b03d50 ("fs: fix acl translation").
> This tests whether setting POSIX ACLs on a tmpfs mounted in a
> non-initial user and mount namespace works as expected.
> 
> Note, once again the idmapped mount testsuite is grossly misnamed at
> this point. It has morphed into a full-blown generic vfs feature
> testsuite.

Hi,

Good to know that, the idmapped-mounts things already been extended to 15k+
lines[1] code, it's even much more than the unionmount-testsuite[2]. So I
think it's time to think about shifting it from fstests/src to be an independent
testsuit, we can learn what 35c7a37928fd ("overlay: run unionmount testsuite test
cases") did, maintain idmapped-mounts testsuite outside, then let fstests to be a
wrapper to run it.


[1]
$ wc -l src/idmapped-mounts/*.[ch]
 14113 src/idmapped-mounts/idmapped-mounts.c
   151 src/idmapped-mounts/missing.h
   201 src/idmapped-mounts/mount-idmapped.c
   425 src/idmapped-mounts/utils.c
   130 src/idmapped-mounts/utils.h
 15020 total

[2]
https://github.com/amir73il/unionmount-testsuite

> 
> Cc: Eryu Guan <guaneryu@gmail.com>
> Cc: Seth Forshee <sforshee@digitalocean.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Zorro Lang <zlang@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> ---
> Hey,
> 
> As promised yesterday in
> https://lore.kernel.org/linux-fsdevel/20220419131423.2367795-1-brauner@kernel.org
> this adds a regression test to xfstests.
> 
> Thanks!
> Christian
> ---
>  src/idmapped-mounts/idmapped-mounts.c | 140 +++++++++++++++++++++++++-
>  tests/generic/683                     |  32 ++++++
>  tests/generic/683.out                 |   2 +
>  3 files changed, 173 insertions(+), 1 deletion(-)
>  create mode 100755 tests/generic/683
>  create mode 100644 tests/generic/683.out
> 

[snip]

> diff --git a/tests/generic/683 b/tests/generic/683
> new file mode 100755
> index 00000000..397548ed
> --- /dev/null
> +++ b/tests/generic/683
> @@ -0,0 +1,32 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Christian Brauner (Microsoft).  All Rights Reserved.
> +#
> +# FS QA Test No. 683
> +#
> +# Test that setting POSIX ACLs in userns-mountable filesystems works.
> +#
> +# Regression test for commit:
> +#
> +# 705191b03d50 ("fs: fix acl translation")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick perms
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +_supported_fs generic
> +_require_test

Better to have _require_idmapped_mounts at here. I'd like to leave idmapped-mounts.c
part for vfs reviewing.

Thanks for this new testing coverage,
Zorro

> +_require_user fsgqa
> +_require_group fsgqa
> +
> +echo "Silence is golden"
> +
> +$here/src/idmapped-mounts/idmapped-mounts --test-setxattr-fix-705191b03d50 \
> +	--device "$TEST_DEV" --mount "$TEST_DIR" --fstype "$FSTYP"
> +
> +status=$?
> +exit
> diff --git a/tests/generic/683.out b/tests/generic/683.out
> new file mode 100644
> index 00000000..7f2a2ace
> --- /dev/null
> +++ b/tests/generic/683.out
> @@ -0,0 +1,2 @@
> +QA output created by 683
> +Silence is golden
> 
> base-commit: fbc6486be09c93a68d3863ebf7e3ed851fc4721c
> -- 
> 2.32.0
> 

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Christian Brauner]

On Thu, Apr 21, 2022 at 01:41:20PM +0800, Zorro Lang wrote:
> On Wed, Apr 20, 2022 at 07:52:22PM +0200, Christian Brauner wrote:
> > Add a regression test for commit 705191b03d50 ("fs: fix acl translation").
> > This tests whether setting POSIX ACLs on a tmpfs mounted in a
> > non-initial user and mount namespace works as expected.
> > 
> > Note, once again the idmapped mount testsuite is grossly misnamed at
> > this point. It has morphed into a full-blown generic vfs feature
> > testsuite.
> 
> Hi,
> 
> Good to know that, the idmapped-mounts things already been extended to 15k+
> lines[1] code, it's even much more than the unionmount-testsuite[2]. So I
> think it's time to think about shifting it from fstests/src to be an independent
> testsuit, we can learn what 35c7a37928fd ("overlay: run unionmount testsuite test
> cases") did, maintain idmapped-mounts testsuite outside, then let fstests to be a
> wrapper to run it.

I'd like to avoid that. The testsuite tests a lot of core vfs
functionality - completely indepenent of idmapped mounts which is why I
should rename it - that isn't covered anwywhere else in xfstests.
It also contains various regressions tests for core vfs work.
Let's keep it in a single repo which will guarantee us that it will be
run as part of xfstests.

Christian

> 
> 
> [1]
> $ wc -l src/idmapped-mounts/*.[ch]
>  14113 src/idmapped-mounts/idmapped-mounts.c
>    151 src/idmapped-mounts/missing.h
>    201 src/idmapped-mounts/mount-idmapped.c
>    425 src/idmapped-mounts/utils.c
>    130 src/idmapped-mounts/utils.h
>  15020 total
> 
> [2]
> https://github.com/amir73il/unionmount-testsuite
> 
> > 
> > Cc: Eryu Guan <guaneryu@gmail.com>
> > Cc: Seth Forshee <sforshee@digitalocean.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Zorro Lang <zlang@redhat.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > ---
> > Hey,
> > 
> > As promised yesterday in
> > https://lore.kernel.org/linux-fsdevel/20220419131423.2367795-1-brauner@kernel.org
> > this adds a regression test to xfstests.
> > 
> > Thanks!
> > Christian
> > ---
> >  src/idmapped-mounts/idmapped-mounts.c | 140 +++++++++++++++++++++++++-
> >  tests/generic/683                     |  32 ++++++
> >  tests/generic/683.out                 |   2 +
> >  3 files changed, 173 insertions(+), 1 deletion(-)
> >  create mode 100755 tests/generic/683
> >  create mode 100644 tests/generic/683.out
> > 
> 
> [snip]
> 
> > diff --git a/tests/generic/683 b/tests/generic/683
> > new file mode 100755
> > index 00000000..397548ed
> > --- /dev/null
> > +++ b/tests/generic/683
> > @@ -0,0 +1,32 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Christian Brauner (Microsoft).  All Rights Reserved.
> > +#
> > +# FS QA Test No. 683
> > +#
> > +# Test that setting POSIX ACLs in userns-mountable filesystems works.
> > +#
> > +# Regression test for commit:
> > +#
> > +# 705191b03d50 ("fs: fix acl translation")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick perms
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +
> > +_supported_fs generic
> > +_require_test
> 
> Better to have _require_idmapped_mounts at here. I'd like to leave idmapped-mounts.c
> part for vfs reviewing.
> 
> Thanks for this new testing coverage,
> Zorro
> 
> > +_require_user fsgqa
> > +_require_group fsgqa
> > +
> > +echo "Silence is golden"
> > +
> > +$here/src/idmapped-mounts/idmapped-mounts --test-setxattr-fix-705191b03d50 \
> > +	--device "$TEST_DEV" --mount "$TEST_DIR" --fstype "$FSTYP"
> > +
> > +status=$?
> > +exit
> > diff --git a/tests/generic/683.out b/tests/generic/683.out
> > new file mode 100644
> > index 00000000..7f2a2ace
> > --- /dev/null
> > +++ b/tests/generic/683.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 683
> > +Silence is golden
> > 
> > base-commit: fbc6486be09c93a68d3863ebf7e3ed851fc4721c
> > -- 
> > 2.32.0
> > 
> 

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Christoph Hellwig]

On Thu, Apr 21, 2022 at 09:05:13AM +0200, Christian Brauner wrote:
> I'd like to avoid that. The testsuite tests a lot of core vfs
> functionality - completely indepenent of idmapped mounts which is why I
> should rename it - that isn't covered anwywhere else in xfstests.
> It also contains various regressions tests for core vfs work.
> Let's keep it in a single repo which will guarantee us that it will be
> run as part of xfstests.

Yeah, that unionmount testsuite is something no casual users will
run as it requires way too much effort to be usable.

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Amir Goldstein]

On Thu, Apr 21, 2022 at 10:18 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Apr 21, 2022 at 09:05:13AM +0200, Christian Brauner wrote:
> > I'd like to avoid that. The testsuite tests a lot of core vfs
> > functionality - completely indepenent of idmapped mounts which is why I
> > should rename it - that isn't covered anwywhere else in xfstests.
> > It also contains various regressions tests for core vfs work.
> > Let's keep it in a single repo which will guarantee us that it will be
> > run as part of xfstests.
>
> Yeah, that unionmount testsuite is something no casual users will
> run as it requires way too much effort to be usable.

Please elaborate.

Quoting from README.overlay:

To enable running unionmount testsuite, clone the git repository from:
  https://github.com/amir73il/unionmount-testsuite.git
under the xfstests src directory, or set the environment variable
UNIONMOUNT_TESTSUITE to the local path where the repository was cloned.

Is that what you are referring to by "way too much effort to be usable"?

To be clear, I too am in favor of keeping unionmount testsuite in separate
repo and keeping idmapped-mounts inside xfstests.

Just puzzled about your comment.

Thanks,
Amir.

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Christoph Hellwig]

On Thu, Apr 21, 2022 at 10:52:35AM +0300, Amir Goldstein wrote:
> Please elaborate.
> 
> Quoting from README.overlay:
> 
> To enable running unionmount testsuite, clone the git repository from:
>   https://github.com/amir73il/unionmount-testsuite.git
> under the xfstests src directory, or set the environment variable
> UNIONMOUNT_TESTSUITE to the local path where the repository was cloned.
> 
> Is that what you are referring to by "way too much effort to be usable"?

I need to install another test suite and point to it.  That isn't
exactly an easy setup compared to having one self contained one.  If it
is just one testsuite for a specific file system that might be fine
(but then why even bother wiring up in xfstests?), but doing that
for lots of bits that test core code is just a nightmare.

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Dave Chinner]

On Thu, Apr 21, 2022 at 01:41:20PM +0800, Zorro Lang wrote:
> On Wed, Apr 20, 2022 at 07:52:22PM +0200, Christian Brauner wrote:
> > Add a regression test for commit 705191b03d50 ("fs: fix acl translation").
> > This tests whether setting POSIX ACLs on a tmpfs mounted in a
> > non-initial user and mount namespace works as expected.
> > 
> > Note, once again the idmapped mount testsuite is grossly misnamed at
> > this point. It has morphed into a full-blown generic vfs feature
> > testsuite.

Yup, and that's really important because this is the exact purpose
for which fstests exists: to cover every aspect of filesystem and
VFS functionality that needs test coverage.

> Hi,
> 
> Good to know that, the idmapped-mounts things already been extended to 15k+
> lines[1] code, it's even much more than the unionmount-testsuite[2]. So I
> think it's time to think about shifting it from fstests/src to be an independent
> testsuit,

Please don't do that. That will mean the testing most fs developers
will get -zero- idmapped-mount test coverage and that's a major
issue. The kernel code that it covers is non trivial, has deep hooks
into every filesystem and these tests ensure that we filesystem
developers don't accidentally break it. It *needs* to be a part of
every filesystem developer's test environment, and we already have
that by having it integrated into fstests.

This is what we want - we want fstests to be the place that fs
developers add new tests to cover new functionality, and so all
filesystems get coverage of that functionality as part of the
development process.

This is exactly what we've spent the last 20 years building xfstests
into - it's gone from a filesystem specific tests suite to
supporting a dozen different filesystems and covers heaps of VFS
functionality as well.

As such, I think the last thing we want to be doing is telling
people to "take their code elsewhere". It sends the wrong message -
we want people to incorporate their complex test code into fstests
because that benefits every developer who uses fstests in their
daily workflow. The more test coverage we get, the better, and the
more test code we get integrated into fstests the better off we will
be.

> we can learn what 35c7a37928fd ("overlay: run unionmount testsuite test
> cases") did, maintain idmapped-mounts testsuite outside, then let fstests to be a
> wrapper to run it.

The unionmount tests suite was never a part of fstests like the
idmapped tests suite is. Very few developers know it exists, let
alone install it. Even fewer run it because it's part of the overlay
tests and almost nobody except overlay developers run overlay
testing...


-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Zorro Lang]

On Thu, Apr 21, 2022 at 06:59:42PM +1000, Dave Chinner wrote:
> On Thu, Apr 21, 2022 at 01:41:20PM +0800, Zorro Lang wrote:
> > On Wed, Apr 20, 2022 at 07:52:22PM +0200, Christian Brauner wrote:
> > > Add a regression test for commit 705191b03d50 ("fs: fix acl translation").
> > > This tests whether setting POSIX ACLs on a tmpfs mounted in a
> > > non-initial user and mount namespace works as expected.
> > > 
> > > Note, once again the idmapped mount testsuite is grossly misnamed at
> > > this point. It has morphed into a full-blown generic vfs feature
> > > testsuite.
> 
> Yup, and that's really important because this is the exact purpose
> for which fstests exists: to cover every aspect of filesystem and
> VFS functionality that needs test coverage.
> 
> > Hi,
> > 
> > Good to know that, the idmapped-mounts things already been extended to 15k+
> > lines[1] code, it's even much more than the unionmount-testsuite[2]. So I
> > think it's time to think about shifting it from fstests/src to be an independent
> > testsuit,
> 
> Please don't do that. That will mean the testing most fs developers
> will get -zero- idmapped-mount test coverage and that's a major
> issue. The kernel code that it covers is non trivial, has deep hooks
> into every filesystem and these tests ensure that we filesystem
> developers don't accidentally break it. It *needs* to be a part of
> every filesystem developer's test environment, and we already have
> that by having it integrated into fstests.

Sure, I won't do that wilfully, just try to ask how we can improve this
huge and 'keep growing' idmapped-mounts.c, not tend to remove the whole
idmapped-mount testing coverage :)

I agree with you, fstests won't reduce existed fs testing coverage, or
much more carefully to do that if have to do. fstests will be more compatible,
And welcome more contributors choose fstests to be their regression
testsuite.

Thanks,
Zirong

> 
> This is what we want - we want fstests to be the place that fs
> developers add new tests to cover new functionality, and so all
> filesystems get coverage of that functionality as part of the
> development process.
> 
> This is exactly what we've spent the last 20 years building xfstests
> into - it's gone from a filesystem specific tests suite to
> supporting a dozen different filesystems and covers heaps of VFS
> functionality as well.
> 
> As such, I think the last thing we want to be doing is telling
> people to "take their code elsewhere". It sends the wrong message -
> we want people to incorporate their complex test code into fstests
> because that benefits every developer who uses fstests in their
> daily workflow. The more test coverage we get, the better, and the
> more test code we get integrated into fstests the better off we will
> be.
> 
> > we can learn what 35c7a37928fd ("overlay: run unionmount testsuite test
> > cases") did, maintain idmapped-mounts testsuite outside, then let fstests to be a
> > wrapper to run it.
> 
> The unionmount tests suite was never a part of fstests like the
> idmapped tests suite is. Very few developers know it exists, let
> alone install it. Even fewer run it because it's part of the overlay
> tests and almost nobody except overlay developers run overlay
> testing...
> 
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Christoph Hellwig]

On Thu, Apr 21, 2022 at 11:35:13PM +0800, Zorro Lang wrote:
> Sure, I won't do that wilfully, just try to ask how we can improve this
> huge and 'keep growing' idmapped-mounts.c, not tend to remove the whole
> idmapped-mount testing coverage :)

It might just be time to split that file up into a few ones if there
is a sensible split.  I'll let Christian think about that, though.

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Christian Brauner]

On Thu, Apr 21, 2022 at 05:37:17PM +0200, Christoph Hellwig wrote:
> On Thu, Apr 21, 2022 at 11:35:13PM +0800, Zorro Lang wrote:
> > Sure, I won't do that wilfully, just try to ask how we can improve this
> > huge and 'keep growing' idmapped-mounts.c, not tend to remove the whole
> > idmapped-mount testing coverage :)
> 
> It might just be time to split that file up into a few ones if there
> is a sensible split.  I'll let Christian think about that, though.

Yep, I agree. I think we need to at least rename it to reflect is vfs
generic nature and then split it into separate test binaries.
I'll think about a good approach.

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Amir Goldstein]

On Thu, Apr 21, 2022 at 9:05 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Apr 21, 2022 at 05:37:17PM +0200, Christoph Hellwig wrote:
> > On Thu, Apr 21, 2022 at 11:35:13PM +0800, Zorro Lang wrote:
> > > Sure, I won't do that wilfully, just try to ask how we can improve this
> > > huge and 'keep growing' idmapped-mounts.c, not tend to remove the whole
> > > idmapped-mount testing coverage :)
> >
> > It might just be time to split that file up into a few ones if there
> > is a sensible split.  I'll let Christian think about that, though.
>
> Yep, I agree. I think we need to at least rename it to reflect is vfs
> generic nature and then split it into separate test binaries.
> I'll think about a good approach.

The majority of test cases still do require_fs_allow_idmap and from
those who don't, most of them are variants for test cases that run
with and without idmapped mounts and possibly also in_userns.

And this new test case is no exception - there is still idmapping
involved, just not idmapped-mounts.

However you decide to break it up and/or rename the test binary
(I am not sure you must split the binary - only the source files),
I think we need to be more consistent about the groups that the tests
that run this binary are in.

'perms' group is adequate, but adding to the 'idmapped' group and
maybe also to a new 'userns' group would be useful.

BTW, the tests that use src/nsexec should also belong to the userns
group as does overlay/020, the only test that uses the 'unshare' tool.

Thanks,
Amir.

Re: [PATCH] generic: add test for tmpfs POSIX ACLs

[Christian Brauner]

On Sat, Apr 23, 2022 at 10:17:59AM +0300, Amir Goldstein wrote:
> On Thu, Apr 21, 2022 at 9:05 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Apr 21, 2022 at 05:37:17PM +0200, Christoph Hellwig wrote:
> > > On Thu, Apr 21, 2022 at 11:35:13PM +0800, Zorro Lang wrote:
> > > > Sure, I won't do that wilfully, just try to ask how we can improve this
> > > > huge and 'keep growing' idmapped-mounts.c, not tend to remove the whole
> > > > idmapped-mount testing coverage :)
> > >
> > > It might just be time to split that file up into a few ones if there
> > > is a sensible split.  I'll let Christian think about that, though.
> >
> > Yep, I agree. I think we need to at least rename it to reflect is vfs
> > generic nature and then split it into separate test binaries.
> > I'll think about a good approach.
> 
> The majority of test cases still do require_fs_allow_idmap and from
> those who don't, most of them are variants for test cases that run
> with and without idmapped mounts and possibly also in_userns.

Just to clarify a few points in more detail to expand on the "grossly
misnamed" theme:

Given that this started out as a dedicated testsuite to provide almost
obsessive testing of idmapped mounts the majority of tests is of course
about idmapped mounts. It'd be rather concerning if it wasn't, in
addition to also being misnamed.

But to put numbers to it, we do have 74 test functions that each have a
separate theme. Spread across the 74 test functions we do have ~120 test
cases (Basically each fork() invocation in each of those 74 test
functions can be considered a separate test case.).

Of these 74 test functions we do have 12 generic vfs test functions,
i.e. test functions that are not concerned with idmapped mounts.

Just going by the number of test functions, not actual test cases that's
almost 20% of the test suite concerned with generic vfs behavior.

Plus, there's additional patchsets that extend the generic behavior
which brings in another ~4-8 additional generic test functions with
multiple test cases. And people will keep adding to it.

> 
> And this new test case is no exception - there is still idmapping
> involved, just not idmapped-mounts.

The point was that there's a decent number of tests that aren't
concerned with idmapped mounts. And this new test-case is only relevant
for tmpfs mounted in a non-initial userns. So I'm not sure what this is
trying to say.

The argument is simply that there is a non-trivial number of tests that
are not concerned with idmapped mounts. But the name of the test binary
does imply that it is only concerned with idmapped mounts when it
clearly isn't. So it is misnamed at this point causing understandable
confusion.

Iow, given that I and others have to respond to a patch or add a comment
in the commit message of a patch to remind people that the thing they're
patching doesn't just do what it says on the tin, we should probably
rename it or do something else to improve the situation.

It'd be concerning to have a can labeled "tomato soup" but to then
discover that is also has spaghetti in it. That might be a nice suprise
but I'd still better put it on the label. :)

That's beside the point that the source file is 15k+ LOC strong which is
just obscene. :)

> 
> However you decide to break it up and/or rename the test binary
> (I am not sure you must split the binary - only the source files),

Rename we must imho; but I haven't yet decided whether or not we really
should use separate binaries. I think I'd make that dependent on whether
or not a good generic name can be found. :)

> I think we need to be more consistent about the groups that the tests
> that run this binary are in.
> 
> 'perms' group is adequate, but adding to the 'idmapped' group and
> maybe also to a new 'userns' group would be useful.
> 
> BTW, the tests that use src/nsexec should also belong to the userns
> group as does overlay/020, the only test that uses the 'unshare' tool.

Good points.