Re: [PATCH] generic: extend fscaps test

[Eryu Guan <guan@eryu.me>]

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