virtiofs: support read-only mounts#623
Conversation
Introduce a new public API, krun_add_virtiofs3(), that extends krun_add_virtiofs2() with an additional read_only flag. When set, the virtio-fs device exposes the host directory as a read-only filesystem to the guest. The implementation adds a PassthroughFsRo wrapper around PassthroughFs that: - Delegates all read-only FUSE operations (lookup, getattr, read, readdir, etc.) to the inner PassthroughFs - Rejects all mutating operations (write, create, mkdir, unlink, rename, setattr, setxattr, etc.) with EROFS - Blocks O_WRONLY/O_RDWR opens and writable DAX mappings - Strips WRITEBACK_CACHE from init options to prevent the guest kernel from buffering writes - Reports ST_RDONLY in statfs so userspace tools see the mount as read-only The wrapper is designed to fail closed: unoverridden FileSystem trait methods fall back to the trait defaults (ENOSYS), so new operations are implicitly blocked until explicitly handled. The existing krun_add_virtiofs() and krun_add_virtiofs2() APIs are refactored to delegate to krun_add_virtiofs3() with read_only=false, reducing code duplication. Null pointer checks are also added to the shared implementation. The FsWorker now uses an FsServer enum to dispatch to either the read-write PassthroughFs or the read-only PassthroughFsRo, and its constructor returns Result to properly propagate filesystem initialization errors instead of panicking. Fixes: containers#343 Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
Open read-only DAX mappings with O_RDONLY so read-only shares do not require host write access on macOS. Allow harmless O_APPEND on read-only opens and cover the flag handling with unit tests. Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
JAORMX
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
JAORMX
force-pushed
the
virtiofs-read-only
branch
from
0c0c04e to
d0209ec
Compare
|
Tested RW implementation and found no regressions. Code also LGTM. Let's see Gemini's opinion. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for read-only virtio-fs devices by adding a new API function, krun_add_virtiofs3, and implementing a PassthroughFsRo wrapper that rejects mutating FUSE operations with EROFS. The changes span the libkrun API, the VMM configuration, and the virtio-fs device implementation, including a refactor of the worker to handle both read-write and read-only servers. Review feedback suggests improving error handling during device activation by replacing an unwrap() with proper error propagation and deduplicating a constant used for root directory removal requests.
| self.shm_region.clone(), | ||
| self.passthrough_cfg.clone(), | ||
| self.read_only, | ||
| self.worker_stopfd.try_clone().unwrap(), |
There was a problem hiding this comment.
Since you are already refactoring this block to return an ActivateError instead of panicking when worker creation fails, you should also handle the potential error from try_clone() on the worker_stopfd. Using unwrap() here still leaves a panic point during device activation.
| self.worker_stopfd.try_clone().unwrap(), | |
| self.worker_stopfd.try_clone().map_err(|e| { | |
| error!("virtio_fs: failed to clone stop_fd: {}", e); | |
| ActivateError::BadActivate | |
| })?, |
|
|
||
| // Keep the Linux ioctl number so read-only virtio-fs can still handle | ||
| // non-mutating control ioctls while rejecting host-side root deletion. | ||
| const VIRTIO_IOC_REMOVE_ROOT_DIR_REQ: u32 = 0x7603; |
There was a problem hiding this comment.
The constant VIRTIO_IOC_REMOVE_ROOT_DIR_REQ is duplicated here and in src/devices/src/virtio/fs/macos/passthrough.rs. It should be moved to a shared location, such as the defs module in src/devices/src/virtio/fs/mod.rs, to ensure consistency and improve maintainability.
References
- Prioritize code readability and focused commits over micro-optimizations. Refactoring for minor efficiency gains may be rejected if it harms readability or expands the scope of a change.
|
Thanks for the PR! Let's get it merged. |
2 participants
This PR adds a read-only virtio-fs mode to the public API and wires the new flag through the virtio-fs device configuration, worker setup, and VMM builder so callers can expose shared directories without allowing guest writes.
The implementation wraps the passthrough backend in a read-only filesystem layer that rejects mutating FUSE operations with
EROFS, disables writeback cache, and still preserves the non-mutating control ioctls the guest uses. On macOS,setupmappingnow opens the backing file withO_RDONLYfor read-only mappings, which fixes read-only DAX mounts.I also changed virtio-fs worker creation to return an activation error instead of panicking if the backend cannot be initialized.