From a8424675d5dcc6e3746a275f8384aa2def27890a Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Mon, 21 Sep 2026 10:24:38 +0000 Subject: [PATCH] fix(layout): grow the filesystem on a thread of its own ephemeralMount unshares the mount namespace so the temporary mount it needs is not visible to the rest of the system. A mount namespace belongs to an OS thread, and Go gives no goroutine a thread of its own, so the unshare landed on whatever thread the caller happened to sit on and the runtime handed that thread to the next goroutine that asked for one. Any caller that mounts something afterwards, from another thread, is then invisible to code running on the stale one: the read-only image underneath shows through instead. In immucore the Grow persistent rootfs stage runs in the same process as the boot mount DAG, so the bind-mount step intermittently stopped seeing COS_PERSISTENT and /etc, and the node booted with a varying subset of the persistent binds missing (kairos-io/kairos#4837, #4743). Run the grow on a goroutine locked to its thread and never unlocked, so the runtime destroys the thread, and the namespace, when it returns. Two smaller fixes in the same path: only turn the mount tree private when the unshare actually succeeded, since in the shared namespace that breaks propagation for everything on the machine, and drop the finalizer, which would have run the unmount on an arbitrary thread in the wrong namespace. Signed-off-by: Ettore Di Giacinto --- pkg/plugins/layout_resizer.go | 49 ++++++-- pkg/plugins/layout_resizer_thread_test.go | 130 ++++++++++++++++++++++ 2 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 pkg/plugins/layout_resizer_thread_test.go diff --git a/pkg/plugins/layout_resizer.go b/pkg/plugins/layout_resizer.go index 5e51551a..136d672e 100644 --- a/pkg/plugins/layout_resizer.go +++ b/pkg/plugins/layout_resizer.go @@ -55,19 +55,48 @@ var DefaultGrowFsToMax GrowFsToMaxInterface = &RealGrowFsToMax{} // GrowFSToMax grows the filesystem on the given block device path // to the maximum available space in the partition. // fsType: "ext4" (works for ext3/ext2 via ext4 driver), "xfs", "btrfs". +// +// Growing needs the filesystem mounted, and that mount is unshared into a +// private mount namespace so it is not visible to the rest of the system. A +// mount namespace belongs to an OS thread, not to a goroutine, so this runs on +// a thread of its own. See runOnDedicatedThread. func (r *RealGrowFsToMax) GrowFSToMax(devicePath, fsType string) error { switch fsType { case Ext4, Ext3, Ext2: - return growExtFSToMax(devicePath) + return runOnDedicatedThread(func() error { return growExtFSToMax(devicePath) }) case Xfs: - return growXfsToMax(devicePath) + return runOnDedicatedThread(func() error { return growXfsToMax(devicePath) }) case Btrfs: - return growBtrfsToMax(devicePath) + return runOnDedicatedThread(func() error { return growBtrfsToMax(devicePath) }) default: return fmt.Errorf("unsupported fsType %q; expected ext4/xfs/btrfs", fsType) } } +// runOnDedicatedThread runs fn on an OS thread that no other goroutine can be +// scheduled onto, and waits for it. +// +// unshare(CLONE_NEWNS) changes the mount namespace of the calling thread. Go +// gives no goroutine a thread of its own, so without this an unshare deep +// inside a library call leaves a thread behind whose view of the mount tree is +// frozen at that moment, and the runtime then hands that thread to whatever +// goroutine asks next. Callers that mount something afterwards, from another +// thread, do not see it there: the read-only image underneath shows through +// instead, and which callers are hit varies from run to run. +// +// Locking the goroutine to its thread and never unlocking it makes the runtime +// destroy the thread when the goroutine returns, so the namespace dies with it. +func runOnDedicatedThread(fn func() error) error { + done := make(chan error, 1) + go func() { + // Deliberately not unlocked: an unlocked thread goes back into the + // pool carrying whatever namespace fn left on it. + runtime.LockOSThread() + done <- fn() + }() + return <-done +} + // GrowExtFSToMax grows an ext4/ext3/ext2 filesystem on the given block device path // to the maximum available space in the partition // fstype should generally be ext4 as it also deals with ext3/ext2. @@ -256,8 +285,14 @@ func ephemeralMount(dev, fstype string) (mountpoint string, cleanup cleanupFn, e // Try to isolate the mount (best-effort): private mount namespace on Linux. // If unshare fails (e.g., old kernels or lacking caps), we still proceed // because we immediately unmount afterwards. - _ = unix.Unshare(unix.CLONE_NEWNS) - _ = unix.Mount("", "/", "", unix.MS_REC|unix.MS_PRIVATE, "") + // + // Callers must already be on a dedicated thread, see runOnDedicatedThread: + // the unshare below applies to the thread, not to the goroutine. + if err := unix.Unshare(unix.CLONE_NEWNS); err == nil { + // Only inside the new namespace. In the shared one this turns every + // mount on the machine private and breaks propagation for everybody. + _ = unix.Mount("", "/", "", unix.MS_REC|unix.MS_PRIVATE, "") + } if err := unix.Mount(dev, mp, fstype, 0, ""); err != nil { _ = os.RemoveAll(mp) @@ -272,8 +307,8 @@ func ephemeralMount(dev, fstype string) (mountpoint string, cleanup cleanupFn, e _ = os.RemoveAll(mp) } - // Ensure cleanup on panic/GC too. - runtime.SetFinalizer(&mp, func(*string) { cleanup() }) + // No finalizer: it would run on an arbitrary thread, in the wrong mount + // namespace, and unmount nothing. Every caller defers cleanup already. return mp, cleanup, nil } diff --git a/pkg/plugins/layout_resizer_thread_test.go b/pkg/plugins/layout_resizer_thread_test.go new file mode 100644 index 00000000..8831bde0 --- /dev/null +++ b/pkg/plugins/layout_resizer_thread_test.go @@ -0,0 +1,130 @@ +package plugins + +import ( + "errors" + "os" + "runtime" + "sync" + "testing" + + "golang.org/x/sys/unix" +) + +// mountNamespace returns the mount namespace of the OS thread the calling +// goroutine runs on right now. +func mountNamespace(t *testing.T) string { + t.Helper() + ns, err := os.Readlink("/proc/thread-self/ns/mnt") + if err != nil { + t.Skipf("cannot read the thread mount namespace: %v", err) + } + return ns +} + +func TestRunOnDedicatedThreadUsesAnotherThread(t *testing.T) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + caller := unix.Gettid() + var inner int + if err := runOnDedicatedThread(func() error { + inner = unix.Gettid() + return nil + }); err != nil { + t.Fatalf("runOnDedicatedThread returned %v, want nil", err) + } + if inner == caller { + t.Fatalf("fn ran on the caller's thread %d; an unshare there would outlive the call", caller) + } +} + +func TestRunOnDedicatedThreadPropagatesError(t *testing.T) { + want := errors.New("boom") + if got := runOnDedicatedThread(func() error { return want }); !errors.Is(got, want) { + t.Fatalf("runOnDedicatedThread returned %v, want %v", got, want) + } +} + +// TestRunOnDedicatedThreadRetiresTheThread is the regression test for the bug +// this helper exists for. Growing a filesystem unshares the mount namespace, +// and before the fix it did so on whatever OS thread the goroutine happened to +// sit on, with no runtime.LockOSThread. The runtime then handed that thread to +// unrelated goroutines, which saw a stale mount tree: in immucore's boot DAG +// the bind-mount step stopped seeing the partitions the earlier steps had just +// mounted (kairos-io/kairos#4837). +// +// The unshare itself needs CAP_SYS_ADMIN, so the property under test here is +// the one that makes it safe and that any user can check: the thread fn ran on +// is retired, and no later goroutine is scheduled onto it. +func TestRunOnDedicatedThreadRetiresTheThread(t *testing.T) { + var used int + if err := runOnDedicatedThread(func() error { + used = unix.Gettid() + return nil + }); err != nil { + t.Fatalf("runOnDedicatedThread returned %v, want nil", err) + } + + // Ask for far more threads than the helper could have left behind, so a + // pooled thread would be picked up with near certainty. + const probes = 256 + var wg sync.WaitGroup + var mu sync.Mutex + start := make(chan struct{}) + hit := false + for i := 0; i < probes; i++ { + wg.Add(1) + go func() { + defer wg.Done() + runtime.LockOSThread() + defer runtime.UnlockOSThread() + <-start + if unix.Gettid() == used { + mu.Lock() + hit = true + mu.Unlock() + } + }() + } + close(start) + wg.Wait() + + if hit { + t.Fatalf("a later goroutine ran on thread %d, the one fn mutated", used) + } +} + +// TestRunOnDedicatedThreadContainsUnshare checks the real thing where the test +// runs with enough privilege: after fn unshares, the caller's mount namespace +// is untouched. +func TestRunOnDedicatedThreadContainsUnshare(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("unsharing a mount namespace needs CAP_SYS_ADMIN") + } + + runtime.LockOSThread() + defer runtime.UnlockOSThread() + before := mountNamespace(t) + + var inside string + if err := runOnDedicatedThread(func() error { + if err := unix.Unshare(unix.CLONE_NEWNS); err != nil { + return err + } + ns, err := os.Readlink("/proc/thread-self/ns/mnt") + if err != nil { + return err + } + inside = ns + return nil + }); err != nil { + t.Skipf("could not unshare in this environment: %v", err) + } + + if inside == before { + t.Fatalf("the unshare did not take effect, namespace stayed %s", inside) + } + if got := mountNamespace(t); got != before { + t.Fatalf("caller namespace changed from %s to %s", before, got) + } +}