refactor: split up dmsetup into type-safe utility

.github/scripts/provision-kvm-host.sh

@@ -34,8 +34,14 @@ sudo udevadm trigger --name-match=kvm
 
 sudo apt-get update
 sudo apt-get install -y \
-  coreutils e2fsprogs iproute2 libc-bin lvm2 nftables skopeo thin-provisioning-tools util-linux \
-  "linux-modules-extra-$(uname -r)"
+  coreutils e2fsprogs iproute2 libc-bin lvm2 nftables skopeo thin-provisioning-tools util-linux
+
+modules_extra="linux-modules-extra-$(uname -r)"
+if apt-cache show "${modules_extra}" >/dev/null 2>&1; then
+  sudo apt-get install -y "${modules_extra}"
+else
+  echo "${modules_extra} is not available; relying on the kernel's base modules"
+fi
 
 # -a is load-bearing: without it modprobe reads the 2nd+ names as module
 # PARAMETERS of the first, and the load fails.

.github/workflows/ci.yml

@@ -528,7 +528,7 @@ jobs:
             -o suidhelper.profdata
           "$TOOLS/llvm-cov" export --format=lcov \
             --instr-profile=suidhelper.profdata \
-            target/release/hyper-suidhelper \
+            target/hyper-suidhelper \
             "$GITHUB_WORKSPACE/native/suidhelper/src" \
             > lcov-e2e.info
           sed -i "s|^SF:$GITHUB_WORKSPACE/|SF:|" lcov-e2e.info

AGENTS.md

@@ -160,6 +160,9 @@ If a case needs its own assertions, it is a separate test, not a row.
 - **Tests live in `tests/`, never inline `#[cfg(test)]` in `src/`.** The crate
   is split into a lib (`src/lib.rs`, crate `hyper_suidhelper`) and a thin bin
   (`src/main.rs`) precisely so integration tests can reach the internals.
+  **Exception: `src/linux/`.** Tests for these utilities are colocated with
+  the code, in a `#[cfg(test)] mod tests` block at the bottom of the file
+  under test.
 - Each test file must be registered as a `[[test]]` target in `Cargo.toml`
   (`tests/` subdirectories are not auto-discovered) — copy an existing
   `[[test]]` block when adding one.

docs/cookbook/config.md

@@ -51,7 +51,6 @@ configurable:
 | `jailer`      | -             | `.jailer`      | -                                   | [Safe Path](#safe-path) |
 | `dmsetup`     | -             | `.dmsetup`     | `"/usr/sbin/dmsetup"`               | [Safe Path](#safe-path) |
 | `losetup`     | -             | `.losetup`     | `"/usr/sbin/losetup"`               | [Safe Path](#safe-path) |
-| `blockdev`    | -             | `.blockdev`    | `"/usr/sbin/blockdev"`              | [Safe Path](#safe-path) |
 | `thin_dump`   | -             | `.thin_dump`   | `"/usr/sbin/thin_dump"`             | [Safe Path](#safe-path). Reads thin snapshot metadata for fork publish; required for `Hyper.Vm.fork/1`'s cross-node path (same-node `fast_fork/1` is unaffected). |
 | `mke2fs`      | `.mke2fs`     | `.mke2fs`      | `$PATH["mke2fs"]`                   |  |
 | `skopeo`      | `.skopeo`     | `.skopeo`      | `$PATH["skopeo"]`                   |  |
@@ -80,7 +79,6 @@ firecracker = "/opt/firecracker/firecracker"
 jailer      = "/opt/firecracker/jailer"
 dmsetup     = "/usr/sbin/dmsetup"
 losetup     = "/usr/sbin/losetup"
-blockdev    = "/usr/sbin/blockdev"
 thin_dump   = "/usr/sbin/thin_dump"
 
 # Node tools -- may also be set in config.exs, which wins when both are set.

docs/cookbook/install.md

@@ -155,7 +155,7 @@ work_dir = "/srv/hyper"
 # Paths to every external binary hyper uses. All paths must be absolute.
 #
 # The privileged binaries the setuid helper runs (firecracker, jailer, dmsetup,
-# losetup, blockdev) must be root-owned and not group/world writable -- the
+# losetup) must be root-owned and not group/world writable -- the
 # helper refuses them otherwise. The node-run tools (skopeo, umoci, mke2fs) have
 # no such requirement.
 [tools]
@@ -168,7 +168,6 @@ jailer = "/opt/firecracker/jailer"
 # optional -- privileged device tools, default to /usr/sbin/<name>.
 # dmsetup  = "/usr/sbin/dmsetup"
 # losetup  = "/usr/sbin/losetup"
-# blockdev = "/usr/sbin/blockdev"
 
 # optional -- node-run tools. skopeo/mke2fs default to the name on PATH; omit
 # umoci to let hyper download and cache a pinned release.

lib/hyper/cfg/tools.ex

@@ -2,10 +2,9 @@ defmodule Hyper.Cfg.Tools do
   @moduledoc """
   Paths to every external binary Hyper runs, read from the `[tools]` table.
 
-  The privileged tools (`firecracker`, `jailer`, `dmsetup`, `losetup`,
-  `blockdev`) are read **only** from `/etc/hyper/config.toml` — the file the
-  setuid helper also parses, so node and helper can never disagree on a
-  root-impacting path. The node-only tools (`skopeo`, `mke2fs`, `umoci`,
+  The privileged tools (`firecracker`, `jailer`, `dmsetup`, `losetup`) are read
+  **only** from `/etc/hyper/config.toml` — the file the setuid helper also
+  parses, so node and helper can never disagree on a root-impacting path. The node-only tools (`skopeo`, `mke2fs`, `umoci`,
   `suidhelper`) run unprivileged, so `/etc/hyper/config.exs` may override them
   (e.g. a path from a secrets manager), then `config.toml`, then the default.
   """

lib/hyper/node/img/publish.ex

@@ -85,21 +85,18 @@ defmodule Hyper.Node.Img.Publish do
   defp write_delta(tmp, ro_dev, {snap_dev, snap_id}, sectors, parent_img_id) do
     cow_path = Path.join(Hyper.Cfg.Dirs.scratch_dir(), "fork-delta-#{tmp}.img")
 
-    with :ok <- create_sparse(cow_path, cow_size(sectors)),
-         {:ok, cow_loop} <- SuidHelper.Losetup.attach_rw(cow_path) do
+    with :ok <- create_sparse(cow_path, cow_size(sectors)) do
       try do
         with {:ok, write_dev} <-
-               SuidHelper.Dmsetup.create_snapshot_rw(cow_name(tmp), ro_dev, cow_loop, sectors),
+               SuidHelper.Dmsetup.create_snapshot_rw(cow_name(tmp), ro_dev, cow_path, sectors),
              {:ok, _stats} <- copy_divergence(snap_dev, snap_id, write_dev),
              # Removing the snapshot device flushes every exception to the store;
              # only then is the COW file complete on disk.
-             :ok <- SuidHelper.Dmsetup.remove(cow_name(tmp)),
-             :ok <- SuidHelper.Losetup.detach(cow_loop) do
+             :ok <- SuidHelper.Dmsetup.remove(cow_name(tmp)) do
           Hyper.Img.create_derived(parent_img_id, cow_path, label: "fork of #{parent_img_id}")
         else
           {:error, _} = err ->
             _ = SuidHelper.Dmsetup.remove(cow_name(tmp))
-            _ = SuidHelper.Losetup.detach(cow_loop)
             err
         end
       after

lib/hyper/node/img/server.ex

@@ -22,6 +22,7 @@ defmodule Hyper.Node.Img.Server do
   alias Hyper.Img.Db
   alias Hyper.Node.Layer
   alias Hyper.SuidHelper
+  alias Unit.Information
 
   use OpenTelemetryDecorator
 
@@ -81,8 +82,8 @@ defmodule Hyper.Node.Img.Server do
     Process.flag(:trap_exit, true)
     layer_ids = resolve_layers(img_id)
 
-    with {:ok, loop_paths} <- acquire_layers(layer_ids),
-         {:ok, %{blk_path: blk_path, dm_names: dm_names}} <- build_chain(img_id, loop_paths) do
+    with {:ok, layer_paths} <- acquire_layers(layer_ids),
+         {:ok, %{blk_path: blk_path, dm_names: dm_names}} <- build_chain(img_id, layer_paths) do
       state = %State{img_id: img_id, layers: layer_ids, dm_names: dm_names, blk_path: blk_path}
       {:ok, arm_idle(state)}
     else
@@ -171,7 +172,7 @@ defmodule Hyper.Node.Img.Server do
     |> Enum.reduce_while({:ok, []}, fn layer_id, {:ok, paths} ->
       with {:ok, pid} <- Layer.Server.for_layer(layer_id),
            :ok <- Layer.Server.acquire(pid) do
-        {:cont, {:ok, [Layer.Server.blk_path(pid) | paths]}}
+        {:cont, {:ok, [Layer.Server.path(pid) | paths]}}
       else
         {:error, reason} -> {:halt, {:error, reason}}
       end
@@ -193,29 +194,55 @@ defmodule Hyper.Node.Img.Server do
   defp build_chain(_img_id, []), do: {:error, :unknown_image}
 
   defp build_chain(img_id, [base | deltas]) do
-    with {:ok, sectors} <- SuidHelper.Blockdev.device_sectors(base) do
-      deltas
-      |> Enum.with_index(1)
-      |> Enum.reduce_while({:ok, base, []}, fn {cow_dev, idx}, {:ok, origin, names} ->
-        name = dm_name(img_id, idx)
-
-        case SuidHelper.Dmsetup.create_snapshot(name, origin, cow_dev, sectors) do
-          {:ok, dev} ->
-            {:cont, {:ok, dev, [name | names]}}
-
-          {:error, reason} ->
-            Enum.each(names, &remove_quietly/1)
-            {:halt, {:error, reason}}
-        end
-      end)
-      |> case do
-        {:ok, blk_path, names} -> {:ok, %{blk_path: blk_path, dm_names: Enum.reverse(names)}}
-        {:error, _} = err -> err
+    with {:ok, sectors} <- file_sectors(base) do
+      case deltas do
+        [] -> build_linear(img_id, base, sectors)
+        deltas -> build_snapshots(img_id, base, deltas, sectors)
       end
     end
   end
 
-  @spec dm_name(Hyper.Img.id(), pos_integer()) :: String.t()
+  @spec build_linear(Hyper.Img.id(), Path.t(), pos_integer()) ::
+          {:ok, %{blk_path: Path.t(), dm_names: [String.t()]}} | {:error, term()}
+  defp build_linear(img_id, base, sectors) do
+    name = dm_name(img_id, 0)
+
+    with {:ok, dev} <- SuidHelper.Dmsetup.create_linear(name, base, sectors) do
+      {:ok, %{blk_path: dev, dm_names: [name]}}
+    end
+  end
+
+  @spec build_snapshots(Hyper.Img.id(), Path.t(), [Path.t()], pos_integer()) ::
+          {:ok, %{blk_path: Path.t(), dm_names: [String.t()]}} | {:error, term()}
+  defp build_snapshots(img_id, base, deltas, sectors) do
+    deltas
+    |> Enum.with_index(1)
+    |> Enum.reduce_while({:ok, base, []}, fn {cow_file, idx}, {:ok, origin, names} ->
+      name = dm_name(img_id, idx)
+
+      case SuidHelper.Dmsetup.create_snapshot(name, origin, cow_file, sectors) do
+        {:ok, dev} ->
+          {:cont, {:ok, dev, [name | names]}}
+
+        {:error, reason} ->
+          Enum.each(names, &remove_quietly/1)
+          {:halt, {:error, reason}}
+      end
+    end)
+    |> case do
+      {:ok, blk_path, names} -> {:ok, %{blk_path: blk_path, dm_names: Enum.reverse(names)}}
+      {:error, _} = err -> err
+    end
+  end
+
+  @spec file_sectors(Path.t()) :: {:ok, non_neg_integer()} | {:error, File.posix()}
+  defp file_sectors(path) do
+    with {:ok, %File.Stat{size: size}} <- File.stat(path) do
+      {:ok, size |> Information.bytes() |> Information.as_sectors()}
+    end
+  end
+
+  @spec dm_name(Hyper.Img.id(), non_neg_integer()) :: String.t()
   defp dm_name(img_id, idx), do: "hyper-img-#{sanitize(img_id)}-#{idx}"
 
   @spec sanitize(String.t()) :: String.t()

lib/hyper/node/img/thin_pool.ex

@@ -25,8 +25,8 @@ defmodule Hyper.Node.Img.ThinPool do
 
   defmodule State do
     @moduledoc false
-    @enforce_keys [:pool_dev, :meta_loop, :data_loop]
-    defstruct [:pool_dev, :meta_loop, :data_loop, next: 0, freed: []]
+    @enforce_keys [:pool_dev, :meta_loop]
+    defstruct [:pool_dev, :meta_loop, next: 0, freed: []]
   end
 
   @spec start_link(keyword()) :: GenServer.on_start()
@@ -94,19 +94,17 @@ defmodule Hyper.Node.Img.ThinPool do
          {:ok, data} <- ensure_backing(@data_file, ImgConfig.thin_pool_data_size()),
          :ok <- reclaim_stale_pool(),
          :ok <- zero_metadata(meta),
-         {:ok, meta_loop} <- SuidHelper.Losetup.attach_rw(meta),
-         {:ok, data_loop} <- SuidHelper.Losetup.attach_rw(data),
          sectors = Information.as_sectors(ImgConfig.thin_pool_data_size()),
-         {:ok, pool_dev} <-
+         {:ok, pool_dev, meta_loop} <-
            SuidHelper.Dmsetup.create_thin_pool(
              @pool_name,
-             meta_loop,
-             data_loop,
+             meta,
+             data,
              sectors,
              ImgConfig.thin_block_sectors(),
              0
            ) do
-      {:ok, %State{pool_dev: pool_dev, meta_loop: meta_loop, data_loop: data_loop}}
+      {:ok, %State{pool_dev: pool_dev, meta_loop: meta_loop}}
     else
       {:error, reason} -> {:stop, reason}
     end
@@ -196,10 +194,8 @@ defmodule Hyper.Node.Img.ThinPool do
   def handle_info({:EXIT, _pid, reason}, state), do: {:stop, reason, state}
 
   @impl true
-  def terminate(_reason, state) do
+  def terminate(_reason, _state) do
     _ = SuidHelper.Dmsetup.remove(@pool_name)
-    _ = SuidHelper.Losetup.detach(state.data_loop)
-    _ = SuidHelper.Losetup.detach(state.meta_loop)
     :ok
   end
 

lib/hyper/node/layer/server.ex

@@ -14,22 +14,20 @@ defmodule Hyper.Node.Layer.Server do
   # it just tore down. See the reconciliation TODO in `Hyper.Node.Reaper` for why
   # coupling resource lifetime to process lifetime is a smell.
   use GenServer, restart: :temporary
-  require Logger
 
   alias Hyper.Node.Layer
   alias Hyper.Node.Layer.Repo
-  alias Hyper.SuidHelper
 
   defmodule State do
     @moduledoc false
 
     @type t :: %__MODULE__{
-            blk_path: Path.t(),
+            path: Path.t(),
             holders: %{pid() => reference()},
             idle_ref: reference() | nil
           }
 
-    defstruct [:blk_path, holders: %{}, idle_ref: nil]
+    defstruct [:path, holders: %{}, idle_ref: nil]
   end
 
   defmodule Opts do
@@ -62,9 +60,8 @@ defmodule Hyper.Node.Layer.Server do
   @spec release(GenServer.server()) :: :ok
   def release(server), do: GenServer.call(server, {:release, self()})
 
-  @doc "Get the block device path of the layer managed by `server`."
-  @spec blk_path(GenServer.server()) :: Path.t()
-  def blk_path(server), do: GenServer.call(server, :blk_path)
+  @spec path(GenServer.server()) :: Path.t()
+  def path(server), do: GenServer.call(server, :path)
 
   @doc "Create a new layer, mounting it as a block device."
   @spec start_link(Opts.t()) :: GenServer.on_start()
@@ -74,13 +71,9 @@ defmodule Hyper.Node.Layer.Server do
 
   @impl true
   def init(%Opts{layer_id: layer_id}) do
-    Process.flag(:trap_exit, true)
-
-    with {:ok, layer_path} <- Repo.find_layer(layer_id),
-         {:ok, blk_path} <- SuidHelper.Losetup.attach_ro(layer_path) do
+    case Repo.find_layer(layer_id) do
       # Start with no holders, so a mounted-but-unused layer reaps itself.
-      {:ok, arm_idle(%State{blk_path: blk_path})}
-    else
+      {:ok, path} -> {:ok, arm_idle(%State{path: path})}
       {:error, reason} -> {:stop, reason}
     end
   end
@@ -103,8 +96,8 @@ defmodule Hyper.Node.Layer.Server do
   end
 
   @impl true
-  def handle_call(:blk_path, _from, %State{blk_path: blk_path} = state) do
-    {:reply, blk_path, state}
+  def handle_call(:path, _from, %State{path: path} = state) do
+    {:reply, path, state}
   end
 
   @impl true
@@ -123,32 +116,6 @@ defmodule Hyper.Node.Layer.Server do
     {:noreply, state}
   end
 
-  @impl true
-  # Each privileged command runs through `System.cmd`, which links a transient
-  # port to this process and returns only once that command has finished. Because
-  # we trap exits (for `terminate/2` teardown), the now-defunct port's exit lands
-  # here afterwards -- stale by construction, whatever its reason -- so ignore it.
-  def handle_info({:EXIT, port, _reason}, state) when is_port(port), do: {:noreply, state}
-
-  @impl true
-  # No process is deliberately linked here beyond those transient command ports,
-  # so a linked *process* EXIT is a genuine fault: propagate its reason (so
-  # terminate/2 still runs teardown) rather than crash opaquely on an unmatched
-  # message.
-  def handle_info({:EXIT, _pid, reason}, state), do: {:stop, reason, state}
-
-  @impl true
-  def terminate(_reason, %State{blk_path: blk_path}) do
-    case SuidHelper.Losetup.detach(blk_path) do
-      :ok ->
-        :ok
-
-      {:error, {errc, out}} ->
-        Logger.error("Failed to unmount layer block device #{blk_path} (exit #{errc}): #{out}")
-        :ok
-    end
-  end
-
   # Remove `pid` from the holder set (no-op if it isn't one) and re-arm the idle
   # timer if that was the last holder.
   @spec drop(State.t(), pid()) :: State.t()