OCI runtime support for nspawn #9762

poettering · 2018-07-31T14:29:12Z

This is a big one, unfortunately.

For full drop-in OCI runtime support two bits are still missing: the OCI hooks need to be executed at the right times, and we need a command line tool that provides runc compatibility. I am on both, but this already grew large enough I didn't want to pile on.

But even without those two bits it's pretty comprehensive and should pretty much work.

poettering · 2018-07-31T14:29:31Z

/cc @alban, @iaguis, @dongsupark

boucman · 2018-07-31T15:00:07Z

man/systemd-nspawn.xml

+
+        <listitem><para>Takes the path to an OCI runtime bundle to invoke, as specified in the <ulink
+        url="https://github.com/opencontainers/runtime-spec/blob/master/spec.md">OCI Runtime Specification</ulink>. In
+        this case no <filename>.settings</filename> file is loaded, and the root directory and various settings are


you mean .nspawn file here ?

evverx · 2018-07-31T15:11:02Z

This pull request introduces 6 alerts when merging a9451ba into 5a8b164 - view on LGTM.com

new alerts:

6 for Comparison result is always the same

Comment posted by LGTM.com

evverx · 2018-07-31T15:49:47Z

This pull request introduces 6 alerts when merging 7572186 into 5a8b164 - view on LGTM.com

new alerts:

6 for Comparison result is always the same

Comment posted by LGTM.com

dnicolodi · 2018-07-31T17:48:22Z

src/basic/json-internal.h

+#define JSON_VALUE_NULL ((JsonValue) {})
+
+/* We use fake JsonVariant objects for some special values, in order to avoid memory allocations for them. Note that
+ * effectively this means that there are multiple ways to encode the some objects: via these magic values or as


encode the some objects -> encode some objects

Typo still here :)

evverx · 2018-08-01T01:49:29Z

src/basic/json.h

+
+/*
+  In case you wonder why we have our own JSON implementation, here are a couple of reasons why this implementation has
+  benefits over various other implementatins:


I'm not going to say it's a bold move to implement a json parser :-), but if it's going to be implemented, maybe it would be helpful to start fuzzing it right away. I ran a fuzzer compiled with UBSan for about ten seconds and it crashed after receiving relatively normal input like -34444444444541444444 and "0" with

../../src/systemd/src/basic/json.c:1845:48: runtime error: signed integer overflow: 10 * -3444444444454144444 cannot be represented in type 'long' #0 0x7fe6beefb805 in json_parse_number /work/build/../../src/systemd/src/basic/json.c:1845:48 #1 0x7fe6beefa5ca in json_tokenize /work/build/../../src/systemd/src/basic/json.c:2009:29

and

../../src/systemd/src/basic/json.c:350:9: runtime error: index 1 out of bounds for type 'char [0]' #0 0x7fab2d4c2042 in json_variant_new_stringn /work/build/../../src/systemd/src/basic/json.c:350:22 #1 0x7fab2d4c8690 in json_parse_internal /work/build/../../src/systemd/src/basic/json.c:2321:29 #2 0x7fab2d4c781a in json_parse /work/build/../../src/systemd/src/basic/json.c:2477:16

There were also a couple warnings from clang, the first of which seems legit:

../../src/systemd/src/basic/json.c:2486:32: warning: unused variable 'opened' [-Wunused-variable] _cleanup_fclose_ FILE *opened = NULL; ^ ../../src/systemd/src/basic/json.c:2949:13: warning: variable 'source' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (variant) { ^~~~~~~ ../../src/systemd/src/basic/json.c:2955:13: note: uninitialized use occurs here if (source && source_line > 0 && source_column > 0) ^~~~~~ ../../src/systemd/src/basic/json.c:2949:9: note: remove the 'if' if its condition is always true if (variant) { ^~~~~~~~~~~~~ ../../src/systemd/src/basic/json.c:2936:27: note: initialize the variable 'source' to silence this warning const char *source; ^ = NULL

../../src/systemd/src/basic/json.c:1845:48: runtime error: signed integer overflow: 10 * -3444444444454144444 cannot be represented in type 'long'

This one is misleading. If you look at the sources you find an overflow check right after, that divides the result again by 10 and checks if that result is -3444444444454144444 again.

../../src/systemd/src/basic/json.c:350:9: runtime error: index 1 out of bounds for type 'char [0]'

And this one is misleading too. If you look at the function you'll see we actually allocate the buffer long enough. The field in the structure is the last one in it, and the only reason we specify its size as [0] rather than leave it C99-style unspecified as [] is that we sometimes want arrays of the structure, in which case we don't use the remaining buffer...

Unfortunately, false positives are inevitable, but I don't think they make fuzzers less useful. In this particular case, __attribute__((no_sanitize("signed-integer-overflow"))) and __attribute__((no_sanitize("bounds"))) could probably be used to silence UBSan.

I figure I could rewrite the overflow check to not trigger ubsan. But for the [0] bounds check I have no idea how I could trick it to not complain. Any idea?

Quite frankly, ubsan should be smart enough to understand that [0] is special, and generally doesn't actually mean "zero sized array", but instead is similar to C99 [], i.e. more akin to "unspecified size"...

@evverx hmm, I figure it would make sense to simply pick up that #9782 and add it to this PR, what do you think?

@poettering it's fine by me, but it shouldn't be merged into master until google/oss-fuzz#1683 is merged.

@poettering google/oss-fuzz#1683 has been merged. I pushed the commit from #9782 on top of your branch.

Fedora CI seems to have vanished. Does anybody know how to bring it back or trigger a build manually?

I'm not sure what's happened, but the fuzzer is gone. It'd probably be better to leave it util later. I'll reopen #9782 so as not to forget that it isn't done yet.

evverx · 2018-08-01T12:38:29Z

src/busctl/busctl.c

+
+                        json_dump_with_flags(v, stdout);
+                        if (r < 0)
+                                return r;


It'd be great if you could also take a look at the LGTM alerts. At least the last two of them don't look like false positives. For example, the if block really seems to be redundant here.

rhatdan · 2018-08-01T13:53:53Z

@giuseppe FYI

poettering · 2018-08-02T13:03:58Z

I force pushed a new version now, addressing all raised issues. Let's see if LGTM likes it more this time. PTAL.

evverx · 2018-08-02T14:10:56Z

test-stat-util seems to have started to fail on Fedora CI with

152/321 test-stat-util                          FAIL     0.19 s (killed by signal 6 SIGABRT)
--- command ---
RPM_OS='linux' _='/usr/bin/python3' LANG='C' container_uuid='05c04656-05d9-479d-aa1d-84652cc6678d' HISTCONTROL='ignoredups' RPM_ARCH='x86_64' HOSTNAME='' OLDPWD='/builddir/build/BUILD' CONFIG_SITE='NONE' RPM_PACKAGE_RELEASE='0.1.20180802134457.470418a.fc29' RPM_BUILD_DIR='/builddir/build/BUILD' RPM_LD_FLAGS='-Wl,-z,relro   -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld' NOTIFY_SOCKET='/run/systemd/nspawn/notify' container='systemd-nspawn' USER='mockbuild' RPM_SOURCE_DIR='/builddir/build/SOURCES' PWD='/builddir/build/BUILD/systemd-240/x86_64-redhat-linux-gnu' RPM_PACKAGE_VERSION='240' HOME='/builddir' RPM_BUILD_ROOT='/builddir/build/BUILDROOT/systemd-240-0.1.20180802134457.470418a.fc29.x86_64' MAIL='/var/spool/mail/mockbuild' SHELL='/bin/bash' TERM='vt100' RPM_PACKAGE_NAME='systemd' RPM_DOC_DIR='/usr/share/doc' SHLVL='3' PROMPT_COMMAND='printf "\033]0;<mock-chroot>\007"' LOGNAME='mockbuild' PATH='/builddir/build/BUILD/systemd-240/x86_64-redhat-linux-gnu:/builddir/.local/bin:/builddir/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/sbin' PKG_CONFIG_PATH=':/usr/lib64/pkgconfig:/usr/share/pkgconfig' RPM_OPT_FLAGS='-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection' HISTSIZE='1000' LC_CTYPE='C.UTF-8' SYSTEMD_KBD_MODEL_MAP='/builddir/build/BUILD/systemd-240/src/locale/kbd-model-map' SYSTEMD_LANGUAGE_FALLBACK_MAP='/builddir/build/BUILD/systemd-240/src/locale/language-fallback-map' /builddir/build/BUILD/systemd-240/x86_64-redhat-linux-gnu/test-stat-util
--- stderr ---
Assertion 'device_path_make_canonical(st.st_mode, st.st_rdev, &resolved) >= 0' failed at ../src/test/test-stat-util.c:113, function test_device_path_make_canonical_one(). Aborting.

It probably has something to do with the absence of /run/systemd/inaccessible, but I wouldn't say I'm 100% certain.

poettering · 2018-08-02T17:37:39Z

Neat! CI all green now!

evverx · 2018-08-02T17:45:37Z

Interestingly, it seems that LGTM restarted the analysis as soon as the label was removed.

poettering · 2018-08-02T17:50:11Z

I force pushed another new version now, fixing the typo @dnicolodi found.

…te networking Fixes: systemd#11755

poettering · 2019-03-15T14:56:59Z

I rebased and added a fix for #11755 on top now.

keszybz

After staring at this for a few hours, I don't see anything wrong. I'm sure there must be something wrong, it just isn't possible to have so many lines bug-free, so this should get tested before we release v242. Let's merge.

src/basic/capability-util.c

keszybz · 2019-03-21T09:13:39Z

src/nspawn/nspawn-mount.c

+
+        r = mount_verbose(m->graceful ? LOG_DEBUG : LOG_ERR, NULL, where, NULL, MS_BIND|MS_RDONLY|MS_REMOUNT, NULL);
+        if (r < 0)
+                return m->graceful ? 0 : r;


I was a bit surprised that there's no unmounting performed if the operation fails halfway with m->graceful. In practice it doesn't probably matter that much, because the file is still protected by restrictive file permissions, but I wonder it wouldn't be proper to unmount it anyway.

src/nspawn/nspawn-oci.c

keszybz · 2019-03-21T10:01:48Z

Oh, I'll submit a PR with some follow-up cleanups shortly.

The OCI changes in systemd#9762 broke a use case in which we use nspawn from inside a container that has dropped capabilities from the bounding set that nspawn expected to retain. In an attempt to keep OCI compliance and support our use case, I made hard failing on setting capabilities not in the bounding set optional and log about it. Fixes systemd#12539

The OCI changes in systemd#9762 broke a use case in which we use nspawn from inside a container that has dropped capabilities from the bounding set that nspawn expected to retain. In an attempt to keep OCI compliance and support our use case, I made hard failing on setting capabilities not in the bounding set optional (hard fail if using OCI and log only if using nspawn cmdline). Fixes systemd#12539

The OCI changes in #9762 broke a use case in which we use nspawn from inside a container that has dropped capabilities from the bounding set that nspawn expected to retain. In an attempt to keep OCI compliance and support our use case, I made hard failing on setting capabilities not in the bounding set optional (hard fail if using OCI and log only if using nspawn cmdline). Fixes #12539

The OCI changes in systemd#9762 broke a use case in which we use nspawn from inside a container that has dropped capabilities from the bounding set that nspawn expected to retain. In an attempt to keep OCI compliance and support our use case, I made hard failing on setting capabilities not in the bounding set optional (hard fail if using OCI and log only if using nspawn cmdline). Fixes systemd#12539

We mean the line number in the json data, not the line number in our C source code. Addresses: systemd/systemd#9762 (comment)

if we sync the legacy and unified trees before moving to the right subcgroup then ultimately the cgroup paths in the hierarchies will be out-of-sync... Hence, let's move the payload first, and sync then. Addresses: systemd/systemd#9762 (comment) (cherry picked from commit 27da7ef) Resolves: #1837094

if we sync the legacy and unified trees before moving to the right subcgroup then ultimately the cgroup paths in the hierarchies will be out-of-sync... Hence, let's move the payload first, and sync then. Addresses: systemd/systemd#9762 (comment) (cherry picked from commit 27da7ef) (cherry picked from commit 8ee1465) Resolves: #1837423

if we sync the legacy and unified trees before moving to the right subcgroup then ultimately the cgroup paths in the hierarchies will be out-of-sync... Hence, let's move the payload first, and sync then. Addresses: systemd/systemd#9762 (comment) (cherry picked from commit 27da7ef0d09e00eae821f3ef26e1a666fe7aa087) Resolves: #1837094

poettering added journal nspawn login sd-bus pid1 util-lib labels Jul 31, 2018

boucman reviewed Jul 31, 2018

View reviewed changes

poettering force-pushed the nspawn-oci branch from a9451ba to 7572186 Compare July 31, 2018 15:12

dnicolodi reviewed Jul 31, 2018

View reviewed changes

evverx reviewed Aug 1, 2018

View reviewed changes

poettering force-pushed the nspawn-oci branch 2 times, most recently from 9913a12 to 0f5b80c Compare August 2, 2018 12:58

poettering force-pushed the nspawn-oci branch 2 times, most recently from a92797e to 470418a Compare August 2, 2018 13:20

evverx added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Aug 2, 2018

poettering force-pushed the nspawn-oci branch from 470418a to 33c9795 Compare August 2, 2018 15:05

evverx mentioned this pull request Aug 2, 2018

[skia] Add skjson fuzzer google/oss-fuzz#1497

Merged

poettering removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Aug 2, 2018

poettering force-pushed the nspawn-oci branch from 33c9795 to a85090b Compare August 2, 2018 17:47

evverx mentioned this pull request Aug 3, 2018

tests: add a fuzzer for the json parser and dumper #9782

Closed

poettering added 2 commits March 15, 2019 15:42

man: document the various new options nspawn learnt

3d6c367

nspawn: mask out CAP_NET_ADMIN again if settings file turns off priva…

a3fc6b5

…te networking Fixes: systemd#11755

poettering force-pushed the nspawn-oci branch from 72f29ee to a3fc6b5 Compare March 15, 2019 14:56

poettering mentioned this pull request Mar 15, 2019

Somehow surprisingly nspawn container has CAP_NET_ADMIN when [Network] Private=no #11755

Closed

poettering requested a review from keszybz March 20, 2019 13:51

keszybz approved these changes Mar 21, 2019

View reviewed changes

keszybz merged commit d0b6a10 into systemd:master Mar 21, 2019

mrc0mmand mentioned this pull request Mar 21, 2019

Compilation fails on openSUSE Tumbleweed due to missing seccomp.h #12060

Closed

anitazha mentioned this pull request Jun 4, 2019

nspawn: don't hard fail when setting capabilities #12729

Merged

Yamakuzure pushed a commit to elogind/elogind that referenced this pull request Sep 23, 2019

json: log about the right line number

abce02b

We mean the line number in the json data, not the line number in our C source code. Addresses: systemd/systemd#9762 (comment)

mrc0mmand mentioned this pull request May 19, 2020

(#1837094) nspawn: move payload to sub-cgroup first, then sync cgroup trees redhat-plumbers/systemd-rhel8#98

Merged

mrc0mmand mentioned this pull request May 19, 2020

(#1837423) nspawn: move payload to sub-cgroup first, then sync cgroup trees redhat-plumbers/systemd-rhel8#100

Merged

This was referenced Jan 24, 2021

oci hooks for nspawn #18367

Open

runc compatible cli tool for nspawn #18368

Open

enhancement: systemd-nspawn to launch real Image= container virtual-kubelet/systemk#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCI runtime support for nspawn #9762

OCI runtime support for nspawn #9762

poettering commented Jul 31, 2018

poettering commented Jul 31, 2018

boucman Jul 31, 2018

poettering Aug 1, 2018

evverx commented Jul 31, 2018

evverx commented Jul 31, 2018

dnicolodi Jul 31, 2018

dnicolodi Aug 2, 2018

evverx Aug 1, 2018

poettering Aug 1, 2018

poettering Aug 1, 2018

evverx Aug 1, 2018

poettering Aug 2, 2018

poettering Aug 2, 2018

evverx Aug 2, 2018

evverx Aug 3, 2018

evverx Aug 3, 2018

evverx Aug 6, 2018

evverx Aug 1, 2018

rhatdan commented Aug 1, 2018

poettering commented Aug 2, 2018

evverx commented Aug 2, 2018

poettering commented Aug 2, 2018

evverx commented Aug 2, 2018

poettering commented Aug 2, 2018

poettering commented Mar 15, 2019

keszybz left a comment

keszybz Mar 21, 2019

poettering Mar 21, 2019

keszybz commented Mar 21, 2019

OCI runtime support for nspawn #9762

OCI runtime support for nspawn #9762

Conversation

poettering commented Jul 31, 2018

poettering commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evverx commented Jul 31, 2018

evverx commented Jul 31, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Aug 1, 2018

poettering commented Aug 2, 2018

evverx commented Aug 2, 2018

poettering commented Aug 2, 2018

evverx commented Aug 2, 2018

poettering commented Aug 2, 2018

poettering commented Mar 15, 2019

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keszybz commented Mar 21, 2019