From f2e9c82280e922155264d8e8ec41761863d840b2 Mon Sep 17 00:00:00 2001 From: Anand-240 Date: Sat, 1 Aug 2026 04:16:03 +0530 Subject: [PATCH] fix(network): remove leaked tap device on networkSetup failure networkSetup() creates the tap device before doing anything that can fail (bringing links up, adding TC qdiscs/filters, assigning an IP), but none of its error paths deleted the tap it had already created. The same gap existed one level up: DynamicNetwork.NetworkSetup() and StaticNetwork.NetworkSetup() each call networkSetup() successfully and then do more work (getInterfaceInfo / setNATRule) that can still fail, again leaving the tap behind. The only existing cleanup, CleanupAllUruncTaps(), only runs from Kill() on `urunc delete --force`, so a plain create/start failure leaves a stray tapN_urunc in the netns. Since getTapIndex() treats any existing tap as "a unikernel is already running here", every subsequent setup attempt in that netns then fails permanently until something explicitly force-deletes it. Track whether the ingress qdisc on the container's real interface was actually added before rolling it back, since unlike the tap it can carry TC state that predates this call. Signed-off-by: Anand-240 --- pkg/network/network.go | 37 ++++++++++++++++++++++++++++++++++ pkg/network/network_dynamic.go | 5 +++++ pkg/network/network_static.go | 5 +++++ 3 files changed, 47 insertions(+) diff --git a/pkg/network/network.go b/pkg/network/network.go index cbc73c707..f218e55f4 100644 --- a/pkg/network/network.go +++ b/pkg/network/network.go @@ -222,6 +222,28 @@ func addRedirectFilter(source netlink.Link, target netlink.Link) error { }) } +// removeTap undoes a partial or complete tap setup performed by networkSetup, +// so a failed attempt does not leave the netns in a state that blocks the +// next one. redirectQdiscAdded must only be true when the ingress qdisc on +// redirectLink was actually added by this attempt: unlike the tap device, +// redirectLink is the container's real interface and can carry TC config +// that predates this call, so it must never be removed unless we know we +// are the ones who put it there. Deleting the tap device removes any qdisc, +// filter or address that was attached to the tap itself. Failures here are +// logged rather than returned: they must not shadow the original error that +// triggered the rollback, and a partial rollback still leaves less behind +// than no rollback at all. +func removeTap(tapDevice netlink.Link, redirectLink netlink.Link, redirectQdiscAdded bool) { + if redirectQdiscAdded { + if err := deleteIngressQdisc(redirectLink); err != nil { + netlog.Warnf("rollback: failed to remove ingress qdisc from %s: %v", redirectLink.Attrs().Name, err) + } + } + if err := netlink.LinkDel(tapDevice); err != nil { + netlog.Warnf("rollback: failed to remove tap device %s: %v", tapDevice.Attrs().Name, err) + } +} + func networkSetup(tapName string, ipAddress string, redirectLink netlink.Link, addTCRules bool, uid uint32, gid uint32) (netlink.Link, error) { netlog.Debugf("starting for tapName=%s ipAddress=%s redirectLink=%s addTCRules=%v", tapName, ipAddress, redirectLink.Attrs().Name, addTCRules) @@ -233,14 +255,22 @@ func networkSetup(tapName string, ipAddress string, redirectLink netlink.Link, a } netlog.Debugf("created tap device %s (index=%d)", newTapDevice.Attrs().Name, newTapDevice.Attrs().Index) + // From this point on, any failure must delete the tap device we just + // created (and any TC rule we managed to add on redirectLink) before + // returning, otherwise it is leaked in the netns and blocks the next + // setup attempt (see: tap count check in getTapIndex). + redirectQdiscAdded := false + // Bring TAP up before qdisc if err = netlink.LinkSetUp(newTapDevice); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("LinkSetUp(%s) failed: %w", newTapDevice.Attrs().Name, err) } netlog.Debugf("TAP %s is UP", newTapDevice.Attrs().Name) // Bring redirectLink (eth0) up before using it in filters if err = netlink.LinkSetUp(redirectLink); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("LinkSetUp(%s) failed: %w", redirectLink.Attrs().Name, err) } netlog.Debugf("redirectLink %s is UP", redirectLink.Attrs().Name) @@ -250,18 +280,23 @@ func networkSetup(tapName string, ipAddress string, redirectLink netlink.Link, a netlog.Debug("adding tc ingress qdisc + redirect filters") if err = addIngressQdisc(newTapDevice); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("addIngressQdisc(tap=%s) failed: %w", newTapDevice.Attrs().Name, err) } if err = addIngressQdisc(redirectLink); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("addIngressQdisc(redirect=%s) failed: %w", redirectLink.Attrs().Name, err) } + redirectQdiscAdded = true if err = addRedirectFilter(newTapDevice, redirectLink); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("addRedirectFilter(%s->%s) failed: %w", newTapDevice.Attrs().Name, redirectLink.Attrs().Name, err) } if err = addRedirectFilter(redirectLink, newTapDevice); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("addRedirectFilter(%s->%s) failed: %w", redirectLink.Attrs().Name, newTapDevice.Attrs().Name, err) } @@ -272,9 +307,11 @@ func networkSetup(tapName string, ipAddress string, redirectLink netlink.Link, a netlog.Debugf("assigning IP %s to %s", ipAddress, newTapDevice.Attrs().Name) ipn, err := netlink.ParseAddr(ipAddress) if err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("ParseAddr(%s) failed: %w", ipAddress, err) } if err = netlink.AddrReplace(newTapDevice, ipn); err != nil { + removeTap(newTapDevice, redirectLink, redirectQdiscAdded) return nil, fmt.Errorf("AddrReplace(%s, %s) failed: %w", newTapDevice.Attrs().Name, ipAddress, err) } diff --git a/pkg/network/network_dynamic.go b/pkg/network/network_dynamic.go index 1d6a2237b..3c1bac106 100644 --- a/pkg/network/network_dynamic.go +++ b/pkg/network/network_dynamic.go @@ -59,6 +59,11 @@ func (n DynamicNetwork) NetworkSetup(uid uint32, gid uint32) (*UnikernelNetworkI netlog.Debugf("fetching info for %s", redirectLink.Attrs().Name) ifInfo, err := getInterfaceInfo(redirectLink.Attrs().Name) if err != nil { + // networkSetup succeeded above, so the tap and its TC rules exist + // in the netns and must be torn down before returning the error, + // otherwise the leftover tap blocks every future setup attempt in + // this netns (see getTapIndex). + removeTap(newTapDevice, redirectLink, true) return nil, fmt.Errorf("getInterfaceInfo(%s) failed: %w", redirectLink.Attrs().Name, err) } diff --git a/pkg/network/network_static.go b/pkg/network/network_static.go index fa46608cc..145c34dba 100644 --- a/pkg/network/network_static.go +++ b/pkg/network/network_static.go @@ -102,6 +102,11 @@ func (n StaticNetwork) NetworkSetup(uid uint32, gid uint32) (*UnikernelNetworkIn } err = setNATRule(redirectLink.Attrs().Name, StaticIPAddr) if err != nil { + // networkSetup succeeded above, so the tap it created is now + // leftover in the netns unless we remove it here. addTCRules is + // false for the static manager, so no TC rule on redirectLink to + // undo, just the tap device itself. + removeTap(newTapDevice, redirectLink, addTCRules) return nil, err } return &UnikernelNetworkInfo{