blockdev: Fix loopback device resource leak on signal interruption #1402

gursewak1997 · 2025-07-10T21:49:04Z

This commit implements issue #799 by creating a signal-safe cleanup helper for loopback devices to prevent resource leaks when bootc install --via-loopback is interrupted by signals like SIGINT (Ctrl-C).

The solution uses an 'out-of-process drop' helper that:

Forks a cleanup helper process when creating a loopback device
Uses PR_SET_PDEATHSIG to detect when the parent process dies
Masks most signals to avoid being killed accidentally
Automatically cleans up leaked loopback devices if the parent dies
Gracefully terminates when the parent performs normal cleanup

This prevents the common issue where interrupting bootc install --via-loopback with Ctrl-C would leave /dev/loopN devices allocated on the system.

Fixes: #799

gemini-code-assist

Code Review

This pull request introduces a signal-safe cleanup mechanism for loopback devices to prevent resource leaks on signal interruption. It uses an out-of-process helper to clean up leaked loopback devices. I've added comments to enhance error logging for better debugging.

blockdev/src/blockdev.rs

cgwalters · 2025-07-11T13:04:06Z

Thanks for working on this! While it will be a bit more awkward can you try doing it this way #799 (comment) - that should 100% avoid all the unsafe code.

Basically instead of a raw fork() (source of basically all the unsafe in general) we fork+exec our own binary /proc/self/exe. Look at e.g. reexec_with_guardenv.

cgwalters

Thanks! Looking closer

cgwalters · 2025-07-11T18:01:53Z

blockdev/src/blockdev.rs

+        anyhow::bail!("This function should only be called as a cleanup helper");
+    }
+
+    // Close stdin, stdout, stderr and redirect to /dev/null


This is better done in the parent process setup above

cgwalters · 2025-07-11T18:02:29Z

blockdev/src/blockdev.rs

+            .context("Failed to read /proc/self/exe")?;
+
+        // Create the helper process using exec
+        let mut cmd = Command::new(self_exe);


Set up std{in,out,err} here via https://doc.rust-lang.org/std/process/struct.Stdio.html#method.null

cgwalters · 2025-07-11T18:03:17Z

blockdev/src/blockdev.rs

+    }
+
+    // Set up death signal notification - we want to be notified when parent dies
+    unsafe {


There's a safe version of this in https://docs.rs/rustix/latest/rustix/process/fn.set_parent_process_death_signal.html

cgwalters · 2025-07-11T18:04:57Z

blockdev/src/blockdev.rs

+
+    // Set up death signal notification - we want to be notified when parent dies
+    unsafe {
+        if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGUSR1) != 0 {


It seems cleaner to me to use SIGTERM and react to that

cgwalters · 2025-07-11T18:05:26Z

blockdev/src/blockdev.rs

+        }
+    }
+
+    // Mask most signals to avoid being killed accidentally


So https://docs.rs/tokio/latest/tokio/signal/index.html is one way to handle this in a safe way (will require making the function async)

I think the tokio API will replace about 50 lines of unsafe code with 5 lines of safe code.

cgwalters · 2025-07-11T18:07:25Z

blockdev/src/blockdev.rs

+
+            match status {
+                Ok(exit_status) if exit_status.success() => {
+                    // Write to stderr since we closed stdout


Mmm my vote here is probably to not inherit stderr at all; if we write to stderr then we have the possibility to intermix the child process writes with the parent's.

One option is to explicitly log to the systemd journal.

I guess speaking of systemd...a whole possibility I hadn't considered until just now is that we fork off via systemd-run. That would have some nice advantages but will be trickier to get right the lifecycle binding, so let's leave that for the future.

(There's this whole giant topic in bootc overall defaulting to running via systemd in some cases, which would similarly have a lot of advantages but be a big nontrivial change)

cgwalters · 2025-07-11T18:08:25Z

blockdev/src/blockdev.rs

-            20961247
-        );
-        Ok(())
+        let data = fs::read_to_string("tests/fixtures/lsblk.json").unwrap();


This change looks unrelated? I mean it's probably fine to do but let's break it into a separate commit

This still needs fixing, can you move this section of the change to a different PR?

cgwalters

Thanks, looking a lot closer!

cgwalters · 2025-07-16T00:56:27Z

lib/src/cli.rs

+        device: String,
+        /// Parent process ID to monitor
+        #[clap(long)]
+        parent_pid: u32,


This isn't necessary, with PDEATHSIG we automatically monitor the parent. It can be dropped.

cgwalters · 2025-07-16T00:57:41Z

blockdev/src/blockdev.rs

+            "loopback-cleanup-helper",
+            "--device",
+            device_path,
+            "--parent-pid",


This can be removed

cgwalters · 2025-07-16T01:00:00Z

blockdev/src/blockdev.rs

+/// Handle to manage the cleanup helper process for loopback devices
+struct LoopbackCleanupHandle {
+    /// Process ID of the cleanup helper
+    helper_pid: u32,


This should actually hold the Child instance. And on Drop we should send it SIGTERM - note not kill but rustix kill with SIGTERM.

cgwalters · 2025-07-16T01:00:56Z

blockdev/src/blockdev.rs

+
+        // Try to spawn cleanup helper process - if it fails, continue without it
+        let cleanup_handle = Self::spawn_cleanup_helper(dev.as_str())
+            .map_err(|e| {


I'm uncertain about ignoring errors here by default. It seems better to make it fatal.

cgwalters · 2025-07-16T01:01:50Z

blockdev/src/blockdev.rs

+        // Kill the cleanup helper since we're cleaning up normally
+        if let Some(cleanup_handle) = self.cleanup_handle.take() {
+            // Kill the helper process since we're doing normal cleanup
+            let _ = std::process::Command::new("kill")


See above, we can directly send a signal instead of invoking an external command.

Also, crucially by holding a reference to the Child we avoid pid reuse problems if the child exits early.

cgwalters · 2025-07-16T01:02:16Z

blockdev/src/blockdev.rs

-            20961247
-        );
-        Ok(())
+        let data = fs::read_to_string("tests/fixtures/lsblk.json").unwrap();


This still needs fixing, can you move this section of the change to a different PR?

Add fork+exec based cleanup helper to prevent loopback device leaks when bootc install --via-loopback is interrupted by signals like SIGINT. - Add loopback-cleanup-helper CLI subcommand - Implement run_loopback_cleanup_helper() with PR_SET_PDEATHSIG - Update LoopbackDevice to spawn cleanup helper process - Add tests for spawn mechanism

gemini-code-assist bot reviewed Jul 10, 2025

View reviewed changes

blockdev/src/blockdev.rs Outdated Show resolved Hide resolved

blockdev/src/blockdev.rs Outdated Show resolved Hide resolved

blockdev/src/blockdev.rs Outdated Show resolved Hide resolved

blockdev/src/blockdev.rs Outdated Show resolved Hide resolved

gursewak1997 force-pushed the bootc-799 branch 6 times, most recently from 70c2b77 to a4ab303 Compare July 11, 2025 05:59

gursewak1997 force-pushed the bootc-799 branch from a4ab303 to d45a8f2 Compare July 11, 2025 16:46

cgwalters requested changes Jul 11, 2025

View reviewed changes

gursewak1997 force-pushed the bootc-799 branch from d45a8f2 to 754d6ff Compare July 15, 2025 19:17

cgwalters requested changes Jul 16, 2025

View reviewed changes

gursewak1997 force-pushed the bootc-799 branch from 754d6ff to 507ad56 Compare July 18, 2025 04:54

gursewak1997 force-pushed the bootc-799 branch from 507ad56 to f0de48a Compare July 18, 2025 04:58

blockdev: Fix loopback device resource leak on signal interruption #1402

Are you sure you want to change the base?

blockdev: Fix loopback device resource leak on signal interruption #1402

Uh oh!

Conversation

gursewak1997 commented Jul 10, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cgwalters commented Jul 11, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!