Re: [systemd-devel] offline updates
On Tue, 2015-07-21 at 03:27 +, Zbigniew Jędrzejewski-Szmek wrote: fedup-system-upgrade.service uses an additional flag file which is checked with ConditionPathExists so it will not run if 'dnf fedup reboot' did not create the flag, even if we go into system-upgrade.target. packagekit-offline-update.service does not have anything like this, and is always run in system-upgrade.target. Strange - from what I can tell from the source, pk-offline-update is supposed to bail out unless the update was prepared by PackageKit itself[1]? Perhaps your system was also trying to install new updates via PackageKit at the time? Maybe packagekit-offline-update.service also needs a ConditionPathExists.. Running two upgrade mechanisms in parallel does not seem like a good idea. (Even if they use a lock file to prevent concurrent access to the rpm database, they are bound to interfere with one another: the first finishes and decides to reboot, or the first updates some packages and messes up the state for the second one...) It seems that *some* mechanism to run only one upgrade mechanism is wanted. The approach that dnf-plugin-fedup uses seems reasonable: it creates the file only when a reboot with 'dnf fedup reboot' is requested. As an alternative we could allow only one upgrade mechanism to be enabled. Dunno. How would that be enforced, though? Special handling for system -updates.target? Or: does systemd need some convention/support for handling the general problem of choosing one of multiple (mutually-exclusive) services that provide the same functionality? Also, which is a minor thing, but related: OnFailure=reboot.target seems inferior to FailureAction=reboot. IIRC, the second one uses irreversible transaction and should be more robust. It also is a higher level setting in some sense. OnFailure=reboot.target is taken directly from the spec, so should be changed there first. I'll add a note about this to the fedup-system-upgrade.service, I guess, and if the spec changes I'll change it there too. So maybe tmpfiles snippet should be used to remove the link. Such a change would mean that the update services should not depend on the symlink being present, and should instead look for their installation data in their own state directory. Right - this is what dnf-plugin-fedup does, since pk-offline-update might run first and remove the symlink before we get there. To summarize, following changes to the spec are proposed: - use Condition* or similar to conditionalize whether a specific upgrade mechanism should run - use Action=reboot FailureAction=reboot, I guess? - use Type=oneshot This is the default, correct? Should it be explicitly listed in the unit file, or should I rely on the default behavior? - check that logind.Reboot() is not called on failure by the service - services should not look for /systemd-update symlink, and the symlink should be removed by tmpfiles before we even get to the upgrade. These all seem reasonable to me. I'll happily make changes to dnf -plugin-fedup to follow any changes to the spec, once consensus is reached and the spec is changed. Thanks for testing this! -w [1] https://github.com/hughsie/PackageKit/blob/PACKAGEKIT_1_0_7/client/ pk-offline-update.c#L404 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] selinux: fix SEGV during switch-root if SELinux policy loaded
If you've got SELinux policy loaded, label_hnd is your labeling handle.
When systemd is shutting down, we free that handle via mac_selinux_finish().
But: switch_root() calls mkdir_p_label(), which tries to look up a label
using that freed handle, and so we get a bunch of garbage and eventually
SEGV in libselinux.
(This doesn't happen in the switch-root from initramfs to real root because
there's no SELinux policy loaded in initramfs, so label_hnd is NULL and we
never attempt any lookups.)
So: make sure that mac_selinux_finish() actually sets label_hnd to NULL, so
nobody tries to use it after it becomes invalid.
Resolves: RHBZ#1185604
---
src/shared/selinux-util.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/shared/selinux-util.c b/src/shared/selinux-util.c
index a8d5fc4..7c58985 100644
--- a/src/shared/selinux-util.c
+++ b/src/shared/selinux-util.c
@@ -116,6 +116,7 @@ void mac_selinux_finish(void) {
return;
selabel_close(label_hnd);
+label_hnd = NULL;
#endif
}
--
2.1.0
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core: let selinux_setup() load policy more than once
On Fri, 2014-04-25 at 18:26 -0400, Will Woods wrote: Currently, systemd refuses to load SELinux policy more than once. Normal systems don't care, because they either: a) have initramfs without policy, then load policy after switch-root, or b) load policy in initramfs, and never switch-root out. But if you *do* switch-root more than once - which fedup does! - you're supposed to run selinux_init_load_policy() afterward to ensure that you set up selinuxfs and load the new SELinux policy correctly. For reference, here's the thread from seli...@tycho.nsa.gov where this was discussed: http://marc.info/?l=selinuxm=139782596307221w=2 The upshot is: yes, we're supposed to do selinux_init_load_policy() after *every* switch-root. -w ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] core: let selinux_setup() load policy more than once
Hey all, Currently, systemd refuses to load SELinux policy more than once. Normal systems don't care, because they either: a) have initramfs without policy, then load policy after switch-root, or b) load policy in initramfs, and never switch-root out. But if you *do* switch-root more than once - which fedup does! - you're supposed to run selinux_init_load_policy() afterward to ensure that you set up selinuxfs and load the new SELinux policy correctly. So: this patchset fixes some weird and subtle upgrade bugs[1] by allowing systemd to load policy after each switch-root. The first patch is cosmetic: selinux-setup.c was indented 7 spaces (!?) and it bothered me too much to leave it alone. The second patch makes systemd try to load policy regardless of whether or not SELinux is already initialized. It also changes the failure behavior slightly: normally if policy load fails and the user requested enforcing, we freeze the system rather than running without policy. But if SELinux was already initialized, then we *do* have policy, so we don't have to freeze. Diffstat ignoring whitespace: src/core/ima-setup.c | 0 src/core/selinux-setup.c | 10 -- 2 files changed, 4 insertions(+), 6 deletions(-) Thanks, -w [1] Here's one weird and subtle bug: https://bugzilla.redhat.com/show_bug.cgi?id=1045864 It turns out there's actually a race between systemd-cryptsetup and systemd-ask-password-*.path to create /run/systemd/ask-password. If systemd wins, it labels the directory correctly, but systemd-cryptsetup doesn't do label_init(), so it doesn't know how to label the directory. Normally that doesn't matter, because systemd relabels /run after it reloads policy... but if it doesn't load policy, it doesn't relabel /run, the directory keeps the wrong label, and then the password agent can't read files from there, and your boot hangs forever waiting for a password... ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/2] core: reindent {selinux, ima, smack}-setup.c
7-space indentation is just too weird to leave alone.
Make it 8 spaces, as per CODING_STYLE. No other changes.
---
src/core/ima-setup.c | 110 +-
src/core/selinux-setup.c | 152 +++
2 files changed, 131 insertions(+), 131 deletions(-)
diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
index ed65096..7bffd8d 100644
--- a/src/core/ima-setup.c
+++ b/src/core/ima-setup.c
@@ -44,63 +44,63 @@
int ima_setup(void) {
#ifdef HAVE_IMA
- struct stat st;
- ssize_t policy_size = 0, written = 0;
- char *policy;
- _cleanup_close_ int policyfd = -1, imafd = -1;
- int result = 0;
-
- if (stat(IMA_POLICY_PATH, st) 0)
- return 0;
-
- policy_size = st.st_size;
- if (stat(IMA_SECFS_DIR, st) 0) {
- log_debug(IMA support is disabled in the kernel, ignoring.);
- return 0;
- }
-
- if (stat(IMA_SECFS_POLICY, st) 0) {
- log_error(Another IMA custom policy has already been loaded,
- ignoring.);
- return 0;
- }
-
- policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
- if (policyfd 0) {
- log_error(Failed to open the IMA custom policy file %s (%m),
- ignoring., IMA_POLICY_PATH);
- return 0;
- }
-
- imafd = open(IMA_SECFS_POLICY, O_WRONLY|O_CLOEXEC);
- if (imafd 0) {
- log_error(Failed to open the IMA kernel interface %s (%m),
- ignoring., IMA_SECFS_POLICY);
- goto out;
- }
-
- policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
- if (policy == MAP_FAILED) {
- log_error(mmap() failed (%m), freezing);
- result = -errno;
- goto out;
- }
-
- written = loop_write(imafd, policy, (size_t)policy_size, false);
- if (written != policy_size) {
- log_error(Failed to load the IMA custom policy file %s (%m),
- ignoring., IMA_POLICY_PATH);
- goto out_mmap;
- }
-
- log_info(Successfully loaded the IMA custom policy %s.,
-IMA_POLICY_PATH);
+struct stat st;
+ssize_t policy_size = 0, written = 0;
+char *policy;
+_cleanup_close_ int policyfd = -1, imafd = -1;
+int result = 0;
+
+if (stat(IMA_POLICY_PATH, st) 0)
+return 0;
+
+policy_size = st.st_size;
+if (stat(IMA_SECFS_DIR, st) 0) {
+log_debug(IMA support is disabled in the kernel, ignoring.);
+return 0;
+}
+
+if (stat(IMA_SECFS_POLICY, st) 0) {
+log_error(Another IMA custom policy has already been loaded,
+ ignoring.);
+return 0;
+}
+
+policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
+if (policyfd 0) {
+log_error(Failed to open the IMA custom policy file %s (%m),
+ ignoring., IMA_POLICY_PATH);
+return 0;
+}
+
+imafd = open(IMA_SECFS_POLICY, O_WRONLY|O_CLOEXEC);
+if (imafd 0) {
+log_error(Failed to open the IMA kernel interface %s (%m),
+ ignoring., IMA_SECFS_POLICY);
+goto out;
+}
+
+policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
+if (policy == MAP_FAILED) {
+log_error(mmap() failed (%m), freezing);
+result = -errno;
+goto out;
+}
+
+written = loop_write(imafd, policy, (size_t)policy_size, false);
+if (written != policy_size) {
+log_error(Failed to load the IMA custom policy file %s (%m),
+ ignoring., IMA_POLICY_PATH);
+goto out_mmap;
+}
+
+log_info(Successfully loaded the IMA custom policy %s.,
+ IMA_POLICY_PATH);
out_mmap:
- munmap(policy, policy_size);
+munmap(policy, policy_size);
out:
- if (result)
-return result;
+if (result)
+ return result;
#endif /* HAVE_IMA */
- return 0;
+return 0;
}
diff --git a/src/core/selinux-setup.c b/src/core/selinux-setup.c
index 9a5d6b2..6d8bc89 100644
--- a/src/core/selinux-setup.c
+++ b/src/core/selinux-setup.c
@@ -46,82 +46,82 @@ static int null_log(int type, const char *fmt, ...) {
int selinux_setup(bool *loaded_policy) {
#ifdef HAVE_SELINUX
- int enforce = 0;
- usec_t before_load, after_load;
- security_context_t con;
- int r;
- union selinux_callback cb;
-
- assert(loaded_policy);
-
- /* Turn off all of SELinux' own logging, we want to do that */
- cb.func_log = null_log;
-
[systemd-devel] [PATCH 2/2] core: let selinux_setup() load policy more than once
When you switch-root into a new root that has SELinux policy, you're
supposed to to run selinux_init_load_policy() to set up SELinux and load
policy. Normally this gets handled by selinux_setup().
But if SELinux was already initialized, selinux_setup() skips loading
policy and returns 0. So if you load policy normally, and then you
switch-root to a new root that has new policy, selinux_setup() never
loads the new policy. What gives?
As far as I can tell, this check is an artifact of how selinux_setup()
worked when it was first written (see commit c4dcdb9 / systemd v12):
* when systemd starts, run selinux_setup()
* if selinux_setup() loads policy OK, restart systemd
So the if policy already loaded, skip load and return 0 check was
there to prevent an infinite re-exec loop.
Modern systemd only calls selinux_setup() on initial load and after
switch-root, and selinux_setup() no longer restarts systemd, so we don't
need that check to guard against the infinite loop anymore.
So: this patch removes the return 0, thus allowing selinux_setup() to
actually perform SELinux setup after switch-root.
We still want to check to see if SELinux is initialized, because if
selinux_init_load_policy() fails *but* SELinux is initialized that means
we still have (old) policy active. So we don't need to halt if
enforce=1.
---
src/core/selinux-setup.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/core/selinux-setup.c b/src/core/selinux-setup.c
index 6d8bc89..578a18f 100644
--- a/src/core/selinux-setup.c
+++ b/src/core/selinux-setup.c
@@ -51,6 +51,7 @@ int selinux_setup(bool *loaded_policy) {
security_context_t con;
int r;
union selinux_callback cb;
+bool initialized = false;
assert(loaded_policy);
@@ -68,13 +69,8 @@ int selinux_setup(bool *loaded_policy) {
/* Already initialized by somebody else? */
r = getcon_raw(con);
if (r == 0) {
-bool initialized;
-
initialized = !streq(con, kernel);
freecon(con);
-
-if (initialized)
-return 0;
}
/* Make sure we have no fds open while loading the policy and
@@ -115,9 +111,11 @@ int selinux_setup(bool *loaded_policy) {
} else {
log_open();
-if (enforce 0) {
+if ((enforce 0) (!initialized)) {
log_error(Failed to load SELinux policy. Freezing.);
return -EIO;
+} else if (enforce 0) {
+log_warning(Failed to load new SELinux policy.
Continuing with old policy.);
} else
log_debug(Unable to load SELinux policy. Ignoring.);
}
--
1.9.0
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: Only attempt to load policy exactly once, in the real root
On Thu, 2014-02-20 at 18:17 +, Colin Walters wrote: I think both of these (particularly the second) are worse than my patch - we don't (to my knowledge) support putting policy in the initramfs now with Fedora or Red Hat Enterprise Linux, so attempting to find it there by default on every bootup is wrong. To turn it around, what is the possible value in also probing the initramfs? Does anyone out there load policy from it with systemd? Oof. I'm late, but: actually, yes. Fedup does this during upgrades. We ship the new system's policy in upgrade.img in an attempt to ensure that files written during the upgrade get the right labels. (For example: if you're upgrading F19-F20, we load F20 policy during initramfs, then switch_root to the F19 system to get disks mounted, then switch_root back to initramfs to run the upgrade.) Loading policy in initramfs seems to have some unfortunate side-effects, though. For example, every process runs with kernel_t, and all the files outside of /run and /dev are root_t. Also, something seems racy - sometimes /run/systemd/ask-password ends up init_var_run_t, which keeps us from unlocking /home later. Ugh. It'd probably be better to load F19 policy first, then load F20 policy after the second switch_root *back* to initramfs.. except systemd won't do that either. I'm open to any ideas about how to make sure that the upgrade runs with the right policy loaded (or, really, how to be sure that files written during the upgrade get the correct labels for the new system). Otherwise, this patch will probably interfere with upgrades to F21. -w ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] mount: make sure m-where is set before unit_add_exec_dependencies()
If you enter unit_add_exec_dependencies with m-where = NULL, you'll
very likely end up aborting somewhere under socket_needs_mount.
(When systemd goes to check to see if the journald socket requires your
mount, it'll do path_startswith(path, m-where)... *kaboom*)
This patch should ensure that:
a) both branches in mount_add_one() set m-where, and
b) mount_add_extras() calls unit_add_exec_dependencies() *after*
setting m-where.
---
src/core/mount.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/core/mount.c b/src/core/mount.c
index e1c240e..14f4863 100644
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -547,10 +547,6 @@ static int mount_add_extras(Mount *m) {
Unit *u = UNIT(m);
int r;
-r = unit_add_exec_dependencies(u, m-exec_context);
-if (r 0)
-return r;
-
if (UNIT(m)-fragment_path)
m-from_fragment = true;
@@ -562,6 +558,10 @@ static int mount_add_extras(Mount *m) {
path_kill_slashes(m-where);
+r = unit_add_exec_dependencies(u, m-exec_context);
+if (r 0)
+return r;
+
if (!UNIT(m)-description) {
r = unit_set_description(u, m-where);
if (r 0)
@@ -1459,6 +1459,14 @@ static int mount_add_one(
delete = false;
free(e);
+if (!MOUNT(u)-where) {
+MOUNT(u)-where = strdup(where);
+if (!MOUNT(u)-where) {
+r = -ENOMEM;
+goto fail;
+}
+}
+
if (u-load_state == UNIT_ERROR) {
u-load_state = UNIT_LOADED;
u-load_error = 0;
--
1.7.11.7
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
