rootless: allow /dev/{tty,ptmx} to be present in linux.devices

[AkihiroSuda]

Fix #2450

Previously, rootless runc was failing when .linux.devices contains an entry for either /dev/tty or /dev/ptmx.

/dev/tty: ENXIO (no such device or address)

$ rootlesskit runc run foo
WARN[0000] signal: killed                               
ERRO[0000] container_linux.go:353: starting container process caused: process_linux.go:459: container init caused: rootfs_linux.go:70: creating device nodes caused: open /home/suda/tmp/runctest/rootfs/dev/tty: no such device or address 
[rootlesskit:child ] error: command [runc run foo] exited: exit status 1
[rootlesskit:parent] error: child exited: exit status 1

/dev/ptmx: EBUSY (device or resource busy)

$ rootlesskit runc run foo
WARN[0000] signal: killed                               
ERRO[0000] container_linux.go:353: starting container process caused: process_linux.go:459: container init caused: rootfs_linux.go:73: setting up ptmx caused: remove /home/suda/tmp/runctest/rootfs/dev/ptmx: device or resource busy 
[rootlesskit:child ] error: command [runc run foo] exited: exit status 1
[rootlesskit:parent] error: child exited: exit status 1

[AkihiroSuda]

@cyphar @kolyshkin PTAL

[AkihiroSuda]

updated

[cyphar]

The "fixed" version still is incorrect AFAICS. Also I'd like to better understand what you're intention here is -- is the idea that in rootless containers we cannot bind-mount device nodes? Or is it purely that the device might be bind-mounted and that we want to unmount it if we get permission errors (at which point, why are we doing all this logic in bindMountDeviceNode?).

[AkihiroSuda]

Another solution might be to just ignore tty and ptmx in linux.devices when rootless.

@cyphar @giuseppe @kolyshkin RFC

[mrunalp]

@AkihiroSuda I think we shouldn't block on this for next rc. wdyt?

[AkihiroSuda]

@mrunalp Not a blocker for rc91, but good to have if possible. (IIUC rc91 is currently blocked due to other PRs)

[AkihiroSuda]

ping @cyphar

[AkihiroSuda]

@cyphar WDYT? #2451 (comment)

[AkihiroSuda]

Moved to rc92 milestone

[ctalledo]

Hi @AkihiroSuda , I was looking into this problem too.

It seems to me the problem is that runc has a set of default devices that it always configures in the container (/dev/null, /dev/tty, etc.) and if the OCI spec it receives has those same devices, it's not detecting the redundancy.

As a result, it creates a libcontainer config with redundant device configs. This hasn't been a problem without rootless (where mknod is used to setup the devices). But in rootless it fails on /dev/tty (and /dev/ptmx apparently too) because it sets up the device by creating a regular file under /dev/tty and bind-mounting the host's /dev/tty to the container's /dev/tty. When this operation is done redundantly, it fails the second time.

I am thinking an alternative fix is to catch the redundant device configs when creating the libcontainer config, and removing the redundancy by favoring the device config that came in the OCI spec over runc's default device config.

Something like this:

diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go
index 6fd98f02..7046ae9c 100644
--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go

 func createDevices(spec *specs.Spec, config *configs.Config) error {
-       // Add default set of devices.
-       for _, device := range AllowedDevices {
-               if device.Path != "" {
-                       config.Devices = append(config.Devices, device)
+
+       // If a spec device is redundant with a default device, remove that default
+       // device (the spec one takes priority).
+       dedupedAllowDevs := []*configs.Device{}
+
+       if spec.Linux != nil {
+               for _, ad := range AllowedDevices {
+                       conflict := false
+                       for _, sd := range spec.Linux.Devices {
+                               if sd.Path == ad.Path {
+                                       conflict = true
+                               }
+                       }
+                       if !conflict {
+                               dedupedAllowDevs = append(dedupedAllowDevs, ad)
+                               if ad.Path != "" {
+                                       config.Devices = append(config.Devices, ad)
+                               }
+                       }
                }
        }
+
+       AllowedDevices = dedupedAllowDevs
+

What do you think?

[cyphar]

I think @ctalledo is right about the root cause and the general solution to the problem, though I think you could simplify the patch down to the following (modifying AllowedDevices doesn't really make sense).

diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go
index 6fd98f027539..6322c5588154 100644
--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go
@@ -651,9 +651,18 @@ func stringToDeviceRune(s string) (configs.DeviceType, error) {
 }
 
 func createDevices(spec *specs.Spec, config *configs.Config) error {
-       // Add default set of devices.
+       // Add default set of devices, but only if the device is not already
+       // configured in the provided spec (in that case, the specfile takes
+       // precedence).
+add_devices:
        for _, device := range AllowedDevices {
                if device.Path != "" {
+                       for _, specDevice := range spec.Linux.Devices {
+                               // XXX: Probably should do CleanPath here or something.
+                               if device.Path == specDevice.Path {
+                                       continue add_devices
+                               }
+                       }
                        config.Devices = append(config.Devices, device)
                }
        }

[AkihiroSuda]

@ctalledo @cyphar Would you open a PR?

[ctalledo]

Thanks @cyphar.

though I think you could simplify the patch down to the following (modifying AllowedDevices doesn't really make sense).

Actually, it turns out AllowedDevices is used in CreateCgroupConfig(), which is called right after createDevices(). This is why I felt modifying allowed devices was the proper thing to do.

Would you open a PR?

Hi @AkihiroSuda: sure, I'll do this tomorrow.

Thanks!

[cyphar]

@ctalledo

Actually, it turns out AllowedDevices is used in CreateCgroupConfig(), which is called right after createDevices(). This is why I felt modifying allowed devices was the proper thing to do.

The issue is that AllowedDevices is a global and while the primary use-case of runc is as a binary operating on a single container, some people do use libcontainer directly and modifying AllowedDevices will result in memory unsafety as well as generally incorrect behaviour (if you start two different containers the AllowedDevices change will affect both).

But if that's the case then we should probably update the logic to only make use of AllowedDevices once.

[ctalledo]

@cyphar

The issue is that AllowedDevices is a global and while the primary use-case of runc is as a binary operating on a single container, some people do use libcontainer directly and modifying AllowedDevices will result in memory unsafety as well as generally incorrect behaviour (if you start two different containers the AllowedDevices change will affect both).

Got it, thanks.

But if that's the case then we should probably update the logic to only make use of AllowedDevices once.

That's the right path I think. Let me tweak it a bit and open a new PR.

[ctalledo]

Hi @cyphar, @AkihiroSuda,

FYI, the PR for the alternative fix for #2450 is here: #2522

Thanks!

[AkihiroSuda]

Thanks