host-device: use temp network namespace for rename

Using a temporary name / doing a fast rename causes some race conditions with udev and NetworkManager: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1599 Signed-off-by: Etienne Champetier <[email protected]>
champtar · Aug 14, 2024 · 12cb704 · 12cb704
1 parent acf8ddc
commit 12cb704
Show file tree

Hide file tree

Showing 2 changed files with 151 additions and 109 deletions.
diff --git a/pkg/ns/ns_linux.go b/pkg/ns/ns_linux.go
@@ -152,6 +152,41 @@ func GetNS(nspath string) (NetNS, error) {
 	return &netNS{file: fd}, nil
 }
 
+// Returns a new empty NetNS.
+func TempNetNS() (NetNS, error) {
+	// save a handle to current network namespace
+	hostNS, err := GetCurrentNS()
+	if err != nil {
+		return nil, fmt.Errorf("failed to open current namespace: %v", err)
+	}
+	defer hostNS.Close()
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+
+	var ns NetNS
+
+	go func() {
+		defer wg.Done()
+		runtime.LockOSThread()
+
+		err = unix.Unshare(unix.CLONE_NEWNET)
+		if err != nil {
+			return
+		}
+
+		ns, err = GetCurrentNS()
+
+		err2 := hostNS.Set()
+		if err2 == nil {
+			runtime.UnlockOSThread()
+		}
+	}()
+
+	wg.Wait()
+	return ns, err
+}
+
 func (ns *netNS) Path() string {
 	return ns.file.Name()
 }

diff --git a/plugins/main/host-device/host-device.go b/plugins/main/host-device/host-device.go
@@ -250,97 +250,92 @@ func setTempName(dev netlink.Link) (netlink.Link, error) {
 }
 
 func moveLinkIn(hostDev netlink.Link, containerNs ns.NetNS, ifName string) (netlink.Link, error) {
-	origLinkFlags := hostDev.Attrs().Flags
 	hostDevName := hostDev.Attrs().Name
-	defaultNs, err := ns.GetCurrentNS()
+
+	tempNS, err := ns.TempNetNS()
 	if err != nil {
-		return nil, fmt.Errorf("failed to get host namespace: %v", err)
+		return nil, fmt.Errorf("failed to create temp NetNS: %v", err)
 	}
+	defer tempNS.Close()
 
-	// Devices can be renamed only when down
-	if err = netlink.LinkSetDown(hostDev); err != nil {
-		return nil, fmt.Errorf("failed to set %q down: %v", hostDev.Attrs().Name, err)
+	if err = netlink.LinkSetNsFd(hostDev, int(tempNS.Fd())); err != nil {
+		return nil, fmt.Errorf("failed to move %q to temp NS: %v", hostDev.Attrs().Name, err)
 	}
+	// detroying tempNS will move the interface back to the host NS
 
-	// restore original link state in case of error
-	defer func() {
+	if err = tempNS.Do(func(_ ns.NetNS) error {
+		tempNsDev, err := netlink.LinkByName(hostDevName)
 		if err != nil {
-			if origLinkFlags&net.FlagUp == net.FlagUp && hostDev != nil {
-				_ = netlink.LinkSetUp(hostDev)
-			}
-		}
-	}()
-
-	hostDev, err = setTempName(hostDev)
-	if err != nil {
-		return nil, fmt.Errorf("failed to rename device %q to temporary name: %v", hostDevName, err)
-	}
-
-	// restore original netdev name in case of error
-	defer func() {
-		if err != nil && hostDev != nil {
-			_ = netlink.LinkSetName(hostDev, hostDevName)
+			return fmt.Errorf("failed to find %q in temp NS: %v", hostDevName, err)
 		}
-	}()
 
-	if err = netlink.LinkSetNsFd(hostDev, int(containerNs.Fd())); err != nil {
-		return nil, fmt.Errorf("failed to move %q to container ns: %v", hostDev.Attrs().Name, err)
-	}
-
-	var contDev netlink.Link
-	tempDevName := hostDev.Attrs().Name
-	if err = containerNs.Do(func(_ ns.NetNS) error {
-		var err error
-		contDev, err = netlink.LinkByName(tempDevName)
-		if err != nil {
-			return fmt.Errorf("failed to find %q: %v", tempDevName, err)
+		// Live device rename is only supported since kernel 6.2
+		// https://github.com/torvalds/linux/commit/bd039b5ea2a91ea707ee8539df26456bd5be80af
+		if err = netlink.LinkSetDown(tempNsDev); err != nil {
+			return fmt.Errorf("failed to set %q down: %v", tempNsDev.Attrs().Name, err)
 		}
 
-		// move netdev back to host namespace in case of error
+		// restore original link state in case of error
 		defer func() {
 			if err != nil {
-				_ = netlink.LinkSetNsFd(contDev, int(defaultNs.Fd()))
-				// we need to get updated link object as link was moved back to host namepsace
-				_ = defaultNs.Do(func(_ ns.NetNS) error {
-					hostDev, _ = netlink.LinkByName(tempDevName)
-					return nil
-				})
+				origLinkFlags := hostDev.Attrs().Flags
+				if origLinkFlags&net.FlagUp == net.FlagUp && tempNsDev != nil {
+					_ = netlink.LinkSetUp(tempNsDev)
+				}
 			}
 		}()
 
-		// Save host device name into the container device's alias property
-		if err = netlink.LinkSetAlias(contDev, hostDevName); err != nil {
-			return fmt.Errorf("failed to set alias to %q: %v", tempDevName, err)
-		}
 		// Rename container device to respect args.IfName
-		if err = netlink.LinkSetName(contDev, ifName); err != nil {
-			return fmt.Errorf("failed to rename device %q to %q: %v", tempDevName, ifName, err)
+		if err = netlink.LinkSetName(tempNsDev, ifName); err != nil {
+			return fmt.Errorf("failed to rename device %q to %q: %v", hostDevName, ifName, err)
 		}
 
-		// restore tempDevName in case of error
+		// restore hostDevName in case of error
 		defer func() {
-			if err != nil {
-				_ = netlink.LinkSetName(contDev, tempDevName)
+			if err != nil && tempNsDev != nil {
+				_ = netlink.LinkSetName(tempNsDev, hostDevName)
 			}
 		}()
 
-		// Bring container device up
-		if err = netlink.LinkSetUp(contDev); err != nil {
-			return fmt.Errorf("failed to set %q up: %v", ifName, err)
+		// Retrieve link again to get up-to-date name and attributes
+		tempNsDev, err = netlink.LinkByName(ifName)
+		if err != nil {
+			return fmt.Errorf("failed to find %q in temp NS: %v", ifName, err)
+		}
+
+		// Save host device name into the container device's alias property
+		if err = netlink.LinkSetAlias(tempNsDev, hostDevName); err != nil {
+			return fmt.Errorf("failed to set alias to %q: %v", hostDevName, err)
 		}
 
-		// bring device down in case of error
 		defer func() {
 			if err != nil {
-				_ = netlink.LinkSetDown(contDev)
+				_ = netlink.LinkSetAlias(tempNsDev, "")
 			}
 		}()
 
-		// Retrieve link again to get up-to-date name and attributes
+		if err = netlink.LinkSetNsFd(tempNsDev, int(containerNs.Fd())); err != nil {
+			return fmt.Errorf("failed to move %q (host: %q) to container NS: %v", ifName, hostDevName, err)
+		}
+
+		return nil
+	}); err != nil {
+		return nil, err
+	}
+
+	var contDev netlink.Link
+	if err = containerNs.Do(func(_ ns.NetNS) error {
+		var err error
 		contDev, err = netlink.LinkByName(ifName)
 		if err != nil {
-			return fmt.Errorf("failed to find %q: %v", ifName, err)
+			return fmt.Errorf("failed to find %q in container NS: %v", ifName, err)
+		}
+
+		// Bring container device up
+		if err = netlink.LinkSetUp(contDev); err != nil {
+			return fmt.Errorf("failed to set %q up: %v", ifName, err)
 		}
+
 		return nil
 	}); err != nil {
 		return nil, err
@@ -350,79 +345,91 @@ func moveLinkIn(hostDev netlink.Link, containerNs ns.NetNS, ifName string) (netl
 }
 
 func moveLinkOut(containerNs ns.NetNS, ifName string) error {
+	tempNS, err := ns.TempNetNS()
+	if err != nil {
+		return fmt.Errorf("failed to create temp NetNS: %v", err)
+	}
+	defer tempNS.Close()
+
+	var contDev netlink.Link
+	err = containerNs.Do(func(_ ns.NetNS) error {
+		var err error
+		contDev, err = netlink.LinkByName(ifName)
+		if err != nil {
+			return fmt.Errorf("failed to find %q in container NS: %v", ifName, err)
+		}
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
+	hostDevName := contDev.Attrs().Alias
+	if hostDevName == "" {
+		return fmt.Errorf("failed to find original ifname for %q", ifName)
+	}
+	_, err = netlink.LinkByName(hostDevName)
+	if err == nil {
+		return fmt.Errorf("%q already exist host", hostDevName)
+	}
+	if _, ok := err.(netlink.LinkNotFoundError); !ok {
+		return fmt.Errorf("failed to check the absence of %q on the host: %v", hostDevName, err)
+	}
+
+	err = containerNs.Do(func(_ ns.NetNS) error {
+		if err = netlink.LinkSetNsFd(contDev, int(tempNS.Fd())); err != nil {
+			return fmt.Errorf("failed to move %q to temp NS: %v", ifName, err)
+		}
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
 	defaultNs, err := ns.GetCurrentNS()
 	if err != nil {
 		return err
 	}
 	defer defaultNs.Close()
 
-	var tempName string
-	var origDev netlink.Link
-	err = containerNs.Do(func(_ ns.NetNS) error {
-		dev, err := netlink.LinkByName(ifName)
+	err = tempNS.Do(func(_ ns.NetNS) error {
+		tempNsDev, err := netlink.LinkByName(ifName)
 		if err != nil {
-			return fmt.Errorf("failed to find %q: %v", ifName, err)
+			return fmt.Errorf("failed to find %q in container NS: %v", ifName, err)
 		}
-		origDev = dev
 
-		// Devices can be renamed only when down
-		if err = netlink.LinkSetDown(dev); err != nil {
-			return fmt.Errorf("failed to set %q down: %v", ifName, err)
+		// Live device rename is only supported since kernel 6.2
+		// https://github.com/torvalds/linux/commit/bd039b5ea2a91ea707ee8539df26456bd5be80af
+		if err = netlink.LinkSetDown(tempNsDev); err != nil {
+			return fmt.Errorf("failed to set %q down: %v", tempNsDev.Attrs().Name, err)
 		}
 
-		defer func() {
-			// If moving the device to the host namespace fails, set its name back to ifName so that this
-			// function can be retried. Also bring the device back up, unless it was already down before.
-			if err != nil {
-				_ = netlink.LinkSetName(dev, ifName)
-				if dev.Attrs().Flags&net.FlagUp == net.FlagUp {
-					_ = netlink.LinkSetUp(dev)
-				}
-			}
-		}()
+		// Rename container device to hostDevName
+		if err = netlink.LinkSetName(tempNsDev, hostDevName); err != nil {
+			return fmt.Errorf("failed to rename device %q to %q: %v", ifName, hostDevName, err)
+		}
 
-		newLink, err := setTempName(dev)
+		// Retrieve link again to get up-to-date name and attributes
+		tempNsDev, err = netlink.LinkByName(hostDevName)
 		if err != nil {
-			return fmt.Errorf("failed to rename device %q to temporary name: %v", ifName, err)
+			return fmt.Errorf("failed to find %q in temp NS: %v", hostDevName, err)
+		}
+
+		// Unset device's alias property
+		if err = netlink.LinkSetAlias(tempNsDev, ""); err != nil {
+			return fmt.Errorf("failed to unset alias to %q: %v", hostDevName, err)
 		}
-		dev = newLink
-		tempName = dev.Attrs().Name
 
-		if err = netlink.LinkSetNsFd(dev, int(defaultNs.Fd())); err != nil {
-			return fmt.Errorf("failed to move %q to host netns: %v", tempName, err)
+		if err = netlink.LinkSetNsFd(tempNsDev, int(defaultNs.Fd())); err != nil {
+			return fmt.Errorf("failed to move %q to host NS: %v", hostDevName, err)
 		}
+
 		return nil
 	})
-
 	if err != nil {
 		return err
 	}
 
-	// Rename the device to its original name from the host namespace
-	tempDev, err := netlink.LinkByName(tempName)
-	if err != nil {
-		return fmt.Errorf("failed to find %q in host namespace: %v", tempName, err)
-	}
-
-	if err = netlink.LinkSetName(tempDev, tempDev.Attrs().Alias); err != nil {
-		// move device back to container ns so it may be retired
-		defer func() {
-			_ = netlink.LinkSetNsFd(tempDev, int(containerNs.Fd()))
-			_ = containerNs.Do(func(_ ns.NetNS) error {
-				lnk, err := netlink.LinkByName(tempName)
-				if err != nil {
-					return err
-				}
-				_ = netlink.LinkSetName(lnk, ifName)
-				if origDev.Attrs().Flags&net.FlagUp == net.FlagUp {
-					_ = netlink.LinkSetUp(lnk)
-				}
-				return nil
-			})
-		}()
-		return fmt.Errorf("failed to restore %q to original name %q: %v", tempName, tempDev.Attrs().Alias, err)
-	}
-
 	return nil
 }