[PATCH 1/3] idmapped-mounts: remove redundant fchownat() call in setgid tests

[PATCH 1/3] idmapped-mounts: remove redundant fchownat() call in setgid tests

[Christian Brauner]

There's another call to fchownat() right above it so we really don't
need the second one.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Eryu Guan <guaneryu@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: fstests@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Christian Brauner <brauner@kernel.org>:
  - fix Seth's mail address in commit message
---
 src/idmapped-mounts/idmapped-mounts.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index da690779..56b26b0c 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -8133,11 +8133,6 @@ static int setgid_create_idmapped_in_userns(void)
 		goto out;
 	}
 
-	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");

base-commit: 770f462e17e52c4b2bc026fd707ad01fcce95f32
-- 
2.32.0

[PATCH 2/3] idmapped-mounts: add more explanations to setgid tests

[Christian Brauner]

The explanations before were a bit thin and people not familiar with
setgid inheritance might get confused. Make it easier to understand the
tests.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Eryu Guan <guaneryu@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: fstests@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Christian Brauner <brauner@kernel.org>:
  - fix Seth's mail address in commit message
---
 src/idmapped-mounts/idmapped-mounts.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 56b26b0c..c53e1942 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -8128,6 +8128,14 @@ static int setgid_create_idmapped_in_userns(void)
 	if (wait_for_pid(pid))
 		goto out;
 
+	/*
+	 * 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;
@@ -8172,12 +8180,19 @@ static int setgid_create_idmapped_in_userns(void)
 				die("failure: is_setgid");
 		}
 
-		/* Files and directories created in setgid directories inherit
-		 * the i_gid of the parent directory.
+		/*
+		 * In setgid directories newly created files always inherit the
+		 * gid from the parent directory. Verify that the file is owned
+		 * by gid 1000, not by gid 0.
 		 */
 		if (!expected_uid_gid(open_tree_fd, FILE1, 0, 0, 1000))
 			die("failure: check ownership");
 
+		/*
+		 * In setgid directories newly created directories always
+		 * inherit the gid from the parent directory. Verify that the
+		 * directory is owned by gid 1000, not by gid 0.
+		 */
 		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 0, 1000))
 			die("failure: check ownership");
 
-- 
2.32.0

[PATCH 3/3] idmapped-mounts: add missing ownership comparisons to setgid tests

[Christian Brauner]

In some setgid tests we missed to check ownership right after file or
directory creation in order to verify whether gid ownership inheritance
from the parent directory to the newly created file or directory works
correctly. Add the missing ones.

Cc: Seth Forshee <sforshee@digitalocean.com>
Cc: Eryu Guan <guaneryu@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: fstests@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Christian Brauner <brauner@kernel.org>:
  - fix Seth's mail address in commit message
---
 src/idmapped-mounts/idmapped-mounts.c | 38 +++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index c53e1942..a5c0a983 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -7863,6 +7863,12 @@ static int setgid_create(void)
 		if (!is_setgid(t_dir1_fd, DIR1, 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 (unlinkat(t_dir1_fd, FILE1, 0))
 			die("failure: delete");
 
@@ -7911,6 +7917,22 @@ static int setgid_create(void)
 				die("failure: is_setgid");
 		}
 
+		/*
+		 * In setgid directories newly created files always inherit the
+		 * gid from the parent directory. Verify that the file is owned
+		 * by gid 0, not by gid 10000.
+		 */
+		if (!expected_uid_gid(t_dir1_fd, FILE1, 0, 0, 0))
+			die("failure: check ownership");
+
+		/*
+		 * In setgid directories newly created directories always
+		 * inherit the gid from the parent directory. Verify that the
+		 * directory is owned by gid 0, not by gid 10000.
+		 */
+		if (!expected_uid_gid(t_dir1_fd, DIR1, 0, 0, 0))
+			die("failure: check ownership");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
@@ -8013,6 +8035,22 @@ static int setgid_create_idmapped(void)
 				die("failure: is_setgid");
 		}
 
+		/*
+		 * In setgid directories newly created files always inherit the
+		 * gid from the parent directory. Verify that the file is owned
+		 * by gid 10000, not by gid 11000.
+		 */
+		if (!expected_uid_gid(open_tree_fd, FILE1, 0, 10000, 10000))
+			die("failure: check ownership");
+
+		/*
+		 * In setgid directories newly created directories always
+		 * inherit the gid from the parent directory. Verify that the
+		 * directory is owned by gid 10000, not by gid 11000.
+		 */
+		if (!expected_uid_gid(open_tree_fd, DIR1, 0, 10000, 10000))
+			die("failure: check ownership");
+
 		exit(EXIT_SUCCESS);
 	}
 	if (wait_for_pid(pid))
-- 
2.32.0

Re: [PATCH 1/3] idmapped-mounts: remove redundant fchownat() call in setgid tests

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] idmapped-mounts: add more explanations to setgid tests

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] idmapped-mounts: add missing ownership comparisons to setgid tests

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 1/3] idmapped-mounts: remove redundant fchownat() call in setgid tests

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

There's another call to fchownat() right above it so we really don't
need the second one.

Cc: Seth Forshee <seth.forshee@digitalocean.com>
Cc: Eryu Guan <guaneryu@gmail.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: fstests@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index da690779..56b26b0c 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -8133,11 +8133,6 @@ static int setgid_create_idmapped_in_userns(void)
 		goto out;
 	}
 
-	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");

base-commit: 770f462e17e52c4b2bc026fd707ad01fcce95f32
-- 
2.32.0