Skip to content

libct/specconf: CreateDevices nits#5255

Open
kolyshkin wants to merge 3 commits intoopencontainers:mainfrom
kolyshkin:specconv
Open

libct/specconf: CreateDevices nits#5255
kolyshkin wants to merge 3 commits intoopencontainers:mainfrom
kolyshkin:specconv

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Apr 18, 2026

A couple of followups to PR #2522.

  1. libct/specconv: fixup TestCreateDevices

    Commit 0709202 added TestCreateDevices which, among the other things,
    checks that defaultDevs, as returned by createDevices, does not contain
    a second entry for /dev/tty, in case user-supplied rules also have an
    entry to /dev/tty.

    Since defaultDevs is a subset of AllowedDevices, having two entries
    is not possible at all, making this check useless.

    What it should probably check is that defaultDevs does not contain an
    entry for /dev/tty (as the user-supplied one prevails).

    Fix the test appropriately. While at it, do some minor code cleanup.

    The alternative to this is to remove this check entirely.

  2. tests/int: demo default device access rule removal

    Since commit 0709202 ("Remove runc default devices that overlap with
    spec devices.") runc removes the default cgroup access rule for a device
    in case a device with the same path is listed in container spec.

    Judging by the commit description, this was not the intention, and yet
    this is what we have.

    As the behavior is now part of runc (since v1.0-rc93), it makes sense
    to at least test it, to ensure it won't be broken in the future.

    Note that the above behavior is only for rootful runc (rootless does not
    use cgroup device access rules and bind-mounts host devices instead).

    In addition, the test case serves as a demo how to limit the container
    device access to a subset of default AllowedDevices, which we thought
    was not possible before. Turns out it is possible, in a way (and for
    rootful runc only).

@kolyshkin
Copy link
Copy Markdown
Contributor Author

Alternative view: the second commit actually reveals a (subtle, corner case) bug and it needs to be fixed.

@kolyshkin kolyshkin requested a review from cyphar April 21, 2026 21:03
@kolyshkin kolyshkin marked this pull request as ready for review April 21, 2026 21:03
Commit 0709202 added TestCreateDevices which, among the other things,
checks that defaultDevs, as returned by createDevices, does not contain
a *second* entry for /dev/tty in case user-supplied rules also have an
entry to /dev/tty.

Since defaultDevs is a subset of AllowedDevices, having two entries is
not possible at all, making this check useless.

What it should probably check is that defaultDevs does not contain an
entry for /dev/tty (as the user-supplied one prevails).

Fix the test appropriately. While at it, do some minor cleanup.

The alternative to this is to remove this check entirely.

Fixes: 0709202
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 0709202 ("Remove runc default devices that overlap with
spec devices.") runc removes the default cgroup device access rule from
the default set in case a device with the same path is also listed in
container spec.

Judging by the commit description, this was not the intention, and yet
this is what we have.

As the behavior is now part of runc (since v1.0-rc93), it makes sense
to at least test it, to ensure it won't be broken in the future.

Note that the above behavior is only for rootful runc (rootless does not
use cgroup device access rules and bind-mounts host devices instead).

In addition, the test case serves as a demo how to limit the container
device access to a subset of default AllowedDevices, which we thought
was not possible before. Turns out it is possible, in a way (and for
rootful runc only).

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The usage of device.Rule is not very obvious, so explain it.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant