pkg/machine: apply ro mount option to virtiofs and 9p shares by AdamMagued · Pull Request #29898 · podman-container-tools/podman

AdamMagued · 2026-10-05T01:58:59Z

When mounting volumes in virtual machines (virtiofs on macOS/Linux or 9p on Windows Hyper-V), volume configurations containing the :ro flag were parsed into vmconfigs.Mount with ReadOnly: true, but the read-only flag was omitted when configuring mounts in the guest. This caused guest VMs to mount the shares read-write.

This change:

Adds ro to virtiofs systemd mount options in GenerateSystemDFilesForVirtiofsMounts when mnt.ReadOnly is true.
Passes --read-only to client9p for Windows Hyper-V 9p shares, adding ,ro to kernel 9p mount options.
Preserves OriginalInput across volume mappings.
Adds unit and machine e2e tests covering read-only virtiofs and 9p volume configurations.

Fixes: #29897

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

I have read and understood our contributing guidelines and will not have more than two open PRs as a new contributor.
PR description, commit message, and GitHub comments are human-written, per LLM Policy
Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
the sign-off email address. See CONTRIBUTING.md
for more information.
Referenced issues using Fixes: #00000 in commit message (if applicable)
Tests have been added/updated (or no tests are needed)
Documentation has been updated (or no documentation changes are needed)
All commits pass make validatepr (format/lint checks)
Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

Fix an issue where machine volumes configured as read-only (:ro) were mounted read-write in guest virtual machines for virtiofs and 9p shares.

Honny1

Very light review. Can you please also add an e2e test and properly sign the commit? Othervise change code looks good.

Luap99

This will need proper machine e2e tests. Also the same fix must be added to the 9p mount code on windows as readonly does not seem to be set there either.

Then as noted on the issue ideally the read only option should be enforced on the hypervisor side not the potential untrusted VM, though I see no option being exposed there today in vfkit/krunkit either so I guess it is not possible.

Luap99 · 2026-10-05T10:50:08Z

 		case vmconfigs.VirtIOFS:
 			virtioMount := machine.NewVirtIoFsMount(source, target, readOnly)
 			mount = virtioMount.ToMount()
+			mount.OriginalInput = volume


what is the point of setting this? It is never read anywhere so I think the logical thing is to drop that.

Luap99 · 2026-10-05T10:51:05Z

-		mountUnit.Add("Mount", "Options", fmt.Sprintf("context=\"%s\"", NFSSELinuxContext))
+		mountOptions := fmt.Sprintf("context=\"%s\"", NFSSELinuxContext)
+		if mnt.ReadOnly {
+			mountOptions = fmt.Sprintf("ro,context=\"%s\"", NFSSELinuxContext)


This is duplicating the line above with the nfs mount option, the branch should only append "ro" to the string

Luap99 · 2026-10-05T10:54:02Z

+	tests := []struct {
+		name     string
+		vfs      VirtIoFs
+		readOnly bool


having an extra readOnly bool makes no sense here and just duplicates the VirtIoFs.ReadOnly field which is the one being tested/used

Luap99 · 2026-10-05T10:54:40Z

+	tests := []struct {
+		name     string
+		mount    *vmconfigs.Mount
+		readOnly bool


same here, the duplicated readonly field makes no sense here as Mount.ReadOnly is being used

Luap99 · 2026-10-05T11:03:38Z

see https://github.com/podman-container-tools/community/blob/main/CONTRIBUTING.md#dco-sign-off for a proper DCO sign off

Luap99 · 2026-10-05T11:04:00Z

And in general please follow the PR template in the future

AdamMagued · 2026-10-05T16:01:27Z

@Luap99 @Honny1

I updated the PR with the following changes:

Windows 9p read-only mount support:

Added --read-only and --ro flags to podman machine client9p. When set, ,ro is appended to the kernel 9p mount options (trans=fd,rfdno=3,wfdno=3,version=9p2000.L,ro).
Updated Hyper-V startShares to forward --read-only to client9p when mount.ReadOnly is true.
Added unit tests in pkg/machine/hyperv/volumes_test.go and cmd/podman/machine/client9p_test.go.

Machine e2e tests:

Added podman volume read-only mount to pkg/machine/e2e/basic_test.go, verifying file reads succeed, in-guest writes fail with Read-only file system, and findmnt options contain ro.
Added machine init with read-only volume test to pkg/machine/e2e/init_test.go.

Commit sign-off and PR metadata:

Amended the commit with DCO sign-off (Signed-off-by: Adam Magued <adamismailmageud@gmail.com>) matching author identity and Fixes: #29897.
Updated the PR title and description to follow the Podman PR template.

packit-as-a-service · 2026-10-05T16:45:19Z

[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore.

packit-as-a-service · 2026-10-05T16:45:19Z

[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore.

Honny1 · 2026-10-06T11:19:56Z

 		Expect(sshSession.outputToString()).To(ContainSubstring("example"))
 	})

+	It("machine init with read-only volume", func() {


I think this overlaps with podman volume read-only mount test. I think it should be dropped to reduce CI time.

Agree, we should only test this once, machine tests are quite expensive

baude · 2026-10-06T13:16:59Z

@mheon prob want your eyes on this too given the history.

mheon · 2026-10-06T14:49:25Z

+
+	flags := client9pCommand.Flags()
+	flags.BoolVar(&client9pReadOnly, "read-only", false, "mount directory read-only")
+	flags.BoolVar(&client9pReadOnly, "ro", false, "mount directory read-only")


Is the duplication necessary? This command should never be called by hand

yes just use one flag name

Luap99

Also please fix the linter errors as pointed out in the CI failure.

Luap99 · 2026-10-06T16:58:26Z

 		Expect(ls.outputToString()).To(ContainSubstring(testString))
 	})

+	It("podman volume read-only mount", func() {


in addition to dropping the other please inline that test into podman volume on non-standard path by adding a second mount path for the ro mount as part of the same machine.

That way we only need to boot one VM and keep the test time about the same, in CI a sign le boot cycle can take up to 60s so we really need to avoid a lot of unnecessary starts

AdamMagued · 2026-10-07T05:04:41Z

@Luap99 I formatted pkg/machine/volume_systemd_test.go with gofumpt and verified the check passes.

Honny1 · 2026-10-07T09:47:07Z

@AdamMagued Please check CI failure: https://productionresultssa17.blob.core.windows.net/actions-results/ca5b1db6-0d54-80e2-93b8-e6b2302b8897/workflow-job-run-1f9eb6b8-0b61-5b69-97da-2fb8638033ca/logs/job/job-logs.txt?rsct=text%2Fplain&se=2026-10-07T09%3A56%3A12Z&sig=7gDDnE6GhogQhyVtDRe4rmr2jMKyoO7ugNe%2B1Sgd8Lw%3D&ske=2026-10-07T11%3A32%3A50Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2026-10-07T07%3A32%3A50Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2025-11-05&sp=r&spr=https&sr=b&st=2026-10-07T09%3A46%3A07Z&sv=2025-11-05

I think it is related.

AdamMagued · 2026-10-07T14:38:00Z

@Honny1 Tracked down the failure. In Windows Hyper-V CI, the guest VM runs the pre-installed system podman binary from the base image. That binary lacks the new --read-only flag on podman machine client9p. Because client9p expects two positional arguments and had no flags defined in older releases, Cobra rejects --read-only with exit status 125 (Error: unknown flag: --read-only).

To maintain backwards compatibility with existing guest VM images, host startShares no longer passes --read-only to guest client9p. Instead, after client9p establishes the 9p mount, host podman executes sudo mount -o remount,ro <target> inside the guest. This flips the mount to read-only in the Linux kernel regardless of the podman version installed in the guest image.

I updated pkg/machine/hyperv/volumes.go and pkg/machine/hyperv/volumes_test.go, verified cross-compilation and unit tests, and pushed the amended commit.

AdamMagued · 2026-10-07T18:02:25Z

@Honny1 Following up on the Hyper-V CI failure in basic_test.go and init_test.go (mount: /var/mnt/ro-test: mount point is busy):

The Linux 9p filesystem driver does not implement superblock remount (reconfigure_super), causing mount -o remount,ro to return EBUSY.

Passing the bind option via mount -o remount,ro,bind directs the kernel VFS do_remount path into do_reconfigure_mnt. This reconfigures the VFS mount flags directly (setting MNT_READONLY) without invoking driver-level superblock reconfigure. This cleanly enforces read-only semantics on guest 9p mounts across all kernel versions.

I updated startShareMountArgs and its unit tests in pkg/machine/hyperv to use remount,ro,bind, verified tests and lint, and force-pushed the branch.

Luap99 · 2026-10-08T13:43:58Z

@AdamMagued You have not addressed any of the review comments we have given

To maintain backwards compatibility with existing guest VM images, host startShares no longer passes --read-only to guest client9p. Instead, after client9p establishes the 9p mount, host podman executes sudo mount -o remount,ro inside the guest. This flips the mount to read-only in the Linux kernel regardless of the podman version installed in the guest image.

A remount seems unnecessary complicated, backwards compat is certainly important but could we instead of using a cli option use an env variable for now? We can always pass the env as the old client binary will ignore it and the new one can use it just fine.

When mounting volumes in virtual machines (virtiofs on macOS/Linux or 9p on Windows Hyper-V), volume configurations containing the :ro flag were parsed into vmconfigs.Mount with ReadOnly: true, but the read-only option was omitted from guest mount configurations. As a result, the guest mounted the shares read-write. Translate the ReadOnly flag into the ro mount option in GenerateSystemDFilesForVirtiofsMounts, support CONTAINERS_MACHINE_9P_READ_ONLY in client9p and pass it from Windows Hyper-V startShares for backwards compatibility with existing guest VM images, and add unit and machine e2e tests. Fixes: podman-container-tools#29897 Signed-off-by: Adam Magued <adamismailmageud@gmail.com>

AdamMagued · 2026-10-08T21:32:28Z

@Luap99 @mheon @Honny1 I have addressed all review feedback across the PR:

Hyper-V 9p Read-Only Mount & Guest Compatibility (pkg/machine/hyperv/volumes.go, cmd/podman/machine/client9p.go):
- Replaced the previous mount -o remount,ro,bind post-mount approach and the CLI flags on client9p with the environment variable CONTAINERS_MACHINE_9P_READ_ONLY=1.
- Host startShareMountArgs now passes CONTAINERS_MACHINE_9P_READ_ONLY=1 to the guest podman machine client9p invocation.
- Guest client9p checks CONTAINERS_MACHINE_9P_READ_ONLY (with fallback to PODMAN_9P_READ_ONLY) and appends ,ro to the 9p mount options when truthy.
- Older Podman binaries pre-installed in existing guest VM images ignore the environment variable without throwing flag errors, maintaining complete backwards compatibility.
- Removed CLI flag definitions (--read-only and --ro) from client9pCommand.
Systemd Mount Unit Generation (pkg/machine/volume_systemd.go):
- Eliminated duplicated NFS context option string formatting in GenerateSystemDFilesForVirtiofsMounts; read-only mode now appends ,ro to mountOptions.
VirtIOFS Shim Input Field (pkg/machine/shim/volume.go):
- Dropped setting mount.OriginalInput = volume in CmdLineVolumesToMounts for the vmconfigs.VirtIOFS case.
Unit Test Deduplication (pkg/machine/volumes_test.go, pkg/machine/volume_systemd_test.go, pkg/machine/hyperv/volumes_test.go):
- Removed duplicated readOnly boolean fields from test table structs in pkg/machine/volumes_test.go, asserting directly against VirtIoFs.ReadOnly and Mount.ReadOnly.
- Updated Hyper-V volume unit tests to verify CONTAINERS_MACHINE_9P_READ_ONLY=1 emission.
CI Optimization and e2e Deduplication (pkg/machine/e2e/init_test.go, pkg/machine/e2e/basic_test.go):
- Dropped the redundant machine init with read-only volume test in pkg/machine/e2e/init_test.go.
- Inlined read-only volume assertions into podman volume on non-standard path in pkg/machine/e2e/basic_test.go by adding a second mount target with :ro to the same machine instance, testing both read access and write denial without booting an additional VM.

All unit tests and go vet have been verified in the container sandbox. The commit has been amended with DCO sign-off and pushed to the branch.

Honny1 · 2026-10-12T15:39:04Z

he CI failure seems related and unusual. Also, please review our LLM policy. Avoid adding AI-generated summaries to your comments so we can keep the PR thread clean.

Honny1

This failure seems to be related:

D:/a/podman/podman/pkg/machine/e2e/basic_test.go:27
  podman volume on non-standard path
  D:/a/podman/podman/pkg/machine/e2e/basic_test.go:271
  > Enter [BeforeEach] TOP-LEVEL - D:/a/podman/podman/pkg/machine/e2e/machine_test.go:218 @ 10/12/26 09:02:41.985
  < Exit [BeforeEach] TOP-LEVEL - D:/a/podman/podman/pkg/machine/e2e/machine_test.go:218 @ 10/12/26 09:02:41.986 (1ms)
  > Enter [It] podman volume on non-standard path - D:/a/podman/podman/pkg/machine/e2e/basic_test.go:271 @ 10/12/26 09:02:41.986
  D:\a\podman\podman\bin\windows\podman.exe machine init --disk-size 11 --image C:\Users\RUNNER~1\AppData\Local\Temp\podman-machine.x86_64.hyperv.vhdx --volume C:\Users\RUNNER~1\AppData\Local\Temp\machine-volume2578504670:/does/not/exist --volume C:\Users\RUNNER~1\AppData\Local\Temp\machine-volume-ro2177619695:/mnt/ro-test:ro --now 47fb387c762c
  Machine init complete
  Starting machine "47fb387c762c"

  This machine is currently configured in rootless mode. If your containers
  require root permissions (e.g. ports < 1024), or if you run into compatibility
  issues with non-podman clients, you can switch using the following command:

  	podman machine set --rootful 47fb387c762c

  API forwarding listening on: npipe:////./pipe/podman-47fb387c762c

  Another process was listening on the default Docker API socket address.
  You can still connect Docker API clients by setting DOCKER_HOST using the
  following command in your terminal session:

          $Env:DOCKER_HOST="npipe:////./pipe/podman-47fb387c762c"

  Machine "47fb387c762c" started successfully
  D:\a\podman\podman\bin\windows\podman.exe machine ssh 47fb387c762c cat /does/not/exist/testfile
  abcdefg1234567D:\a\podman\podman\bin\windows\podman.exe machine ssh 47fb387c762c cat /mnt/ro-test/ro-testfile
  readonly-volume-contentD:\a\podman\podman\bin\windows\podman.exe machine ssh 47fb387c762c touch /mnt/ro-test/fail-write
  [FAILED] Expected
      <int>: 0
  not to match exit code:
      <int>: 0
  In [It] at: D:/a/podman/podman/pkg/machine/e2e/basic_test.go:319 @ 10/12/26 09:03:32.508

  Full Stack Trace
    go.podman.io/podman/v6/pkg/machine/e2e_test.init.func6.5()
    	D:/a/podman/podman/pkg/machine/e2e/basic_test.go:319 +0xea3
  < Exit [It] podman volume on non-standard path - D:/a/podman/podman/pkg/machine/e2e/basic_test.go:271 @ 10/12/26 09:03:32.508 (50.522s)

…s true Signed-off-by: Adam Magued <adamismailmageud@gmail.com>

AdamMagued · 2026-10-20T07:01:40Z

@Honny1 @Luap99 The root cause of the Hyper-V CI failure was that the Windows CI runner uses a pre-built guest VM image containing an existing released Podman binary. That guest binary mounts 9p shares without ,ro (and sudo drops non-whitelisted env vars like CONTAINERS_MACHINE_9P_READ_ONLY).

To ensure read-only enforcement works reliably across both existing VM images and new builds, I updated startShareMountArgs in pkg/machine/hyperv/volumes.go to chain sudo mount -o remount,ro <target> immediately after client9p. Linux VFS enforces the read-only remount regardless of whether the guest binary supports the flag natively. Unit tests have also been updated to cover the remount arguments.

Honny1 · 2026-10-26T10:01:19Z

+		args = append(args, "sudo", "chattr", "-i", "/", "; ")
+	}
+	args = append(args, "sudo", "mkdir", "-p", strconv.Quote(cleanTarget), "; ")
+	if requiresChattr {
+		args = append(args, "sudo", "chattr", "+i", "/", "; ")


I would join all commands with && instead of ;.

Honny1 · 2026-10-26T10:02:15Z

PTAL @Luap99 @l0rd @mheon

Luap99

thanks we are almost there now

Luap99 · 2026-10-26T11:51:49Z

+	if !readOnly {
+		readOnly, _ = strconv.ParseBool(os.Getenv("PODMAN_9P_READ_ONLY"))
+	}
+	if err := client9p(cmd.Context(), uint32(port), args[1], readOnly); err != nil {


this should not use two different env vars, there is no need for this at all
since this is podman specific just use PODMAN_9P_READ_ONLY

Luap99 · 2026-10-26T11:53:51Z

+	if mount.ReadOnly {
+		args = append(args, "; ", "sudo", "mount", "-o", "remount,ro", strconv.Quote(cleanTarget))
+	}


please add a comment here that the remount should be removed in the future when the client inside is new enough to do this by default

github-actions Bot added the machine label Oct 5, 2026

Honny1 reviewed Oct 5, 2026

View reviewed changes

Luap99 requested changes Oct 5, 2026

View reviewed changes

AdamMagued force-pushed the fix-issue-29897 branch from cbc12cc to 822c43b Compare October 5, 2026 16:00

AdamMagued changed the title ~~pkg/machine: apply ro mount option to virtiofs shares~~ pkg/machine: apply ro mount option to virtiofs and 9p shares Oct 5, 2026

AdamMagued force-pushed the fix-issue-29897 branch from 822c43b to 15e9ed3 Compare October 6, 2026 10:18

Honny1 reviewed Oct 6, 2026

View reviewed changes

mheon reviewed Oct 6, 2026

View reviewed changes

Luap99 requested changes Oct 6, 2026

View reviewed changes

AdamMagued force-pushed the fix-issue-29897 branch from 15e9ed3 to 10203cb Compare October 7, 2026 05:02

AdamMagued force-pushed the fix-issue-29897 branch from 10203cb to c51bf6c Compare October 7, 2026 14:37

AdamMagued force-pushed the fix-issue-29897 branch from c51bf6c to bca7a7d Compare October 7, 2026 18:02

Honny1 requested review from Luap99 and mheon October 8, 2026 12:47

AdamMagued force-pushed the fix-issue-29897 branch from bca7a7d to e0c73b2 Compare October 8, 2026 21:32

Honny1 requested changes Oct 19, 2026

View reviewed changes

pkg/machine/hyperv: remount 9p shares read-only when mount.ReadOnly i…

8050a04

…s true Signed-off-by: Adam Magued <adamismailmageud@gmail.com>

Honny1 reviewed Oct 26, 2026

View reviewed changes

Luap99 requested changes Oct 26, 2026

View reviewed changes

Conversation

AdamMagued commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Does this PR introduce a user-facing change?

Uh oh!

Honny1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luap99 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!

Luap99 commented Oct 5, 2026

Uh oh!

Luap99 commented Oct 5, 2026

Uh oh!

AdamMagued commented Oct 5, 2026

Uh oh!

packit-as-a-service Bot commented Oct 5, 2026

Uh oh!

packit-as-a-service Bot commented Oct 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Oct 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamMagued commented Oct 7, 2026

Uh oh!

Honny1 commented Oct 7, 2026

Uh oh!

AdamMagued commented Oct 7, 2026

Uh oh!

AdamMagued commented Oct 7, 2026

Uh oh!

Luap99 commented Oct 8, 2026

Uh oh!

AdamMagued commented Oct 8, 2026

Uh oh!

Honny1 commented Oct 12, 2026

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

AdamMagued commented Oct 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Honny1 commented Oct 26, 2026

Uh oh!

Luap99 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!

Reviewers

Uh oh!

Assignees

Labels

AdamMagued commented Oct 5, 2026 •

edited

Loading

Honny1 left a comment •

edited

Loading