Skip to content

Commit

Permalink
[1.1] fix failed exec after systemctl daemon-reload
Browse files Browse the repository at this point in the history
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 7219387 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 58b1374
to release-1.1 branch.

Fixes: #3551
Fixes: 7219387
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
  • Loading branch information
kolyshkin committed Aug 18, 2022
1 parent 1c6dc76 commit 204c673
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 7 deletions.
16 changes: 9 additions & 7 deletions libcontainer/cgroups/systemd/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
16 changes: 16 additions & 0 deletions tests/integration/dev.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]
}

0 comments on commit 204c673

Please sign in to comment.