loader: clean up tcx bpf_links created by newer Cilium versions

A follow-up commit will introduce attaching TC programs using tcx. Those
attachments cannot be overridden using netlink. If an older version of
Cilium wants to replace an TC program on a managed interface, it'll need to
remove the tcx attachment first.

This commit teaches the agent to remove leftover tcx link objects from previous
installs, before reattaching it using netlink. Note that this transition is
never seamless, since some time passes between deleting the link and attaching
the new program using netlink. However, as explained in 7a8e3c8
("loader: clean up XDP bpf_links created by newer Cilium versions"), this
downgrade path should rarely happen.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
Co-authored-by: Timo Beckers <timo@isovalent.com>

pkg/bpf/bpffs_linux.go

@@ -6,6 +6,7 @@
 package bpf
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -64,6 +65,15 @@ func MkdirBPF(path string) error {
 	return os.MkdirAll(path, 0755)
 }
 
+// Remove path ignoring ErrNotExist.
+func Remove(path string) error {
+	err := os.RemoveAll(path)
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		return fmt.Errorf("removing bpffs directory at %s: %w", path, err)
+	}
+	return err
+}
+
 func tcPathFromMountInfo(name string) string {
 	readMountInfo.Do(func() {
 		mountInfos, err := mountinfo.GetMountInfo()

pkg/datapath/loader/base.go

@@ -209,11 +209,17 @@ func (l *loader) reinitializeIPSec(ctx context.Context) error {
 			log.WithError(err).WithField(logfields.Interface, iface).Warn("Rpfilter could not be disabled, node to node encryption may fail")
 		}
 
+		device, err := netlink.LinkByName(iface)
+		if err != nil {
+			return fmt.Errorf("retrieving device %s: %w", iface, err)
+		}
+
 		finalize, err := replaceDatapath(ctx,
 			replaceDatapathOptions{
 				device:   iface,
 				elf:      networkObj,
 				programs: progs,
+				linkDir:  bpffsDeviceLinksDir(bpf.CiliumPath(), device),
 			},
 		)
 		if err != nil {

pkg/datapath/loader/loader.go

@@ -384,6 +384,11 @@ func (l *loader) reloadHostDatapath(ctx context.Context, ep datapath.Endpoint, o
 	// missing all tail calls.
 
 	// Replace programs on cilium_host.
+	host, err := netlink.LinkByName(ep.InterfaceName())
+	if err != nil {
+		return fmt.Errorf("retrieving device %s: %w", ep.InterfaceName(), err)
+	}
+
 	progs := []progDefinition{
 		{progName: symbolToHostEp, direction: dirIngress},
 		{progName: symbolFromHostEp, direction: dirEgress},
@@ -393,6 +398,7 @@ func (l *loader) reloadHostDatapath(ctx context.Context, ep datapath.Endpoint, o
 			device:   ep.InterfaceName(),
 			elf:      objPath,
 			programs: progs,
+			linkDir:  bpffsDeviceLinksDir(bpf.CiliumPath(), host),
 		},
 	)
 	if err != nil {
@@ -412,7 +418,8 @@ func (l *loader) reloadHostDatapath(ctx context.Context, ep datapath.Endpoint, o
 	defer finalize()
 
 	// Replace program on cilium_net.
-	if _, err := netlink.LinkByName(defaults.SecondHostDevice); err != nil {
+	net, err := netlink.LinkByName(defaults.SecondHostDevice)
+	if err != nil {
 		log.WithError(err).WithField("device", defaults.SecondHostDevice).Error("Link does not exist")
 		return fmt.Errorf("device '%s' not found: %w", defaults.SecondHostDevice, err)
 	}
@@ -431,6 +438,7 @@ func (l *loader) reloadHostDatapath(ctx context.Context, ep datapath.Endpoint, o
 			device:   defaults.SecondHostDevice,
 			elf:      secondDevObjPath,
 			programs: progs,
+			linkDir:  bpffsDeviceLinksDir(bpf.CiliumPath(), net),
 		},
 	)
 	if err != nil {
@@ -447,7 +455,8 @@ func (l *loader) reloadHostDatapath(ctx context.Context, ep datapath.Endpoint, o
 
 	// Replace programs on physical devices.
 	for _, device := range option.Config.GetDevices() {
-		if _, err := netlink.LinkByName(device); err != nil {
+		iface, err := netlink.LinkByName(device)
+		if err != nil {
 			log.WithError(err).WithField("device", device).Warn("Link does not exist")
 			continue
 		}
@@ -482,6 +491,7 @@ func (l *loader) reloadHostDatapath(ctx context.Context, ep datapath.Endpoint, o
 				device:   device,
 				elf:      netdevObjPath,
 				programs: progs,
+				linkDir:  bpffsDeviceLinksDir(bpf.CiliumPath(), iface),
 			},
 		)
 		if err != nil {
@@ -537,6 +547,7 @@ func (l *loader) reloadDatapath(ctx context.Context, ep datapath.Endpoint, dirs
 				device:   ep.InterfaceName(),
 				elf:      objPath,
 				programs: progs,
+				linkDir:  bpffsEndpointLinksDir(bpf.CiliumPath(), ep),
 			},
 		)
 		if err != nil {
@@ -579,6 +590,11 @@ func (l *loader) replaceOverlayDatapath(ctx context.Context, cArgs []string, ifa
 		log.WithError(err).Fatal("failed to compile overlay programs")
 	}
 
+	device, err := netlink.LinkByName(iface)
+	if err != nil {
+		return fmt.Errorf("retrieving device %s: %w", iface, err)
+	}
+
 	progs := []progDefinition{
 		{progName: symbolFromOverlay, direction: dirIngress},
 		{progName: symbolToOverlay, direction: dirEgress},
@@ -589,6 +605,7 @@ func (l *loader) replaceOverlayDatapath(ctx context.Context, cArgs []string, ifa
 			device:   iface,
 			elf:      overlayObj,
 			programs: progs,
+			linkDir:  bpffsDeviceLinksDir(bpf.CiliumPath(), device),
 		},
 	)
 	if err != nil {
@@ -722,6 +739,15 @@ func (l *loader) Unload(ep datapath.Endpoint) {
 			removeEndpointRoute(ep, *iputil.AddrToIPNet(ip))
 		}
 	}
+
+	// If Cilium and the kernel support tcx to attach TC programs to the
+	// endpoint's veth device, its bpf_link object is pinned to a per-endpoint
+	// bpffs directory. When the endpoint gets deleted, removing the whole
+	// directory cleans up any pinned maps and links.
+	bpffsPath := bpffsEndpointDir(bpf.CiliumPath(), ep)
+	if err := bpf.Remove(bpffsPath); err != nil {
+		log.WithError(err).WithField(logfields.EndpointID, ep.StringID())
+	}
 }
 
 // EndpointHash hashes the specified endpoint configuration with the current

pkg/datapath/loader/loader_test.go

@@ -184,23 +184,17 @@ func (s *LoaderTestSuite) TestReload(c *C) {
 		{progName: symbolFromEndpoint, direction: dirIngress},
 		{progName: symbolToEndpoint, direction: dirEgress},
 	}
-	finalize, err := replaceDatapath(ctx,
-		replaceDatapathOptions{
-			device:   ep.InterfaceName(),
-			elf:      objPath,
-			programs: progs,
-		},
-	)
+	opts := replaceDatapathOptions{
+		device:   ep.InterfaceName(),
+		elf:      objPath,
+		programs: progs,
+		linkDir:  testutils.TempBPFFS(c),
+	}
+	finalize, err := replaceDatapath(ctx, opts)
 	c.Assert(err, IsNil)
 	finalize()
 
-	finalize, err = replaceDatapath(ctx,
-		replaceDatapathOptions{
-			device:   ep.InterfaceName(),
-			elf:      objPath,
-			programs: progs,
-		},
-	)
+	finalize, err = replaceDatapath(ctx, opts)
 
 	c.Assert(err, IsNil)
 	finalize()
@@ -343,6 +337,7 @@ func BenchmarkReplaceDatapath(b *testing.B) {
 	}
 
 	objPath := fmt.Sprintf("%s/%s", dirInfo.Output, endpointObj)
+	linkDir := testutils.TempBPFFS(b)
 	progs := []progDefinition{{progName: symbolFromEndpoint, direction: dirIngress}}
 	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
@@ -351,6 +346,7 @@ func BenchmarkReplaceDatapath(b *testing.B) {
 				device:   ep.InterfaceName(),
 				elf:      objPath,
 				programs: progs,
+				linkDir:  linkDir,
 			},
 		)
 		if err != nil {

pkg/datapath/loader/netlink.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"net"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/vishvananda/netlink"
@@ -66,6 +67,7 @@ type replaceDatapathOptions struct {
 	elf      string           // path to object file
 	programs []progDefinition // programs that we want to attach/replace
 	xdpMode  string           // XDP driver mode, only applies when attaching XDP programs
+	linkDir  string           // path to bpffs dir holding bpf_links for the device/endpoint
 }
 
 // replaceDatapath replaces the qdisc and BPF program for an endpoint or XDP program.
@@ -87,6 +89,10 @@ func replaceDatapath(ctx context.Context, opts replaceDatapathOptions) (_ func()
 		return nil, err
 	}
 
+	if opts.linkDir == "" {
+		return nil, errors.New("opts.linkDir not set in replaceDatapath")
+	}
+
 	link, err := netlink.LinkByName(opts.device)
 	if err != nil {
 		return nil, fmt.Errorf("getting interface %s by name: %w", opts.device, err)
@@ -230,17 +236,17 @@ func replaceDatapath(ctx context.Context, opts replaceDatapathOptions) (_ func()
 	// flow in.
 	for _, prog := range opts.programs {
 		scopedLog := l.WithField("progName", prog.progName).WithField("direction", prog.direction)
-		if opts.xdpMode != "" {
-			linkDir := bpffsDeviceLinksDir(bpf.CiliumPath(), link)
-			if err := bpf.MkdirBPF(linkDir); err != nil {
-				return nil, fmt.Errorf("creating bpffs link dir for device %s: %w", link.Attrs().Name, err)
-			}
 
+		if err := bpf.MkdirBPF(opts.linkDir); err != nil {
+			return nil, fmt.Errorf("creating bpffs link dir for device %s: %w", link.Attrs().Name, err)
+		}
+
+		if opts.xdpMode != "" {
 			scopedLog.Debug("Attaching XDP program to interface")
-			err = attachXDPProgram(link, coll.Programs[prog.progName], prog.progName, linkDir, xdpConfigModeToFlag(opts.xdpMode))
+			err = attachXDPProgram(link, coll.Programs[prog.progName], prog.progName, opts.linkDir, xdpConfigModeToFlag(opts.xdpMode))
 		} else {
 			scopedLog.Debug("Attaching TC program to interface")
-			err = attachTCProgram(link, coll.Programs[prog.progName], prog.progName, directionToParent(prog.direction))
+			err = attachTCProgram(link, coll.Programs[prog.progName], prog.progName, opts.linkDir, directionToParent(prog.direction))
 		}
 
 		if err != nil {
@@ -283,11 +289,24 @@ func resolveAndInsertCalls(coll *ebpf.Collection, mapName string, calls []ebpf.M
 }
 
 // attachTCProgram attaches the TC program 'prog' to link.
-func attachTCProgram(link netlink.Link, prog *ebpf.Program, progName string, qdiscParent uint32) error {
+func attachTCProgram(link netlink.Link, prog *ebpf.Program, progName, bpffsDir string, qdiscParent uint32) error {
 	if prog == nil {
 		return errors.New("cannot attach a nil program")
 	}
 
+	// Remove tcx bpf_links created by newer versions of Cilium. They cannot be
+	// overwritten by netlink-based tc attachments, as tcx is a separate hook
+	// altogether. Remove the tcx link first to avoid tc programs being run twice
+	// for every packet. This cannot be done seamlessly and will cause a small
+	// window of connection interruption.
+	pin := filepath.Join(bpffsDir, progName)
+	if err := os.Remove(pin); err == nil {
+		log.WithField("device", link.Attrs().Name).WithField("pinPath", pin).
+			Info("Removed tcx link before legacy tc downgrade, possible connectivity interruption")
+	} else if !errors.Is(err, os.ErrNotExist) {
+		return fmt.Errorf("unpinning defunct link %s: %w", pin, err)
+	}
+
 	if err := replaceQdisc(link); err != nil {
 		return fmt.Errorf("replacing clsact qdisc for interface %s: %w", link.Attrs().Name, err)
 	}

pkg/datapath/loader/netlink_test.go

@@ -370,7 +370,8 @@ func TestAttachRemoveTCProgram(t *testing.T) {
 
 		prog := mustTCProgram(t)
 
-		err = attachTCProgram(dummy, prog, "test", directionToParent(dirEgress))
+		bpffs := testutils.TempBPFFS(t)
+		err = attachTCProgram(dummy, prog, "test", bpffsDeviceLinksDir(bpffs, dummy), directionToParent(dirEgress))
 		require.NoError(t, err)
 
 		filters, err := netlink.FilterList(dummy, directionToParent(dirEgress))

pkg/datapath/loader/paths.go

@@ -7,6 +7,8 @@ import (
 	"path/filepath"
 
 	"github.com/vishvananda/netlink"
+
+	datapath "github.com/cilium/cilium/pkg/datapath/types"
 )
 
 // bpffsDevicesDir returns the path to the 'devices' directory on bpffs, usually
@@ -18,12 +20,50 @@ func bpffsDevicesDir(base string) string {
 	return filepath.Join(base, "devices")
 }
 
+// bpffsDeviceDir returns the path to the per-device directory on bpffs, usually
+// /sys/fs/bpf/cilium/devices/<device>. It does not ensure the directory exists.
+//
+// base is typically set to /sys/fs/bpf/cilium, but can be a temp directory
+// during tests.
+func bpffsDeviceDir(base string, device netlink.Link) string {
+	return filepath.Join(bpffsDevicesDir(base), device.Attrs().Name)
+}
+
 // bpffsDeviceLinksDir returns the bpffs path to the per-device links directory,
 // usually /sys/fs/bpf/cilium/devices/<device>/links. It does not ensure the
 // directory exists.
 //
 // base is typically set to /sys/fs/bpf/cilium, but can be a temp directory
 // during tests.
 func bpffsDeviceLinksDir(base string, device netlink.Link) string {
-	return filepath.Join(bpffsDevicesDir(base), device.Attrs().Name, "links")
+	return filepath.Join(bpffsDeviceDir(base, device), "links")
+}
+
+// bpffsEndpointsDir returns the path to the 'endpoints' directory on bpffs, usually
+// /sys/fs/bpf/cilium/endpoints. It does not ensure the directory exists.
+//
+// base is typically set to /sys/fs/bpf/cilium, but can be a temp directory
+// during tests.
+func bpffsEndpointsDir(base string) string {
+	return filepath.Join(base, "endpoints")
+}
+
+// bpffsEndpointDir returns the path to the per-endpoint directory on bpffs,
+// usually /sys/fs/bpf/cilium/endpoints/<endpoint-id>. It does not ensure the
+// directory exists.
+//
+// base is typically set to /sys/fs/bpf/cilium, but can be a temp directory
+// during tests.
+func bpffsEndpointDir(base string, ep datapath.Endpoint) string {
+	return filepath.Join(bpffsEndpointsDir(base), ep.StringID())
+}
+
+// bpffsEndpointLinksDir returns the bpffs path to the per-endpoint links directory,
+// usually /sys/fs/bpf/cilium/endpoints/<endpoint-id>/links. It does not ensure the
+// directory exists.
+//
+// base is typically set to /sys/fs/bpf/cilium, but can be a temp directory
+// during tests.
+func bpffsEndpointLinksDir(base string, ep datapath.Endpoint) string {
+	return filepath.Join(bpffsEndpointDir(base, ep), "links")
 }

pkg/datapath/loader/xdp.go

@@ -154,13 +154,19 @@ func compileAndLoadXDPProg(ctx context.Context, xdpDev, xdpMode string, extraCAr
 		return err
 	}
 
+	iface, err := netlink.LinkByName(xdpDev)
+	if err != nil {
+		return fmt.Errorf("retrieving device %s: %w", xdpDev, err)
+	}
+
 	progs := []progDefinition{{progName: symbolFromHostNetdevXDP, direction: ""}}
 	finalize, err := replaceDatapath(ctx,
 		replaceDatapathOptions{
 			device:   xdpDev,
 			elf:      objPath,
 			programs: progs,
 			xdpMode:  xdpMode,
+			linkDir:  bpffsDeviceLinksDir(bpf.CiliumPath(), iface),
 		},
 	)
 	if err != nil {