pkg/sensors: cleanup kprobe entry from table on destroy

[upstream commit 5b31544]

Commit 310846e introduced a cleanup of
the kprobe table entry on unload, which introduced some issues because
unload is using when disabling sensors (that might be re-enabled later).
Previous pre and post-hooks are still useful for resources management on
unload.

The kprobe table entry is added on creation (not load) and then it must
be cleaned up on deletion introducing the new sensor.Destroy() method.
'Destroy' expresses the idea that the sensor is not usable past this
point and must be recreated to be loaded.

Signed-off-by: Mahe Tardy <[email protected]>

pkg/sensors/collection.go

@@ -80,3 +80,10 @@ func (c *collection) unload() error {
 	}
 	return nil
 }
+
+// destroy will attempt to destroy all the sensors in a collection
+func (c *collection) destroy() {
+	for _, s := range c.sensors {
+		s.Destroy()
+	}
+}

pkg/sensors/exec/cgroups_test.go

@@ -70,11 +70,6 @@ const (
 )
 
 var (
-	loadedSensors = []*sensors.Sensor{
-		testsensor.GetTestSensor(),
-		testsensor.GetCgroupSensor(),
-	}
-
 	defaultKubeCgroupHierarchy = []cgroupHierarchy{
 		{"tetragon-tests-39d631b6f0fbc4e261c7a0ee636cf434-defaultKubeCgroupHierarchy-system.slice", true, false, false, false, 0, 0, "", "", nil},
 		{"kubelet.slice", true, false, false, false, 0, 0, "", "", nil},
@@ -95,6 +90,13 @@ var (
 	}
 )
 
+func getLoadedSensors() []*sensors.Sensor {
+	return []*sensors.Sensor{
+		testsensor.GetTestSensor(),
+		testsensor.GetCgroupSensor(),
+	}
+}
+
 func getTrackingLevel(cgroupHierarchy []cgroupHierarchy) uint32 {
 	level := 0
 	for i, cgroup := range cgroupHierarchy {
@@ -651,7 +653,7 @@ func TestCgroupNoEvents(t *testing.T) {
 
 	testManager := setupObserver(ctx, t)
 
-	testManager.AddAndEnableSensors(ctx, t, loadedSensors)
+	testManager.AddAndEnableSensors(ctx, t, getLoadedSensors())
 
 	// Set Cgroup Tracking level to Zero means no tracking and no
 	// cgroup events, all bpf cgroups related programs have no effect
@@ -703,9 +705,9 @@ func TestCgroupEventMkdirRmdir(t *testing.T) {
 
 	testManager := setupObserver(ctx, t)
 
-	testManager.AddAndEnableSensors(ctx, t, loadedSensors)
+	testManager.AddAndEnableSensors(ctx, t, getLoadedSensors())
 	t.Cleanup(func() {
-		testManager.DisableSensors(ctx, t, loadedSensors)
+		testManager.DisableSensors(ctx, t, getLoadedSensors())
 	})
 
 	// Set Tracking level to 3 so we receive notifcations about
@@ -882,9 +884,9 @@ func testCgroupv2HierarchyInUnified(ctx context.Context, t *testing.T,
 func testCgroupv2K8sHierarchy(ctx context.Context, t *testing.T, mode cgroups.CgroupModeCode, withExec bool) {
 	testManager := setupObserver(ctx, t)
 
-	testManager.AddAndEnableSensors(ctx, t, loadedSensors)
+	testManager.AddAndEnableSensors(ctx, t, getLoadedSensors())
 	t.Cleanup(func() {
-		testManager.DisableSensors(ctx, t, loadedSensors)
+		testManager.DisableSensors(ctx, t, getLoadedSensors())
 	})
 
 	_, err := testutils.GetTgRuntimeConf()
@@ -1096,7 +1098,7 @@ func testCgroupv1K8sHierarchyInHybrid(t *testing.T, withExec bool, selectedContr
 
 	testManager := setupObserver(ctx, t)
 
-	testManager.AddAndEnableSensors(ctx, t, loadedSensors)
+	testManager.AddAndEnableSensors(ctx, t, getLoadedSensors())
 
 	// Probe full environment detection
 	_, err := testutils.GetTgRuntimeConf()

pkg/sensors/handler.go

@@ -135,20 +135,17 @@ func (h *handler) deleteTracingPolicy(op *tracingPolicyDelete) error {
 	if !exists {
 		return fmt.Errorf("tracing policy %s does not exist", op.name)
 	}
-	err := col.unload()
-	if err != nil {
-		col.err = fmt.Errorf("failed to unload tracing policy: %w", err)
-		return err
-	}
+	defer delete(h.collections, op.name)
+
+	col.destroy()
 
 	filterID := policyfilter.PolicyID(col.policyfilterID)
-	err = h.pfState.DelPolicy(filterID)
+	err := h.pfState.DelPolicy(filterID)
 	if err != nil {
 		col.err = fmt.Errorf("failed to remove from policyfilter: %w", err)
 		return err
 	}
 
-	delete(h.collections, op.name)
 	return nil
 }
 
@@ -197,9 +194,10 @@ func (h *handler) removeSensor(op *sensorRemove) error {
 	if !exists {
 		return fmt.Errorf("sensor %s does not exist", op.name)
 	}
-	err := col.unload()
+
+	col.destroy()
 	delete(h.collections, op.name)
-	return err
+	return nil
 }
 
 func (h *handler) enableSensor(op *sensorEnable) error {

pkg/sensors/load.go

@@ -69,6 +69,10 @@ func (s *Sensor) Load(bpfDir, mapDir, ciliumDir string) error {
 		return nil
 	}
 
+	if s.Destroyed {
+		return fmt.Errorf("sensor %s has been previously destroyed, please recreate it before loading", s.Name)
+	}
+
 	// Add the loaded programs and maps to All* so they can be unloaded on shutdown.
 	AllPrograms = append(AllPrograms, s.Progs...)
 	AllMaps = append(AllMaps, s.Maps...)
@@ -149,6 +153,25 @@ func (s *Sensor) Unload() error {
 	return nil
 }
 
+// Destroy will unload the hook and call DestroyHook, this hook is usually used
+// to clean up resources that were created during creation of the sensor.
+func (s *Sensor) Destroy() {
+	err := s.Unload()
+	if err != nil {
+		// do not return on error but just log since Unload can only error on
+		// sensor being already not loaded
+		logger.GetLogger().WithError(err).WithField("sensor", s.Name).Warn("Unload failed during destroy")
+	}
+
+	if s.DestroyHook != nil {
+		err = s.DestroyHook()
+		if err != nil {
+			logger.GetLogger().WithError(err).WithField("sensor", s.Name).Warn("Destroy hook failed")
+		}
+	}
+	s.Destroyed = true
+}
+
 func (s *Sensor) findProgram(p *program.Program) error {
 	logger.GetLogger().WithField("file", p.Name).Debug("Checking for bpf file")
 	if _, err := os.Stat(p.Name); err == nil {

pkg/sensors/sensors.go

@@ -41,19 +41,25 @@ type Sensor struct {
 	Maps []*program.Map
 	// Loaded indicates whether the sensor has been Loaded.
 	Loaded bool
+	// Destroyed indicates whether the sensor had been destroyed.
+	Destroyed bool
 	// PreUnloadHook can optionally contain a pointer to a function to be
 	// called during sensor unloading, prior to the programs and maps being
 	// unloaded.
-	PreUnloadHook SensorUnloadHook
+	PreUnloadHook SensorHook
 	// PostUnloadHook can optionally contain a pointer to a function to be
 	// called during sensor unloading, after the programs and maps being
 	// unloaded.
-	PostUnloadHook SensorUnloadHook
+	PostUnloadHook SensorHook
+	// DestroyHook can optionally contain a pointer to a function to be called
+	// when removing the sensor, sensor cannot be loaded again after this hook
+	// being triggered and must be recreated.
+	DestroyHook SensorHook
 }
 
-// SensorUnloadHook is the function signature for an optional function
-// that can be called during sensor unloading.
-type SensorUnloadHook func() error
+// SensorHook is the function signature for an optional function
+// that can be called during sensor unloading and removing.
+type SensorHook func() error
 
 func SensorCombine(name string, sensors ...*Sensor) *Sensor {
 	progs := []*program.Program{}

pkg/sensors/tracing/generickprobe.go

@@ -489,7 +489,7 @@ func createGenericKprobeSensor(
 		Name:  name,
 		Progs: progs,
 		Maps:  maps,
-		PostUnloadHook: func() error {
+		DestroyHook: func() error {
 			var errs error
 			for _, idx := range addedKprobeIndices {
 				_, err := genericKprobeTable.RemoveEntry(idtable.EntryID{ID: idx})

pkg/sensors/tracing/generickprobe_test.go

@@ -54,15 +54,15 @@ func Test_parseString(t *testing.T) {
 	})
 }
 
-func Test_SensorPostUnloadHook(t *testing.T) {
+func Test_SensorDestroyHook(t *testing.T) {
 	if genericKprobeTable.Len() != 0 {
 		t.Errorf("genericKprobeTable expected initial length: 0, got: %d", genericKprobeTable.Len())
 	}
 
-	// we use createGenericKprobeSensor because it's where the PostUnloadHook is
+	// we use createGenericKprobeSensor because it's where the DestroyHook is
 	// created. It would be technically more correct if it was added just after
 	// insertion in the table in AddKprobe, but this is done by the caller to
-	// have just PostUnloadHook that regroups all the potential multiple kprobes
+	// have just DestroyHook that regroups all the potential multiple kprobes
 	// contained in one sensor.
 	sensor, err := createGenericKprobeSensor("test_sensor", []v1alpha1.KProbeSpec{
 		{
@@ -91,12 +91,9 @@ func Test_SensorPostUnloadHook(t *testing.T) {
 	// never loaded
 	sensor.Loaded = true
 
-	// Unload should call the PostUnloadHook that was set in
+	// Destroy should call the DestroyHook that was set in
 	// createGenericKprobeSensor and do the cleanup
-	err = sensor.Unload()
-	if err != nil {
-		t.Errorf("sensor.Unload err expected: nil, got: %s", err)
-	}
+	sensor.Destroy()
 
 	// Table implem detail: the entry still technically exists in the table but
 	// is invalid, thus is not taken into account in the length