cmd/create: stop bind-mounting the host /dev , fixing VirtualBox issues#1778
cmd/create: stop bind-mounting the host /dev , fixing VirtualBox issues#1778Knogle wants to merge 1 commit intocontainers:mainfrom
Conversation
Signed-off-by: Fabian Druschke <fdruschke@outlook.com>
Knogle
force-pushed
the
fix-no-host-dev-bind-mount
branch
from
42690e9 to
727292d
Compare
![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 changes how host devices are handled by removing the global bind-mount of /dev and instead projecting individual devices from /run/host/dev during container initialization. This approach excludes runtime-managed devices and symbolic links. Feedback suggests optimizing the device projection loop by utilizing the existing DirEntry information instead of re-stating files and considering the use of the unix.Mount syscall to improve performance when handling a large number of device nodes.
| fileInfo, err := os.Lstat(source) | ||
| if err != nil { | ||
| logrus.Debugf("%s: failed to lstat %s: %s", logPrefix, source, err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Calling os.Lstat here is redundant and inefficient because os.ReadDir already provides a DirEntry which contains the file type information. You can use entry.Type() and entry.IsDir() directly, or entry.Info() if you need the full FileInfo.
| fileInfo, err := os.Lstat(source) | |
| if err != nil { | |
| logrus.Debugf("%s: failed to lstat %s: %s", logPrefix, source, err) | |
| continue | |
| } | |
| fileInfo, err := entry.Info() | |
| if err != nil { | |
| logrus.Debugf("%s: failed to get info for %s: %s", logPrefix, source, err) | |
| continue | |
| } |
| if err := mountBind(containerPath, source, flags); err != nil { | ||
| logrus.Debugf("%s: failed to bind %s to %s: %s", logPrefix, containerPath, source, err) | ||
| } |
There was a problem hiding this comment.
Executing the mount command in a loop for every entry in /dev can be quite slow due to the overhead of process creation (fork/exec). Since init-container already imports golang.org/x/sys/unix, consider using the unix.Mount syscall directly for better performance, especially when dealing with a large number of device nodes.
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 12s |
1 participant
Toolbx currently bind-mounts the host
/devwholesale into the container.That breaks container startup on hosts where the user has group-only access to device
subtrees under
/dev, for example VirtualBox's/dev/vboxusb/*owned byroot:vboxuserswith0750permissions. In that situation the OCI runtime ends upwalking the host
/devduring container startup and fails before the Toolbx containercan be entered.
Instead of trying to preserve host supplementary groups inside the container, this
change takes a different approach:
/devinto the container duringtoolbox create/devinit-container, project host devices from/run/host/devinto/devon a best-effort basis
null,zero,urandom,tty,ptmx,pts,shm,stdin,stdout, andstderrThis keeps the startup path away from the problematic host
/devbind mount whilepreserving the existing Toolbx user model inside the container, including the current
wheel/sudobehaviour.Why this approach
The previous
keep-groupsdirection fixes the startup failure, but it also changesthe container's user/group semantics and breaks existing expectations around
wheelandsudo.This patch is intentionally narrower:
/devsubtrees"This is a startup fix, not a full host-device-access policy.
Behavioural impact
With this change:
/devwholesale/dev/vboxusbexists andthe user is in
vboxuserswheelmembership andsudoinside the container continue to workThis does not automatically grant usable access to VirtualBox USB device nodes
inside the container as an unprivileged user. The goal here is to prevent Toolbx from
failing to start in the first place.
Tests
Manual checks on a host with:
/dev/vboxusbpresent/dev/vboxusb/*owned byroot:vboxuserswith0750vboxusersVerified that:
toolbox createsucceeds with this branch where the old/devbind mount pathwas the problematic part
/devbind mount (podman inspectonlyshows
/dev/pts)groupsinside the container still shows the expectedwheelmembershipsudo idinside the container still works/dev/urandomstill exists and is readable inside the containerAlso added a system test that checks the host
/devis no longer bind-mountedinto the container.
Related
Fixes: #1348
Related: #1297, #247
Superseeds: #1732