checkout: Add force_copy+SELinux options for checkout, use in deploy #797

cgwalters · 2017-04-14T22:02:33Z

This is a variant of the efforts in #741
Working on rpm-ostree livefs, I realized though I needed to just
check out new files directly into the live /etc (and possibly
delete obsolete files).

The way the current /etc merge works is fundamentally different from
that. So my plan currently is to probably do something like:

Compute diff
Check out each new file individually (as a copy)
Optionally delete obsolete files

Also, a few other things become more important - in the current deploy code, we
copy all of the files, then relabel them. But we shouldn't expose to live
systems the race conditions of doing that, plus we should only relabel files we
checked out.

By converting the deploy's /etc code to use this, we fix the same TODO item
there around atomically having the label set up as we create files. And further,
if we kill the /var relabeling which I think is unnecessary since Anaconda
does it, we could delete large chunks of code there.

cgwalters · 2017-04-14T22:03:21Z

WIP, needs more testing. Also I note that we should really make more use of setfscreatecon() for dealing with the SELinux xattr... and basically have glnx_set_xattrs_except_selinux().

cgwalters · 2017-04-19T18:27:52Z

OK, removing WIP. I've tested this manually on a F25AH host. Note this rolls in #801

cgwalters · 2017-04-19T18:31:40Z

src/libostree/ostree-repo.h

@@ -767,14 +767,17 @@ typedef struct {
  gboolean enable_fsync;  /* Deprecated */
  gboolean process_whiteouts;
  gboolean no_copy_fallback;
-  gboolean unused_bools[7];
+  gboolean force_copy;


I may break this out as a separate PR too with exposure via the cmdline as well.

rh-atomic-bot · 2017-04-20T21:13:38Z

☔ The latest upstream changes (presumably 50ca653) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-04-21T19:57:31Z

🏄‍♂️ Rebased

This depends on #804 and #801

jlebon

This looks good. Though it'd be really nice if we had #771 as an added check here.

jlebon · 2017-04-24T17:02:43Z

src/libostree/ostree-repo-checkout.c

+                                          &label, cancellable, error))
+            return FALSE;
+          if (fsetxattr (temp_fd, "security.selinux", label, strlen (label), 0) < 0)
+            return glnx_throw_errno_prefix (error, "Setting security.selinux");


Nit: would be helpful to include the name of the file in the error msg.

We already do that via the
return g_prefix_error (error, "Checkout of %s to %s: ", checksum, destination_name), FALSE;
in the caller.

Ahh, got it.

jlebon · 2017-04-24T17:10:24Z

src/libostree/ostree-repo-checkout.c

+      state.selabel_path_buf = buf;
+
+      /* Otherwise it'd just be corrupting things, and there's no use case */
+      g_assert (options->force_copy);


Hmm, let's enforce this at the API entry point too.

This was already very close to the entrypoint, but OK. Fixup ⬇️

jlebon · 2017-04-24T17:19:55Z

src/libostree/ostree-repo-checkout.c

@@ -579,22 +652,15 @@ checkout_tree_at (OstreeRepo                        *self,
                       (guint64)repo_dfd_stat.st_dev, (guint64)destination_stat.st_dev);

  /* Set the xattrs now, so any derived labeling works */


Can we update this comment as well since we might have done labeling already above?

rh-atomic-bot · 2017-04-25T14:04:41Z

☔ The latest upstream changes (presumably 511b31c) made this pull request unmergeable. Please resolve the merge conflicts.

jlebon · 2017-04-25T14:32:38Z

Let's land #807 first before merging this one.

jlebon · 2017-04-25T15:27:29Z

🚄 @rh-atomic-bot r+ 8aefb93

rh-atomic-bot · 2017-04-25T15:27:33Z

⌛ Testing commit 8aefb93 with merge dadb5f8...

This is a variant of the efforts in #741 Working on `rpm-ostree livefs`, I realized though I needed to just check out *new* files directly into the live `/etc` (and possibly delete obsolete files). The way the current `/etc` merge works is fundamentally different from that. So my plan currently is to probably do something like: - Compute diff - Check out each *new* file individually (as a copy) - Optionally delete obsolete files Also, a few other things become more important - in the current deploy code, we copy all of the files, then relabel them. But we shouldn't expose to *live* systems the race conditions of doing that, plus we should only relabel files we checked out. By converting the deploy's /etc code to use this, we fix the same TODO item there around atomically having the label set up as we create files. And further, if we kill the `/var` relabeling which I think is unnecessary since Anaconda does it, we could delete large chunks of code there. In the implementation, there are two types of things: regular files, and symlinks. For regular files, in the `O_TMPFILE` case, we have the ability to do *everything* atomically (including SELinux labeling) before linking it into place. So let's just use that. For symlinks, we use `setfscreatecon()`. Closes: #797 Approved by: jlebon

This is a variant of the efforts in ostreedev#741 Working on `rpm-ostree livefs`, I realized though I needed to just check out *new* files directly into the live `/etc` (and possibly delete obsolete files). The way the current `/etc` merge works is fundamentally different from that. So my plan currently is to probably do something like: - Compute diff - Check out each *new* file individually (as a copy) - Optionally delete obsolete files Also, a few other things become more important - in the current deploy code, we copy all of the files, then relabel them. But we shouldn't expose to *live* systems the race conditions of doing that, plus we should only relabel files we checked out. By converting the deploy's /etc code to use this, we fix the same TODO item there around atomically having the label set up as we create files. And further, if we kill the `/var` relabeling which I think is unnecessary since Anaconda does it, we could delete large chunks of code there. In the implementation, there are two types of things: regular files, and symlinks. For regular files, in the `O_TMPFILE` case, we have the ability to do *everything* atomically (including SELinux labeling) before linking it into place. So let's just use that. For symlinks, we use `setfscreatecon()`.

jlebon · 2017-04-25T16:47:31Z

Weird, the status did get reported.
@rh-atomic-bot retry

jlebon · 2017-04-25T16:51:51Z

@rh-atomic-bot retry

jlebon · 2017-04-25T16:52:32Z

Ohh I see.
@rh-atomic-bot r+ 2048b88

rh-atomic-bot · 2017-04-25T16:52:35Z

⌛ Testing commit 2048b88 with merge e8efd1c...

rh-atomic-bot · 2017-04-25T17:03:36Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing e8efd1c to master...

This fixes a regression from: ostreedev#797 which is really due to an underlying bug in libselinux which we're working around: http://marc.info/?l=selinux&m=149323809332417&w=2 We drop the per-policy instance variable, since the SELinux state is *really* per-kernel.

This fixes a regression from: ostreedev#797 which is really due to an underlying bug in libselinux which we're working around: http://marc.info/?l=selinux&m=149323809332417&w=2 We drop the per-policy instance variable, since the SELinux state is *really* per-kernel. Closes: ostreedev#814

This breaks every further call to e.g. `is_selinux_enabled()` after a policy root has been set. This tripped up some code landed in libostree: ostreedev/ostree#797 Since in some cases we initialize a policy twice in process, and we'd call `is_selinux_enabled()` each time. More info in: http://marc.info/?l=selinux&m=149323809332417&w=2 Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

This fixes a regression from: #797 which is really due to an underlying bug in libselinux which we're working around: http://marc.info/?l=selinux&m=149323809332417&w=2 We drop the per-policy instance variable, since the SELinux state is *really* per-kernel. Closes: #814 Closes: #815 Approved by: jlebon

Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the ostreedev#797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example.

This breaks every further call to e.g. `is_selinux_enabled()` after a policy root has been set. This tripped up some code landed in libostree: ostreedev/ostree#797 Since in some cases we initialize a policy twice in process, and we'd call `is_selinux_enabled()` each time. More info in: http://marc.info/?l=selinux&m=149323809332417&w=2 Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> (cherry picked from SELinuxProject commit f3a264c)

Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the ostreedev#797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example. Update submodule: libglnx

Rather than `g_output_stream_splice()`, where the input is a regular file. See GNOME/libglnx#44 for some more information. I didn't try to measure the performance difference, but seeing the read()/write() to/from userspace mixed in with the pointless `poll()` annoyed me when reading strace. As a bonus, we will again start using reflinks (if available) for `/etc`, which is a regression from the #797 changes (which before used `glnx_file_copy_at()`). Also, for the first time we'll use reflinks when doing commits from file-backed content. This happens in `rpm-ostree compose tree` today for example. Update submodule: libglnx Closes: #817 Approved by: jlebon

This breaks every further call to e.g. `is_selinux_enabled()` after a policy root has been set. This tripped up some code landed in libostree: ostreedev/ostree#797 Since in some cases we initialize a policy twice in process, and we'd call `is_selinux_enabled()` each time. More info in: http://marc.info/?l=selinux&m=149323809332417&w=2 Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> (cherry picked from SELinuxProject commit f3a264c)

This breaks every further call to e.g. `is_selinux_enabled()` after a policy root has been set. This tripped up some code landed in libostree: ostreedev/ostree#797 Since in some cases we initialize a policy twice in process, and we'd call `is_selinux_enabled()` each time. More info in: http://marc.info/?l=selinux&m=149323809332417&w=2 Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

cgwalters added the WIP label Apr 14, 2017

cgwalters force-pushed the checkout-labeling branch 2 times, most recently from 79ea5ba to 3d14c7a Compare April 19, 2017 18:26

cgwalters removed the WIP label Apr 19, 2017

cgwalters commented Apr 19, 2017

View reviewed changes

cgwalters force-pushed the checkout-labeling branch from e761345 to 663ee93 Compare April 19, 2017 21:19

cgwalters force-pushed the checkout-labeling branch from 663ee93 to 26b698d Compare April 21, 2017 19:56

jlebon reviewed Apr 24, 2017

View reviewed changes

cgwalters force-pushed the checkout-labeling branch from 26b698d to a76b8ee Compare April 25, 2017 01:24

cgwalters force-pushed the checkout-labeling branch from a76b8ee to 8aefb93 Compare April 25, 2017 14:11

cgwalters force-pushed the checkout-labeling branch from 8aefb93 to 2048b88 Compare April 25, 2017 15:30

rh-atomic-bot closed this in e8efd1c Apr 25, 2017

cgwalters mentioned this pull request Apr 26, 2017

sepolicy: Cache the value of is_selinux_enabled() to work around bug #815

Closed

cgwalters mentioned this pull request Apr 27, 2017

checkout/commit: Use glnx_regfile_copy_bytes() if possible #817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checkout: Add force_copy+SELinux options for checkout, use in deploy #797

checkout: Add force_copy+SELinux options for checkout, use in deploy #797

cgwalters commented Apr 14, 2017 •

edited

Loading

cgwalters commented Apr 14, 2017

cgwalters commented Apr 19, 2017

cgwalters Apr 19, 2017

rh-atomic-bot commented Apr 20, 2017

cgwalters commented Apr 21, 2017

jlebon left a comment

jlebon Apr 24, 2017

cgwalters Apr 25, 2017

jlebon Apr 25, 2017

jlebon Apr 24, 2017

cgwalters Apr 25, 2017

jlebon Apr 24, 2017

rh-atomic-bot commented Apr 25, 2017

jlebon commented Apr 25, 2017

jlebon commented Apr 25, 2017

rh-atomic-bot commented Apr 25, 2017

jlebon commented Apr 25, 2017

jlebon commented Apr 25, 2017

jlebon commented Apr 25, 2017

rh-atomic-bot commented Apr 25, 2017

rh-atomic-bot commented Apr 25, 2017

		@@ -579,22 +652,15 @@ checkout_tree_at (OstreeRepo *self,
		(guint64)repo_dfd_stat.st_dev, (guint64)destination_stat.st_dev);

		/* Set the xattrs now, so any derived labeling works */

checkout: Add force_copy+SELinux options for checkout, use in deploy #797

checkout: Add force_copy+SELinux options for checkout, use in deploy #797

Conversation

cgwalters commented Apr 14, 2017 • edited Loading

cgwalters commented Apr 14, 2017

cgwalters commented Apr 19, 2017

cgwalters Apr 19, 2017

Choose a reason for hiding this comment

rh-atomic-bot commented Apr 20, 2017

cgwalters commented Apr 21, 2017

jlebon left a comment

Choose a reason for hiding this comment

jlebon Apr 24, 2017

Choose a reason for hiding this comment

cgwalters Apr 25, 2017

Choose a reason for hiding this comment

jlebon Apr 25, 2017

Choose a reason for hiding this comment

jlebon Apr 24, 2017

Choose a reason for hiding this comment

cgwalters Apr 25, 2017

Choose a reason for hiding this comment

jlebon Apr 24, 2017

Choose a reason for hiding this comment

rh-atomic-bot commented Apr 25, 2017

jlebon commented Apr 25, 2017

jlebon commented Apr 25, 2017

rh-atomic-bot commented Apr 25, 2017

jlebon commented Apr 25, 2017

jlebon commented Apr 25, 2017

jlebon commented Apr 25, 2017

rh-atomic-bot commented Apr 25, 2017

rh-atomic-bot commented Apr 25, 2017

cgwalters commented Apr 14, 2017 •

edited

Loading