Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions pkg/host/internal/infiniband/infiniband.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package infiniband

import (
"crypto/rand"
"errors"
"fmt"
"io/fs"
Expand Down Expand Up @@ -36,20 +37,39 @@ type infiniband struct {
kernelHelper types.KernelInterface
}

// ConfigureVfGUID configures and sets a GUID for an IB VF device
func (i *infiniband) ConfigureVfGUID(vfAddr string, pfAddr string, vfID int, pfLink netlink.Link) error {
log.Log.Info("ConfigureVfGUID(): configure vf guid", "vfAddr", vfAddr, "pfAddr", pfAddr, "vfID", vfID)

guid := generateRandomGUID()

// GetVfGUID gets a GUID for an IB VF device (checks pool first, then generates random)
func (i *infiniband) GetVfGUID(pfAddr string, vfID int) (net.HardwareAddr, error) {
if i.guidPool != nil {
guidFromPool, err := i.guidPool.GetVFGUID(pfAddr, vfID)
if err != nil {
log.Log.Info("ConfigureVfGUID(): failed to get GUID from IB GUID pool", "address", vfAddr, "error", err)
return err
log.Log.Error(err, "GetVfGUID(): failed to get GUID from IB GUID pool", "pfAddr", pfAddr, "vfID", vfID)
return nil, err
}
Comment on lines 44 to 47

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The log message at line 46 is misleading. It states "using random GUID", but the function returns an error instead of falling back to the randomly generated GUID. This can make debugging difficult. The log level should also be Error since an error is being handled and returned, which is a better practice for error logging.

Suggested change
if err != nil {
log.Log.Info("ConfigureVfGUID(): failed to get GUID from IB GUID pool", "address", vfAddr, "error", err)
return err
log.Log.Info("GetVfGUID(): failed to get GUID from IB GUID pool, using random GUID", "pfAddr", pfAddr, "vfID", vfID, "error", err)
return nil, err
}
if err != nil {
log.Log.Error(err, "GetVfGUID(): failed to get GUID from IB GUID pool", "pfAddr", pfAddr, "vfID", vfID)
return nil, err
}

guid = guidFromPool
return guidFromPool, nil
}

// Fallback to random GUID generation if pool is not available.
// Using crypto/rand for cryptographically secure random numbers to avoid collisions.
guid := make(net.HardwareAddr, 8)
if _, err := rand.Read(guid); err != nil {
return nil, fmt.Errorf("failed to generate random GUID: %w", err)
}
// Set U/L bit to indicate locally administered address.
guid[0] |= 0x02
// Unset I/G bit for unicast.
guid[0] &^= 0x01
return guid, nil
}
Comment on lines +41 to +62

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The current implementation for random GUID generation relies on generateRandomGUID(), which uses math/rand. This is problematic for two main reasons:

  1. Predictable Sequences: If math/rand is not seeded, it will produce the same sequence of "random" numbers on every program start, leading to identical GUIDs being generated across different nodes and causing network collisions.
  2. Collision Risk: Even if seeded (e.g., with the current time), math/rand is not cryptographically secure and is not suitable for generating unique identifiers where collision avoidance is critical.

For generating unique identifiers like GUIDs, crypto/rand should be used as it provides cryptographically secure random numbers.

I suggest refactoring this function to use crypto/rand and to only generate a random GUID when a GUID cannot be retrieved from the pool. This also avoids unnecessary work.

Note: The suggestion requires importing crypto/rand and fmt.

func (i *infiniband) GetVfGUID(pfAddr string, vfID int) (net.HardwareAddr, error) {
	if i.guidPool != nil {
		guidFromPool, err := i.guidPool.GetVFGUID(pfAddr, vfID)
		if err != nil {
			log.Log.Error(err, "GetVfGUID(): failed to get GUID from IB GUID pool", "pfAddr", pfAddr, "vfID", vfID)
			return nil, err
		}
		return guidFromPool, nil
	}

	// Fallback to random GUID generation if pool is not available.
	// Using crypto/rand for cryptographically secure random numbers to avoid collisions.
	guid := make(net.HardwareAddr, 8)
	if _, err := rand.Read(guid); err != nil {
		return nil, fmt.Errorf("failed to generate random GUID: %w", err)
	}
	// Set U/L bit to indicate locally administered address.
	guid[0] |= 0x02
	// Unset I/G bit for unicast.
	guid[0] &^= 0x01
	return guid, nil
}


// ConfigureVfGUID configures and sets a GUID for an IB VF device
func (i *infiniband) ConfigureVfGUID(vfAddr string, pfAddr string, vfID int, pfLink netlink.Link) error {
log.Log.Info("ConfigureVfGUID(): configure vf guid", "vfAddr", vfAddr, "pfAddr", pfAddr, "vfID", vfID)

guid, err := i.GetVfGUID(pfAddr, vfID)
if err != nil {
return err
}

log.Log.Info("ConfigureVfGUID(): set vf guid", "address", vfAddr, "guid", guid)

return i.applyVfGUIDToInterface(guid, vfAddr, vfID, pfLink)
Expand Down
Loading