From 0b641f00994f9cf037aa23e052d4114dc2d2c52d Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Mon, 24 Aug 2026 18:01:34 -0700 Subject: [PATCH] mount: honor a claimed transform as a chain suffix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A claimed transform was recorded but never actually left unapplied: every parsed transformer in a mount's chain was applied in full regardless of what the caller had claimed, so a shim advertising it performs "mkdir" in "format/mkdir/overlay" never got the chance to, since the manager silently applied it anyway. This predates the shim capability protocol — it was reachable before through the "format/*" annotation convention — but this series turns it into a documented, supported way for a shim to claim a transform, so a shim relying on it (at least one is: Kata's EROFS snapshotter shim, per its annotation usage) would find its claim silently ignored. A claimed transform can only be honored as a suffix of the chain: each transform's input is the previous one's output, so an outer, unclaimed transform must still be applied even when an inner one is claimed, since the inner one cannot run without it. planActivation now tracks, per mount, how much of its chain — starting from the beginning — the manager must apply itself: through the last unclaimed transform, which by construction covers every transform before it too. Applying only that prefix and returning the true, contiguous claimed suffix to the caller is what actually makes the claim meaningful. This is only observable for mounts[firstSystemMount], the one mount partially transformed by the manager and returned to the caller to finish; every earlier mount is fully internal to the manager and was, and remains, always fully transformed regardless of any claim. While changing this expression, also guard it against the case where firstSystemMount reaches len(mounts) — every mount consumed inside the manager, with no system mount left to transform — which indexes out of range today. This is a distinct, pre-existing latent panic (for example, reachable via WithTemporary combined with a transformed mount type), tracked separately from the claim-handling fix above; guarding it here only prevents a crash newly reachable by touching this line, it does not address the mount.ActivationInfo.System non-empty invariant that scenario also violates. Signed-off-by: Derek McGowan --- core/mount/manager/manager.go | 14 +++++-- core/mount/manager/plan.go | 37 ++++++++++++++++++- core/mount/manager/plan_test.go | 65 ++++++++++++++++++++++++++++++--- 3 files changed, 107 insertions(+), 9 deletions(-) diff --git a/core/mount/manager/manager.go b/core/mount/manager/manager.go index 97e67b4d22..bf826d7b4a 100644 --- a/core/mount/manager/manager.go +++ b/core/mount/manager/manager.go @@ -408,9 +408,17 @@ func (mm *mountManager) Activate(ctx context.Context, name string, mounts []moun mounted = append(mounted, active) } - // If first system mount is converted, fill in the format - if mountConv != nil { - for _, tr := range mountConv[firstSystemMount] { + // If the first system mount has transforms, apply the ones the caller + // has not claimed. A claim can only be honored as a suffix of the + // chain: applyCount always covers at least every transform up to and + // including the last unclaimed one, since each depends on the last's + // output; see planActivation. + // + // firstSystemMount can reach len(mounts) when every mount is handled + // inside the manager (for example, every mount claimed via Temporary); + // there is then no system mount left to transform. + if mountConv != nil && firstSystemMount < len(mounts) { + for _, tr := range mountConv[firstSystemMount][:plan.applyCount[firstSystemMount]] { newM, err := tr.Transform(ctx, mounts[firstSystemMount], mounted) if err != nil { return mount.ActivationInfo{}, err diff --git a/core/mount/manager/plan.go b/core/mount/manager/plan.go index ae71712129..498cc0ae30 100644 --- a/core/mount/manager/plan.go +++ b/core/mount/manager/plan.go @@ -42,6 +42,18 @@ type activationPlan struct { // prefix. transforms [][]mount.Transformer + // applyCount[i] is how many of transforms[i], from the start, the + // manager must apply itself: through the last transform in the chain the + // caller did not claim. Meaningful only for mounts[firstSystemMount], + // the one mount returned to the caller with some of its own transforms + // possibly still unapplied; every earlier mount's transforms are always + // applied in full; they are internal to the manager and never exposed + // to the caller. A transform, once applied, resolves the input the next + // one in the chain depends on, so a run of claimed transforms can only + // be honored as a suffix: transforms strictly after the last unclaimed + // one in the chain. + applyCount []int + // handlers[i] is the Handler to use for mounts[i], or nil to perform a // plain system mount. handlers []mount.Handler @@ -51,6 +63,16 @@ type activationPlan struct { // must apply and whether the manager or the caller performs the mount // itself, honoring the mount types and transforms config.AllowMountTypes and // config.AllowTransforms claim. +// +// A claimed transform is honored only for mounts[firstSystemMount] in the +// result, and only as a suffix of its transform chain: transforms apply +// outside-in, so an inner transform's input depends on an outer, unclaimed +// transform's output, and the manager must still apply that outer transform +// even though the caller performs the suffix itself. For example, in +// "format/mkdir/overlay", claiming "mkdir" causes the manager to apply +// "format" and hand the caller "mkdir/overlay" to finish; claiming "format" +// alone does nothing, since "mkdir" cannot run without it having already +// run. func planActivation(ctx context.Context, mounts []mount.Mount, config mount.ActivateOptions, transforms map[string]mount.Transformer, handlers map[string]mount.Handler) activationPlan { shouldTransform := func(p string, t string) bool { return !slices.Contains(config.AllowTransforms, p) && !slices.Contains(config.AllowMountTypes, t) @@ -73,7 +95,10 @@ func planActivation(ctx context.Context, mounts []mount.Mount, config mount.Acti break } - if shouldTransform(transformType, mounts[i].Type) { //nolint:gosec // G602: i is bounded by range mounts + // mustApply reports whether the manager itself must apply this + // transform, as opposed to the caller having claimed it. + mustApply := shouldTransform(transformType, mounts[i].Type) //nolint:gosec // G602: i is bounded by range mounts + if mustApply { // At least everything before this must be mounted // by the mount manager plan.firstSystemMount = i @@ -85,11 +110,21 @@ func planActivation(ctx context.Context, mounts []mount.Mount, config mount.Acti if plan.transforms == nil { plan.transforms = make([][]mount.Transformer, len(mounts)) } + if plan.applyCount == nil { + plan.applyCount = make([]int, len(mounts)) + } plan.transforms[i] = append(plan.transforms[i], typeTransformer{ Transformer: tr, mountType: mt, }) + if mustApply { + // Every transform up to and including this one must be + // applied by the manager: this one because it is unclaimed, + // and every one before it because this one's input depends + // on their output. + plan.applyCount[i] = len(plan.transforms[i]) + } mountType = mt } diff --git a/core/mount/manager/plan_test.go b/core/mount/manager/plan_test.go index 097df2867f..aa9bf1e22b 100644 --- a/core/mount/manager/plan_test.go +++ b/core/mount/manager/plan_test.go @@ -65,30 +65,47 @@ func TestPlanActivationTransformChain(t *testing.T) { name string opts []mount.ActivateOpt expectFirstSystem int + expectApplyCount int }{ { - name: "nothing claimed requires the manager", + // Nothing claimed: the manager must apply the whole chain, the + // same as it always has. + name: "nothing claimed applies the whole chain", expectFirstSystem: 0, + expectApplyCount: 2, }, { - name: "a claimed transform still requires the manager for now", + // Claiming the innermost transform lets the manager stop after + // the one before it: mkdir is left for the caller. + name: "innermost transform claimed is honored", opts: []mount.ActivateOpt{mount.WithAllowTransform("mkdir")}, expectFirstSystem: 0, + expectApplyCount: 1, }, { - name: "every transform claimed no longer requires the manager", + // Claiming the outermost transform alone cannot be honored: + // mkdir's input depends on format having already run. + name: "outermost transform claimed alone is not honored", + opts: []mount.ActivateOpt{mount.WithAllowTransform("format")}, + expectFirstSystem: 0, + expectApplyCount: 2, + }, + { + name: "both transforms claimed applies neither", opts: []mount.ActivateOpt{ mount.WithAllowTransform("format"), mount.WithAllowTransform("mkdir"), }, expectFirstSystem: -1, + expectApplyCount: 0, }, { // Claiming the full literal mount type is equivalent to // claiming every transform in its chain. - name: "full mount type claimed no longer requires the manager", + name: "full mount type claimed applies neither", opts: []mount.ActivateOpt{mount.WithAllowMountType("format/mkdir/overlay")}, expectFirstSystem: -1, + expectApplyCount: 0, }, } { t.Run(tc.name, func(t *testing.T) { @@ -99,11 +116,33 @@ func TestPlanActivationTransformChain(t *testing.T) { plan := planActivation(context.Background(), mounts, config, testTransforms, nil) assert.Equal(t, tc.expectFirstSystem, plan.firstSystemMount) - require.Len(t, plan.transforms[0], 2) + if tc.expectFirstSystem >= 0 { + require.NotNil(t, plan.applyCount) + assert.Equal(t, tc.expectApplyCount, plan.applyCount[0]) + require.Len(t, plan.transforms[0], 2) + } }) } } +// TestPlanActivationClaimedGapInMiddle covers a three transform chain where +// an unclaimed transform sits between two claimed ones: the middle one still +// forces the manager to apply everything up to and including it, regardless +// of the outer claim, and only the true suffix after it is left claimed. +func TestPlanActivationClaimedGapInMiddle(t *testing.T) { + mounts := []mount.Mount{{Type: "format/mkfs/mkdir/overlay"}} + config := mount.ActivateOptions{} + mount.WithAllowTransform("format")(&config) + mount.WithAllowTransform("mkdir")(&config) + // mkfs is left unclaimed. + + plan := planActivation(context.Background(), mounts, config, testTransforms, nil) + require.Equal(t, 0, plan.firstSystemMount) + require.Len(t, plan.transforms[0], 3) + // format(0) and mkfs(1) must be applied; mkdir(2) is left to the caller. + assert.Equal(t, 2, plan.applyCount[0]) +} + func TestPlanActivationUnknownTransform(t *testing.T) { mounts := []mount.Mount{{Type: "unknown/overlay"}} plan := planActivation(context.Background(), mounts, mount.ActivateOptions{}, testTransforms, nil) @@ -173,3 +212,19 @@ func TestPlanActivationTemporary(t *testing.T) { require.Len(t, plan.handlers, 1) assert.Equal(t, 1, plan.firstSystemMount) } + +func TestPlanActivationTemporaryIgnoresTransformClaims(t *testing.T) { + mounts := []mount.Mount{{Type: "format/mkdir/overlay"}} + config := mount.ActivateOptions{} + mount.WithTemporary(&config) + mount.WithAllowTransform("format")(&config) + mount.WithAllowTransform("mkdir")(&config) + + plan := planActivation(context.Background(), mounts, config, testTransforms, nil) + // Temporary forces this, the only mount, to be handled, advancing + // firstSystemMount past it. It does not change how much of a transform + // chain is required, which is governed solely by what is claimed: both + // transforms here are claimed, so applyCount stays at 0. + assert.Equal(t, 1, plan.firstSystemMount) + assert.Equal(t, 0, plan.applyCount[0]) +}