| Message ID | 20210423111539.3591487-1-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic: extend fscaps test | expand |
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
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
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: