seccompfilter: Use SCMP_ACT_KILL_PROCESS only on compatible kernels #234

mbargull · 2020-11-15T09:22:42Z

Supersedes gh-232.

seccompfilter: Conditional SCMP_ACT_KILL_PROCESS

As before, this only uses SCMP_ACT_KILL_PROCESS for libseccomp versions
which define it.
Additionally, it is only used if at run time the kernel reports it as a
supported action. For this SECCOMP_GET_ACTION_AVAIL must be available
(supported from Linux 4.14 onward and backported to CentOS 7's kernel)
and SECCOMP_RET_KILL_PROCESS (Linux 4.14+, but not in CentOS 7).

This also adds/changes the following:

Add a CentOS 7 VM/Vagrant-based test run for compatibility (regression) testing on older kernels.
Add a test case for --enable-seccomp.
Small adjustments to allow CI tests to run on CentOS 7, run out of tree, complete faster.

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

mbargull · 2020-11-15T09:23:58Z

(The commit with the actual fix is not yet pushed to let the test-centos CI job at least fail once.)

mbargull · 2020-11-15T09:34:10Z

https://github.com/rootless-containers/slirp4netns/pull/234/checks?check_run_id=1402162438#step:5:705

  env:
    LIBSECCOMP_COMMIT: v2.4.3
    LIBSLIRP_COMMIT: v4.1.0
...
PASS: tests/test-slirp4netns-ready-fd.sh
PASS: tests/test-slirp4netns-configure.sh
PASS: tests/test-slirp4netns-exit-fd.sh
PASS: tests/test-slirp4netns-cidr.sh
PASS: tests/test-slirp4netns-outbound-addr.sh
PASS: tests/test-slirp4netns-disable-host-loopback.sh
PASS: tests/test-slirp4netns-disable-dns.sh
PASS: tests/test-slirp4netns-api-socket.sh
FAIL: tests/test-slirp4netns-seccomp.sh
PASS: tests/test-slirp4netns.sh

As intended: FAIL: tests/test-slirp4netns-seccomp.sh

As before, this only uses SCMP_ACT_KILL_PROCESS for libseccomp versions which define it. Additionally, it is only used if at run time the kernel reports it as a supported action. For this SECCOMP_GET_ACTION_AVAIL must be available (supported from Linux 4.14 onward and backported to CentOS 7's kernel) and SECCOMP_RET_KILL_PROCESS (Linux 4.14+, but not in CentOS 7). Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

mbargull · 2020-11-15T09:46:17Z

.github/workflows/main.yaml

+      LIBSECCOMP_COMMIT: v2.3.3
+      LIBSLIRP_COMMIT: v4.1.0


Since the VM setup takes approx. twice as much time as an actual build+test, I opted for a single CI job because it seemed more economical. (But it is also just in the realm of 2 min. setup + 1 min. for each run.)

mbargull · 2020-11-15T09:49:38Z

.github/workflows/main.yaml

+    env:
+      LIBSECCOMP_COMMIT: v2.3.3
+      LIBSLIRP_COMMIT: v4.1.0
+      BENCHMARK_IPERF3_DURATION: 3


It probably makes sense to not run the full 4 x 60 seconds benchmark in each CI job. I only reduced the time for the duration for the newly added VM-based job.

mbargull · 2020-11-15T09:53:03Z

.github/workflows/main.yaml

+        LIBSLIRP_COMMIT: v4.2.0
+        # Fails with --disable-dns from libslirp >=4.3.0
+        # (no timeout in test-slirp4netns-disable-dns.sh).


I did not investigate this further, so can't tell if it's kernel or ncat version related, but with libslirp 4.3.1 the --disable-dns did not show the expected timeout.

timeout of what?

https://github.com/rootless-containers/slirp4netns/blob/v1.1.6/tests/test-slirp4netns-disable-dns.sh#L33-L35 does not get a timeout message to grep for (the ncat seems to succeed).

mbargull · 2020-11-15T09:57:19Z

Vagrantfile

+        touch ./build-and-test
+        chmod a+x ./build-and-test
+      '
+
+      cat > ./build-and-test <<'EOS'


I have only included this static build-and-test script inline here because I wasn't sure if you'd prefer to have the Vagrantfile as self-contained as possible or if a separate script file would be okay/preferable.

mbargull · 2020-11-15T09:59:38Z

Vagrantfile

+      "${src_dir}/slirp4netns/configure" --prefix="${prefix}"
+      make -j "$( nproc )"
+
+      make ci 'CLANGTIDY=echo skipping:' 'CLANGFORMAT=echo skipping:'


I did not look into how to install clang-tidy/clang-format on CentOS and thus just "disabled" those targets by replacing the command with dummy echos.

seccompfilter.c

mbargull · 2020-11-15T10:06:00Z

tests/slirp4netns-no-unmount.sh

@@ -11,7 +11,7 @@ mkdir /run/foo
 mount -t tmpfs tmpfs /run/foo
 mount --make-rshared /run

-unshare -n sleep infinity &
+unshare -r -n sleep infinity &


Without this the tests (also on Ubuntu) are prolonged by 40s due to the 2 x 40 x 0.5s waiting from wait_for_network_namespace and wait_for_network_device.

This seems to need investigation. I don't think we should need -r here.

cc @giuseppe

the user should already have enough privileges to run these tests, why does the new user namespace make a difference?

I have no idea, unfortunately (not my area of expertise). I only noticed this while running the tests.
I reverted the associated commit for now.
@AkihiroSuda, @giuseppe can you open an issue to track this?
(Asking you because you can give more context than unknowledgeable me.)

mbargull · 2020-11-15T10:07:50Z

tests/test-slirp4netns-cidr.sh

+result="$(nsenter --preserve-credentials -U -n --target=$child ip a show dev tun11)"
+echo "$result" | grep -om1 '^\s*inet .*/' | grep -qF 10.0.135.228


The older ip on CentOS 7 does not support -json so I had to work around this to make this test runnable on CentOS 7. Please let me know if this needs further adjustment.

mbargull · 2020-11-15T10:14:23Z

If there are any changes/cleanup/refactoring (within the actual seccompfilter.c change or CI setup/matrix, inline scripts, etc.) you deem helpful, please do feel free to push to this branch. (Allow edit by maintainers box is checked ;) .)

AkihiroSuda · 2020-11-16T05:48:50Z

Can we have 189e1ed as a separate PR?

This reverts commit 0587bbf. Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

AkihiroSuda · 2020-11-25T02:36:29Z

@mbargull Could you open a separate PR for 189e1ed ?

mbargull · 2020-11-25T04:51:03Z

@mbargull Could you open a separate PR for 189e1ed ?

Done in gh-235.

Is there anything else for me to help with in this PR?

AkihiroSuda · 2020-11-25T04:55:23Z

Thanks for opening #235, but that one is not 189e1ed ("seccompfilter: Conditional SCMP_ACT_KILL_PROCESS") and it seems controversial. I'd like to merge 189e1ed ahead of other commits.

AkihiroSuda · 2020-11-25T04:58:51Z

Ah, sorry, I didn't notice the unshare -r commit was already reverted in this PR, I think I can merge this PR now. (Is unshare -r still needed?)

mbargull · 2020-11-25T05:00:05Z

Oh, very sorry, for the mix up!
But why do we want to have that commit without any tests then? The point of all other commits is to accompany that one to have a test. I am confused.

mbargull · 2020-11-25T05:00:49Z

Ah, sorry, I didn't notice the unshare -r commit was already reverted in this PR, I think I can merge this PR now. (Is unshare -r still needed?)

Ah, yes, nevermind my previous comments then :). Thank you!

mbargull added 7 commits November 15, 2020 09:49

configure: Use AM_PROG_AR

b04f711

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

make: Allow targets ci / benchmark run out of tree

9763e40

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

tests: Avoid timeout in slirp4netns-no-unmount.sh

0587bbf

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

benchmark: Enable benchmark duration adjustment

62d0028

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

tests: Avoid ip -json (not available on CentOS 7)

76e7a52

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

tests: Add test-slirp4netns-seccomp.sh

61da2a6

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

CI: Add CentOS 7 test via Vagrantfile

a15a4f3

Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

mbargull force-pushed the conditional-SCMP_ACT_KILL_PROCESS branch from ea29112 to 189e1ed Compare November 15, 2020 09:41

mbargull commented Nov 15, 2020

View reviewed changes

seccompfilter.c Show resolved Hide resolved

mbargull commented Nov 15, 2020

View reviewed changes

mbargull mentioned this pull request Nov 15, 2020

Avoid SCMP_ACT_KILL_PROCESS in seccompfilter #232

Closed

Revert "tests: Avoid timeout in slirp4netns-no-unmount.sh"

bf13e3e

This reverts commit 0587bbf. Signed-off-by: Marcel Bargull <marcel.bargull@udo.edu>

mbargull mentioned this pull request Nov 25, 2020

tests: Avoid timeout in slirp4netns-no-unmount.sh #235

Closed

AkihiroSuda merged commit 4bce8d8 into rootless-containers:master Nov 25, 2020

mbargull mentioned this pull request Nov 25, 2020

CI: Test with libslirp 4.3.1 on CentOS 7 #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

seccompfilter: Use SCMP_ACT_KILL_PROCESS only on compatible kernels #234

seccompfilter: Use SCMP_ACT_KILL_PROCESS only on compatible kernels #234

mbargull commented Nov 15, 2020

mbargull commented Nov 15, 2020

mbargull commented Nov 15, 2020

mbargull Nov 15, 2020

mbargull Nov 15, 2020

mbargull Nov 15, 2020

AkihiroSuda Nov 16, 2020

mbargull Nov 16, 2020

mbargull Nov 15, 2020

mbargull Nov 15, 2020

mbargull Nov 15, 2020

AkihiroSuda Nov 16, 2020

giuseppe Nov 16, 2020

mbargull Nov 16, 2020 •

edited

Loading

mbargull Nov 15, 2020

mbargull commented Nov 15, 2020

AkihiroSuda commented Nov 16, 2020

AkihiroSuda commented Nov 25, 2020

mbargull commented Nov 25, 2020

AkihiroSuda commented Nov 25, 2020

AkihiroSuda commented Nov 25, 2020

mbargull commented Nov 25, 2020

mbargull commented Nov 25, 2020

		result="$(nsenter --preserve-credentials -U -n --target=$child ip a show dev tun11)"
		echo "$result" \| grep -om1 '^\sinet ./' \| grep -qF 10.0.135.228

seccompfilter: Use SCMP_ACT_KILL_PROCESS only on compatible kernels #234

seccompfilter: Use SCMP_ACT_KILL_PROCESS only on compatible kernels #234

Conversation

mbargull commented Nov 15, 2020

mbargull commented Nov 15, 2020

mbargull commented Nov 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbargull Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbargull commented Nov 15, 2020

AkihiroSuda commented Nov 16, 2020

AkihiroSuda commented Nov 25, 2020

mbargull commented Nov 25, 2020

AkihiroSuda commented Nov 25, 2020

AkihiroSuda commented Nov 25, 2020

mbargull commented Nov 25, 2020

mbargull commented Nov 25, 2020

mbargull Nov 16, 2020 •

edited

Loading