Add support for extra device cgroup rules

[marquiz]

Adds a deviceCgroupRules field to containerEdits, for granting access to a set of devices in the devices cgroup without creating any device nodes with it. The main motivation is hotplug in device management and device monitoring scenarios, where the device numbers that appear on the host after the container was created cannot be known at container creation time. An omitted minor is a wildcard matching all device numbers. An example:

cdiVersion: "1.2.0"
kind: "vendor.com/device"
devices:
  - name: "all"
    containerEdits:
      deviceCgroupRules:
        - type: "c"
          major: 226
          permissions: "rw"
        - type: "b"
          major: 8
          minor: 0

Rules are applied as allow entries in linux.resources.devices of the OCI runtime spec.

Also bumps the spec version to v1.2.0.

[eero-t]

An omitted major or minor is a wildcard matching all device numbers

May be safer to start by erroring in this case (and relaxing it only if somebody provides strong enough use-case for it, that cannot be handled reasonable in any other way).

[klihub]

An omitted major or minor is a wildcard matching all device numbers

May be safer to start by erroring in this case (and relaxing it only if somebody provides strong enough use-case for it, that cannot be handled reasonable in any other way).

Well, I think we at least must allow omitting the minor number, otherwise this can't really be used for what is stated as the primary use case in the description: "The main motivation is hotplug in device management and device monitoring scenarios, where the device numbers that appear on the host after the container was created cannot be known at container creation time."

@marquiz But I was also wondering if we really need to allow such wide permissions, or if we could instead get along with only allowing the minor number to be omitted. Do we really have a use case for injecting access permissions to all block or character devices ?

[marquiz]

But I was also wondering if we really need to allow such wide permissions, or if we could instead get along with only allowing the minor number to be omitted. Do we really have a use case for injecting access permissions to all block or character devices ?

Yeah, agree. I think allowing "wildcard" on minor only is a sound choice (nobody should have the need to allow everything with one single rule). I'll change the PR

[marquiz]

But I was also wondering if we really need to allow such wide permissions, or if we could instead get along with only allowing the minor number to be omitted. Do we really have a use case for injecting access permissions to all block or character devices ?

Done. Major is now required (and must be > 0)

[klihub]

@marquiz @elezar @bart0sh Should we explicitly spell out here, that an actual device injection in the Spec is always an implicit corresponding device cgroup rule injection. IOW, there is absolutely no need to manually specify device cgroup rules for devices you inject by a CDI Spec. You only need to specify device cgroup rules, if you do not inject the actual devices using a CDI Spec. This might not otherwise be fully obvious after this addition to the Spec, especially not for hasty readers.

[elezar]

Yes, I think making it clear that on systems where cgroups are used, a device node implies a corresponding cgroup rule.

[elezar]

I still need to do a proper review, but this seems to align with what I proposed in #314. My first question would be whether a separate containerEdits type is preferred over reusing "deviceNodes"? Using a specific type would help for clarity.

[elezar]

As far as I know these are Linux only? Should we name them as such? (we do so for the LinuxNetDevices).

[marquiz]

Hmm, good point. "Linux" is probably is justified here. I'll change this

[klihub]

I still need to do a proper review, but this seems to align with what I proposed in #314. My first question would be whether a separate containerEdits type is preferred over reusing "deviceNodes"? Using a specific type would help for clarity.

@elezar I think that is the major advantage of this PR's approach. It is rather explicit about what is happening so what users could expect. The wildcard approach is quite a bit more implicit. For instance, without (reading the) documentation some users might not get a clear idea whether any device nodes get injected. To some intuition might give the wrong impression that since its a globbing pattern, matching nodes will be injected. This approach does not suffer from that problem. It would also leave the door open for any future wildcard-based matching device injection if we'd ever to decide to consider adding something like that.

[bart0sh]

This is a bit implicit. Would mandatory minor supporting wildcards be a safer option?

[marquiz]

Dunno 🤷‍♂️, what others think. The "wildcard" would be -1 then

[elezar]

In order to collect data, how does Docker or Podman handle this?

[bart0sh]

My rationale FWIW: If minor is unintentionally not added, all minors would be implicitly allowed and everything would work with wider permissions, allowing access to all the minors. This could cause security issues and after all this is not what user actually wanted.

[elezar]

Yes, I understand. I suppose the issue is that communicating what -1 means may not be so straightforward.