From 204c673cced91b03aa337c804ab8b31a452c6777 Mon Sep 17 00:00:00 2001

From: Kir Kolyshkin <kolyshkin@gmail.com>

Date: Wed, 10 Aug 2022 17:09:23 -0700

Subject: [PATCH] [1.1] fix failed exec after systemctl daemon-reload

A regression reported for runc v1.1.3 says that "runc exec -t" fails

after doing "systemctl daemon-reload":

> exec failed: unable to start container process: open /dev/pts/0: operation not permitted: unknown

Apparently, with commit 7219387eb7db69b we are no longer adding

"DeviceAllow=char-pts rwm" rule (because os.Stat("char-pts") returns

ENOENT).

The bug can only be seen after "systemctl daemon-reload" because runc

also applies the same rules manually (by writing to devices.allow for

cgroup v1), and apparently reloading systemd leads to re-applying the

rules that systemd has (thus removing the char-pts access).

The fix is to do os.Stat only for "/dev" paths.

Also, emit a warning that the path was skipped. Since the original idea

was to emit less warnings, demote the level to debug.

Note this also fixes the issue of not adding "m" permission for block-*

and char-* devices.

A test case is added, which reliably fails before the fix

on both cgroup v1 and v2.

This is a backport of commit 58b1374f0ad98cc9390adc43dc22ddbb4f84d72e

to release-1.1 branch.

Fixes: https://github.com/opencontainers/runc/issues/3551

Fixes: 7219387eb7db69b4dae740c9d37d973d9a735801

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

---

 libcontainer/cgroups/systemd/common.go | 16 +++++++++-------

 tests/integration/dev.bats             | 16 ++++++++++++++++

 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go

index 5a68a3cf39..45744c15c0 100644

--- a/libcontainer/cgroups/systemd/common.go

+++ b/libcontainer/cgroups/systemd/common.go

@@ -288,14 +288,16 @@ func generateDeviceProperties(r *configs.Resources) ([]systemdDbus.Property, err

 			case devices.CharDevice:

 				entry.Path = fmt.Sprintf("/dev/char/%d:%d", rule.Major, rule.Minor)

 			}

+			// systemd will issue a warning if the path we give here doesn't exist.

+			// Since all of this logic is best-effort anyway (we manually set these

+			// rules separately to systemd) we can safely skip entries that don't

+			// have a corresponding path.

+			if _, err := os.Stat(entry.Path); err != nil {

+				logrus.Debugf("skipping device %s for systemd: %s", entry.Path, err)

+				continue

+			}

 		}

-		// systemd will issue a warning if the path we give here doesn't exist.

-		// Since all of this logic is best-effort anyway (we manually set these

-		// rules separately to systemd) we can safely skip entries that don't

-		// have a corresponding path.

-		if _, err := os.Stat(entry.Path); err == nil {

-			deviceAllowList = append(deviceAllowList, entry)

-		}

+		deviceAllowList = append(deviceAllowList, entry)

 	}

 	properties = append(properties, newProp("DeviceAllow", deviceAllowList))

diff --git a/tests/integration/dev.bats b/tests/integration/dev.bats

index 01f6778598..243315717a 100644

--- a/tests/integration/dev.bats

+++ b/tests/integration/dev.bats

@@ -128,3 +128,19 @@ function teardown() {

 	runc exec test_allow_block sh -c 'fdisk -l '"$device"''

 	[ "$status" -eq 0 ]

 }

+

+# https://github.com/opencontainers/runc/issues/3551

+@test "runc exec vs systemctl daemon-reload" {

+	requires systemd root

+

+	runc run -d --console-socket "$CONSOLE_SOCKET" test_exec

+	[ "$status" -eq 0 ]

+

+	runc exec -t test_exec sh -c "ls -l /proc/self/fd/0; echo 123"

+	[ "$status" -eq 0 ]

+

+	systemctl daemon-reload

+

+	runc exec -t test_exec sh -c "ls -l /proc/self/fd/0; echo 123"

+	[ "$status" -eq 0 ]

+}