Use interface index instead of name

It's possible to have a race in the VFIsReady function. vf netdevice can have a default eth0 device name and be the time we call the netlink syscall to get the device information eth0 can be a different device. this cause duplicate mac allocation on vf admin mac address Signed-off-by: Sebastian Sch <[email protected]>
k8snetworkplumbingwg · Jul 2, 2024 · aa5e1a1 · aa5e1a1
1 parent efa6384
commit aa5e1a1
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 11 deletions.
diff --git a/pkg/helper/mock/mock_helper.go b/pkg/helper/mock/mock_helper.go
diff --git a/pkg/host/internal/lib/netlink/mock/mock_netlink.go b/pkg/host/internal/lib/netlink/mock/mock_netlink.go
diff --git a/pkg/host/internal/lib/netlink/netlink.go b/pkg/host/internal/lib/netlink/netlink.go
@@ -24,6 +24,8 @@ type NetlinkLib interface {
 	LinkSetVfPortGUID(link Link, vf int, portguid net.HardwareAddr) error
 	// LinkByName finds a link by name and returns a pointer to the object.
 	LinkByName(name string) (Link, error)
+	// LinkByName finds a link by index and returns a pointer to the object.
+	LinkByIndex(index int) (Link, error)
 	// LinkSetVfHardwareAddr sets the hardware address of a vf for the link.
 	// Equivalent to: `ip link set $link vf $vf mac $hwaddr`
 	LinkSetVfHardwareAddr(link Link, vf int, hwaddr net.HardwareAddr) error
@@ -79,11 +81,16 @@ func (w *libWrapper) LinkSetVfPortGUID(link Link, vf int, portguid net.HardwareA
 	return netlink.LinkSetVfPortGUID(link, vf, portguid)
 }
 
-// LinkByName finds a link by name and returns a pointer to the object.// LinkByName finds a link by name and returns a pointer to the object.
+// LinkByName finds a link by name and returns a pointer to the object.
 func (w *libWrapper) LinkByName(name string) (Link, error) {
 	return netlink.LinkByName(name)
 }
 
+// LinkByIndex finds a link by index and returns a pointer to the object.
+func (w *libWrapper) LinkByIndex(index int) (Link, error) {
+	return netlink.LinkByIndex(index)
+}
+
 // LinkSetVfHardwareAddr sets the hardware address of a vf for the link.
 // Equivalent to: `ip link set $link vf $vf mac $hwaddr`
 func (w *libWrapper) LinkSetVfHardwareAddr(link Link, vf int, hwaddr net.HardwareAddr) error {

diff --git a/pkg/host/internal/network/network.go b/pkg/host/internal/network/network.go
@@ -75,6 +75,7 @@ func (n *network) TryToGetVirtualInterfaceName(pciAddr string) string {
 func (n *network) TryGetInterfaceName(pciAddr string) string {
 	names, err := n.dputilsLib.GetNetNames(pciAddr)
 	if err != nil || len(names) < 1 {
+		log.Log.Error(err, "TryGetInterfaceName(): failed to get interface name")
 		return ""
 	}
 	netDevName := names[0]
@@ -95,10 +96,33 @@ func (n *network) TryGetInterfaceName(pciAddr string) string {
 		return name
 	}
 
-	log.Log.V(2).Info("tryGetInterfaceName()", "name", netDevName)
+	log.Log.V(2).Info("TryGetInterfaceName()", "name", netDevName)
 	return netDevName
 }
 
+// TryGetInterfaceIndex tries to find the SR-IOV virtual interface index base on pci address
+func (n *network) TryGetInterfaceIndex(pciAddr string) int {
+	ifName := n.TryGetInterfaceName(pciAddr)
+	if ifName == "" {
+		return -1
+	}
+
+	// read the ifindex file from the interface folder
+	indexFile := filepath.Join(vars.FilesystemRoot, consts.SysBusPciDevices, pciAddr, "net", ifName, "ifindex")
+	ifIndex, err := os.ReadFile(indexFile)
+	if err != nil {
+		log.Log.Error(err, "TryGetInterfaceIndex(): failed to read ifindex file", "indexFile", indexFile)
+		return -1
+	}
+
+	intIfIndex, err := strconv.Atoi(strings.TrimSpace(string(ifIndex)))
+	if err != nil {
+		log.Log.Error(err, "TryGetInterfaceIndex(): failed to parse ifindex file content", "ifIndex", string(ifIndex))
+		return -1
+	}
+	return intIfIndex
+}
+
 func (n *network) GetPhysSwitchID(name string) (string, error) {
 	swIDFile := filepath.Join(vars.FilesystemRoot, consts.SysClassNet, name, "phys_switch_id")
 	physSwitchID, err := os.ReadFile(swIDFile)

diff --git a/pkg/host/internal/network/network_test.go b/pkg/host/internal/network/network_test.go
@@ -238,4 +238,35 @@ var _ = Describe("Network", func() {
 			Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal("1122:3344:5566:7788"))
 		})
 	})
+	Context("TryGetInterfaceIndex", func() {
+		It("should return valid index", func() {
+			helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{
+				Dirs: []string{
+					"/sys/bus/pci/devices/0000:4b:00.3/net/eth0/",
+					"/sys/class/net/eth0/",
+				},
+				Files: map[string][]byte{
+					"/sys/bus/pci/devices/0000:4b:00.3/net/eth0/ifindex": []byte("42"),
+					"/sys/class/net/eth0/phys_switch_id":                 {},
+				},
+			})
+			dputilsLibMock.EXPECT().GetNetNames("0000:4b:00.3").Return([]string{"eth0"}, nil)
+			index := n.TryGetInterfaceIndex("0000:4b:00.3")
+			Expect(index).To(Equal(42))
+		})
+		It("should return invalid index", func() {
+			helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{
+				Dirs: []string{
+					"/sys/bus/pci/devices/0000:4b:00.3/net/eth0",
+					"/sys/class/net/eth0/",
+				},
+				Files: map[string][]byte{
+					"/sys/class/net/eth0/phys_switch_id": {},
+				},
+			})
+			dputilsLibMock.EXPECT().GetNetNames("0000:4b:00.3").Return([]string{"eth0"}, nil)
+			index := n.TryGetInterfaceIndex("0000:4b:00.3")
+			Expect(index).To(Equal(-1))
+		})
+	})
 })
diff --git a/pkg/host/internal/sriov/sriov.go b/pkg/host/internal/sriov/sriov.go
@@ -189,12 +189,17 @@ func (s *sriov) VFIsReady(pciAddr string) (netlink.Link, error) {
 	var err error
 	var vfLink netlink.Link
 	err = wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) {
-		vfName := s.networkHelper.TryGetInterfaceName(pciAddr)
-		vfLink, err = s.netlinkLib.LinkByName(vfName)
+		vfIndex := s.networkHelper.TryGetInterfaceIndex(pciAddr)
+		if vfIndex == -1 {
+			log.Log.Error(nil, "VFIsReady(): invalid index number -1")
+			return false, nil
+		}
+		vfLink, err = s.netlinkLib.LinkByIndex(vfIndex)
 		if err != nil {
 			log.Log.Error(err, "VFIsReady(): unable to get VF link", "device", pciAddr)
+			return false, nil
 		}
-		return err == nil, nil
+		return true, nil
 	})
 	if err != nil {
 		return vfLink, err

diff --git a/pkg/host/internal/sriov/sriov_test.go b/pkg/host/internal/sriov/sriov_test.go
@@ -235,11 +235,11 @@ var _ = Describe("SRIOV", func() {
 			hostMock.EXPECT().UnbindDriverIfNeeded("0000:d8:00.2", true).Return(nil)
 			hostMock.EXPECT().BindDefaultDriver("0000:d8:00.2").Return(nil)
 			hostMock.EXPECT().SetNetdevMTU("0000:d8:00.2", 2000).Return(nil)
-			hostMock.EXPECT().TryGetInterfaceName("0000:d8:00.2").Return("enp216s0f0_0")
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(42)
 			vf0LinkMock := netlinkMockPkg.NewMockLink(testCtrl)
 			vf0Mac, _ := net.ParseMAC("02:42:19:51:2f:af")
-			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: vf0Mac})
-			netlinkLibMock.EXPECT().LinkByName("enp216s0f0_0").Return(vf0LinkMock, nil)
+			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Name: "enp216s0f0_0", HardwareAddr: vf0Mac}).AnyTimes()
+			netlinkLibMock.EXPECT().LinkByIndex(42).Return(vf0LinkMock, nil)
 			netlinkLibMock.EXPECT().LinkSetVfHardwareAddr(vf0LinkMock, 0, vf0Mac).Return(nil)
 
 			dputilsLibMock.EXPECT().GetVFID("0000:d8:00.3").Return(1, nil)
@@ -359,11 +359,11 @@ var _ = Describe("SRIOV", func() {
 			hostMock.EXPECT().UnbindDriverIfNeeded("0000:d8:00.2", true).Return(nil)
 			hostMock.EXPECT().BindDefaultDriver("0000:d8:00.2").Return(nil)
 			hostMock.EXPECT().SetNetdevMTU("0000:d8:00.2", 2000).Return(nil)
-			hostMock.EXPECT().TryGetInterfaceName("0000:d8:00.2").Return("enp216s0f0_0")
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(42).AnyTimes()
 			vf0LinkMock := netlinkMockPkg.NewMockLink(testCtrl)
 			vf0Mac, _ := net.ParseMAC("02:42:19:51:2f:af")
-			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: vf0Mac})
-			netlinkLibMock.EXPECT().LinkByName("enp216s0f0_0").Return(vf0LinkMock, nil)
+			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Name: "enp216s0f0_0", HardwareAddr: vf0Mac})
+			netlinkLibMock.EXPECT().LinkByIndex(42).Return(vf0LinkMock, nil).AnyTimes()
 			netlinkLibMock.EXPECT().LinkSetVfHardwareAddr(vf0LinkMock, 0, vf0Mac).Return(nil)
 			hostMock.EXPECT().GetPhysPortName("enp216s0f0np0").Return("p0", nil)
 			hostMock.EXPECT().GetPhysSwitchID("enp216s0f0np0").Return("7cfe90ff2cc0", nil)
@@ -537,6 +537,20 @@ var _ = Describe("SRIOV", func() {
 			helpers.GinkgoAssertFileContentsEquals("/sys/bus/pci/devices/0000:d8:00.0/sriov_numvfs", "2")
 		})
 	})
+
+	Context("VfIsReady", func() {
+		It("Should retry if interface index is -1", func() {
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(-1).Times(1)
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(42).Times(1)
+			vf0LinkMock := netlinkMockPkg.NewMockLink(testCtrl)
+			vf0Mac, _ := net.ParseMAC("02:42:19:51:2f:af")
+			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Name: "enp216s0f0_0", HardwareAddr: vf0Mac})
+			netlinkLibMock.EXPECT().LinkByIndex(42).Return(vf0LinkMock, nil).Times(1)
+			vfLink, err := s.VFIsReady("0000:d8:00.2")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(vfLink.Attrs().HardwareAddr).To(Equal(vf0Mac))
+		})
+	})
 })
 
 func getTestPCIDevices() []*ghw.PCIDevice {

diff --git a/pkg/host/mock/mock_host.go b/pkg/host/mock/mock_host.go
diff --git a/pkg/host/types/interfaces.go b/pkg/host/types/interfaces.go
@@ -81,6 +81,8 @@ type NetworkInterface interface {
 	TryToGetVirtualInterfaceName(pciAddr string) string
 	// TryGetInterfaceName tries to find the SR-IOV virtual interface name base on pci address
 	TryGetInterfaceName(pciAddr string) string
+	// TryGetInterfaceIndex tries to find the SR-IOV virtual interface index base on pci address
+	TryGetInterfaceIndex(pciAddr string) int
 	// GetPhysSwitchID returns the physical switch ID for a specific pci address
 	GetPhysSwitchID(name string) (string, error)
 	// GetPhysPortName returns the physical port name for a specific pci address