Add support for injecting additional GIDs

This change adds support for injecting additional GIDs using CDI.
This is on by default in cases that use the internal representation
and can be opted out of by enabling the `no-additional-gids` feature.

Signed-off-by: Evan Lezar <[email protected]>

Author: elezar

internal/config/features.go

@@ -19,18 +19,20 @@ package config
 type featureName string
 
 const (
-	FeatureGDS      = featureName("gds")
-	FeatureMOFED    = featureName("mofed")
-	FeatureNVSWITCH = featureName("nvswitch")
-	FeatureGDRCopy  = featureName("gdrcopy")
+	FeatureGDS              = featureName("gds")
+	FeatureMOFED            = featureName("mofed")
+	FeatureNVSWITCH         = featureName("nvswitch")
+	FeatureGDRCopy          = featureName("gdrcopy")
+	FeatureNoAdditionalGIDs = featureName("no-additional-gids")
 )
 
 // features specifies a set of named features.
 type features struct {
-	GDS      *feature `toml:"gds,omitempty"`
-	MOFED    *feature `toml:"mofed,omitempty"`
-	NVSWITCH *feature `toml:"nvswitch,omitempty"`
-	GDRCopy  *feature `toml:"gdrcopy,omitempty"`
+	GDS              *feature `toml:"gds,omitempty"`
+	MOFED            *feature `toml:"mofed,omitempty"`
+	NVSWITCH         *feature `toml:"nvswitch,omitempty"`
+	GDRCopy          *feature `toml:"gdrcopy,omitempty"`
+	NoAdditionalGIDs *feature `toml:"no-additional-gids,omitempty"`
 }
 
 type feature bool
@@ -40,10 +42,11 @@ type feature bool
 // variables can also be supplied.
 func (fs features) IsEnabled(n featureName, in ...getenver) bool {
 	featureEnvvars := map[featureName]string{
-		FeatureGDS:      "NVIDIA_GDS",
-		FeatureMOFED:    "NVIDIA_MOFED",
-		FeatureNVSWITCH: "NVIDIA_NVSWITCH",
-		FeatureGDRCopy:  "NVIDIA_GDRCOPY",
+		FeatureGDS:              "NVIDIA_GDS",
+		FeatureMOFED:            "NVIDIA_MOFED",
+		FeatureNVSWITCH:         "NVIDIA_NVSWITCH",
+		FeatureGDRCopy:          "NVIDIA_GDRCOPY",
+		FeatureNoAdditionalGIDs: "NVIDIA_NO_ADDITIONAL_GIDS",
 	}
 
 	envvar := featureEnvvars[n]
@@ -56,6 +59,8 @@ func (fs features) IsEnabled(n featureName, in ...getenver) bool {
 		return fs.NVSWITCH.isEnabled(envvar, in...)
 	case FeatureGDRCopy:
 		return fs.GDRCopy.isEnabled(envvar, in...)
+	case FeatureNoAdditionalGIDs:
+		return fs.NoAdditionalGIDs.isEnabled(envvar, in...)
 	default:
 		return false
 	}

internal/edits/device.go

@@ -17,6 +17,9 @@
 package edits
 
 import (
+	"os"
+
+	"golang.org/x/sys/unix"
 	"tags.cncf.io/container-device-interface/pkg/cdi"
 	"tags.cncf.io/container-device-interface/specs-go"
 
@@ -26,15 +29,23 @@ import (
 type device discover.Device
 
 // toEdits converts a discovered device to CDI Container Edits.
-func (d device) toEdits() (*cdi.ContainerEdits, error) {
+func (d device) toEdits(allowAdditionalGIDs bool) (*cdi.ContainerEdits, error) {
 	deviceNode, err := d.toSpec()
 	if err != nil {
 		return nil, err
 	}
 
+	var additionalGIDs []uint32
+	if allowAdditionalGIDs {
+		if requiredGID, _ := d.getRequiredGID(); requiredGID != 0 {
+			additionalGIDs = append(additionalGIDs, requiredGID)
+		}
+	}
+
 	e := cdi.ContainerEdits{
 		ContainerEdits: &specs.ContainerEdits{
-			DeviceNodes: []*specs.DeviceNode{deviceNode},
+			DeviceNodes:    []*specs.DeviceNode{deviceNode},
+			AdditionalGIDs: additionalGIDs,
 		},
 	}
 	return &e, nil
@@ -52,10 +63,37 @@ func (d device) toSpec() (*specs.DeviceNode, error) {
 	if hostPath == d.Path {
 		hostPath = ""
 	}
+
 	s := specs.DeviceNode{
 		HostPath: hostPath,
 		Path:     d.Path,
 	}
 
 	return &s, nil
 }
+
+// getRequiredGID returns the group id of the device if the device is not world read/writable
+func (d device) getRequiredGID() (uint32, error) {
+	path := d.HostPath
+	if path == "" {
+		path = d.Path
+	}
+	if path == "" {
+		return 0, nil
+	}
+
+	var stat unix.Stat_t
+	if err := unix.Lstat(path, &stat); err != nil {
+		return 0, err
+	}
+	// This is only supported for char devices
+	if stat.Mode&unix.S_IFMT != unix.S_IFCHR {
+		return 0, nil
+	}
+
+	permissions := os.FileMode(stat.Mode).Perm()
+	if permissions&06 == 0 {
+		return stat.Gid, nil
+	}
+	return 0, nil
+}

internal/edits/lib.go

@@ -58,7 +58,7 @@ func (o *options) EditsFromDiscoverer(d discover.Discover) (*cdi.ContainerEdits,
 
 	c := NewContainerEdits()
 	for _, d := range devices {
-		edits, err := device(d).toEdits()
+		edits, err := device(d).toEdits(o.allowAdditionalGIDs)
 		if err != nil {
 			return nil, fmt.Errorf("failed to created container edits for device: %v", err)
 		}

internal/edits/options.go

@@ -19,7 +19,8 @@ package edits
 import "github.com/NVIDIA/nvidia-container-toolkit/internal/logger"
 
 type options struct {
-	logger logger.Interface
+	logger              logger.Interface
+	allowAdditionalGIDs bool
 }
 
 type Option func(*options)
@@ -29,3 +30,9 @@ func WithLogger(logger logger.Interface) Option {
 		o.logger = logger
 	}
 }
+
+func WithAllowAdditionalGIDs(allowAdditionalGIDs bool) Option {
+	return func(o *options) {
+		o.allowAdditionalGIDs = allowAdditionalGIDs
+	}
+}

internal/modifier/discover.go

@@ -28,16 +28,18 @@ import (
 )
 
 type discoverModifier struct {
-	logger     logger.Interface
-	discoverer discover.Discover
+	logger              logger.Interface
+	discoverer          discover.Discover
+	allowAdditionalGIDs bool
 }
 
 // NewModifierFromDiscoverer creates a modifier that applies the discovered
 // modifications to an OCI spec if required by the runtime wrapper.
-func NewModifierFromDiscoverer(logger logger.Interface, d discover.Discover) (oci.SpecModifier, error) {
+func NewModifierFromDiscoverer(logger logger.Interface, d discover.Discover, allowAdditionalGIDs bool) (oci.SpecModifier, error) {
 	m := discoverModifier{
-		logger:     logger,
-		discoverer: d,
+		logger:              logger,
+		discoverer:          d,
+		allowAdditionalGIDs: allowAdditionalGIDs,
 	}
 	return &m, nil
 }
@@ -47,6 +49,7 @@ func NewModifierFromDiscoverer(logger logger.Interface, d discover.Discover) (oc
 func (m discoverModifier) Modify(spec *specs.Spec) error {
 	e := edits.New(
 		edits.WithLogger(m.logger),
+		edits.WithAllowAdditionalGIDs(m.allowAdditionalGIDs),
 	)
 
 	specEdits, err := e.SpecModifierFromDiscoverer(m.discoverer)

internal/modifier/discover_test.go

@@ -132,7 +132,7 @@ func TestDiscoverModifier(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
-			m, err := NewModifierFromDiscoverer(logger, tc.discover)
+			m, err := NewModifierFromDiscoverer(logger, tc.discover, true)
 			require.NoError(t, err)
 
 			err = m.Modify(tc.spec)

internal/modifier/gated.go

@@ -78,5 +78,7 @@ func NewFeatureGatedModifier(logger logger.Interface, cfg *config.Config, image
 		discoverers = append(discoverers, d)
 	}
 
-	return NewModifierFromDiscoverer(logger, discover.Merge(discoverers...))
+	allowAdditionalGIDs := !cfg.Features.IsEnabled(config.FeatureNoAdditionalGIDs, image)
+
+	return NewModifierFromDiscoverer(logger, discover.Merge(discoverers...), allowAdditionalGIDs)
 }

internal/modifier/graphics.go

@@ -62,7 +62,9 @@ func NewGraphicsModifier(logger logger.Interface, cfg *config.Config, image imag
 		drmNodes,
 		mounts,
 	)
-	return NewModifierFromDiscoverer(logger, d)
+
+	allowAdditionalGIDs := !cfg.Features.IsEnabled(config.FeatureNoAdditionalGIDs, image)
+	return NewModifierFromDiscoverer(logger, d, allowAdditionalGIDs)
 }
 
 // requiresGraphicsModifier determines whether a graphics modifier is required.

internal/oci/spec_mock.go

@@ -65,6 +65,8 @@ type nvcdilib struct {
 	infolib info.Interface
 
 	mergedDeviceOptions []transform.MergedDeviceOption
+
+	allowAdditionalGIDs bool
 }
 
 // New creates a new nvcdi library
@@ -196,6 +198,7 @@ func (m *wrapper) GetCommonEdits() (*cdi.ContainerEdits, error) {
 func (l *nvcdilib) editsFromDiscoverer(d discover.Discover) (*cdi.ContainerEdits, error) {
 	e := edits.New(
 		edits.WithLogger(l.logger),
+		edits.WithAllowAdditionalGIDs(l.allowAdditionalGIDs),
 	)
 	return e.EditsFromDiscoverer(d)
 }

pkg/nvcdi/lib.go

@@ -155,3 +155,11 @@ func WithLibrarySearchPaths(paths []string) Option {
 		o.librarySearchPaths = paths
 	}
 }
+
+// WithAllowAdditionalGIDs specifies whether a generated CDI spec can contain
+// the additionaGIDs field.
+func WithAllowAdditionalGIDs(allowAdditionalGIDs bool) Option {
+	return func(o *nvcdilib) {
+		o.allowAdditionalGIDs = allowAdditionalGIDs
+	}
+}

pkg/nvcdi/namer_nvml_mock.go

@@ -17,6 +17,8 @@
 package transform
 
 import (
+	"slices"
+
 	"tags.cncf.io/container-device-interface/specs-go"
 )
 
@@ -50,6 +52,12 @@ func (d dedupe) Transform(spec *specs.Spec) error {
 }
 
 func (d dedupe) transformEdits(edits *specs.ContainerEdits) error {
+	additionalGIDs, err := d.deduplicateAdditionalGIDs(edits.AdditionalGIDs)
+	if err != nil {
+		return err
+	}
+	edits.AdditionalGIDs = additionalGIDs
+
 	deviceNodes, err := d.deduplicateDeviceNodes(edits.DeviceNodes)
 	if err != nil {
 		return err
@@ -150,3 +158,19 @@ func (d dedupe) deduplicateMounts(entities []*specs.Mount) ([]*specs.Mount, erro
 	}
 	return mounts, nil
 }
+
+func (d dedupe) deduplicateAdditionalGIDs(entities []uint32) ([]uint32, error) {
+	seen := make(map[uint32]bool)
+	var additionalGIDs []uint32
+	for _, e := range entities {
+		if seen[e] {
+			continue
+		}
+		seen[e] = true
+		additionalGIDs = append(additionalGIDs, e)
+	}
+
+	slices.Sort(additionalGIDs)
+
+	return additionalGIDs, nil
+}