feat(shim): forward ro bind-mount option to virtio-fs share by ndeloof · Pull Request #204 · containerd/nerdbox

ndeloof · 2026-05-27T19:51:04Z

Summary

Parse the ro OCI mount option in bindMounter.FromBundle and carry it on bindMount, mirroring what the ext4 branch in the same file already does.
Forward the flag through sandbox.WithFS to vmInstance.AddFS, which now applies vm.MountOpts before calling into libkrun.
Bind the optional krun_add_virtiofs3 symbol (libkrun >= 1.18) via a small registerOptionalLibFunc helper and a ,optional struct-tag flag; route AddVirtiofs through it when present so the share is marked read-only at the host edge.
Fall back to krun_add_virtiofs with a single warn log when the v3 symbol is not exported; guest-side ro enforcement via OCI mount options is unchanged.
Cover the new behavior with unit tests for parseCTag and the AddVirtiofs v3-vs-legacy matrix (prefers v3, falls back when v3 is nil, surfaces non-zero return codes on both paths, errors when neither symbol is bound).

Behavior

libkrun version	Effect of this change
`< 1.18` (no `krun_add_virtiofs3` exported, e.g. the v1.17.4 currently pinned)	No-op on the host side. The fallback path runs and logs a single warn for read-only shares; the guest continues to enforce `ro` through the OCI mount options exactly as today.
`>= 1.18` (`krun_add_virtiofs3` exported)	The virtio-fs share is marked read-only at the host edge, so the VMM rejects writes in addition to the guest bind mount. This is defense-in-depth and lets the VMM treat the share as immutable for any future caching optimizations.

Test plan

go build ./...
go test ./internal/shim/task/... ./internal/vm/libkrun/...

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds virtio-fs read-only propagation end-to-end by wiring OCI ro mount intent into libkrun’s newer krun_add_virtiofs3 API (with a safe fallback when the symbol is missing), and extends tests to validate both symbol binding and mount option flow.

Changes:

Add optional libkrun symbol binding support via C:"...,optional" tags and registerOptionalLibFunc.
Update virtio-fs plumbing to pass a readonly flag (shim → sandbox opts → vm instance → libkrun), preferring krun_add_virtiofs3 when available.
Add unit tests for tag parsing and virtio-fs routing/fallback/error propagation, plus mount flow assertions.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/vm/libkrun/krun_test.go	Adds unit tests for `parseCTag` and virtio-fs v3/legacy routing behavior.
internal/vm/libkrun/krun.go	Introduces optional symbol parsing/binding and updates `AddVirtiofs` to support read-only + logging.
internal/vm/libkrun/instance.go	Propagates mount options into the new `AddVirtiofs(ctx, ..., readonly)` signature.
internal/vm/libkrun/dlfcn_windows.go	Adds non-panicking optional symbol registration for Windows.
internal/vm/libkrun/dlfcn_unix.go	Adds non-panicking optional symbol registration for Unix.
internal/shim/task/mount_test.go	Extends tests to assert `sandbox.Filesystem.Readonly` is set correctly from OCI mounts.
internal/shim/task/mount.go	Tracks `ro` option on bind mounts and passes it into sandbox filesystem opts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ndeloof · 2026-05-28T06:09:04Z

+	for _, c := range cases {
+		t.Run(c.in, func(t *testing.T) {
+			name, opt := parseCTag(c.in)
+			if name != c.wantName || opt != c.wantOpt {
+				t.Fatalf("parseCTag(%q) = (%q, %v), want (%q, %v)", c.in, name, opt, c.wantName, c.wantOpt)
+			}
+		})
+	}


Fixed in cb20f8b — switched the table to named subtests (empty, plain, etc.) so t.Run no longer receives an empty string for the "" case.

ndeloof · 2026-05-28T06:09:07Z

+		readOnly := false
+		for _, opt := range m.Options {
+			if opt == "ro" {
+				readOnly = true
+				break
+			}
+		}


Fixed in cb20f8b — the loop now scans every option and lets later ro/rw override earlier ones (no break), matching standard mount-option semantics. Added two regression tests (bind mount ro then rw and bind mount rw then ro) in mount_test.go to lock the behavior down.

The ext4 mount path in mount.go already parses `ro` from the OCI mount options; the bind-mount path silently dropped it. Plumb the flag through bindMounter and vmInstance.AddFS so the virtio-fs share itself is marked read-only at the host edge via krun_add_virtiofs3 (libkrun >= 1.18), giving us defense-in-depth on top of the guest-side `ro` bind mount and opening the door to VMM-side optimizations on immutable shares. When the loaded libkrun is older and does not export krun_add_virtiofs3, the binding falls back to krun_add_virtiofs with an info log and enforcement stays on the guest mount options exactly as today. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof · 2026-05-28T06:12:31Z

Downgraded the fallback log from warn to info: missing krun_add_virtiofs3 isn't a blocker — guest-side enforcement still applies. New head: 2f60004.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

+// AddVirtiofs attaches a virtio-fs share to the VM. When readonly is true
+// it prefers krun_add_virtiofs3 (libkrun >= 1.18) which natively marks the
+// share read-only at the host edge. If that symbol is not exported by the
+// loaded libkrun, it falls back to krun_add_virtiofs and logs an info
+// message: in that case the read-only enforcement relies on the guest
+// applying the `ro` mount option, which the OCI spec already carries.
+func (vmc *vmcontext) AddVirtiofs(ctx context.Context, tag, path string, readonly bool) error {
+	if vmc.lib.AddVirtiofs3 != nil {
+		ret := vmc.lib.AddVirtiofs3(vmc.ctxID, tag, path, 0, readonly)


+	if readonly {
+		log.G(ctx).WithField("tag", tag).Info(
+			"libkrun does not export krun_add_virtiofs3; " +
+				"read-only virtiofs enforcement deferred to the guest mount options",
+		)
+	}


+		tag := ik.Type().Field(i).Tag.Get("C")
+		cName, optional := parseCTag(tag)
 		fn := ik.Field(i).Addr().Interface()
+		if optional {
+			// registerOptionalLibFunc reports whether the symbol was
+			// resolved; if not, the field is left nil and call sites
+			// must guard against it.
+			registerOptionalLibFunc(fn, f, cName)
+			continue
+		}


+	if err != nil || addr == 0 {
+		return false
+	}
+	purego.RegisterFunc(fn, uintptr(addr))


ndeloof · 2026-05-28T06:15:22Z

Branch was renamed to drop the nicolasdeloof/ prefix, which auto-closed this PR (GitHub does not propagate fork-branch renames to cross-repo PRs). Recreated as #205 with the same commit (2f60004) on branch ndeloof:virtiofs-ro-host-edge. Please continue review there.

Copilot AI review requested due to automatic review settings May 27, 2026 19:51

Copilot AI reviewed May 27, 2026

View reviewed changes

ndeloof force-pushed the nicolasdeloof/virtiofs-ro-host-edge branch from 00b167a to cb20f8b Compare May 28, 2026 06:08

Copilot AI review requested due to automatic review settings May 28, 2026 06:12

ndeloof force-pushed the nicolasdeloof/virtiofs-ro-host-edge branch from cb20f8b to 2f60004 Compare May 28, 2026 06:12

ndeloof closed this May 28, 2026

ndeloof deleted the nicolasdeloof/virtiofs-ro-host-edge branch May 28, 2026 06:14

Copilot AI reviewed May 28, 2026

View reviewed changes

ndeloof mentioned this pull request May 28, 2026

feat(shim): forward ro bind-mount option to virtio-fs share #205

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(shim): forward ro bind-mount option to virtio-fs share#204

feat(shim): forward ro bind-mount option to virtio-fs share#204
ndeloof wants to merge 1 commit into
containerd:mainfrom
ndeloof:nicolasdeloof/virtiofs-ro-host-edge

ndeloof commented May 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

ndeloof May 28, 2026

Uh oh!

ndeloof May 28, 2026

Uh oh!

ndeloof commented May 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

ndeloof commented May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ndeloof commented May 27, 2026

Summary

Behavior

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

ndeloof May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ndeloof May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ndeloof commented May 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ndeloof commented May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants