Use virtiofs to support bind-mounts #81
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements bind mount support for containers running inside VMs by using virtiofs filesystem sharing. The implementation transforms host bind mounts into virtiofs shares that are mounted inside the VM and then bind-mounted into containers.
Key changes include:
- Replace the previous file-based bind mount mechanism with virtiofs-based sharing
- Add support for both directory and file bind mounts (files share their parent directory)
- Include comprehensive documentation about security implications of file bind mounts
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/shim/task/service.go | Replaces transformBindMounts function with new bindMounter type that uses virtiofs |
| internal/shim/task/mount.go | Implements bindMounter with methods to transform mounts, setup VM filesystems, and generate init args |
| internal/shim/task/mount_test.go | Adds comprehensive test coverage for bind mount transformation logic |
| cmd/vminitd/main.go | Adds mount flag and integrates mount setup into VM initialization |
| cmd/vminitd/bind_mounts.go | Implements virtiofs mount logic inside the VM |
| cmd/vminitd/bind_mounts_test.go | Tests bind mount parsing and string conversion |
| docs/bind-mounts.md | Documents how bind mounts work and security implications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/shim/task/mount_test.go
Outdated
| }, | ||
| } | ||
|
|
||
| p := &bindMounter{} |
There was a problem hiding this comment.
The variable name p is ambiguous in this context. Consider renaming it to bm or mounter to better reflect that it's a bindMounter instance.
akerouanton
force-pushed
the
virtiofs-bind-mounts
branch
from
c21b02c to
2c2e18a
Compare
Transform bind mounts received by the shim into virtiofs mounts, and update the OCI spec to let the VM-side shim mount from these virtiofs. File bind-mounts are supported by mounting their parent directory into the VM. A docs file is added to explain how it works, and the security implications of file bind-mounts. Signed-off-by: Albin Kerouanton <albin.kerouanton@docker.com>
akerouanton
force-pushed
the
virtiofs-bind-mounts
branch
from
2c2e18a to
2db71d6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| hash := sha256.Sum256([]byte(m.Destination)) | ||
| tag := fmt.Sprintf("bind-%x", hash[:8]) |
There was a problem hiding this comment.
Using only 8 bytes of the SHA256 hash for the tag creates potential for hash collisions when multiple bind mounts are used. Consider using the full hash or a longer prefix to ensure uniqueness, especially in scenarios with many bind mounts.
| tag := fmt.Sprintf("bind-%x", hash[:8]) | |
| tag := fmt.Sprintf("bind-%x", hash) |
There was a problem hiding this comment.
Does this need consistent hashing or could a random element be injected?
| "target": bm.target, | ||
| }).Info("mounting virtiofs filesystem") | ||
|
|
||
| if err := os.MkdirAll(bm.target, 0700); err != nil { |
There was a problem hiding this comment.
The directory is created with 0700 permissions which restricts access to the owner only. Consider if this permission level is appropriate for all use cases, as containers may need broader access depending on their user configuration.
| if err := os.MkdirAll(bm.target, 0700); err != nil { | |
| if err := os.MkdirAll(bm.target, 0755); err != nil { |
|
just one suggestion, maybe we could introduce a unique sharefs virtiofs mount and map these all host bind mounts as sub-directories, is it possible? because many virtiofses also take overheads too. |
| specSrc := vmTarget | ||
| if !fi.IsDir() { | ||
| hostSrc = filepath.Dir(m.Source) | ||
| specSrc = filepath.Join(vmTarget, filepath.Base(m.Source)) |
There was a problem hiding this comment.
maybe considering use temporary bind mount in a new empty temporary dir in the host for this? is that practical for libkrun?
3 participants
Transform bind mounts received by the shim into virtiofs mounts, and update the OCI spec to let the VM-side shim mount from these virtiofs. File bind-mounts are supported by mounting their parent directory into the VM.
A docs file is added to explain how it works, and the security implications of file bind-mounts.