* [PATCH] generic: extend fscaps test
@ 2021-04-23 11:15 Christian Brauner
2021-04-25 8:45 ` Eryu Guan
0 siblings, 1 reply; 3+ messages in thread
From: Christian Brauner @ 2021-04-23 11:15 UTC (permalink / raw)
To: Eryu Guan, fstests; +Cc: Christoph Hellwig, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] generic: extend fscaps test
2021-04-23 11:15 [PATCH] generic: extend fscaps test Christian Brauner
@ 2021-04-25 8:45 ` Eryu Guan
2021-04-26 7:51 ` Christian Brauner
0 siblings, 1 reply; 3+ messages in thread
From: Eryu Guan @ 2021-04-25 8:45 UTC (permalink / raw)
To: Christian Brauner; +Cc: fstests, Christoph Hellwig, Christian Brauner
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] generic: extend fscaps test
2021-04-25 8:45 ` Eryu Guan
@ 2021-04-26 7:51 ` Christian Brauner
0 siblings, 0 replies; 3+ messages in thread
From: Christian Brauner @ 2021-04-26 7:51 UTC (permalink / raw)
To: Eryu Guan; +Cc: Christian Brauner, fstests, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-26 7:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-23 11:15 [PATCH] generic: extend fscaps test Christian Brauner
2021-04-25 8:45 ` Eryu Guan
2021-04-26 7:51 ` Christian Brauner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.