Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What if char devices are being used as regular files? #2568

Open
petersenna opened this issue Mar 18, 2022 · 4 comments · May be fixed by #3318
Open

What if char devices are being used as regular files? #2568

petersenna opened this issue Mar 18, 2022 · 4 comments · May be fixed by #3318

Comments

@petersenna
Copy link

petersenna commented Mar 18, 2022

I am trying to commit Podman's local storage to OSTree. Now the issue is that Overlayfs uses char devices to represent specific states. From: https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt

   whiteouts and opaque directories
   
   In order to support rm and rmdir without changing the lower
   filesystem, an overlay filesystem needs to record in the upper filesystem
   that files have been removed.  This is done using whiteouts and opaque
   directories (non-directories are always opaque).
   
   A whiteout is created as a character device with 0/0 device number.
   When a whiteout is found in the upper level of a merged directory, any
   matching name in the lower level is ignored, and the whiteout itself
   is also hidden.

And now I am stuck because OSTree wants only regular files and links, and trying to commit a tree that contains char devices does not work.

I am trying to convince OSTree that, hey these char devices are actually regular files, because they represent state within the filesystem. But OSTree does not listen to me. Do I have any hope to use OSTree to also commit char devices for cases such as this one?

@cgwalters
Copy link
Member

Hi, this is related to ostreedev/ostree-rs-ext#246

I see the desire to simply ship podman's storage, but trying to change ostree to accept character devices at this time would be a large change. In the end, inherently the "ostree nature" of e.g. not shipping timestamps will also leak through into the containers, so it's really better for us to handle it by flattening I think.

@cgwalters
Copy link
Member

First, it makes no sense to ship device nodes in OS images (ostree commits, containers, disk images) in my opinion. These things represent dynamic state - container runtimes and systemd will ensure that /dev is populated dynamically.

Also because container runtimes do that, you have to go to effort to get content in /dev in your containers - like by having a naive non-container native build process that's something like chroot+tar.

There's two paths:

  • We could teach ostree to have a mode where we just ignore them (somewhat hackily, if we detect they're in a directory named dev or just unconditionally); I'm pretty sure that would Just Work in most cases, however there may be corner cases where e.g. container runtimes will see files missing that they expect
  • We could generalize the whiteout escaping in ostree, which I would really not be a big fan of but...it's doable.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Oct 1, 2024
We hit another case where people are pulling a container image
with devices in `/dev` in the tar stream; they're then trying
to commit this to an ostree.

There's much better ways to fix this:

- Change the image to stop including devices as there's no reason
  to do so
- Switch to logically bound images instead of physically bound
- Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but
it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work
for "podman run" of such images; podman will just use overlayfs
to stitch together the `diff` directories, and doesn't try to do
any validation of their contents today.
(Queue the composefs integration, which *would* do that but would
 also fix this anwyays)
@cgwalters
Copy link
Member

OK I looked at this briefly but it touches some of the most complex and hairy code in ostree...the write_quoted_device bit is just a stub and we need to handle all sorts of things like xattrs, uid/gid in a consistent way there but still do the "quoting". We'd need to define an encoding (filename like .ostree-quoted-device.[c|b].<major>.<minor>) that we serialize/deserialize etc.

From fdfeb0ba7b45b821c23fd3bd21f386d93dea6296 Mon Sep 17 00:00:00 2001
From: Colin Walters <[email protected]>
Date: Tue, 1 Oct 2024 17:07:17 -0400
Subject: [PATCH 1/2] checkout: Add commentary around whiteout "quoting"

Signed-off-by: Colin Walters <[email protected]>
---
 src/libostree/ostree-repo-checkout.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c
index 39ecc1d5..e83713d8 100644
--- a/src/libostree/ostree-repo-checkout.c
+++ b/src/libostree/ostree-repo-checkout.c
@@ -35,7 +35,13 @@
 #define WHITEOUT_PREFIX ".wh."
 #define OPAQUE_WHITEOUT_NAME ".wh..wh..opq"
 
-#define OVERLAYFS_WHITEOUT_PREFIX ".ostree-wh."
+// ostree doesn't have native support for devices. Whiteouts in overlayfs
+// are a 0:0 character device, and in some cases people are copying docker/podman
+// style overlayfs container storage directly into ostree commits. This
+// adds special support for "quoting" the whiteout so it just appears as a regular
+// file in the ostree commit, but can be converted back into a character device
+// on checkout.
+#define OSTREE_QUOTED_OVERLAYFS_WHITEOUT_PREFIX ".ostree-wh."
 
 /* Per-checkout call state/caching */
 typedef struct
@@ -711,7 +717,8 @@ checkout_one_file_at (OstreeRepo *repo, OstreeRepoCheckoutAtOptions *options, Ch
   const gboolean is_whiteout = (!is_symlink && options->process_whiteouts
                                 && g_str_has_prefix (destination_name, WHITEOUT_PREFIX));
   const gboolean is_overlayfs_whiteout
-      = (!is_symlink && g_str_has_prefix (destination_name, OVERLAYFS_WHITEOUT_PREFIX));
+      = (!is_symlink
+         && g_str_has_prefix (destination_name, OSTREE_QUOTED_OVERLAYFS_WHITEOUT_PREFIX));
   const gboolean is_reg_zerosized = (!is_symlink && g_file_info_get_size (source_info) == 0);
   const gboolean override_user_unreadable
       = (options->mode == OSTREE_REPO_CHECKOUT_MODE_USER && is_unreadable);
@@ -735,7 +742,7 @@ checkout_one_file_at (OstreeRepo *repo, OstreeRepoCheckoutAtOptions *options, Ch
     }
   else if (is_overlayfs_whiteout && options->process_passthrough_whiteouts)
     {
-      const char *name = destination_name + (sizeof (OVERLAYFS_WHITEOUT_PREFIX) - 1);
+      const char *name = destination_name + (sizeof (OSTREE_QUOTED_OVERLAYFS_WHITEOUT_PREFIX) - 1);
 
       if (!name[0])
         return glnx_throw (error, "Invalid empty overlayfs whiteout '%s'", name);
-- 
2.44.0


From 36b0f51e1d2c96112be7a9be663feabd854401b2 Mon Sep 17 00:00:00 2001
From: Colin Walters <[email protected]>
Date: Tue, 1 Oct 2024 17:31:18 -0400
Subject: [PATCH 2/2] wip

Signed-off-by: Colin Walters <[email protected]>
---
 man/ostree-commit.xml                |  9 +++++++++
 src/libostree/ostree-core-private.h  | 14 ++++++++++++++
 src/libostree/ostree-repo-checkout.c | 11 +++--------
 src/libostree/ostree-repo-commit.c   |  6 ++++++
 src/libostree/ostree-repo.h          |  7 ++++++-
 src/ostree/ot-builtin-commit.c       |  5 +++++
 6 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/man/ostree-commit.xml b/man/ostree-commit.xml
index 12f4fd10..d612282c 100644
--- a/man/ostree-commit.xml
+++ b/man/ostree-commit.xml
@@ -177,6 +177,15 @@ License along with this library. If not, see <https://www.gnu.org/licenses/>.
                 </para></listitem>
             </varlistentry>
 
+            <varlistentry>
+                <term><option>--quote-devices</option></term>
+                <listitem><para>
+                    By default, ostree rejects block and character devices. This option instead "quotes" them
+                    as regular files. In order to be processed back into block and character devices,
+                    the corresponding <literal>--unquote-devices</literal> must be passed to <literal>ostree checkout</literal>.
+                </para></listitem>
+            </varlistentry>
+
             <varlistentry>
                 <term><option>--no-xattrs</option></term>
                 <listitem><para>
diff --git a/src/libostree/ostree-core-private.h b/src/libostree/ostree-core-private.h
index 283944b4..a9b98789 100644
--- a/src/libostree/ostree-core-private.h
+++ b/src/libostree/ostree-core-private.h
@@ -78,6 +78,20 @@ G_BEGIN_DECLS
  */
 #define _OSTREE_ZLIB_FILE_HEADER_GVARIANT_FORMAT G_VARIANT_TYPE ("(tuuuusa(ayay))")
 
+// ostree doesn't have native support for devices. Whiteouts in overlayfs
+// are a 0:0 character device, and in some cases people are copying docker/podman
+// style overlayfs container storage directly into ostree commits. This
+// adds special support for "quoting" the whiteout so it just appears as a regular
+// file in the ostree commit, but can be converted back into a character device
+// on checkout.
+#define OSTREE_QUOTED_OVERLAYFS_WHITEOUT_PREFIX ".ostree-wh."
+// Filename prefix to signify a character or block device. This
+// is not supported natively by ostree (because there is no reason
+// to ship devices in images). But because OCI supports it, and in
+// some cases one wants to map OCI to ostree, we have support for
+// "quoting" them.
+#define OSTREE_QUOTED_DEVICE_PREFIX ".ostree-quoted-device."
+
 GBytes *_ostree_file_header_new (GFileInfo *file_info, GVariant *xattrs);
 
 GBytes *_ostree_zlib_file_header_new (GFileInfo *file_info, GVariant *xattrs);
diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c
index e83713d8..18d4d84c 100644
--- a/src/libostree/ostree-repo-checkout.c
+++ b/src/libostree/ostree-repo-checkout.c
@@ -35,14 +35,6 @@
 #define WHITEOUT_PREFIX ".wh."
 #define OPAQUE_WHITEOUT_NAME ".wh..wh..opq"
 
-// ostree doesn't have native support for devices. Whiteouts in overlayfs
-// are a 0:0 character device, and in some cases people are copying docker/podman
-// style overlayfs container storage directly into ostree commits. This
-// adds special support for "quoting" the whiteout so it just appears as a regular
-// file in the ostree commit, but can be converted back into a character device
-// on checkout.
-#define OSTREE_QUOTED_OVERLAYFS_WHITEOUT_PREFIX ".ostree-wh."
-
 /* Per-checkout call state/caching */
 typedef struct
 {
@@ -1437,6 +1429,9 @@ canonicalize_options (OstreeRepo *self, OstreeRepoCheckoutAtOptions *options)
   /* Force USER mode for BARE_USER_ONLY always - nothing else makes sense */
   if (ostree_repo_get_mode (self) == OSTREE_REPO_MODE_BARE_USER_ONLY)
     options->mode = OSTREE_REPO_CHECKOUT_MODE_USER;
+
+  if (options->unquote_devices)
+    options->process_whiteouts = TRUE;
 }
 
 /**
diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index 0ee97288..a7a04aab 100644
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -3889,6 +3889,12 @@ write_dfd_iter_to_mtree_internal (OstreeRepo *self, GLnxDirFdIterator *src_dfd_i
                                          error))
             return FALSE;
         }
+      else if (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_QUOTE_DEVICES)
+        {
+          if (!write_quoted_device (self, NULL, NULL, src_dfd_iter, flags, child_info, mtree,
+                                    modifier, path, cancellable, error))
+            return FALSE;
+        }
       else
         {
           return glnx_throw (error, "Not a regular file or symlink: %s", dent->d_name);
diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h
index d38fad9a..fc27a5da 100644
--- a/src/libostree/ostree-repo.h
+++ b/src/libostree/ostree-repo.h
@@ -519,6 +519,9 @@ typedef OstreeRepoCommitFilterResult (*OstreeRepoCommitFilter) (OstreeRepo *repo
  * modifier filters (non-directories only); Since: 2017.14
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SELINUX_LABEL_V1: For SELinux and other systems, label
  * /usr/etc as if it was /etc.
+ * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_QUOTE_DEVICES: Instead of erroring out on block/character
+ * devices, "quote" them as regular files that can optionally be unpacked back into native devices.
+ * Since: 2024.9
  *
  * Flags modifying commit behavior. In bare-user-only mode,
  * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS and
@@ -535,6 +538,7 @@ typedef enum
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME = (1 << 4),
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL = (1 << 5),
   OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SELINUX_LABEL_V1 = (1 << 6),
+  OSTREE_REPO_COMMIT_MODIFIER_FLAGS_QUOTE_DEVICES = (1 << 7),
 } OstreeRepoCommitModifierFlags;
 
 /**
@@ -802,12 +806,13 @@ typedef struct
   gboolean enable_uncompressed_cache; /* Deprecated */
   gboolean enable_fsync;              /* Deprecated */
   gboolean process_whiteouts;
+  gboolean unquote_devices; /* Since: 2024.9 */
   gboolean no_copy_fallback;
   gboolean force_copy;           /* Since: 2017.6 */
   gboolean bareuseronly_dirs;    /* Since: 2017.7 */
   gboolean force_copy_zerosized; /* Since: 2018.9 */
   gboolean process_passthrough_whiteouts;
-  gboolean unused_bools[3];
+  gboolean unused_bools[2];
   /* 3 byte hole on 64 bit */
 
   const char *subpath;
diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c
index 7c6d63e4..5e3a6b9d 100644
--- a/src/ostree/ot-builtin-commit.c
+++ b/src/ostree/ot-builtin-commit.c
@@ -62,6 +62,7 @@ static char *opt_base;
 static char **opt_trees;
 static gint opt_owner_uid = -1;
 static gint opt_owner_gid = -1;
+static gboolean opt_quote_devices;
 static gboolean opt_table_output;
 #ifndef OSTREE_DISABLE_GPGME
 static char **opt_gpg_key_ids;
@@ -124,6 +125,8 @@ static GOptionEntry options[] = {
   { "owner-gid", 0, 0, G_OPTION_ARG_INT, &opt_owner_gid, "Set file ownership group id", "GID" },
   { "canonical-permissions", 0, 0, G_OPTION_ARG_NONE, &opt_canonical_permissions,
     "Canonicalize permissions in the same way bare-user does for hardlinked files", NULL },
+  { "quote-devices", 0, 0, G_OPTION_ARG_NONE, &opt_quote_devices,
+    "Instead of erroring out on block/character devices, \"quote\" them as regular files", NULL },
   { "bootable", 0, 0, G_OPTION_ARG_NONE, &opt_bootable,
     "Flag this commit as a bootable OSTree (e.g. contains a Linux kernel)", NULL },
   { "mode-ro-executables", 0, 0, G_OPTION_ARG_NONE, &opt_ro_executables,
@@ -601,6 +604,8 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS;
   if (opt_consume)
     flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME;
+  if (opt_quote_devices)
+    flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_QUOTE_DEVICES;
   switch (opt_selinux_labeling_epoch)
     {
     case 0:
-- 
2.44.0

@cgwalters cgwalters linked a pull request Oct 2, 2024 that will close this issue
@cgwalters
Copy link
Member

Put up a bit more of a draft in #3318 but again - nontrivial and invasive and basically: Don't put devices in your images, it makes no sense.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Oct 2, 2024
We hit another case where people are pulling a container image
with devices in `/dev` in the tar stream; they're then trying
to commit this to an ostree.

There's much better ways to fix this:

- Change the image to stop including devices as there's no reason
  to do so
- Switch to logically bound images instead of physically bound
- Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but
it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work
for "podman run" of such images; podman will just use overlayfs
to stitch together the `diff` directories, and doesn't try to do
any validation of their contents today.
(Queue the composefs integration, which *would* do that but would
 also fix this anwyays)

Signed-off-by: Colin Walters <[email protected]>
cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Oct 14, 2024
We hit another case where people are pulling a container image
with devices in `/dev` in the tar stream; they're then trying
to commit this to an ostree.

There's much better ways to fix this:

- Change the image to stop including devices as there's no reason
  to do so
- Switch to logically bound images instead of physically bound
- Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but
it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work
for "podman run" of such images; podman will just use overlayfs
to stitch together the `diff` directories, and doesn't try to do
any validation of their contents today.
(Queue the composefs integration, which *would* do that but would
 also fix this anwyays)

Signed-off-by: Colin Walters <[email protected]>
cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Oct 14, 2024
We hit another case where people are pulling a container image
with devices in `/dev` in the tar stream; they're then trying
to commit this to an ostree.

There's much better ways to fix this:

- Change the image to stop including devices as there's no reason
  to do so
- Switch to logically bound images instead of physically bound
- Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but
it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work
for "podman run" of such images; podman will just use overlayfs
to stitch together the `diff` directories, and doesn't try to do
any validation of their contents today.
(Queue the composefs integration, which *would* do that but would
 also fix this anwyays)

Signed-off-by: Colin Walters <[email protected]>
cgwalters added a commit to cgwalters/rpm-ostree that referenced this issue Oct 14, 2024
We hit another case where people are pulling a container image
with devices in `/dev` in the tar stream; they're then trying
to commit this to an ostree.

There's much better ways to fix this:

- Change the image to stop including devices as there's no reason
  to do so
- Switch to logically bound images instead of physically bound
- Use the composefs backend for c/storage

Eventually I may look at "quoting" generally in ostree, but
it's fairly invasive: ostreedev/ostree#2568

In practice today, simply ignoring the files will happen to work
for "podman run" of such images; podman will just use overlayfs
to stitch together the `diff` directories, and doesn't try to do
any validation of their contents today.
(Queue the composefs integration, which *would* do that but would
 also fix this anwyays)

Signed-off-by: Colin Walters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants