From fb0f15c89ad77de7b86dc5584781656dc4b44277 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 29 Jul 2026 15:03:39 -0400 Subject: [PATCH] daemon/libnetwork/osl: fix panic on repeated interface removal Namespace.RemoveInterface closes the Interface's stopCh to stop the unsolicited ARP/NA sender, but only unregisters the Interface from the Namespace once the removal has completed. Several failure exits sit in between, so a removal that fails part-way leaves the Interface registered with a closed stopCh. Callers tear interfaces down by iterating over Namespace.Interfaces, so the same *Interface is handed back to RemoveInterface during a later teardown, and the second close panics, taking down the API request being served: http: panic serving @: close of closed channel Seen in CI while disconnecting an endpoint during Sandbox.Refresh: LinkSetNsFd failed with EBADF after the link had already been renamed back to its source name, so the following re-join couldn't find the link, and the rollback in Endpoint.sbJoin removed the same Interface again. Close stopCh through a sync.Once, so the ARP/NA sender is still guaranteed to stop whether or not the netlink teardown succeeds, and however many times removal is attempted. Keep leaving a failed Interface registered in the Namespace: the link may still be present there, and generateIfaceName relies on n.iFaces to avoid handing out its name again. Co-Authored-By: Claude Opus 5 Signed-off-by: Cory Snider --- daemon/libnetwork/osl/interface_linux.go | 23 ++++++++- daemon/libnetwork/osl/interface_linux_test.go | 51 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/daemon/libnetwork/osl/interface_linux.go b/daemon/libnetwork/osl/interface_linux.go index c4dc18081c..ff2ead4129 100644 --- a/daemon/libnetwork/osl/interface_linux.go +++ b/daemon/libnetwork/osl/interface_linux.go @@ -12,6 +12,7 @@ import ( "sort" "strconv" "strings" + "sync" "syscall" "time" @@ -92,7 +93,13 @@ func newInterface(ns *Namespace, srcName, dstPrefix, dstName string, options ... // host namespace to DstName in a different net namespace with the appropriate // network settings. type Interface struct { - stopCh chan struct{} // stopCh is closed before the interface is deleted. + // stopCh is closed before the interface is deleted, to stop background tasks + // that outlive [Namespace.AddInterface] (the unsolicited ARP/NA sends). Close + // it via stop(), removal can be attempted more than once for the same + // Interface. + stopCh chan struct{} + stopOnce sync.Once + srcName string dstPrefix string dstName string @@ -172,6 +179,12 @@ func (i *Interface) Remove() error { return nameSpace.RemoveInterface(i) } +// stop stops the interface's background tasks. It is safe to call more than +// once, and from multiple goroutines. +func (i *Interface) stop() { + i.stopOnce.Do(func() { close(i.stopCh) }) +} + // Statistics returns the sandbox's side veth interface statistics. func (i *Interface) Statistics() (*types.InterfaceStatistics, error) { l, err := i.ns.nlHandle.LinkByName(i.DstName()) @@ -818,8 +831,14 @@ func (n *Namespace) prepAdvertiseAddrs(ctx context.Context, i *Interface, ifInde // RemoveInterface removes an interface from the namespace by renaming to // original name and moving it out of the sandbox. +// +// On failure, i is left in the Namespace's list of interfaces - the interface +// may still be present in the namespace, and its name is still reserved. So, +// callers that iterate over [Namespace.Interfaces] will try to remove i again +// during a later teardown. RemoveInterface must therefore tolerate being called +// more than once for the same Interface. func (n *Namespace) RemoveInterface(i *Interface) error { - close(i.stopCh) + i.stop() // Find the network interface identified by the DstName attribute. iface, err := n.nlHandle.LinkByName(i.DstName()) diff --git a/daemon/libnetwork/osl/interface_linux_test.go b/daemon/libnetwork/osl/interface_linux_test.go index d9781a4d64..0ab725c73b 100644 --- a/daemon/libnetwork/osl/interface_linux_test.go +++ b/daemon/libnetwork/osl/interface_linux_test.go @@ -14,6 +14,7 @@ import ( "github.com/vishvananda/netlink" "github.com/vishvananda/netns" "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" ) func TestGenerateIfaceName(t *testing.T) { @@ -91,3 +92,53 @@ func TestAddInterfaceInParallel(t *testing.T) { sort.Strings(eths) assert.DeepEqual(t, eths, []string{"eth0", "eth1", "eth2", "eth3", "eth4", "eth5", "eth6", "eth7", "eth8", "eth9"}) } + +// TestRemoveInterfaceTwice checks that a second attempt to remove an interface +// returns an error instead of panicking. +// +// A failed RemoveInterface leaves the Interface in the Namespace. Callers +// tear interfaces down by iterating over [Namespace.Interfaces], so the same +// *Interface is passed to RemoveInterface again during a later teardown - for +// example, by the rollback in libnetwork's Endpoint.sbJoin after a failed +// re-join. +func TestRemoveInterfaceTwice(t *testing.T) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + nsh, err := netns.NewNamed(t.Name()) + assert.NilError(t, err) + defer netns.DeleteNamed(t.Name()) + defer nsh.Close() + + nlh, err := nlwrap.NewHandleAt(nsh) + assert.NilError(t, err) + defer nlh.Close() + + ns := &Namespace{ + path: "/run/netns/" + t.Name(), + nlHandle: nlh, + } + + assert.NilError(t, nlh.LinkAdd(&netlink.Dummy{ + LinkAttrs: netlink.LinkAttrs{Name: "dummy0"}, + })) + assert.NilError(t, ns.AddInterface(context.Background(), "dummy0", "eth", "", WithCreatedInContainer(true))) + + ifaces := ns.Interfaces() + assert.Assert(t, is.Len(ifaces, 1)) + + // Delete the link so that the first removal fails part-way through, like the + // failures seen in CI ("LinkSetNsFd failed for interface vethc9f9c7d: bad + // file descriptor"). + link, err := nlh.LinkByName(ifaces[0].DstName()) + assert.NilError(t, err) + assert.NilError(t, nlh.LinkDel(link)) + + assert.Assert(t, ns.RemoveInterface(ifaces[0]) != nil) + // The interface is still in the Namespace, so it'll be removed again later. + remaining := ns.Interfaces() + assert.Assert(t, is.Len(remaining, 1)) + assert.Equal(t, remaining[0], ifaces[0]) + + assert.Assert(t, ns.RemoveInterface(ifaces[0]) != nil) +}