core: do not fail at step SECCOMP if there is no kernel support #4004

fsateler · 2016-08-20T17:44:25Z

keszybz · 2016-08-20T21:02:45Z

src/shared/seccomp-util.c

+        _cleanup_free_ char* field = NULL;
+        static int cached_enabled = -1;
+        if (cached_enabled < 0)
+                cached_enabled = get_proc_field("/proc/self/status", "Seccomp", "\n", &field) == 0;


It would seem cleaner to change get_proc_field to accept NULL argument, and assign the value only if is non-null.

fsateler · 2016-08-21T21:18:04Z

Force pushed with all comments addressed.

evverx · 2016-08-21T23:12:53Z

src/core/execute.c

+static bool skip_seccomp_unavailable(const char* msg) {
+        if (!is_seccomp_available()) {
+                log_open();
+                log_debug("SECCOMP not detected in the kernel, skipping %s", msg);


Why not log_unit_debug?
I think

Aug 21 21:50:58 systemd[968]: systemd-journald.service: SECCOMP not detected in the kernel, skipping MemoryDenyWriteExecute

is better than

Aug 21 21:50:58 systemd[968]: SECCOMP not detected in the kernel, skipping MemoryDenyWriteExecute

poettering · 2016-08-22T11:42:15Z

looks pretty OK overall, only the two minor things left: log_unit_debug() would indeed be nicer to use, and please add the "=" to the options logged. Otherwise looks fine.

(the test-execute issue may be fixed separately, it's after all unrelated to your work)

evverx · 2016-08-22T14:04:20Z

the test-execute issue may be fixed separately, it's after all unrelated to your work

https://github.com/systemd/systemd/pull/4004/files#diff-cbcbc0f7e2e04ddc0a50f2ef7cdf4c03R33

Should be:

#ifdef HAVE_SECCOMP
#include "seccomp-util.h"
#endif

Am I missing something? :)

Fixes systemd#3882

fsateler · 2016-08-22T14:06:36Z

Force pushed will all comments addressed (and added the #ifdef guard to test-execute.c)

poettering · 2016-08-22T14:15:16Z

lgtm

evverx · 2016-08-22T14:28:53Z

@fsateler , many thanks!
Works fine:

-bash-4.3# grep -i seccomp /proc/self/status # CONFIG_SECCOMP is not set

-bash-4.3# journalctl -b --no-hostname | grep -i seccomp
Aug 22 04:13:03 systemd[1]: systemd 231 running in system mode. (+PAM +AUDIT +SELINUX +IMA -APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ -LZ4 +SECCOMP +BLKID +ELFUTILS +KMOD +IDN)
Aug 22 04:13:07 systemd[969]: systemd-journald.service: SECCOMP not detected in the kernel, skipping MemoryDenyWriteExecute=
Aug 22 04:13:07 systemd[969]: systemd-journald.service: SECCOMP not detected in the kernel, skipping syscall filtering
Aug 22 04:13:20 systemd[1472]: systemd-timesyncd.service: SECCOMP not detected in the kernel, skipping MemoryDenyWriteExecute=
Aug 22 04:13:20 systemd[1472]: systemd-timesyncd.service: SECCOMP not detected in the kernel, skipping syscall filtering
Aug 22 04:13:22 systemd[1477]: systemd-logind.service: SECCOMP not detected in the kernel, skipping MemoryDenyWriteExecute=
Aug 22 04:13:22 systemd[1477]: systemd-logind.service: SECCOMP not detected in the kernel, skipping syscall filtering

But

  CC       src/core/libcore_la-load-fragment.lo
In file included from src/core/execute.h:252:0,
                 from src/core/execute.c:68:
src/core/execute.c: In function ‘skip_seccomp_unavailable’:
src/core/unit.h:642:28: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
                 Unit *_u = (unit);                                      \
                            ^
src/core/unit.h:647:37: note: in expansion of macro ‘log_unit_full’
 #define log_unit_debug(unit, ...)   log_unit_full(unit, LOG_DEBUG, 0, ##__VA_ARGS__)
                                     ^
src/core/execute.c:1080:17: note: in expansion of macro ‘log_unit_debug’
                 log_unit_debug(u, "SECCOMP not detected in the kernel, skipping %s", msg);
                 ^

I guess we should change Unit *_u to const Unit *_u

fsateler · 2016-08-22T14:31:27Z

@evverx yes I noticed, I'm prepping a separate PR to fix that :)

In general, there is very little const correctness (eg, sd_bus_get_* do not take a const sd_bus*). Is this something worth pursuing? At least for the public API.

keszybz · 2016-08-22T14:59:40Z

Unfortunately sd_bus_get_* are not fixable (public API). We discussed that a while ago. Maybe when we bump the so version.

poettering · 2016-08-22T15:16:09Z

I really don't think sd_bus_get_xyz() should take const bus objects. We want the liberty that we cache whatever they return, and we really shouldn't encode in the API whether some calls might internally cache something (and thus alter the object) or not. Caching is internal, and callers should not be bothered with such logic.

fsateler · 2016-08-22T15:53:57Z

@keszybz I can't find a reference to the old discussion either in the issue tracker or on the mailing lists, do you have a pointer to that?

Adding a const qualifier should be backwards-compatible both API and ABI-wise: http://stackoverflow.com/a/5083976/1117815 refers to the C99 standard, I can't find a C90 standard copy online, but I think the result should be the same.

Moreover, the constness of a parameter has already been changed at least once: #529

@poettering do you mean that currently, sd_bus_get_xyz can block if some value is not already cached?

poettering · 2016-08-22T17:19:54Z

@fsateler changing const of function parameters breaks API as pointers taken of that function will change their type.

PR #529 was before we actually declared the sd-bus API stable, hence it was fine to go in.

@fsateler This is not about blocking or not. It's simply about making guarantees about the future. Right now, some sd_bus_get_xyz() calls might not read write access, but let's say one of these options becomes obsolete one day, and replaced by a related, but different API. The old calls are mapped internally to the new ones, to maintain compat. Since the mapping isn't free we might choose to cache what we return, to speed things up when something is called multiple times.

Or think of a different case: we have a couple of cases where we cache stuff in process, things like "what is the current cgroup root?", "what is the current log level?" or "what does $SYSTEMD_COLORS evaluate to?". Maybe one of those days we want to make some caches like that per-bus-object, instead of per-process. Right now, that's totally possible, we can hide that neatly. However, if we make the guarantee through const-ness that these initialize-once variables are never ever part of sd_bus, then we limit our liberties to change internals quite drastically.

It's really a general question: if you make the object argument "const" then this means, you can never write to it, ever, from that function, not even for cache variables, or statistics, or debugging, or anyting. And that's just a very weird bet to take for the future...

I am very sure that "const" has no place on the "object" argument of functions. It's fine for "non-object" arguments, but not otherwise.

fsateler · 2016-08-22T17:58:57Z

OK, makes sense. I won't work on the const thing then.

evverx · 2016-08-22T19:46:23Z

BTW
@martinpitt , @fsateler

https://anonscm.debian.org/cgit/pkg-systemd/systemd.git/commit/?id=39ccf2b0a0d5899898452efec7bcb6b04100a871

Revert "units: add a basic SystemCallFilter
...
This can be reactivated once #3882 is fixed.

Please note #3962
This affects Debian/Ubuntu too
Fix: https://anonscm.debian.org/git/selinux/libselinux.git/commit/?id=f8713974c7e925bc82506a090eade13193f57783

martinpitt · 2016-08-23T04:48:39Z

@evverx : Thanks for pointing out! Debian's and Ubuntu's development series already have that fix in libselinux. In systemd packaging git the reversion of system call filters now got dropped and replaced by a cherry-pick of this upstream patch.

Thanks @fsateler for fixing!

When running `sys-apps/systemd[seccomp]` on a Kernel without `CONFIG_SECCOMP` enabled (e.g. when running a Gentoo container for bootstrapping purposes on another OS buildhost), starting services with defined SECCOMP filters will fail (`status=228/SECCOMP`) due to missing runtime checks for SECCOMP availability. Upstream fixed this in 83f12b27 which will be included in systemd-232. See also: - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=832893 - systemd/systemd#3882 - systemd/systemd#4004 Package-Manager: portage-2.2.28

In systemd#4004 , a runtime detection method for seccomp was added. However, it does not detect the case where CONFIG_SECCOMP=y but CONFIG_SECCOMP_FILTER=n. This is possible if the architecture does not support filtering yet. Add a check for that case too. While at it, change get_proc_field usage to use PR_GET_SECCOMP prctl, as that should save a few system calls and (unnecessary) allocations. Previously, reading of /proc/self/stat was done as recommended by prctl(2) as safer. However, given that we need to do the prctl call anyway, lets skip opening, reading and parsing the file. Code for checking inspired by https://outflux.net/teach-seccomp/autodetect.html

keszybz reviewed Aug 20, 2016
View reviewed changes

poettering added pid1 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks seccomp labels Aug 21, 2016

fsateler force-pushed the detect-seccomp branch from 8c5af82 to ee4c0c9 Compare August 21, 2016 21:17

evverx reviewed Aug 21, 2016
View reviewed changes

fsateler force-pushed the detect-seccomp branch from ee4c0c9 to a9b1a29 Compare August 22, 2016 13:54

core: do not fail at step SECCOMP if there is no kernel support

f093220

Fixes systemd#3882

fsateler force-pushed the detect-seccomp branch from a9b1a29 to f093220 Compare August 22, 2016 14:06

poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Aug 22, 2016

fsateler mentioned this pull request Aug 22, 2016

core,network: Use const qualifiers for block-local variables in macro… #4019

Merged

evverx merged commit 83f12b2 into systemd:master Aug 22, 2016

fsateler deleted the detect-seccomp branch August 22, 2016 19:51

eliasp mentioned this pull request Aug 31, 2016

sys-apps/systemd: Add patch for failing SECCOMP checks gentoo/gentoo#2175

Closed

fsateler mentioned this pull request Sep 2, 2016

seccomp: also detect if seccomp filtering is available #4087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: do not fail at step SECCOMP if there is no kernel support #4004

core: do not fail at step SECCOMP if there is no kernel support #4004

fsateler commented Aug 20, 2016

keszybz Aug 20, 2016

fsateler commented Aug 21, 2016

evverx Aug 21, 2016

poettering commented Aug 22, 2016

evverx commented Aug 22, 2016

fsateler commented Aug 22, 2016

poettering commented Aug 22, 2016

evverx commented Aug 22, 2016

fsateler commented Aug 22, 2016

keszybz commented Aug 22, 2016

poettering commented Aug 22, 2016 •

edited

fsateler commented Aug 22, 2016

poettering commented Aug 22, 2016

fsateler commented Aug 22, 2016

evverx commented Aug 22, 2016

martinpitt commented Aug 23, 2016

core: do not fail at step SECCOMP if there is no kernel support #4004

core: do not fail at step SECCOMP if there is no kernel support #4004

Conversation

fsateler commented Aug 20, 2016

keszybz Aug 20, 2016

Choose a reason for hiding this comment

fsateler commented Aug 21, 2016

evverx Aug 21, 2016

Choose a reason for hiding this comment

poettering commented Aug 22, 2016

evverx commented Aug 22, 2016

fsateler commented Aug 22, 2016

poettering commented Aug 22, 2016

evverx commented Aug 22, 2016

fsateler commented Aug 22, 2016

keszybz commented Aug 22, 2016

poettering commented Aug 22, 2016 • edited

fsateler commented Aug 22, 2016

poettering commented Aug 22, 2016

fsateler commented Aug 22, 2016

evverx commented Aug 22, 2016

martinpitt commented Aug 23, 2016

poettering commented Aug 22, 2016 •

edited