Merge pull request #53237 from corhere/claude/nervous-swirles-f408d7

daemon/libnetwork/osl: fix panic on repeated interface removal

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())

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)
+}