[PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[Yang Xu]

If we run case on old kernel that doesn't support mount_setattr and
then fail on our own function before call is_setgid/is_setuid function
to reset errno, run_test will print "Function not implement" error.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 4cf6c3bb..8e6405c5 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
 		die("failed to open %s", t_mountpoint_scratch);
 
 	t_fs_allow_idmap = fs_allow_idmap();
+	/* don't copy ENOSYS errno to child process on older kernel */
+	errno = 0;
 	if (supported) {
 		/*
 		 * Caller just wants to know whether the filesystem we're on
-- 
2.27.0

[PATCH v3 2/5] idmapped-mounts: Add mknodat operation in setgid test

[Yang Xu]

Since mknodat can create file, we should also check whether strip S_ISGID.
Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
depend on this cap and keep other cap(ie CAP_MKNOD) because create character
device needs it when using mknod.

Only test mknodat with character device in setgid_create function and the another
two functions test mknodat with whiteout device.

Since kernel commit a3c751a50 ("vfs: allow unprivileged whiteout creation") in
v5.8-rc1, we can create whiteout device in userns test. Since kernel 5.12, mount_setattr
and MOUNT_ATTR_IDMAP was supported, we don't need to detect kernel whether allow
unprivileged whiteout creation. Using fs_allow_idmap as a proxy is safe.

Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 219 +++++++++++++++++++++++++-
 1 file changed, 213 insertions(+), 6 deletions(-)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 8e6405c5..617f56e0 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -241,6 +241,34 @@ static inline bool caps_supported(void)
 	return ret;
 }
 
+static int caps_down_fsetid(void)
+{
+	bool fret = false;
+#ifdef HAVE_SYS_CAPABILITY_H
+	cap_t caps = NULL;
+	cap_value_t cap = CAP_FSETID;
+	int ret = -1;
+
+	caps = cap_get_proc();
+	if (!caps)
+		goto out;
+
+	ret = cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap, 0);
+	if (ret)
+		goto out;
+
+	ret = cap_set_proc(caps);
+	if (ret)
+		goto out;
+
+	fret = true;
+
+out:
+	cap_free(caps);
+#endif
+	return fret;
+}
+
 /* caps_down - lower all effective caps */
 static int caps_down(void)
 {
@@ -7805,8 +7833,8 @@ out_unmap:
 #endif /* HAVE_LIBURING_H */
 
 /* The following tests are concerned with setgid inheritance. These can be
- * filesystem type specific. For xfs, if a new file or directory is created
- * within a setgid directory and irix_sgid_inhiert is set then inherit the
+ * filesystem type specific. For xfs, if a new file or directory or node is
+ * created within a setgid directory and irix_sgid_inhiert is set then inherit the
  * setgid bit if the caller is in the group of the directory.
  */
 static int setgid_create(void)
@@ -7863,18 +7891,44 @@ static int setgid_create(void)
 		if (!is_setgid(t_dir1_fd, DIR1, 0))
 			die("failure: is_setgid");
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (!is_setgid(t_dir1_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (!is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
 		if (!expected_uid_gid(t_dir1_fd, FILE1, 0, 0, 0))
 			die("failure: check ownership");
 
 		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(t_dir1_fd, FILE2, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
+			die("failure: check ownership");
+
 		if (unlinkat(t_dir1_fd, FILE1, 0))
 			die("failure: delete");
 
 		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
 			die("failure: delete");
 
+		if (unlinkat(t_dir1_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -7889,8 +7943,8 @@ static int setgid_create(void)
 		if (!switch_ids(0, 10000))
 			die("failure: switch_ids");
 
-		if (!caps_down())
-			die("failure: caps_down");
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
 
 		/* create regular file via open() */
 		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
@@ -7917,6 +7971,19 @@ static int setgid_create(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
 		/*
 		 * In setgid directories newly created files always inherit the
 		 * gid from the parent directory. Verify that the file is owned
@@ -7933,6 +8000,24 @@ static int setgid_create(void)
 		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(t_dir1_fd, FILE2, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (unlinkat(t_dir1_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8035,6 +8120,20 @@ static int setgid_create_idmapped(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
 		/*
 		 * In setgid directories newly created files always inherit the
 		 * gid from the parent directory. Verify that the file is owned
@@ -8051,6 +8150,24 @@ static int setgid_create_idmapped(void)
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 10000, 10000))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, FILE2, 0, 10000, 10000))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(open_tree_fd, CHRDEV1, 0, 10000, 10000))
+			die("failure: check ownership");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8149,18 +8266,44 @@ static int setgid_create_idmapped_in_userns(void)
 		if (!is_setgid(open_tree_fd, DIR1, 0))
 			die("failure: is_setgid");
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (!is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (!is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
 		if (!expected_uid_gid(open_tree_fd, FILE1, 0, 0, 0))
 			die("failure: check ownership");
 
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, FILE2, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(open_tree_fd, CHRDEV1, 0, 0, 0))
+			die("failure: check ownership");
+
 		if (unlinkat(open_tree_fd, FILE1, 0))
 			die("failure: delete");
 
 		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
 			die("failure: delete");
 
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8190,9 +8333,12 @@ static int setgid_create_idmapped_in_userns(void)
 			exit(EXIT_SUCCESS);
 		}
 
-		if (!switch_userns(attr.userns_fd, 0, 0, true))
+		if (!switch_userns(attr.userns_fd, 0, 0, false))
 			die("failure: switch_userns");
 
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
 		/* create regular file via open() */
 		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
 		if (file1_fd < 0)
@@ -8218,6 +8364,20 @@ static int setgid_create_idmapped_in_userns(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
 		/*
 		 * In setgid directories newly created files always inherit the
 		 * gid from the parent directory. Verify that the file is owned
@@ -8234,12 +8394,24 @@ static int setgid_create_idmapped_in_userns(void)
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 1000))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, FILE2, 0, 0, 1000))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(open_tree_fd, CHRDEV1, 0, 0, 1000))
+			die("failure: check ownership");
+
 		if (unlinkat(open_tree_fd, FILE1, 0))
 			die("failure: delete");
 
 		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
 			die("failure: delete");
 
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8266,9 +8438,12 @@ static int setgid_create_idmapped_in_userns(void)
 			exit(EXIT_SUCCESS);
 		}
 
-		if (!switch_userns(attr.userns_fd, 0, 1000, true))
+		if (!switch_userns(attr.userns_fd, 0, 1000, false))
 			die("failure: switch_userns");
 
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
 		/* create regular file via open() */
 		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
 		if (file1_fd < 0)
@@ -8295,12 +8470,44 @@ static int setgid_create_idmapped_in_userns(void)
 				die("failure: is_setgid");
 		}
 
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
 		if (!expected_uid_gid(open_tree_fd, FILE1, 0, 0, 0))
 			die("failure: check ownership");
 
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 0))
 			die("failure: check ownership");
 
+		if (!expected_uid_gid(open_tree_fd, FILE2, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (!expected_uid_gid(open_tree_fd, CHRDEV1, 0, 0, 0))
+			die("failure: check ownership");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
-- 
2.27.0

Re: [PATCH v3 2/5] idmapped-mounts: Add mknodat operation in setgid test

[Christian Brauner]

On Tue, Apr 12, 2022 at 07:33:43PM +0800, Yang Xu wrote:
> Since mknodat can create file, we should also check whether strip S_ISGID.
> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
> depend on this cap and keep other cap(ie CAP_MKNOD) because create character
> device needs it when using mknod.
> 
> Only test mknodat with character device in setgid_create function and the another
> two functions test mknodat with whiteout device.
> 
> Since kernel commit a3c751a50 ("vfs: allow unprivileged whiteout creation") in
> v5.8-rc1, we can create whiteout device in userns test. Since kernel 5.12, mount_setattr
> and MOUNT_ATTR_IDMAP was supported, we don't need to detect kernel whether allow
> unprivileged whiteout creation. Using fs_allow_idmap as a proxy is safe.
> 
> Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  src/idmapped-mounts/idmapped-mounts.c | 219 +++++++++++++++++++++++++-
>  1 file changed, 213 insertions(+), 6 deletions(-)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index 8e6405c5..617f56e0 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -241,6 +241,34 @@ static inline bool caps_supported(void)
>  	return ret;
>  }
>  
> +static int caps_down_fsetid(void)
> +{
> +	bool fret = false;
> +#ifdef HAVE_SYS_CAPABILITY_H
> +	cap_t caps = NULL;
> +	cap_value_t cap = CAP_FSETID;
> +	int ret = -1;
> +
> +	caps = cap_get_proc();
> +	if (!caps)
> +		goto out;
> +
> +	ret = cap_set_flag(caps, CAP_EFFECTIVE, 1, &cap, 0);
> +	if (ret)
> +		goto out;
> +
> +	ret = cap_set_proc(caps);
> +	if (ret)
> +		goto out;
> +
> +	fret = true;
> +
> +out:
> +	cap_free(caps);
> +#endif
> +	return fret;
> +}
> +
>  /* caps_down - lower all effective caps */
>  static int caps_down(void)
>  {
> @@ -7805,8 +7833,8 @@ out_unmap:
>  #endif /* HAVE_LIBURING_H */
>  
>  /* The following tests are concerned with setgid inheritance. These can be
> - * filesystem type specific. For xfs, if a new file or directory is created
> - * within a setgid directory and irix_sgid_inhiert is set then inherit the
> + * filesystem type specific. For xfs, if a new file or directory or node is
> + * created within a setgid directory and irix_sgid_inhiert is set then inherit the
>   * setgid bit if the caller is in the group of the directory.
>   */
>  static int setgid_create(void)
> @@ -7863,18 +7891,44 @@ static int setgid_create(void)
>  		if (!is_setgid(t_dir1_fd, DIR1, 0))
>  			die("failure: is_setgid");
>  
> +		/* create a special file via mknodat() vfs_create */
> +		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
> +			die("failure: mknodat");
> +
> +		if (!is_setgid(t_dir1_fd, FILE2, 0))
> +			die("failure: is_setgid");
> +
> +		/* create a character device via mknodat() vfs_mknod */
> +		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
> +			die("failure: mknodat");
> +
> +		if (!is_setgid(t_dir1_fd, CHRDEV1, 0))
> +			die("failure: is_setgid");
> +
>  		if (!expected_uid_gid(t_dir1_fd, FILE1, 0, 0, 0))
>  			die("failure: check ownership");
>  
>  		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
>  			die("failure: check ownership");
>  
> +		if (!expected_uid_gid(t_dir1_fd, FILE2, 0, 0, 0))
> +			die("failure: check ownership");
> +
> +		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
> +			die("failure: check ownership");
> +
>  		if (unlinkat(t_dir1_fd, FILE1, 0))
>  			die("failure: delete");
>  
>  		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
>  			die("failure: delete");
>  
> +		if (unlinkat(t_dir1_fd, FILE2, 0))
> +			die("failure: delete");
> +
> +		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
> +			die("failure: delete");
> +
>  		exit(EXIT_SUCCESS);
>  	}
>  	if (wait_for_pid(pid))
> @@ -7889,8 +7943,8 @@ static int setgid_create(void)
>  		if (!switch_ids(0, 10000))
>  			die("failure: switch_ids");
>  
> -		if (!caps_down())
> -			die("failure: caps_down");
> +		if (!caps_down_fsetid())
> +			die("failure: caps_down_fsetid");
>  
>  		/* create regular file via open() */
>  		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> @@ -7917,6 +7971,19 @@ static int setgid_create(void)
>  				die("failure: is_setgid");
>  		}

Please see my comment on the earlier thread:
https://lore.kernel.org/linux-fsdevel/20220407134009.s4shhomfxjz5cf5r@wittgenstein

The same issue still exists afaict, i.e. you're not handling the irix
case.

Re: [PATCH v3 2/5] idmapped-mounts: Add mknodat operation in setgid test

[xuyang2018.jy]

on 2022/4/13 15:59, Christian Brauner wrote:
> On Tue, Apr 12, 2022 at 07:33:43PM +0800, Yang Xu wrote:
>> Since mknodat can create file, we should also check whether strip S_ISGID.
>> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
>> depend on this cap and keep other cap(ie CAP_MKNOD) because create character
>> device needs it when using mknod.
>>
>> Only test mknodat with character device in setgid_create function and the another
>> two functions test mknodat with whiteout device.
>>
>> Since kernel commit a3c751a50 ("vfs: allow unprivileged whiteout creation") in
>> v5.8-rc1, we can create whiteout device in userns test. Since kernel 5.12, mount_setattr
>> and MOUNT_ATTR_IDMAP was supported, we don't need to detect kernel whether allow
>> unprivileged whiteout creation. Using fs_allow_idmap as a proxy is safe.
>>
>> Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   src/idmapped-mounts/idmapped-mounts.c | 219 +++++++++++++++++++++++++-
>>   1 file changed, 213 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index 8e6405c5..617f56e0 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -241,6 +241,34 @@ static inline bool caps_supported(void)
>>   	return ret;
>>   }
>>
>> +static int caps_down_fsetid(void)
>> +{
>> +	bool fret = false;
>> +#ifdef HAVE_SYS_CAPABILITY_H
>> +	cap_t caps = NULL;
>> +	cap_value_t cap = CAP_FSETID;
>> +	int ret = -1;
>> +
>> +	caps = cap_get_proc();
>> +	if (!caps)
>> +		goto out;
>> +
>> +	ret = cap_set_flag(caps, CAP_EFFECTIVE, 1,&cap, 0);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = cap_set_proc(caps);
>> +	if (ret)
>> +		goto out;
>> +
>> +	fret = true;
>> +
>> +out:
>> +	cap_free(caps);
>> +#endif
>> +	return fret;
>> +}
>> +
>>   /* caps_down - lower all effective caps */
>>   static int caps_down(void)
>>   {
>> @@ -7805,8 +7833,8 @@ out_unmap:
>>   #endif /* HAVE_LIBURING_H */
>>
>>   /* The following tests are concerned with setgid inheritance. These can be
>> - * filesystem type specific. For xfs, if a new file or directory is created
>> - * within a setgid directory and irix_sgid_inhiert is set then inherit the
>> + * filesystem type specific. For xfs, if a new file or directory or node is
>> + * created within a setgid directory and irix_sgid_inhiert is set then inherit the
>>    * setgid bit if the caller is in the group of the directory.
>>    */
>>   static int setgid_create(void)
>> @@ -7863,18 +7891,44 @@ static int setgid_create(void)
>>   		if (!is_setgid(t_dir1_fd, DIR1, 0))
>>   			die("failure: is_setgid");
>>
>> +		/* create a special file via mknodat() vfs_create */
>> +		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
>> +			die("failure: mknodat");
>> +
>> +		if (!is_setgid(t_dir1_fd, FILE2, 0))
>> +			die("failure: is_setgid");
>> +
>> +		/* create a character device via mknodat() vfs_mknod */
>> +		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
>> +			die("failure: mknodat");
>> +
>> +		if (!is_setgid(t_dir1_fd, CHRDEV1, 0))
>> +			die("failure: is_setgid");
>> +
>>   		if (!expected_uid_gid(t_dir1_fd, FILE1, 0, 0, 0))
>>   			die("failure: check ownership");
>>
>>   		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
>>   			die("failure: check ownership");
>>
>> +		if (!expected_uid_gid(t_dir1_fd, FILE2, 0, 0, 0))
>> +			die("failure: check ownership");
>> +
>> +		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
>> +			die("failure: check ownership");
>> +
>>   		if (unlinkat(t_dir1_fd, FILE1, 0))
>>   			die("failure: delete");
>>
>>   		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
>>   			die("failure: delete");
>>
>> +		if (unlinkat(t_dir1_fd, FILE2, 0))
>> +			die("failure: delete");
>> +
>> +		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
>> +			die("failure: delete");
>> +
>>   		exit(EXIT_SUCCESS);
>>   	}
>>   	if (wait_for_pid(pid))
>> @@ -7889,8 +7943,8 @@ static int setgid_create(void)
>>   		if (!switch_ids(0, 10000))
>>   			die("failure: switch_ids");
>>
>> -		if (!caps_down())
>> -			die("failure: caps_down");
>> +		if (!caps_down_fsetid())
>> +			die("failure: caps_down_fsetid");
>>
>>   		/* create regular file via open() */
>>   		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
>> @@ -7917,6 +7971,19 @@ static int setgid_create(void)
>>   				die("failure: is_setgid");
>>   		}
>
> Please see my comment on the earlier thread:
> https://lore.kernel.org/linux-fsdevel/20220407134009.s4shhomfxjz5cf5r@wittgenstein
>
> The same issue still exists afaict, i.e. you're not handling the irix
> case.
I remember it has two issues
1)replace 0755 with valid flags
2)consider fs.xfs.irix_sgid_inherit enable situation because it will 
strip S_ISGID for directory

I used t_dir1_fd directory instead of old DIR1, so I don't need to raise 
the S_ISGID when we enable fs.xfs.irix_sgid_inherit.

I have tested this on enable and disable fs.xfs.irix_sgid_inherit 
situations, they all pass.

Or I miss something?

Best Regards
Yang Xu

Re: [PATCH v3 2/5] idmapped-mounts: Add mknodat operation in setgid test

[Christian Brauner]

On Wed, Apr 13, 2022 at 08:31:45AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/13 15:59, Christian Brauner wrote:
> > On Tue, Apr 12, 2022 at 07:33:43PM +0800, Yang Xu wrote:
> >> Since mknodat can create file, we should also check whether strip S_ISGID.
> >> Also add new helper caps_down_fsetid to drop CAP_FSETID because strip S_ISGID
> >> depend on this cap and keep other cap(ie CAP_MKNOD) because create character
> >> device needs it when using mknod.
> >>
> >> Only test mknodat with character device in setgid_create function and the another
> >> two functions test mknodat with whiteout device.
> >>
> >> Since kernel commit a3c751a50 ("vfs: allow unprivileged whiteout creation") in
> >> v5.8-rc1, we can create whiteout device in userns test. Since kernel 5.12, mount_setattr
> >> and MOUNT_ATTR_IDMAP was supported, we don't need to detect kernel whether allow
> >> unprivileged whiteout creation. Using fs_allow_idmap as a proxy is safe.
> >>
> >> Tested-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >>   src/idmapped-mounts/idmapped-mounts.c | 219 +++++++++++++++++++++++++-
> >>   1 file changed, 213 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> >> index 8e6405c5..617f56e0 100644
> >> --- a/src/idmapped-mounts/idmapped-mounts.c
> >> +++ b/src/idmapped-mounts/idmapped-mounts.c
> >> @@ -241,6 +241,34 @@ static inline bool caps_supported(void)
> >>   	return ret;
> >>   }
> >>
> >> +static int caps_down_fsetid(void)
> >> +{
> >> +	bool fret = false;
> >> +#ifdef HAVE_SYS_CAPABILITY_H
> >> +	cap_t caps = NULL;
> >> +	cap_value_t cap = CAP_FSETID;
> >> +	int ret = -1;
> >> +
> >> +	caps = cap_get_proc();
> >> +	if (!caps)
> >> +		goto out;
> >> +
> >> +	ret = cap_set_flag(caps, CAP_EFFECTIVE, 1,&cap, 0);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	ret = cap_set_proc(caps);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	fret = true;
> >> +
> >> +out:
> >> +	cap_free(caps);
> >> +#endif
> >> +	return fret;
> >> +}
> >> +
> >>   /* caps_down - lower all effective caps */
> >>   static int caps_down(void)
> >>   {
> >> @@ -7805,8 +7833,8 @@ out_unmap:
> >>   #endif /* HAVE_LIBURING_H */
> >>
> >>   /* The following tests are concerned with setgid inheritance. These can be
> >> - * filesystem type specific. For xfs, if a new file or directory is created
> >> - * within a setgid directory and irix_sgid_inhiert is set then inherit the
> >> + * filesystem type specific. For xfs, if a new file or directory or node is
> >> + * created within a setgid directory and irix_sgid_inhiert is set then inherit the
> >>    * setgid bit if the caller is in the group of the directory.
> >>    */
> >>   static int setgid_create(void)
> >> @@ -7863,18 +7891,44 @@ static int setgid_create(void)
> >>   		if (!is_setgid(t_dir1_fd, DIR1, 0))
> >>   			die("failure: is_setgid");
> >>
> >> +		/* create a special file via mknodat() vfs_create */
> >> +		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
> >> +			die("failure: mknodat");
> >> +
> >> +		if (!is_setgid(t_dir1_fd, FILE2, 0))
> >> +			die("failure: is_setgid");
> >> +
> >> +		/* create a character device via mknodat() vfs_mknod */
> >> +		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
> >> +			die("failure: mknodat");
> >> +
> >> +		if (!is_setgid(t_dir1_fd, CHRDEV1, 0))
> >> +			die("failure: is_setgid");
> >> +
> >>   		if (!expected_uid_gid(t_dir1_fd, FILE1, 0, 0, 0))
> >>   			die("failure: check ownership");
> >>
> >>   		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
> >>   			die("failure: check ownership");
> >>
> >> +		if (!expected_uid_gid(t_dir1_fd, FILE2, 0, 0, 0))
> >> +			die("failure: check ownership");
> >> +
> >> +		if (!expected_uid_gid(t_dir1_fd, CHRDEV1, 0, 0, 0))
> >> +			die("failure: check ownership");
> >> +
> >>   		if (unlinkat(t_dir1_fd, FILE1, 0))
> >>   			die("failure: delete");
> >>
> >>   		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
> >>   			die("failure: delete");
> >>
> >> +		if (unlinkat(t_dir1_fd, FILE2, 0))
> >> +			die("failure: delete");
> >> +
> >> +		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
> >> +			die("failure: delete");
> >> +
> >>   		exit(EXIT_SUCCESS);
> >>   	}
> >>   	if (wait_for_pid(pid))
> >> @@ -7889,8 +7943,8 @@ static int setgid_create(void)
> >>   		if (!switch_ids(0, 10000))
> >>   			die("failure: switch_ids");
> >>
> >> -		if (!caps_down())
> >> -			die("failure: caps_down");
> >> +		if (!caps_down_fsetid())
> >> +			die("failure: caps_down_fsetid");
> >>
> >>   		/* create regular file via open() */
> >>   		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> >> @@ -7917,6 +7971,19 @@ static int setgid_create(void)
> >>   				die("failure: is_setgid");
> >>   		}
> >
> > Please see my comment on the earlier thread:
> > https://lore.kernel.org/linux-fsdevel/20220407134009.s4shhomfxjz5cf5r@wittgenstein
> >
> > The same issue still exists afaict, i.e. you're not handling the irix
> > case.
> I remember it has two issues
> 1)replace 0755 with valid flags
> 2)consider fs.xfs.irix_sgid_inherit enable situation because it will 
> strip S_ISGID for directory
> 
> I used t_dir1_fd directory instead of old DIR1, so I don't need to raise 

Ah, sorry, I missed this. Then this should all be fine:
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

[PATCH v3 3/5] idmapped-mounts: Add open with O_TMPFILE operation in setgid test

[Yang Xu]

Since we can create temp file by using O_TMPFILE flag and filesystem driver also
has this api, we should also check this operation whether strip S_ISGID.

Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 148 ++++++++++++++++++++++++++
 1 file changed, 148 insertions(+)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 617f56e0..02f91558 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -51,6 +51,7 @@
 #define FILE1_RENAME "file1_rename"
 #define FILE2 "file2"
 #define FILE2_RENAME "file2_rename"
+#define FILE3 "file3"
 #define DIR1 "dir1"
 #define DIR2 "dir2"
 #define DIR3 "dir3"
@@ -337,6 +338,24 @@ out:
 	return fret;
 }
 
+static bool openat_tmpfile_supported(int dirfd)
+{
+	int fd = -1;
+
+	fd = openat(dirfd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+	if (fd == -1) {
+		if (errno == ENOTSUP)
+			return false;
+		else
+			return log_errno(false, "failure: create");
+	}
+
+	if (close(fd))
+		log_stderr("failure: close");
+
+	return true;
+}
+
 /* __expected_uid_gid - check whether file is owned by the provided uid and gid */
 static bool __expected_uid_gid(int dfd, const char *path, int flags,
 			       uid_t expected_uid, gid_t expected_gid, bool log)
@@ -7841,7 +7860,10 @@ static int setgid_create(void)
 {
 	int fret = -1;
 	int file1_fd = -EBADF;
+	int tmpfile_fd = -EBADF;
 	pid_t pid;
+	bool supported = false;
+	char path[PATH_MAX];
 
 	if (!caps_supported())
 		return 0;
@@ -7866,6 +7888,8 @@ static int setgid_create(void)
 		goto out;
 	}
 
+	supported = openat_tmpfile_supported(t_dir1_fd);
+
 	pid = fork();
 	if (pid < 0) {
 		log_stderr("failure: fork");
@@ -7929,6 +7953,25 @@ static int setgid_create(void)
 		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (!is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(t_dir1_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8018,6 +8061,25 @@ static int setgid_create(void)
 		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(t_dir1_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8039,6 +8101,9 @@ static int setgid_create_idmapped(void)
 		.attr_set = MOUNT_ATTR_IDMAP,
 	};
 	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
 
 	if (!caps_supported())
 		return 0;
@@ -8086,6 +8151,8 @@ static int setgid_create_idmapped(void)
 		goto out;
 	}
 
+	supported = openat_tmpfile_supported(open_tree_fd);
+
 	pid = fork();
 	if (pid < 0) {
 		log_stderr("failure: fork");
@@ -8168,6 +8235,25 @@ static int setgid_create_idmapped(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 10000, 10000))
+				die("failure: check ownership");
+			if  (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8191,6 +8277,9 @@ static int setgid_create_idmapped_in_userns(void)
 		.attr_set = MOUNT_ATTR_IDMAP,
 	};
 	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
 
 	if (!caps_supported())
 		return 0;
@@ -8238,6 +8327,8 @@ static int setgid_create_idmapped_in_userns(void)
 		goto out;
 	}
 
+	supported = openat_tmpfile_supported(open_tree_fd);
+
 	pid = fork();
 	if (pid < 0) {
 		log_stderr("failure: fork");
@@ -8304,6 +8395,25 @@ static int setgid_create_idmapped_in_userns(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (!is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8412,6 +8522,25 @@ static int setgid_create_idmapped_in_userns(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 0, 1000))
+				die("failure: check ownership");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8508,6 +8637,25 @@ static int setgid_create_idmapped_in_userns(void)
 		if (unlinkat(open_tree_fd, CHRDEV1, 0))
 			die("failure: delete");
 
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (!expected_uid_gid(open_tree_fd, FILE3, 0, 0, 0))
+				die("failure: check ownership");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
-- 
2.27.0

Re: [PATCH v3 3/5] idmapped-mounts: Add open with O_TMPFILE operation in setgid test

[Christian Brauner]

On Tue, Apr 12, 2022 at 07:33:44PM +0800, Yang Xu wrote:
> Since we can create temp file by using O_TMPFILE flag and filesystem driver also
> has this api, we should also check this operation whether strip S_ISGID.
> 
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  src/idmapped-mounts/idmapped-mounts.c | 148 ++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index 617f56e0..02f91558 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -51,6 +51,7 @@
>  #define FILE1_RENAME "file1_rename"
>  #define FILE2 "file2"
>  #define FILE2_RENAME "file2_rename"
> +#define FILE3 "file3"
>  #define DIR1 "dir1"
>  #define DIR2 "dir2"
>  #define DIR3 "dir3"
> @@ -337,6 +338,24 @@ out:
>  	return fret;
>  }
>  
> +static bool openat_tmpfile_supported(int dirfd)
> +{
> +	int fd = -1;
> +
> +	fd = openat(dirfd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
> +	if (fd == -1) {
> +		if (errno == ENOTSUP)
> +			return false;
> +		else
> +			return log_errno(false, "failure: create");
> +	}
> +
> +	if (close(fd))
> +		log_stderr("failure: close");
> +
> +	return true;
> +}
> +
>  /* __expected_uid_gid - check whether file is owned by the provided uid and gid */
>  static bool __expected_uid_gid(int dfd, const char *path, int flags,
>  			       uid_t expected_uid, gid_t expected_gid, bool log)
> @@ -7841,7 +7860,10 @@ static int setgid_create(void)
>  {
>  	int fret = -1;
>  	int file1_fd = -EBADF;
> +	int tmpfile_fd = -EBADF;
>  	pid_t pid;
> +	bool supported = false;
> +	char path[PATH_MAX];
>  
>  	if (!caps_supported())
>  		return 0;
> @@ -7866,6 +7888,8 @@ static int setgid_create(void)
>  		goto out;
>  	}
>  
> +	supported = openat_tmpfile_supported(t_dir1_fd);
> +
>  	pid = fork();
>  	if (pid < 0) {
>  		log_stderr("failure: fork");
> @@ -7929,6 +7953,25 @@ static int setgid_create(void)
>  		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
>  			die("failure: delete");
>  
> +		/* create tmpfile via filesystem tmpfile api */
> +		if (supported) {
> +			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
> +			if (tmpfile_fd < 0)
> +				die("failure: create");
> +			/* link the temporary file into the filesystem, making it permanent */
> +			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
> +			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
> +				die("failure: linkat");

Fwiw, I don't think you need that snprintf() dance as you should be able
to use AT_EMPTY_PATH:

if (linkat(fd, "", t_dir1_fd, FILE3, AT_EMPTY_PATH))

for this.

Re: [PATCH v3 3/5] idmapped-mounts: Add open with O_TMPFILE operation in setgid test

[xuyang2018.jy]

on 2022/4/13 16:07, Christian Brauner wrote:
> On Tue, Apr 12, 2022 at 07:33:44PM +0800, Yang Xu wrote:
>> Since we can create temp file by using O_TMPFILE flag and filesystem driver also
>> has this api, we should also check this operation whether strip S_ISGID.
>>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   src/idmapped-mounts/idmapped-mounts.c | 148 ++++++++++++++++++++++++++
>>   1 file changed, 148 insertions(+)
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index 617f56e0..02f91558 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -51,6 +51,7 @@
>>   #define FILE1_RENAME "file1_rename"
>>   #define FILE2 "file2"
>>   #define FILE2_RENAME "file2_rename"
>> +#define FILE3 "file3"
>>   #define DIR1 "dir1"
>>   #define DIR2 "dir2"
>>   #define DIR3 "dir3"
>> @@ -337,6 +338,24 @@ out:
>>   	return fret;
>>   }
>>
>> +static bool openat_tmpfile_supported(int dirfd)
>> +{
>> +	int fd = -1;
>> +
>> +	fd = openat(dirfd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
>> +	if (fd == -1) {
>> +		if (errno == ENOTSUP)
>> +			return false;
>> +		else
>> +			return log_errno(false, "failure: create");
>> +	}
>> +
>> +	if (close(fd))
>> +		log_stderr("failure: close");
>> +
>> +	return true;
>> +}
>> +
>>   /* __expected_uid_gid - check whether file is owned by the provided uid and gid */
>>   static bool __expected_uid_gid(int dfd, const char *path, int flags,
>>   			       uid_t expected_uid, gid_t expected_gid, bool log)
>> @@ -7841,7 +7860,10 @@ static int setgid_create(void)
>>   {
>>   	int fret = -1;
>>   	int file1_fd = -EBADF;
>> +	int tmpfile_fd = -EBADF;
>>   	pid_t pid;
>> +	bool supported = false;
>> +	char path[PATH_MAX];
>>
>>   	if (!caps_supported())
>>   		return 0;
>> @@ -7866,6 +7888,8 @@ static int setgid_create(void)
>>   		goto out;
>>   	}
>>
>> +	supported = openat_tmpfile_supported(t_dir1_fd);
>> +
>>   	pid = fork();
>>   	if (pid<  0) {
>>   		log_stderr("failure: fork");
>> @@ -7929,6 +7953,25 @@ static int setgid_create(void)
>>   		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
>>   			die("failure: delete");
>>
>> +		/* create tmpfile via filesystem tmpfile api */
>> +		if (supported) {
>> +			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
>> +			if (tmpfile_fd<  0)
>> +				die("failure: create");
>> +			/* link the temporary file into the filesystem, making it permanent */
>> +			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
>> +			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
>> +				die("failure: linkat");
>
> Fwiw, I don't think you need that snprintf() dance as you should be able
> to use AT_EMPTY_PATH:
>
> if (linkat(fd, "", t_dir1_fd, FILE3, AT_EMPTY_PATH))
>
> for this.
Oh, Yes, it works well. Thanks.

ps:I also use this way but failed before(I used wrong argument NULL 
instead of "" when see open(2) man-pages ) .

Best Regards
Yang Xu

[PATCH v3 4/5] idmapped-mounts: Add new setgid_create_umask test

[Yang Xu]

The current_umask() is stripped from the mode directly in the vfs if the
filesystem either doesn't support acls or the filesystem has been
mounted without posic acl support.

If the filesystem does support acls then current_umask() stripping is
deferred to posix_acl_create(). So when the filesystem calls
posix_acl_create() and there are no acls set or not supported then
current_umask() will be stripped.

Here we only use umask(S_IXGRP) to check whether inode strip
S_ISGID works correctly.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 505 +++++++++++++++++++++++++-
 tests/generic/680                     |  26 ++
 tests/generic/680.out                 |   2 +
 3 files changed, 532 insertions(+), 1 deletion(-)
 create mode 100755 tests/generic/680
 create mode 100644 tests/generic/680.out

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 02f91558..e6c14586 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -14146,6 +14146,494 @@ out:
 	return fret;
 }
 
+/* The following tests are concerned with setgid inheritance. These can be
+ * filesystem type specific. For xfs, if a new file or directory or node is
+ * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
+ * setgid bit if the caller is in the group of the directory.
+ *
+ * The current_umask() is stripped from the mode directly in the vfs if the
+ * filesystem either doesn't support acls or the filesystem has been
+ * mounted without posic acl support.
+ *
+ * If the filesystem does support acls then current_umask() stripping is
+ * deferred to posix_acl_create(). So when the filesystem calls
+ * posix_acl_create() and there are no acls set or not supported then
+ * current_umask() will be stripped.
+ *
+ * Use umask(S_IXGRP) to check whether inode strip S_ISGID works correctly.
+ */
+static int setgid_create_umask(void)
+{
+	int fret = -1;
+	int file1_fd = -EBADF;
+	int tmpfile_fd = -EBADF;
+	pid_t pid;
+	bool supported = false;
+	char path[PATH_MAX];
+	mode_t mode;
+
+	if (!caps_supported())
+		return 0;
+
+	if (fchmod(t_dir1_fd, S_IRUSR |
+			      S_IWUSR |
+			      S_IRGRP |
+			      S_IWGRP |
+			      S_IROTH |
+			      S_IWOTH |
+			      S_IXUSR |
+			      S_IXGRP |
+			      S_IXOTH |
+			      S_ISGID), 0) {
+		log_stderr("failure: fchmod");
+		goto out;
+	}
+
+	   /* Verify that the setgid bit got raised. */
+	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
+		log_stderr("failure: is_setgid");
+		goto out;
+	}
+
+	supported = openat_tmpfile_supported(t_dir1_fd);
+
+	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
+	 * whether has group execute or search permission.
+	 */
+	umask(S_IXGRP);
+	mode = umask(S_IXGRP);
+	if (!(mode & S_IXGRP))
+		die("failure: umask");
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		if (!switch_ids(0, 10000))
+			die("failure: switch_ids");
+
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
+		/* create regular file via open() */
+		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(t_dir1_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(t_dir1_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(t_dir1_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(t_dir1_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(t_dir1_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(file1_fd);
+	return fret;
+}
+
+static int setgid_create_umask_idmapped(void)
+{
+	int fret = -1;
+	int file1_fd = -EBADF, open_tree_fd = -EBADF;
+	struct mount_attr attr = {
+		.attr_set = MOUNT_ATTR_IDMAP,
+	};
+	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
+	mode_t mode;
+
+	if (!caps_supported())
+		return 0;
+
+	if (fchmod(t_dir1_fd, S_IRUSR |
+			      S_IWUSR |
+			      S_IRGRP |
+			      S_IWGRP |
+			      S_IROTH |
+			      S_IWOTH |
+			      S_IXUSR |
+			      S_IXGRP |
+			      S_IXOTH |
+			      S_ISGID), 0) {
+		log_stderr("failure: fchmod");
+		goto out;
+	}
+
+	/* Verify that the sid bits got raised. */
+	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
+		log_stderr("failure: is_setgid");
+		goto out;
+	}
+
+	/* Changing mount properties on a detached mount. */
+	attr.userns_fd  = get_userns_fd(0, 10000, 10000);
+	if (attr.userns_fd < 0) {
+		log_stderr("failure: get_userns_fd");
+		goto out;
+	}
+
+	open_tree_fd = sys_open_tree(t_dir1_fd, "",
+				     AT_EMPTY_PATH |
+				     AT_NO_AUTOMOUNT |
+				     AT_SYMLINK_NOFOLLOW |
+				     OPEN_TREE_CLOEXEC |
+				     OPEN_TREE_CLONE);
+	if (open_tree_fd < 0) {
+		log_stderr("failure: sys_open_tree");
+		goto out;
+	}
+
+	if (sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr))) {
+		log_stderr("failure: sys_mount_setattr");
+		goto out;
+	}
+
+	supported = openat_tmpfile_supported(open_tree_fd);
+
+	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
+	 * whether has group execute or search permission.
+	 */
+	umask(S_IXGRP);
+	mode = umask(S_IXGRP);
+	if (!(mode & S_IXGRP))
+		die("failure: umask");
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		if (!switch_ids(10000, 11000))
+			die("failure: switch fsids");
+
+		/* create regular file via open() */
+		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(open_tree_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(open_tree_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(attr.userns_fd);
+	safe_close(file1_fd);
+	safe_close(open_tree_fd);
+
+	return fret;
+}
+
+static int setgid_create_umask_idmapped_in_userns(void)
+{
+	int fret = -1;
+	int file1_fd = -EBADF, open_tree_fd = -EBADF;
+	struct mount_attr attr = {
+		.attr_set = MOUNT_ATTR_IDMAP,
+	};
+	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
+	mode_t mode;
+
+	if (!caps_supported())
+		return 0;
+
+	if (fchmod(t_dir1_fd, S_IRUSR |
+			      S_IWUSR |
+			      S_IRGRP |
+			      S_IWGRP |
+			      S_IROTH |
+			      S_IWOTH |
+			      S_IXUSR |
+			      S_IXGRP |
+			      S_IXOTH |
+			      S_ISGID), 0) {
+		log_stderr("failure: fchmod");
+		goto out;
+	}
+
+	/* Verify that the sid bits got raised. */
+	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
+		log_stderr("failure: is_setgid");
+		goto out;
+	}
+
+	/* Changing mount properties on a detached mount. */
+	attr.userns_fd  = get_userns_fd(0, 10000, 10000);
+	if (attr.userns_fd < 0) {
+		log_stderr("failure: get_userns_fd");
+		goto out;
+	}
+
+	open_tree_fd = sys_open_tree(t_dir1_fd, "",
+				     AT_EMPTY_PATH |
+				     AT_NO_AUTOMOUNT |
+				     AT_SYMLINK_NOFOLLOW |
+				     OPEN_TREE_CLOEXEC |
+				     OPEN_TREE_CLONE);
+	if (open_tree_fd < 0) {
+		log_stderr("failure: sys_open_tree");
+		goto out;
+	}
+
+	if (sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr))) {
+		log_stderr("failure: sys_mount_setattr");
+		goto out;
+	}
+
+	supported = openat_tmpfile_supported(open_tree_fd);
+
+	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
+	 * whether has group execute or search permission.
+	 */
+	umask(S_IXGRP);
+	mode = umask(S_IXGRP);
+	if (!(mode & S_IXGRP))
+		die("failure: umask");
+
+	/* Below we verify that setgid inheritance for a newly created file or
+	 * directory works correctly. As part of this we need to verify that
+	 * newly created files or directories inherit their gid from their
+	 * parent directory. So we change the parent directorie's gid to 1000
+	 * and create a file with fs{g,u}id 0 and verify that the newly created
+	 * file and directory inherit gid 1000, not 0.
+	 */
+	if (fchownat(t_dir1_fd, "", -1, 1000, AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) {
+		log_stderr("failure: fchownat");
+		goto out;
+	}
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		if (!switch_userns(attr.userns_fd, 0, 0, false))
+			die("failure: switch_userns");
+
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
+		/* create regular file via open() */
+		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(open_tree_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(open_tree_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(attr.userns_fd);
+	safe_close(file1_fd);
+	safe_close(open_tree_fd);
+
+	return fret;
+}
+
 static void usage(void)
 {
 	fprintf(stderr, "Description:\n");
@@ -14164,6 +14652,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-setgid-umask                 Run setgid create umask test\n");
 
 	_exit(EXIT_SUCCESS);
 }
@@ -14181,6 +14670,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-setgid-umask",			no_argument,		0,	'u'},
 	{NULL,					0,			0,	  0},
 };
 
@@ -14278,6 +14768,12 @@ struct t_idmapped_mounts t_setattr_fix_968219708108[] = {
 	{ setattr_fix_968219708108,					true,	"test that setattr works correctly",								},
 };
 
+struct t_idmapped_mounts t_setgid_umask[] = {
+	{ setgid_create_umask,						false,	"create operations by using umask in directories with setgid bit set",					},
+	{ setgid_create_umask_idmapped,					true,	"create operations by using umask in directories with setgid bit set on idmapped mount",		},
+	{ setgid_create_umask_idmapped_in_userns,			true,	"create operations by using umask in directories with setgid bit set on idmapped mounts inside userns",	},
+};
+
 static bool run_test(struct t_idmapped_mounts suite[], size_t suite_size)
 {
 	int i;
@@ -14355,7 +14851,7 @@ 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_setgid_umask = false;
 
 	while ((ret = getopt_long_only(argc, argv, "", longopts, &index)) != -1) {
 		switch (ret) {
@@ -14392,6 +14888,9 @@ int main(int argc, char *argv[])
 		case 'i':
 			test_setattr_fix_968219708108 = true;
 			break;
+		case 'u':
+			test_setgid_umask = true;
+			break;
 		case 'h':
 			/* fallthrough */
 		default:
@@ -14463,6 +14962,10 @@ int main(int argc, char *argv[])
 		      ARRAY_SIZE(t_setattr_fix_968219708108)))
 		goto out;
 
+	if (test_setgid_umask &&
+	    !run_test(t_setgid_umask, ARRAY_SIZE(t_setgid_umask)))
+		goto out;
+
 	fret = EXIT_SUCCESS;
 
 out:
diff --git a/tests/generic/680 b/tests/generic/680
new file mode 100755
index 00000000..aa9c7375
--- /dev/null
+++ b/tests/generic/680
@@ -0,0 +1,26 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test 680
+#
+# Test that idmapped mounts setgid's behave correctly when using umask.
+#
+. ./common/preamble
+_begin_fstest auto quick cap idmapped mount perms rw unlink
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+_supported_fs generic
+_require_test
+
+echo "Silence is golden"
+
+$here/src/idmapped-mounts/idmapped-mounts --test-setgid-umask --device "$TEST_DEV" \
+	--mount "$TEST_DIR" --fstype "$FSTYP"
+
+status=$?
+exit
diff --git a/tests/generic/680.out b/tests/generic/680.out
new file mode 100644
index 00000000..f4950cda
--- /dev/null
+++ b/tests/generic/680.out
@@ -0,0 +1,2 @@
+QA output created by 680
+Silence is golden
-- 
2.27.0

Re: [PATCH v3 4/5] idmapped-mounts: Add new setgid_create_umask test

[Christian Brauner]

On Tue, Apr 12, 2022 at 07:33:45PM +0800, Yang Xu wrote:
> The current_umask() is stripped from the mode directly in the vfs if the
> filesystem either doesn't support acls or the filesystem has been
> mounted without posic acl support.
> 
> If the filesystem does support acls then current_umask() stripping is
> deferred to posix_acl_create(). So when the filesystem calls
> posix_acl_create() and there are no acls set or not supported then
> current_umask() will be stripped.
> 
> Here we only use umask(S_IXGRP) to check whether inode strip
> S_ISGID works correctly.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  src/idmapped-mounts/idmapped-mounts.c | 505 +++++++++++++++++++++++++-
>  tests/generic/680                     |  26 ++
>  tests/generic/680.out                 |   2 +
>  3 files changed, 532 insertions(+), 1 deletion(-)
>  create mode 100755 tests/generic/680
>  create mode 100644 tests/generic/680.out
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index 02f91558..e6c14586 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -14146,6 +14146,494 @@ out:
>  	return fret;
>  }
>  
> +/* The following tests are concerned with setgid inheritance. These can be
> + * filesystem type specific. For xfs, if a new file or directory or node is
> + * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
> + * setgid bit if the caller is in the group of the directory.
> + *
> + * The current_umask() is stripped from the mode directly in the vfs if the
> + * filesystem either doesn't support acls or the filesystem has been
> + * mounted without posic acl support.
> + *
> + * If the filesystem does support acls then current_umask() stripping is
> + * deferred to posix_acl_create(). So when the filesystem calls
> + * posix_acl_create() and there are no acls set or not supported then
> + * current_umask() will be stripped.
> + *
> + * Use umask(S_IXGRP) to check whether inode strip S_ISGID works correctly.
> + */
> +static int setgid_create_umask(void)
> +{
> +	int fret = -1;
> +	int file1_fd = -EBADF;
> +	int tmpfile_fd = -EBADF;
> +	pid_t pid;
> +	bool supported = false;
> +	char path[PATH_MAX];
> +	mode_t mode;
> +
> +	if (!caps_supported())
> +		return 0;
> +
> +	if (fchmod(t_dir1_fd, S_IRUSR |
> +			      S_IWUSR |
> +			      S_IRGRP |
> +			      S_IWGRP |
> +			      S_IROTH |
> +			      S_IWOTH |
> +			      S_IXUSR |
> +			      S_IXGRP |
> +			      S_IXOTH |
> +			      S_ISGID), 0) {
> +		log_stderr("failure: fchmod");
> +		goto out;
> +	}
> +
> +	   /* Verify that the setgid bit got raised. */
> +	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
> +		log_stderr("failure: is_setgid");
> +		goto out;
> +	}
> +
> +	supported = openat_tmpfile_supported(t_dir1_fd);
> +
> +	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
> +	 * whether has group execute or search permission.
> +	 */
> +	umask(S_IXGRP);
> +	mode = umask(S_IXGRP);
> +	if (!(mode & S_IXGRP))
> +		die("failure: umask");
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		log_stderr("failure: fork");
> +		goto out;
> +	}
> +	if (pid == 0) {
> +		if (!switch_ids(0, 10000))
> +			die("failure: switch_ids");
> +
> +		if (!caps_down_fsetid())
> +			die("failure: caps_down_fsetid");
> +
> +		/* create regular file via open() */
> +		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> +		if (file1_fd < 0)
> +			die("failure: create");
> +
> +		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
> +		 * bit needs to be stripped.
> +		 */
> +		if (is_setgid(t_dir1_fd, FILE1, 0))
> +			die("failure: is_setgid");

This test is wrong. Specifically, it is a false positive. I have
explained this in more detail on v2 of this patchset.

You want to test that umask(S_IXGRP) + setgid inheritance work together
correctly. First, we need to establish what that means because from your
patch series it at least seems to me as you're not completely clear
about what you want to test just yet.

A requested setgid bit for a non-directory object is only considered for
stripping if S_IXGRP is simultaneously requested. In other words, both
S_SISGID and S_IXGRP must be requested for the new file in order for
additional checks such as CAP_FSETID to become relevant.

We need to distinguish two cases afaict:

1. The filesystem does support ACLs and has an applicable ACL
-------------------------------------------------------------

umask(S_IXGRP) is not used by the kernel and thus S_IXGRP is not
stripped (unless the ACL does make it so) and so when e.g.
inode_init_owner() is called we do not expect the file to inherit the
setgid bit.

This is what your test above is handling correctly. But it is a false
positive because what you're trying to test is the behavior of
umask(S_IXGRP) but it is made irrelevant by ACL support of the
underlying filesystem.

2. The filesystem does not support ACLs, has been mounted without
-----------------------------------------------------------------
support for it, or has no applicable ACL
----------------------------------------

umask(S_IXGRP) is used by the kernel and will be stripped from the mode.
So when inode_init_owner() is called we expect the file to inherit the
setgid bit.

This means the test for this case needs to be:

	file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
	if (file1_fd < 0)
		die("failure: create");
	
	/* 
	 * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
	 * new file to be S_ISGID.
	 */
	if (!is_setgid(t_dir1_fd, FILE1, 0))
		die("failure: is_setgid");

And additionally you might want to also add a new helper is_ixgrp() to
add an additional check:

/* 
 * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
 * new file to not be S_IXGRP.
 */
if (is_ixgrp(t_dir1_fd, FILE1, 0))
	die("failure: is_ixgrp");


---

So for the umask() tests you need to first check whether the underlying
filesystem does support ACLs and remember that somewhere. If it does
support ACLs, then you should first remove any ACLs set on the
directories you're performing the tests on. Then you can run your
umask() tests.

Re: [PATCH v3 4/5] idmapped-mounts: Add new setgid_create_umask test

[xuyang2018.jy]

on 2022/4/13 16:59, Christian Brauner wrote:
> On Tue, Apr 12, 2022 at 07:33:45PM +0800, Yang Xu wrote:
>> The current_umask() is stripped from the mode directly in the vfs if the
>> filesystem either doesn't support acls or the filesystem has been
>> mounted without posic acl support.
>>
>> If the filesystem does support acls then current_umask() stripping is
>> deferred to posix_acl_create(). So when the filesystem calls
>> posix_acl_create() and there are no acls set or not supported then
>> current_umask() will be stripped.
>>
>> Here we only use umask(S_IXGRP) to check whether inode strip
>> S_ISGID works correctly.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   src/idmapped-mounts/idmapped-mounts.c | 505 +++++++++++++++++++++++++-
>>   tests/generic/680                     |  26 ++
>>   tests/generic/680.out                 |   2 +
>>   3 files changed, 532 insertions(+), 1 deletion(-)
>>   create mode 100755 tests/generic/680
>>   create mode 100644 tests/generic/680.out
>>
>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>> index 02f91558..e6c14586 100644
>> --- a/src/idmapped-mounts/idmapped-mounts.c
>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>> @@ -14146,6 +14146,494 @@ out:
>>   	return fret;
>>   }
>>
>> +/* The following tests are concerned with setgid inheritance. These can be
>> + * filesystem type specific. For xfs, if a new file or directory or node is
>> + * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
>> + * setgid bit if the caller is in the group of the directory.
>> + *
>> + * The current_umask() is stripped from the mode directly in the vfs if the
>> + * filesystem either doesn't support acls or the filesystem has been
>> + * mounted without posic acl support.
>> + *
>> + * If the filesystem does support acls then current_umask() stripping is
>> + * deferred to posix_acl_create(). So when the filesystem calls
>> + * posix_acl_create() and there are no acls set or not supported then
>> + * current_umask() will be stripped.
>> + *
>> + * Use umask(S_IXGRP) to check whether inode strip S_ISGID works correctly.
>> + */
>> +static int setgid_create_umask(void)
>> +{
>> +	int fret = -1;
>> +	int file1_fd = -EBADF;
>> +	int tmpfile_fd = -EBADF;
>> +	pid_t pid;
>> +	bool supported = false;
>> +	char path[PATH_MAX];
>> +	mode_t mode;
>> +
>> +	if (!caps_supported())
>> +		return 0;
>> +
>> +	if (fchmod(t_dir1_fd, S_IRUSR |
>> +			      S_IWUSR |
>> +			      S_IRGRP |
>> +			      S_IWGRP |
>> +			      S_IROTH |
>> +			      S_IWOTH |
>> +			      S_IXUSR |
>> +			      S_IXGRP |
>> +			      S_IXOTH |
>> +			      S_ISGID), 0) {
>> +		log_stderr("failure: fchmod");
>> +		goto out;
>> +	}
>> +
>> +	   /* Verify that the setgid bit got raised. */
>> +	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
>> +		log_stderr("failure: is_setgid");
>> +		goto out;
>> +	}
>> +
>> +	supported = openat_tmpfile_supported(t_dir1_fd);
>> +
>> +	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
>> +	 * whether has group execute or search permission.
>> +	 */
>> +	umask(S_IXGRP);
>> +	mode = umask(S_IXGRP);
>> +	if (!(mode&  S_IXGRP))
>> +		die("failure: umask");
>> +
>> +	pid = fork();
>> +	if (pid<  0) {
>> +		log_stderr("failure: fork");
>> +		goto out;
>> +	}
>> +	if (pid == 0) {
>> +		if (!switch_ids(0, 10000))
>> +			die("failure: switch_ids");
>> +
>> +		if (!caps_down_fsetid())
>> +			die("failure: caps_down_fsetid");
>> +
>> +		/* create regular file via open() */
>> +		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
>> +		if (file1_fd<  0)
>> +			die("failure: create");
>> +
>> +		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
>> +		 * bit needs to be stripped.
>> +		 */
>> +		if (is_setgid(t_dir1_fd, FILE1, 0))
>> +			die("failure: is_setgid");
>
> This test is wrong. Specifically, it is a false positive. I have
> explained this in more detail on v2 of this patchset.
>
> You want to test that umask(S_IXGRP) + setgid inheritance work together
> correctly. First, we need to establish what that means because from your
> patch series it at least seems to me as you're not completely clear
> about what you want to test just yet.
>
> A requested setgid bit for a non-directory object is only considered for
> stripping if S_IXGRP is simultaneously requested. In other words, both
> S_SISGID and S_IXGRP must be requested for the new file in order for
> additional checks such as CAP_FSETID to become relevant.
Yes, only keep S_IXGRP, then we can run into the next judgement for 
group and cap_fsetid.
>
> We need to distinguish two cases afaict:
>
> 1. The filesystem does support ACLs and has an applicable ACL
> -------------------------------------------------------------
>
> umask(S_IXGRP) is not used by the kernel and thus S_IXGRP is not
> stripped (unless the ACL does make it so) and so when e.g.
> inode_init_owner() is called we do not expect the file to inherit the
> setgid bit.
>
> This is what your test above is handling correctly. But it is a false
> positive because what you're trying to test is the behavior of
> umask(S_IXGRP) but it is made irrelevant by ACL support of the
> underlying filesystem.
I test this situation in the next patch as below:
umask(S_IXGRP);
snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw 
%s/%s", t_mountpoint, T_DIR1)

and
umask(S_IXGRP);
snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx, 
%s/%s", t_mountpoint, T_DIR1

>
> 2. The filesystem does not support ACLs, has been mounted without
> -----------------------------------------------------------------
> support for it, or has no applicable ACL
> ----------------------------------------
>
> umask(S_IXGRP) is used by the kernel and will be stripped from the mode.
> So when inode_init_owner() is called we expect the file to inherit the
> setgid bit.
This is why my kernel patch put strip setgid code into vfs.
xfs will inherit the setgid bit but ext4 not because the new_inode 
function and posix_acl_create function order(S_IXGRP mode has been 
stripped before pass to inode_init_owner).
>
> This means the test for this case needs to be:
>
> 	file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> 	if (file1_fd<  0)
> 		die("failure: create");
> 	
> 	/*
> 	 * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
> 	 * new file to be S_ISGID.
No No No, see the kernel patch url
https://patchwork.kernel.org/project/linux-fsdevel/patch/1648461389-2225-2-git-send-email-xuyang2018.jy@fujitsu.com/
> 	 */
> 	if (!is_setgid(t_dir1_fd, FILE1, 0))
> 		die("failure: is_setgid");
>
> And additionally you might want to also add a new helper is_ixgrp() to
> add an additional check:
>
> /*
>   * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
>   * new file to not be S_IXGRP.
>   */
> if (is_ixgrp(t_dir1_fd, FILE1, 0))
> 	die("failure: is_ixgrp");
This sound reasonable.
>
>
> ---
>
> So for the umask() tests you need to first check whether the underlying
> filesystem does support ACLs and remember that somewhere. If it does
> support ACLs, then you should first remove any ACLs set on the
> directories you're performing the tests on. Then you can run your
> umask() tests.
Yes, I can just add a remove step in the beginning.

ps: When I writing my v2 kernel patch, I found I still miss the GRPID 
testcase in fstests.

Best Regrads
Yang Xu

Re: [PATCH v3 4/5] idmapped-mounts: Add new setgid_create_umask test

[Christian Brauner]

On Wed, Apr 13, 2022 at 09:45:11AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/13 16:59, Christian Brauner wrote:
> > On Tue, Apr 12, 2022 at 07:33:45PM +0800, Yang Xu wrote:
> >> The current_umask() is stripped from the mode directly in the vfs if the
> >> filesystem either doesn't support acls or the filesystem has been
> >> mounted without posic acl support.
> >>
> >> If the filesystem does support acls then current_umask() stripping is
> >> deferred to posix_acl_create(). So when the filesystem calls
> >> posix_acl_create() and there are no acls set or not supported then
> >> current_umask() will be stripped.
> >>
> >> Here we only use umask(S_IXGRP) to check whether inode strip
> >> S_ISGID works correctly.
> >>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >>   src/idmapped-mounts/idmapped-mounts.c | 505 +++++++++++++++++++++++++-
> >>   tests/generic/680                     |  26 ++
> >>   tests/generic/680.out                 |   2 +
> >>   3 files changed, 532 insertions(+), 1 deletion(-)
> >>   create mode 100755 tests/generic/680
> >>   create mode 100644 tests/generic/680.out
> >>
> >> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> >> index 02f91558..e6c14586 100644
> >> --- a/src/idmapped-mounts/idmapped-mounts.c
> >> +++ b/src/idmapped-mounts/idmapped-mounts.c
> >> @@ -14146,6 +14146,494 @@ out:
> >>   	return fret;
> >>   }
> >>
> >> +/* The following tests are concerned with setgid inheritance. These can be
> >> + * filesystem type specific. For xfs, if a new file or directory or node is
> >> + * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
> >> + * setgid bit if the caller is in the group of the directory.
> >> + *
> >> + * The current_umask() is stripped from the mode directly in the vfs if the
> >> + * filesystem either doesn't support acls or the filesystem has been
> >> + * mounted without posic acl support.
> >> + *
> >> + * If the filesystem does support acls then current_umask() stripping is
> >> + * deferred to posix_acl_create(). So when the filesystem calls
> >> + * posix_acl_create() and there are no acls set or not supported then
> >> + * current_umask() will be stripped.
> >> + *
> >> + * Use umask(S_IXGRP) to check whether inode strip S_ISGID works correctly.
> >> + */
> >> +static int setgid_create_umask(void)
> >> +{
> >> +	int fret = -1;
> >> +	int file1_fd = -EBADF;
> >> +	int tmpfile_fd = -EBADF;
> >> +	pid_t pid;
> >> +	bool supported = false;
> >> +	char path[PATH_MAX];
> >> +	mode_t mode;
> >> +
> >> +	if (!caps_supported())
> >> +		return 0;
> >> +
> >> +	if (fchmod(t_dir1_fd, S_IRUSR |
> >> +			      S_IWUSR |
> >> +			      S_IRGRP |
> >> +			      S_IWGRP |
> >> +			      S_IROTH |
> >> +			      S_IWOTH |
> >> +			      S_IXUSR |
> >> +			      S_IXGRP |
> >> +			      S_IXOTH |
> >> +			      S_ISGID), 0) {
> >> +		log_stderr("failure: fchmod");
> >> +		goto out;
> >> +	}
> >> +
> >> +	   /* Verify that the setgid bit got raised. */
> >> +	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
> >> +		log_stderr("failure: is_setgid");
> >> +		goto out;
> >> +	}
> >> +
> >> +	supported = openat_tmpfile_supported(t_dir1_fd);
> >> +
> >> +	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
> >> +	 * whether has group execute or search permission.
> >> +	 */
> >> +	umask(S_IXGRP);
> >> +	mode = umask(S_IXGRP);
> >> +	if (!(mode&  S_IXGRP))
> >> +		die("failure: umask");
> >> +
> >> +	pid = fork();
> >> +	if (pid<  0) {
> >> +		log_stderr("failure: fork");
> >> +		goto out;
> >> +	}
> >> +	if (pid == 0) {
> >> +		if (!switch_ids(0, 10000))
> >> +			die("failure: switch_ids");
> >> +
> >> +		if (!caps_down_fsetid())
> >> +			die("failure: caps_down_fsetid");
> >> +
> >> +		/* create regular file via open() */
> >> +		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> >> +		if (file1_fd<  0)
> >> +			die("failure: create");
> >> +
> >> +		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
> >> +		 * bit needs to be stripped.
> >> +		 */
> >> +		if (is_setgid(t_dir1_fd, FILE1, 0))
> >> +			die("failure: is_setgid");
> >
> > This test is wrong. Specifically, it is a false positive. I have
> > explained this in more detail on v2 of this patchset.
> >
> > You want to test that umask(S_IXGRP) + setgid inheritance work together
> > correctly. First, we need to establish what that means because from your
> > patch series it at least seems to me as you're not completely clear
> > about what you want to test just yet.
> >
> > A requested setgid bit for a non-directory object is only considered for
> > stripping if S_IXGRP is simultaneously requested. In other words, both
> > S_SISGID and S_IXGRP must be requested for the new file in order for
> > additional checks such as CAP_FSETID to become relevant.
> Yes, only keep S_IXGRP, then we can run into the next judgement for 
> group and cap_fsetid.
> >
> > We need to distinguish two cases afaict:
> >
> > 1. The filesystem does support ACLs and has an applicable ACL
> > -------------------------------------------------------------
> >
> > umask(S_IXGRP) is not used by the kernel and thus S_IXGRP is not
> > stripped (unless the ACL does make it so) and so when e.g.
> > inode_init_owner() is called we do not expect the file to inherit the
> > setgid bit.
> >
> > This is what your test above is handling correctly. But it is a false
> > positive because what you're trying to test is the behavior of
> > umask(S_IXGRP) but it is made irrelevant by ACL support of the
> > underlying filesystem.
> I test this situation in the next patch as below:
> umask(S_IXGRP);
> snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw 
> %s/%s", t_mountpoint, T_DIR1)
> 
> and
> umask(S_IXGRP);
> snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx, 
> %s/%s", t_mountpoint, T_DIR1
> 
> >
> > 2. The filesystem does not support ACLs, has been mounted without
> > -----------------------------------------------------------------
> > support for it, or has no applicable ACL
> > ----------------------------------------
> >
> > umask(S_IXGRP) is used by the kernel and will be stripped from the mode.
> > So when inode_init_owner() is called we expect the file to inherit the
> > setgid bit.
> This is why my kernel patch put strip setgid code into vfs.
> xfs will inherit the setgid bit but ext4 not because the new_inode 
> function and posix_acl_create function order(S_IXGRP mode has been 
> stripped before pass to inode_init_owner).
> >
> > This means the test for this case needs to be:
> >
> > 	file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
> > 	if (file1_fd<  0)
> > 		die("failure: create");
> > 	
> > 	/*
> > 	 * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
> > 	 * new file to be S_ISGID.
> No No No, see the kernel patch url
> https://patchwork.kernel.org/project/linux-fsdevel/patch/1648461389-2225-2-git-send-email-xuyang2018.jy@fujitsu.com/

Ok, so your testing patches are premised on your kernel patchset. That
wasn't clear to me.

The kernel patchset removes the setgid bit _before_ the umask is applied
which is why you're expecting this file to not be setgid because you
also dropped CAP_FSETID and you'r not in the group of the directory.

(Ok, let's defer that discussion to the updated patchset.)

Re: [PATCH v3 4/5] idmapped-mounts: Add new setgid_create_umask test

[xuyang2018.jy]

on 2022/4/13 17:59, Christian Brauner write:
> On Wed, Apr 13, 2022 at 09:45:11AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/13 16:59, Christian Brauner wrote:
>>> On Tue, Apr 12, 2022 at 07:33:45PM +0800, Yang Xu wrote:
>>>> The current_umask() is stripped from the mode directly in the vfs if the
>>>> filesystem either doesn't support acls or the filesystem has been
>>>> mounted without posic acl support.
>>>>
>>>> If the filesystem does support acls then current_umask() stripping is
>>>> deferred to posix_acl_create(). So when the filesystem calls
>>>> posix_acl_create() and there are no acls set or not supported then
>>>> current_umask() will be stripped.
>>>>
>>>> Here we only use umask(S_IXGRP) to check whether inode strip
>>>> S_ISGID works correctly.
>>>>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    src/idmapped-mounts/idmapped-mounts.c | 505 +++++++++++++++++++++++++-
>>>>    tests/generic/680                     |  26 ++
>>>>    tests/generic/680.out                 |   2 +
>>>>    3 files changed, 532 insertions(+), 1 deletion(-)
>>>>    create mode 100755 tests/generic/680
>>>>    create mode 100644 tests/generic/680.out
>>>>
>>>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>>>> index 02f91558..e6c14586 100644
>>>> --- a/src/idmapped-mounts/idmapped-mounts.c
>>>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>>>> @@ -14146,6 +14146,494 @@ out:
>>>>    	return fret;
>>>>    }
>>>>
>>>> +/* The following tests are concerned with setgid inheritance. These can be
>>>> + * filesystem type specific. For xfs, if a new file or directory or node is
>>>> + * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
>>>> + * setgid bit if the caller is in the group of the directory.
>>>> + *
>>>> + * The current_umask() is stripped from the mode directly in the vfs if the
>>>> + * filesystem either doesn't support acls or the filesystem has been
>>>> + * mounted without posic acl support.
>>>> + *
>>>> + * If the filesystem does support acls then current_umask() stripping is
>>>> + * deferred to posix_acl_create(). So when the filesystem calls
>>>> + * posix_acl_create() and there are no acls set or not supported then
>>>> + * current_umask() will be stripped.
>>>> + *
>>>> + * Use umask(S_IXGRP) to check whether inode strip S_ISGID works correctly.
>>>> + */
>>>> +static int setgid_create_umask(void)
>>>> +{
>>>> +	int fret = -1;
>>>> +	int file1_fd = -EBADF;
>>>> +	int tmpfile_fd = -EBADF;
>>>> +	pid_t pid;
>>>> +	bool supported = false;
>>>> +	char path[PATH_MAX];
>>>> +	mode_t mode;
>>>> +
>>>> +	if (!caps_supported())
>>>> +		return 0;
>>>> +
>>>> +	if (fchmod(t_dir1_fd, S_IRUSR |
>>>> +			      S_IWUSR |
>>>> +			      S_IRGRP |
>>>> +			      S_IWGRP |
>>>> +			      S_IROTH |
>>>> +			      S_IWOTH |
>>>> +			      S_IXUSR |
>>>> +			      S_IXGRP |
>>>> +			      S_IXOTH |
>>>> +			      S_ISGID), 0) {
>>>> +		log_stderr("failure: fchmod");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	   /* Verify that the setgid bit got raised. */
>>>> +	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
>>>> +		log_stderr("failure: is_setgid");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	supported = openat_tmpfile_supported(t_dir1_fd);
>>>> +
>>>> +	/* Only umask with S_IXGRP because inode strip S_ISGID will check mode
>>>> +	 * whether has group execute or search permission.
>>>> +	 */
>>>> +	umask(S_IXGRP);
>>>> +	mode = umask(S_IXGRP);
>>>> +	if (!(mode&   S_IXGRP))
>>>> +		die("failure: umask");
>>>> +
>>>> +	pid = fork();
>>>> +	if (pid<   0) {
>>>> +		log_stderr("failure: fork");
>>>> +		goto out;
>>>> +	}
>>>> +	if (pid == 0) {
>>>> +		if (!switch_ids(0, 10000))
>>>> +			die("failure: switch_ids");
>>>> +
>>>> +		if (!caps_down_fsetid())
>>>> +			die("failure: caps_down_fsetid");
>>>> +
>>>> +		/* create regular file via open() */
>>>> +		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
>>>> +		if (file1_fd<   0)
>>>> +			die("failure: create");
>>>> +
>>>> +		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
>>>> +		 * bit needs to be stripped.
>>>> +		 */
>>>> +		if (is_setgid(t_dir1_fd, FILE1, 0))
>>>> +			die("failure: is_setgid");
>>>
>>> This test is wrong. Specifically, it is a false positive. I have
>>> explained this in more detail on v2 of this patchset.
>>>
>>> You want to test that umask(S_IXGRP) + setgid inheritance work together
>>> correctly. First, we need to establish what that means because from your
>>> patch series it at least seems to me as you're not completely clear
>>> about what you want to test just yet.
>>>
>>> A requested setgid bit for a non-directory object is only considered for
>>> stripping if S_IXGRP is simultaneously requested. In other words, both
>>> S_SISGID and S_IXGRP must be requested for the new file in order for
>>> additional checks such as CAP_FSETID to become relevant.
>> Yes, only keep S_IXGRP, then we can run into the next judgement for
>> group and cap_fsetid.
>>>
>>> We need to distinguish two cases afaict:
>>>
>>> 1. The filesystem does support ACLs and has an applicable ACL
>>> -------------------------------------------------------------
>>>
>>> umask(S_IXGRP) is not used by the kernel and thus S_IXGRP is not
>>> stripped (unless the ACL does make it so) and so when e.g.
>>> inode_init_owner() is called we do not expect the file to inherit the
>>> setgid bit.
>>>
>>> This is what your test above is handling correctly. But it is a false
>>> positive because what you're trying to test is the behavior of
>>> umask(S_IXGRP) but it is made irrelevant by ACL support of the
>>> underlying filesystem.
>> I test this situation in the next patch as below:
>> umask(S_IXGRP);
>> snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw
>> %s/%s", t_mountpoint, T_DIR1)
>>
>> and
>> umask(S_IXGRP);
>> snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx,
>> %s/%s", t_mountpoint, T_DIR1
>>
>>>
>>> 2. The filesystem does not support ACLs, has been mounted without
>>> -----------------------------------------------------------------
>>> support for it, or has no applicable ACL
>>> ----------------------------------------
>>>
>>> umask(S_IXGRP) is used by the kernel and will be stripped from the mode.
>>> So when inode_init_owner() is called we expect the file to inherit the
>>> setgid bit.
>> This is why my kernel patch put strip setgid code into vfs.
>> xfs will inherit the setgid bit but ext4 not because the new_inode
>> function and posix_acl_create function order(S_IXGRP mode has been
>> stripped before pass to inode_init_owner).
>>>
>>> This means the test for this case needs to be:
>>>
>>> 	file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
>>> 	if (file1_fd<   0)
>>> 		die("failure: create");
>>> 	
>>> 	/*
>>> 	 * S_IXGRP will have been removed due to umask(S_IXGRP) so we expect the
>>> 	 * new file to be S_ISGID.
>> No No No, see the kernel patch url
>> https://patchwork.kernel.org/project/linux-fsdevel/patch/1648461389-2225-2-git-send-email-xuyang2018.jy@fujitsu.com/
>
> Ok, so your testing patches are premised on your kernel patchset. That
> wasn't clear to me.
>
Because of Darrick's request when reviewing this kernel patchset, so 
then I begin to refactor setgid_create code in fstets...

  I should have added this in this patch' commit message, sorry. Will 
add it in v4.
> The kernel patchset removes the setgid bit _before_ the umask is applied
> which is why you're expecting this file to not be setgid because you
> also dropped CAP_FSETID and you'r not in the group of the directory.
Yes.
>
> (Ok, let's defer that discussion to the updated patchset.)
I am rewritting the kernel patch commit message to clarify why put this 
strip code into vfs is safer, but I still have  several filesystem code 
not to see. I will send a v2 kernel patchset tomorrow.

Best Regards
Yang Xu

[PATCH v3 5/5] idmapped-mounts: Add new setgid_create_acl test

[Yang Xu]

The current_umask() is stripped from the mode directly in the vfs if the
filesystem either doesn't support acls or the filesystem has been
mounted without posic acl support.

If the filesystem does support acls then current_umask() stripping is
deferred to posix_acl_create(). So when the filesystem calls
posix_acl_create() and there are no acls set or not supported then
current_umask() will be stripped.

If the parent directory has a default acl then permissions are based off
of that and current_umask() is ignored. Specifically, if the ACL has an
ACL_MASK entry, the group permissions correspond to the permissions of
the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
group permissions correspond to the permissions of the ACL_GROUP_OBJ
entry.

Here we only use setfacl to set default acl or add ACL_MASK to check
whether inode strip  S_ISGID works correctly.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 793 +++++++++++++++++++++++++-
 tests/generic/681                     |  28 +
 tests/generic/681.out                 |   2 +
 3 files changed, 822 insertions(+), 1 deletion(-)
 create mode 100755 tests/generic/681
 create mode 100644 tests/generic/681.out

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index e6c14586..0dbbc64c 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -14634,6 +14634,781 @@ out:
 	return fret;
 }
 
+/* The following tests are concerned with setgid inheritance. These can be
+ * filesystem type specific. For xfs, if a new file or directory or node is
+ * created within a setgid directory and irix_sgid_inhiert is set then inheritthe
+ * setgid bit if the caller is in the group of the directory.
+ *
+ * The current_umask() is stripped from the mode directly in the vfs if the
+ * filesystem either doesn't support acls or the filesystem has been
+ * mounted without posic acl support.
+ *
+ * If the filesystem does support acls then current_umask() stripping is
+ * deferred to posix_acl_create(). So when the filesystem calls
+ * posix_acl_create() and there are no acls set or not supported then
+ * current_umask() will be stripped.
+ *
+ * If the parent directory has a default acl then permissions are based off
+ * of that and current_umask() is ignored. Specifically, if the ACL has an
+ * ACL_MASK entry, the group permissions correspond to the permissions of
+ * the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
+ * group permissions correspond to the permissions of the ACL_GROUP_OBJ
+ * entry.
+ *
+ * Use setfacl to check whether inode strip S_ISGID works correctly.
+ */
+static int setgid_create_acl(void)
+{
+	int fret = -1;
+	int file1_fd = -EBADF;
+	int tmpfile_fd = -EBADF;
+	pid_t pid;
+	bool supported = false;
+	char path[PATH_MAX];
+
+	if (!caps_supported())
+		return 0;
+
+	if (fchmod(t_dir1_fd, S_IRUSR |
+			      S_IWUSR |
+			      S_IRGRP |
+			      S_IWGRP |
+			      S_IROTH |
+			      S_IWOTH |
+			      S_IXUSR |
+			      S_IXGRP |
+			      S_IXOTH |
+			      S_ISGID), 0) {
+		log_stderr("failure: fchmod");
+		goto out;
+	}
+
+	/* Verify that the setgid bit got raised. */
+	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
+		log_stderr("failure: is_setgid");
+		goto out;
+	}
+
+	supported = openat_tmpfile_supported(t_dir1_fd);
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		/* If the parent directory has a default acl then permissions are based off
+		 * of that and current_umask() is ignored. Specifically, if the ACL has an
+		 * ACL_MASK entry, the group permissions correspond to the permissions of
+		 * the ACL_MASK entry.
+		 */
+		umask(S_IXGRP);
+		snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw %s/%s", t_mountpoint, T_DIR1);
+		if (system(t_buf))
+			die("failure: system");
+
+		if (!switch_ids(0, 10000))
+			die("failure: switch_ids");
+
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
+		/* create regular file via open() */
+		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(t_dir1_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(t_dir1_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(t_dir1_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(t_dir1_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(t_dir1_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		/* If the parent directory has a default acl then permissions are based off
+		 * of that and current_umask() is ignored. Specifically, if the ACL has an
+		 * ACL_MASK entry, the group permissions correspond to the permissions of
+		 * the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
+		 * group permissions correspond to the permissions of the ACL_GROUP_OBJ
+		 * entry.
+		 */
+		umask(S_IXGRP);
+		snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx, %s/%s", t_mountpoint, T_DIR1);
+		if (system(t_buf))
+			die("failure: system");
+
+		if (!switch_ids(0, 10000))
+			die("failure: switch_ids");
+
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
+		/* create regular file via open() */
+		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(t_dir1_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(t_dir1_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(t_dir1_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(t_dir1_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(t_dir1_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(t_dir1_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, makedev(5, 1)))
+			die("failure: mknodat");
+
+		if (is_setgid(t_dir1_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(t_dir1_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(t_dir1_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(t_dir1_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, t_dir1_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(t_dir1_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(t_dir1_fd, FILE3, 0))
+				die("failure: delete");
+		}
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(file1_fd);
+	return fret;
+}
+
+static int setgid_create_acl_idmapped(void)
+{
+	int fret = -1;
+	int file1_fd = -EBADF, open_tree_fd = -EBADF;
+	struct mount_attr attr = {
+		.attr_set = MOUNT_ATTR_IDMAP,
+	};
+	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
+
+	if (!caps_supported())
+		return 0;
+
+	if (fchmod(t_dir1_fd, S_IRUSR |
+			      S_IWUSR |
+			      S_IRGRP |
+			      S_IWGRP |
+			      S_IROTH |
+			      S_IWOTH |
+			      S_IXUSR |
+			      S_IXGRP |
+			      S_IXOTH |
+			      S_ISGID), 0) {
+		log_stderr("failure: fchmod");
+		goto out;
+	}
+
+	/* Verify that the setgid bit got raised. */
+	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
+		log_stderr("failure: is_setgid");
+		goto out;
+	}
+
+	attr.userns_fd  = get_userns_fd(0, 10000, 10000);
+	if (attr.userns_fd < 0) {
+		log_stderr("failure: get_userns_fd");
+		goto out;
+	}
+
+	open_tree_fd = sys_open_tree(t_dir1_fd, "",
+				     AT_EMPTY_PATH |
+				     AT_NO_AUTOMOUNT |
+				     AT_SYMLINK_NOFOLLOW |
+				     OPEN_TREE_CLOEXEC |
+				     OPEN_TREE_CLONE);
+	if (open_tree_fd < 0) {
+		log_stderr("failure: sys_open_tree");
+		goto out;
+	}
+
+	if (sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr))) {
+		log_stderr("failure: sys_mount_setattr");
+		goto out;
+	}
+
+	supported = openat_tmpfile_supported(open_tree_fd);
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		/* If the parent directory has a default acl then permissions are based off
+		 * of that and current_umask() is ignored. Specifically, if the ACL has an
+		 * ACL_MASK entry, the group permissions correspond to the permissions of
+		 * the ACL_MASK entry.
+		 */
+		umask(S_IXGRP);
+		snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw %s/%s", t_mountpoint, T_DIR1);
+		if (system(t_buf))
+			die("failure: system");
+
+		if (!switch_ids(10000, 11000))
+			die("failure: switch_ids");
+
+		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(open_tree_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(open_tree_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		/* If the parent directory has a default acl then permissions are based off
+		 * of that and current_umask() is ignored. Specifically, if the ACL has an
+		 * ACL_MASK entry, the group permissions correspond to the permissions of
+		 * the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
+		 * group permissions correspond to the permissions of the ACL_GROUP_OBJ
+		 * entry.
+		 */
+		umask(S_IXGRP);
+		snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx, %s/%s", t_mountpoint, T_DIR1);
+		if (system(t_buf))
+			die("failure: system");
+
+		if (!switch_ids(10000, 11000))
+			die("failure: switch_ids");
+
+		/* create regular file via open() */
+		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(open_tree_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(open_tree_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a character device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(attr.userns_fd);
+	safe_close(file1_fd);
+	safe_close(open_tree_fd);
+
+	return fret;
+}
+
+static int setgid_create_acl_idmapped_in_userns(void)
+{
+	int fret = -1;
+	int file1_fd = -EBADF, open_tree_fd = -EBADF;
+	struct mount_attr attr = {
+		.attr_set = MOUNT_ATTR_IDMAP,
+	};
+	pid_t pid;
+	int tmpfile_fd = -EBADF;
+	bool supported = false;
+	char path[PATH_MAX];
+
+	if (fchmod(t_dir1_fd, S_IRUSR |
+			      S_IWUSR |
+			      S_IRGRP |
+			      S_IWGRP |
+			      S_IROTH |
+			      S_IWOTH |
+			      S_IXUSR |
+			      S_IXGRP |
+			      S_IXOTH |
+			      S_ISGID), 0) {
+		log_stderr("failure: fchmod");
+		goto out;
+	}
+
+	/* Verify that the sid bits got raised. */
+	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
+		log_stderr("failure: is_setgid");
+		goto out;
+	}
+
+	/* Changing mount properties on a detached mount. */
+	attr.userns_fd  = get_userns_fd(0, 10000, 10000);
+	if (attr.userns_fd < 0) {
+		log_stderr("failure: get_userns_fd");
+		goto out;
+	}
+
+	open_tree_fd = sys_open_tree(t_dir1_fd, "",
+				     AT_EMPTY_PATH |
+				     AT_NO_AUTOMOUNT |
+				     AT_SYMLINK_NOFOLLOW |
+				     OPEN_TREE_CLOEXEC |
+				     OPEN_TREE_CLONE);
+	if (open_tree_fd < 0) {
+		log_stderr("failure: sys_open_tree");
+		goto out;
+	}
+
+	if (sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr))) {
+		log_stderr("failure: sys_mount_setattr");
+		goto out;
+	}
+
+	supported = openat_tmpfile_supported(open_tree_fd);
+
+	/* Below we verify that setgid inheritance for a newly created file or
+	 * directory works correctly. As part of this we need to verify that
+	 * newly created files or directories inherit their gid from their
+	 * parent directory. So we change the parent directorie's gid to 1000
+	 * and create a file with fs{g,u}id 0 and verify that the newly created
+	 * file and directory inherit gid 1000, not 0.
+	 */
+	if (fchownat(t_dir1_fd, "", -1, 1000, AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) {
+		log_stderr("failure: fchownat");
+		goto out;
+	}
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		/* If the parent directory has a default acl then permissions are based off
+		 * of that and current_umask() is ignored. Specifically, if the ACL has an
+		 * ACL_MASK entry, the group permissions correspond to the permissions of
+		 * the ACL_MASK entry.
+		 */
+		umask(S_IXGRP);
+		snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rwx,o::rwx,m:rw %s/%s", t_mountpoint, T_DIR1);
+		if (system(t_buf))
+			die("failure: system");
+
+		if (!switch_userns(attr.userns_fd, 0, 0, false))
+			die("failure: switch_userns");
+
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
+		/* create regular file via open() */
+		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(open_tree_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(open_tree_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	pid = fork();
+	if (pid < 0) {
+		log_stderr("failure: fork");
+		goto out;
+	}
+	if (pid == 0) {
+		/* If the parent directory has a default acl then permissions are based off
+		 * of that and current_umask() is ignored. Specifically, if the ACL has an
+		 * ACL_MASK entry, the group permissions correspond to the permissions of
+		 * the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
+		 * group permissions correspond to the permissions of the ACL_GROUP_OBJ
+		 * entry.
+		 */
+		umask(S_IXGRP);
+		snprintf(t_buf, sizeof(t_buf), "setfacl -d -m u::rwx,g::rw,o::rwx %s/%s", t_mountpoint, T_DIR1);
+		if (system(t_buf))
+			die("failure: system");
+
+		if (!switch_userns(attr.userns_fd, 0, 0, false))
+			die("failure: switch_userns");
+
+		if (!caps_down_fsetid())
+			die("failure: caps_down_fsetid");
+
+		/* create regular file via open() */
+		file1_fd = openat(open_tree_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
+		if (file1_fd < 0)
+			die("failure: create");
+
+		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
+		 * bit needs to be stripped.
+		 */
+		if (is_setgid(open_tree_fd, FILE1, 0))
+			die("failure: is_setgid");
+
+		/* create directory */
+		if (mkdirat(open_tree_fd, DIR1, 0000))
+			die("failure: create");
+
+		if (xfs_irix_sgid_inherit_enabled()) {
+			/* We're not in_group_p(). */
+			if (is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		} else {
+			/* Directories always inherit the setgid bit. */
+			if (!is_setgid(open_tree_fd, DIR1, 0))
+				die("failure: is_setgid");
+		}
+
+		/* create a special file via mknodat() vfs_create */
+		if (mknodat(open_tree_fd, FILE2, S_IFREG | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, FILE2, 0))
+			die("failure: is_setgid");
+
+		/* create a whiteout device via mknodat() vfs_mknod */
+		if (mknodat(open_tree_fd, CHRDEV1, S_IFCHR | S_ISGID | S_IXGRP, 0))
+			die("failure: mknodat");
+
+		if (is_setgid(open_tree_fd, CHRDEV1, 0))
+			die("failure: is_setgid");
+
+		if (unlinkat(open_tree_fd, FILE1, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, DIR1, AT_REMOVEDIR))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, FILE2, 0))
+			die("failure: delete");
+
+		if (unlinkat(open_tree_fd, CHRDEV1, 0))
+			die("failure: delete");
+
+		/* create tmpfile via filesystem tmpfile api */
+		if (supported) {
+			tmpfile_fd = openat(open_tree_fd, ".", O_TMPFILE | O_RDWR, S_IXGRP | S_ISGID);
+			if (tmpfile_fd < 0)
+				die("failure: create");
+			/* link the temporary file into the filesystem, making it permanent */
+			snprintf(path, PATH_MAX,  "/proc/self/fd/%d", tmpfile_fd);
+			if (linkat(AT_FDCWD, path, open_tree_fd, FILE3, AT_SYMLINK_FOLLOW))
+				die("failure: linkat");
+			if (close(tmpfile_fd))
+				die("failure: close");
+			if (is_setgid(open_tree_fd, FILE3, 0))
+				die("failure: is_setgid");
+			if (unlinkat(open_tree_fd, FILE3, 0))
+				die("failure: delete");
+		}
+		exit(EXIT_SUCCESS);
+	}
+	if (wait_for_pid(pid))
+		goto out;
+
+	fret = 0;
+	log_debug("Ran test");
+out:
+	safe_close(attr.userns_fd);
+	safe_close(file1_fd);
+	safe_close(open_tree_fd);
+
+	return fret;
+}
+
 static void usage(void)
 {
 	fprintf(stderr, "Description:\n");
@@ -14653,6 +15428,7 @@ static void usage(void)
 	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-setgid-umask                 Run setgid create umask test\n");
+	fprintf(stderr, "--test-setgid-acl                   Run setgid create acl tests\n");
 
 	_exit(EXIT_SUCCESS);
 }
@@ -14671,6 +15447,7 @@ static const struct option longopts[] = {
 	{"test-btrfs",				no_argument,		0,	'b'},
 	{"test-setattr-fix-968219708108",	no_argument,		0,	'i'},
 	{"test-setgid-umask",			no_argument,		0,	'u'},
+	{"test-setgid-acl",			no_argument,		0,	'l'},
 	{NULL,					0,			0,	  0},
 };
 
@@ -14774,6 +15551,12 @@ struct t_idmapped_mounts t_setgid_umask[] = {
 	{ setgid_create_umask_idmapped_in_userns,			true,	"create operations by using umask in directories with setgid bit set on idmapped mounts inside userns",	},
 };
 
+struct t_idmapped_mounts t_setgid_acl[] = {
+	{ setgid_create_acl,						false,	"create operations by using acl in directories with setgid bit set",					},
+	{ setgid_create_acl_idmapped,					false,	"create operations by using acl in directories with setgid bit set on idmapped mount",			},
+	{ setgid_create_acl_idmapped_in_userns,				false,	"create operations by using acl in directories with setgid bit set on idmapped mount inside userns",	},
+};
+
 static bool run_test(struct t_idmapped_mounts suite[], size_t suite_size)
 {
 	int i;
@@ -14851,7 +15634,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_setgid_umask = false;
+	     test_setattr_fix_968219708108 = false, test_setgid_umask = false,
+	     test_setgid_acl = false;
 
 	while ((ret = getopt_long_only(argc, argv, "", longopts, &index)) != -1) {
 		switch (ret) {
@@ -14891,6 +15675,9 @@ int main(int argc, char *argv[])
 		case 'u':
 			test_setgid_umask = true;
 			break;
+		case 'l':
+			test_setgid_acl = true;
+			break;
 		case 'h':
 			/* fallthrough */
 		default:
@@ -14966,6 +15753,10 @@ int main(int argc, char *argv[])
 	    !run_test(t_setgid_umask, ARRAY_SIZE(t_setgid_umask)))
 		goto out;
 
+	if (test_setgid_acl &&
+	    !run_test(t_setgid_acl, ARRAY_SIZE(t_setgid_acl)))
+		goto out;
+
 	fret = EXIT_SUCCESS;
 
 out:
diff --git a/tests/generic/681 b/tests/generic/681
new file mode 100755
index 00000000..a78ca2d1
--- /dev/null
+++ b/tests/generic/681
@@ -0,0 +1,28 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Fujitsu Limited. All Rights Reserved.
+#
+# FS QA Test 681
+#
+# Test that idmapped mounts setgid's behave correctly when using acl.
+#
+. ./common/preamble
+_begin_fstest auto quick acl cap idmapped mount perms rw unlink
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+
+# real QA test starts here
+
+_supported_fs generic
+_require_test
+_require_acls
+
+echo "Silence is golden"
+
+$here/src/idmapped-mounts/idmapped-mounts --test-setgid-acl --device "$TEST_DEV" \
+	--mount "$TEST_DIR" --fstype "$FSTYP"
+
+status=$?
+exit
diff --git a/tests/generic/681.out b/tests/generic/681.out
new file mode 100644
index 00000000..15274cbc
--- /dev/null
+++ b/tests/generic/681.out
@@ -0,0 +1,2 @@
+QA output created by 681
+Silence is golden
-- 
2.27.0

Re: [PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[Christian Brauner]

On Tue, Apr 12, 2022 at 07:33:42PM +0800, Yang Xu wrote:
> If we run case on old kernel that doesn't support mount_setattr and
> then fail on our own function before call is_setgid/is_setuid function
> to reset errno, run_test will print "Function not implement" error.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Looks good to me,
Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Re: [PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[xuyang2018.jy]

Hi Zorro

Since  Christian doesn't send  a new patchset(for rename idmap-mount)
based on lastest xfstests, should I send a v4 patch for the following
patches today?
"idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
" idmapped-mounts: Add mknodat operation in setgid test"
"idmapped-mounts: Add open with O_TMPFILE operation in setgid test"

So you can merge these three patches if you plan to announce a new
xfstests version in this weekend.

What do you think about it?

Best Regards
Yang Xu
> If we run case on old kernel that doesn't support mount_setattr and
> then fail on our own function before call is_setgid/is_setuid function
> to reset errno, run_test will print "Function not implement" error.
> 
> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> ---
>   src/idmapped-mounts/idmapped-mounts.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index 4cf6c3bb..8e6405c5 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
>   		die("failed to open %s", t_mountpoint_scratch);
> 
>   	t_fs_allow_idmap = fs_allow_idmap();
> +	/* don't copy ENOSYS errno to child process on older kernel */
> +	errno = 0;
>   	if (supported) {
>   		/*
>   		 * Caller just wants to know whether the filesystem we're on

Re: [PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[Zorro Lang]

On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
> Hi Zorro
> 
> Since  Christian doesn't send  a new patchset(for rename idmap-mount)
> based on lastest xfstests, should I send a v4 patch for the following
> patches today?
> "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
> " idmapped-mounts: Add mknodat operation in setgid test"
> "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
> 
> So you can merge these three patches if you plan to announce a new
> xfstests version in this weekend.
> 
> What do you think about it?

Sure, you can send V4 of patch 1/5 ～ 3/5 (base on latest for-next branch
please), as they have been reviewed and tested. Christian's patch (about
refactor idmapped testing) might need more review, he just sent it out to
get some review points I think (cc Christian).

If you'd like to catch up the release of this weekend, please send your
v4 patch ASAP. Due to I need time to do regression test before pushing.
It'll wait for next week if too late.

Thanks,
Zorro

> 
> Best Regards
> Yang Xu
> > If we run case on old kernel that doesn't support mount_setattr and
> > then fail on our own function before call is_setgid/is_setuid function
> > to reset errno, run_test will print "Function not implement" error.
> > 
> > Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> > ---
> >   src/idmapped-mounts/idmapped-mounts.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> > index 4cf6c3bb..8e6405c5 100644
> > --- a/src/idmapped-mounts/idmapped-mounts.c
> > +++ b/src/idmapped-mounts/idmapped-mounts.c
> > @@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
> >   		die("failed to open %s", t_mountpoint_scratch);
> > 
> >   	t_fs_allow_idmap = fs_allow_idmap();
> > +	/* don't copy ENOSYS errno to child process on older kernel */
> > +	errno = 0;
> >   	if (supported) {
> >   		/*
> >   		 * Caller just wants to know whether the filesystem we're on

Re: [PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[xuyang2018.jy]

on 2022/5/7 16:52, Zorro Lang wrote:
> On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> Hi Zorro
>>
>> Since  Christian doesn't send  a new patchset(for rename idmap-mount)
>> based on lastest xfstests, should I send a v4 patch for the following
>> patches today?
>> "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
>> " idmapped-mounts: Add mknodat operation in setgid test"
>> "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
>>
>> So you can merge these three patches if you plan to announce a new
>> xfstests version in this weekend.
>>
>> What do you think about it?
>
> Sure, you can send V4 of patch 1/5 ～ 3/5 (base on latest for-next branch
> please), as they have been reviewed and tested. Christian's patch (about
> refactor idmapped testing) might need more review, he just sent it out to
> get some review points I think (cc Christian).
>
> If you'd like to catch up the release of this weekend, please send your
> v4 patch ASAP. Due to I need time to do regression test before pushing.
> It'll wait for next week if too late.

OK. I will send v4 patch ASAP today.

>
> Thanks,
> Zorro
>
>>
>> Best Regards
>> Yang Xu
>>> If we run case on old kernel that doesn't support mount_setattr and
>>> then fail on our own function before call is_setgid/is_setuid function
>>> to reset errno, run_test will print "Function not implement" error.
>>>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>>    src/idmapped-mounts/idmapped-mounts.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>>> index 4cf6c3bb..8e6405c5 100644
>>> --- a/src/idmapped-mounts/idmapped-mounts.c
>>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>>> @@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
>>>    		die("failed to open %s", t_mountpoint_scratch);
>>>
>>>    	t_fs_allow_idmap = fs_allow_idmap();
>>> +	/* don't copy ENOSYS errno to child process on older kernel */
>>> +	errno = 0;
>>>    	if (supported) {
>>>    		/*
>>>    		 * Caller just wants to know whether the filesystem we're on
>

Re: [PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[Christian Brauner]

On Sat, May 07, 2022 at 04:52:09PM +0800, Zorro Lang wrote:
> On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
> > Hi Zorro
> > 
> > Since  Christian doesn't send  a new patchset(for rename idmap-mount)
> > based on lastest xfstests, should I send a v4 patch for the following
> > patches today?
> > "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
> > " idmapped-mounts: Add mknodat operation in setgid test"
> > "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
> > 
> > So you can merge these three patches if you plan to announce a new
> > xfstests version in this weekend.
> > 
> > What do you think about it?
> 
> Sure, you can send V4 of patch 1/5 ～ 3/5 (base on latest for-next branch
> please), as they have been reviewed and tested. Christian's patch (about
> refactor idmapped testing) might need more review, he just sent it out to
> get some review points I think (cc Christian).

LSFMM happened last week so with travel and conference there simply was
no time to rebase. It should be ready for merging once rebased.

> 
> If you'd like to catch up the release of this weekend, please send your
> v4 patch ASAP. Due to I need time to do regression test before pushing.
> It'll wait for next week if too late.

Rebasing the patchset is _massively_ painful which is why in the cover
letter to it I requested that patches which is why I requested
that currently pending patchsets that touch the same code please be
applied on top of it. (I'm happy to apply them manually on top of my
branch.)

In any case, I'll have a rebased version ready on Monday (If there's no
urgent issues I have to address somewhere else that I missed during
travel.)

Christian

Re: [PATCH v3 1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

[Zorro Lang]

On Sat, May 07, 2022 at 01:40:32PM +0200, Christian Brauner wrote:
> On Sat, May 07, 2022 at 04:52:09PM +0800, Zorro Lang wrote:
> > On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
> > > Hi Zorro
> > > 
> > > Since  Christian doesn't send  a new patchset(for rename idmap-mount)
> > > based on lastest xfstests, should I send a v4 patch for the following
> > > patches today?
> > > "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
> > > " idmapped-mounts: Add mknodat operation in setgid test"
> > > "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
> > > 
> > > So you can merge these three patches if you plan to announce a new
> > > xfstests version in this weekend.
> > > 
> > > What do you think about it?
> > 
> > Sure, you can send V4 of patch 1/5 ～ 3/5 (base on latest for-next branch
> > please), as they have been reviewed and tested. Christian's patch (about
> > refactor idmapped testing) might need more review, he just sent it out to
> > get some review points I think (cc Christian).
> 
> LSFMM happened last week so with travel and conference there simply was
> no time to rebase. It should be ready for merging once rebased.

Yes, there's only a few of patches be reviewed this week, due to most of
related people are in LSF meeting :)

> 
> > 
> > If you'd like to catch up the release of this weekend, please send your
> > v4 patch ASAP. Due to I need time to do regression test before pushing.
> > It'll wait for next week if too late.
> 
> Rebasing the patchset is _massively_ painful which is why in the cover
> letter to it I requested that patches which is why I requested
> that currently pending patchsets that touch the same code please be
> applied on top of it. (I'm happy to apply them manually on top of my
> branch.)

Understand, I did tend to merge your change soon, due to it changes the
fundament of whole idmapped test, but I can't refused other people's
patches if they're ready. One of you or XuYang has to do once rebase at
least, so I have to comply with the sequence of patch reviewed.

If you'd like to merge your patch next week, I'll notice others wait one
more week in the [ANNOUNCE] of this week, if they want to write idmapped
related patches. Is that good to you? Please try to give your patch enough
test, make sure it won't bring in big regression, then we can merge it
quickly and smoothly :)

Thanks,
Zorro

> 
> In any case, I'll have a rebased version ready on Monday (If there's no
> urgent issues I have to address somewhere else that I missed during
> travel.)
> 
> Christian
>