[PATCH] generic: extend fscaps test

[PATCH] generic: extend fscaps test

[Christian Brauner]

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

Add a test to verify that setting a v3 fscap that is valid in an
ancestor user namespace works.

Cc: fstests@vger.kernel.org
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 56 +++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 870a8fe7..4e3252ca 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -3193,6 +3193,62 @@ static int fscaps_idmapped_mounts_in_userns(void)
 		goto out;
 	}
 
+	/*
+	 * Verify we can set an v3 fscap for real root this was regressed at
+	 * some point. Make sure this doesn't happen again!
+	 */
+	if (fremovexattr(file1_fd, "security.capability")) {
+		log_stderr("failure: fremovexattr");
+		goto out;
+	}
+	if (expected_dummy_vfs_caps_uid(file1_fd, -1)) {
+		log_stderr("failure: expected_dummy_vfs_caps_uid");
+		goto out;
+	}
+	if (errno != ENODATA) {
+		log_stderr("failure: errno");
+		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 (expected_dummy_vfs_caps_uid(file1_fd2, -1))
+			die("failure: expected_dummy_vfs_caps_uid");
+		if (errno != ENODATA)
+			die("failure: errno");
+
+		if (set_dummy_vfs_caps(file1_fd2, 0, 0))
+			die("failure: set_dummy_vfs_caps");
+
+		if (!expected_dummy_vfs_caps_uid(file1_fd2, 0))
+			die("failure: expected_dummy_vfs_caps_uid");
+
+		if (!expected_dummy_vfs_caps_uid(file1_fd, 0) && errno != EOVERFLOW)
+			die("failure: expected_dummy_vfs_caps_uid");
+
+		exit(EXIT_SUCCESS);
+	}
+
+	if (wait_for_pid(pid))
+		goto out;
+
+	if (!expected_dummy_vfs_caps_uid(file1_fd2, 10000)) {
+		log_stderr("failure: expected_dummy_vfs_caps_uid");
+		goto out;
+	}
+
+	if (!expected_dummy_vfs_caps_uid(file1_fd, 0)) {
+		log_stderr("failure: expected_dummy_vfs_caps_uid");
+		goto out;
+	}
+
 	fret = 0;
 	log_debug("Ran test");
 out:

base-commit: 15510d3a208187e234333f7974580786d54d52dc
-- 
2.27.0

Re: [PATCH] generic: extend fscaps test

[Eryu Guan]

On Fri, Apr 23, 2021 at 01:15:39PM +0200, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Add a test to verify that setting a v3 fscap that is valid in an
> ancestor user namespace works.

The subject is not clear which test it updates, I can only know it's
generic/633 that calls idmapped-mounts binary to do the test.

> 
> Cc: fstests@vger.kernel.org
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  src/idmapped-mounts/idmapped-mounts.c | 56 +++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index 870a8fe7..4e3252ca 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -3193,6 +3193,62 @@ static int fscaps_idmapped_mounts_in_userns(void)
>  		goto out;
>  	}
>  
> +	/*
> +	 * Verify we can set an v3 fscap for real root this was regressed at
> +	 * some point. Make sure this doesn't happen again!
> +	 */

We usually don't add new test cases to existing tests, as that may
introduce new failures and let people think there's a regression, then
find out it's the new case introduced the failure.

But this test was just merged last week, and the test is closely related
to existing cases and could re-use the test framework/setups, so I think
it's fine to add this case.

But as above comment said, this new cases is targeted to a regression
happened previously, I think it'd be better to put it in a seperate test
function, not folded into an existing test function.

And is there a commit that fixed the mentioned regression? Reference it
in the comments would help people find the correct fix, if there's any.

Thanks,
Eryu

> +	if (fremovexattr(file1_fd, "security.capability")) {
> +		log_stderr("failure: fremovexattr");
> +		goto out;
> +	}
> +	if (expected_dummy_vfs_caps_uid(file1_fd, -1)) {
> +		log_stderr("failure: expected_dummy_vfs_caps_uid");
> +		goto out;
> +	}
> +	if (errno != ENODATA) {
> +		log_stderr("failure: errno");
> +		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 (expected_dummy_vfs_caps_uid(file1_fd2, -1))
> +			die("failure: expected_dummy_vfs_caps_uid");
> +		if (errno != ENODATA)
> +			die("failure: errno");
> +
> +		if (set_dummy_vfs_caps(file1_fd2, 0, 0))
> +			die("failure: set_dummy_vfs_caps");
> +
> +		if (!expected_dummy_vfs_caps_uid(file1_fd2, 0))
> +			die("failure: expected_dummy_vfs_caps_uid");
> +
> +		if (!expected_dummy_vfs_caps_uid(file1_fd, 0) && errno != EOVERFLOW)
> +			die("failure: expected_dummy_vfs_caps_uid");
> +
> +		exit(EXIT_SUCCESS);
> +	}
> +
> +	if (wait_for_pid(pid))
> +		goto out;
> +
> +	if (!expected_dummy_vfs_caps_uid(file1_fd2, 10000)) {
> +		log_stderr("failure: expected_dummy_vfs_caps_uid");
> +		goto out;
> +	}
> +
> +	if (!expected_dummy_vfs_caps_uid(file1_fd, 0)) {
> +		log_stderr("failure: expected_dummy_vfs_caps_uid");
> +		goto out;
> +	}
> +
>  	fret = 0;
>  	log_debug("Ran test");
>  out:
> 
> base-commit: 15510d3a208187e234333f7974580786d54d52dc
> -- 
> 2.27.0

Re: [PATCH] generic: extend fscaps test

[Christian Brauner]

On Sun, Apr 25, 2021 at 04:45:05PM +0800, Eryu Guan wrote:
> On Fri, Apr 23, 2021 at 01:15:39PM +0200, Christian Brauner wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > Add a test to verify that setting a v3 fscap that is valid in an
> > ancestor user namespace works.
> 
> The subject is not clear which test it updates, I can only know it's
> generic/633 that calls idmapped-mounts binary to do the test.

Right, sorry. Will add that.

> 
> > 
> > Cc: fstests@vger.kernel.org
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >  src/idmapped-mounts/idmapped-mounts.c | 56 +++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> > index 870a8fe7..4e3252ca 100644
> > --- a/src/idmapped-mounts/idmapped-mounts.c
> > +++ b/src/idmapped-mounts/idmapped-mounts.c
> > @@ -3193,6 +3193,62 @@ static int fscaps_idmapped_mounts_in_userns(void)
> >  		goto out;
> >  	}
> >  
> > +	/*
> > +	 * Verify we can set an v3 fscap for real root this was regressed at
> > +	 * some point. Make sure this doesn't happen again!
> > +	 */
> 
> We usually don't add new test cases to existing tests, as that may
> introduce new failures and let people think there's a regression, then
> find out it's the new case introduced the failure.

Hm, okay. I'm pretty sure that I'll grow the idmapped mount test-suite
quite a bit more so I need to think about how to make this easily
extensible. I want the ability to use the binary itself to run all
tests. So I may just introduce flags to allow for running specific tests
or subsets of tests such as:

idmapped-mounts --fscaps --acl

> 
> But this test was just merged last week, and the test is closely related
> to existing cases and could re-use the test framework/setups, so I think
> it's fine to add this case.
> 
> But as above comment said, this new cases is targeted to a regression
> happened previously, I think it'd be better to put it in a seperate test
> function, not folded into an existing test function.
> 
> And is there a commit that fixed the mentioned regression? Reference it
> in the comments would help people find the correct fix, if there's any.

That is an annoyingly convoluted story involving a buggy "fix" a revert
and then a proper fix. But I'll sure add details. Thank you!

Christian