bugfix: matchBinaries in multiple selectors

Using the following tracing policy:
kprobes:
- call: "fd_install"
  syscall: false
  args:
  - index: 0
    type: int
  - index: 1
    type: "file"
  selectors:
  - matchBinaries:
    - operator: "In"
      values:
      - "/usr/bin/cat"
    matchArgs:
    - index: 1
      operator: "Equal"
      values:
      - "/etc/passwd"
  - matchBinaries:
    - operator: "In"
      values:
      - "/usr/bin/tail"
    matchArgs:
    - index: 1
      operator: "Equal"
      values:
      - "/etc/shadow"

we expect to get events if:
[ binary == /usr/bin/cat AND arg1 == /etc/passwd ] OR [ binary ==
/usr/bin/tail AND arg1 == /etc/shadow ]

Using the previous tracing policy and running:
/usr/bin/cat /etc/passwd && /usr/bin/cat /etc/shadow && /usr/bin/tail /etc/passwd && /usr/bin/tail /etc/shadow

We get events for all of these.

The issue is that in #686 we use
a single map for all matchBinaries selectors. That makes the previous
tracing policy to behave as:
[ (binary == /usr/bin/cat OR binary ==
/usr/bin/tail) AND arg1 == /etc/passwd ] OR [ (binary == /usr/bin/cat OR binary ==
/usr/bin/tail) AND arg1 == /etc/shadow ]

This patch fixes that issues. We move from a BPF_MAP_TYPE_HASH
to a BPF_MAP_TYPE_HASH_OF_MAPS. We index the outter map based on
selector ID and the inner map is exactly the same as the one we
used in #686.

Based on the previous tracing policy, the user side will generate the names_map and
sel_names_map. names_map is shared among all selectors that have matchBinaries. Each
distinct binary name will appear in the names_map with a unique value per entry (> 0).
The contents of the names_map for the previous tracing policy will be:

names_map["/usr/bin/cat"] = 1
names_map["/usr/bin/tail"] = 2

Whenever we have an execve event, we check the execve binary name and if is that
inside names_map, we set execve_map_value->binary to the value of that entry. If
we cannot find that entry inside names_map, we set execve_map_value->binary equals
to 0 which means that this binary name is nowhere in all matchBinaries.

Based in that we also generate the sel_names_map. This is a BPF_MAP_TYPE_HASH_OF_MAPS
map where we index in the outter map using the selector ID and we index in the inner
map using the execve_map_value->binary. If that exists, then matchBinary will be
matched.

The contents of the sel_names_map based on the previous tracing policy will be:
sel_names_map[0 /* sel_index */ ] = {
  hash_map[1 /* value of cat in names_map */] = 1 /* always 1 for now */
}
sel_names_map[1 /* sel_index */ ] = {
  hash_map[2 /* value of tail in names_map */] = 1 /* always 1 for now */
}

Finally, if sel_names_map[sel_idx] is NULL this means that the specific selector
does not container a matchBinaries action. If sel_names_map[sel_idx].hash_map[idx]
is NULL this means that we don't care about the specific binary.

Signed-off-by: Anastasios Papagiannis <tasos.papagiannnis@gmail.com>

bpf/process/types/basic.h

@@ -155,6 +155,12 @@ static inline __attribute__((always_inline)) __u32 get_index(void *ctx)
 #define MAX_ENTRIES_CONFIG 1
 #endif
 
+// Filter tailcalls are {kprobe,tracepoint}/{6,7,8,9,10}
+// We do one tail-call per selector, so we can have up to 5 selectors.
+#define MIN_FILTER_TAILCALL 6
+#define MAX_FILTER_TAILCALL 10
+#define MAX_SELECTORS	    (MAX_FILTER_TAILCALL - MIN_FILTER_TAILCALL + 1)
+
 static inline __attribute__((always_inline)) bool ty_is_nop(int ty)
 {
 	switch (ty) {
@@ -938,7 +944,10 @@ static inline __attribute__((always_inline)) size_t type_to_min_size(int type,
 /*
  * For matchBinaries we use two maps:
  * 1. names_map: global (for all sensors) keeps a mapping from names -> ids
- * 2. sel_names_map: per-sensor: keeps a mapping from id -> selector val
+ * 2. sel_names_map: per-sensor: keeps a mapping from selector_id -> id -> selector val
+ *
+ * For each selector we have a separate inner map. We choose the appropriate
+ * inner map based on the selector ID.
  *
  * At exec time, we check names_map and set ->binary in execve_map equal to
  * the id stored in names_map. Assuming the binary name exists in the map,
@@ -948,20 +957,25 @@ static inline __attribute__((always_inline)) size_t type_to_min_size(int type,
  * whether the selector matches or not.
  */
 struct {
-	__uint(type, BPF_MAP_TYPE_HASH);
-	__uint(max_entries, 256);
-	__type(key, __u32);
-	__type(value, __u32);
+	__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
+	__uint(max_entries, MAX_SELECTORS);
+	__uint(key_size, sizeof(u32)); /* selector id */
+	__array(
+		values, struct {
+			__uint(type, BPF_MAP_TYPE_HASH);
+			__uint(max_entries, 256);
+			__type(key, __u32);
+			__type(value, __u32);
+		});
 } sel_names_map SEC(".maps");
 
 static inline __attribute__((always_inline)) int
 selector_arg_offset(__u8 *f, struct msg_generic_kprobe *e, __u32 selidx)
 {
 	struct selector_arg_filter *filter;
 	long seloff, argoff, pass;
-	__u32 index, *op;
-	char *args;
-	__u32 max = 0xffffffff; // UINT32_MAX
+	__u32 index;
+	char *args, *binaries_map;
 
 	seloff = 4; /* start of the relative offsets */
 	seloff += (selidx * 4); /* relative offset for this selector */
@@ -982,32 +996,40 @@ selector_arg_offset(__u8 *f, struct msg_generic_kprobe *e, __u32 selidx)
 	/* skip the matchCapabilityChanges by reading its length */
 	seloff += *(__u32 *)((__u64)f + (seloff & INDEX_MASK));
 
-	op = map_lookup_elem(&sel_names_map, &max);
-	if (op) {
-		struct execve_map_value *execve;
-		bool walker = 0;
-		__u32 ppid, bin_key, *bin_val;
-
-		execve = event_find_curr(&ppid, &walker);
-		if (!execve)
-			return 0;
+	// if binaries_map is NULL for the specific selidx, this
+	// means that the specific selector does not contain any
+	// matchBinaries actions. So we just proceed.
+	binaries_map = map_lookup_elem(&sel_names_map, &selidx);
+	if (binaries_map) {
+		__u32 *op, max = 0xffffffff; // UINT32_MAX
+
+		op = map_lookup_elem(binaries_map, &max);
+		if (op) {
+			struct execve_map_value *execve;
+			bool walker = 0;
+			__u32 ppid, bin_key, *bin_val;
+
+			execve = event_find_curr(&ppid, &walker);
+			if (!execve)
+				return 0;
 
-		bin_key = execve->binary;
-		bin_val = map_lookup_elem(&sel_names_map, &bin_key);
+			bin_key = execve->binary;
+			bin_val = map_lookup_elem(binaries_map, &bin_key);
 
-		/*
-		 * The following things may happen:
-		 * binary is not part of names_map, execve_map->binary will be `0` and `bin_val` will always be `0`
-		 * binary is part of `names_map`:
-		 *  if binary is not part of this selector, bin_val will be`0`
-		 *  if binary is part of this selector: `bin_val will be `!0`
-		 */
-		if (*op == op_filter_in) {
-			if (!bin_val)
-				return 0;
-		} else if (*op == op_filter_notin) {
-			if (bin_val)
-				return 0;
+			/*
+			 * The following things may happen:
+			 * binary is not part of names_map, execve_map->binary will be `0` and `bin_val` will always be `0`
+			 * binary is part of `names_map`:
+			 *  if binary is not part of this selector, bin_val will be`0`
+			 *  if binary is part of this selector: `bin_val will be `!0`
+			 */
+			if (*op == op_filter_in) {
+				if (!bin_val)
+					return 0;
+			} else if (*op == op_filter_notin) {
+				if (bin_val)
+					return 0;
+			}
 		}
 	}
 
@@ -1302,12 +1324,6 @@ do_actions(struct msg_generic_kprobe *e, struct selector_action *actions,
 	return i > 0 ? true : 0;
 }
 
-// Filter tailcalls are {kprobe,tracepoint}/{6,7,8,9,10}
-// We do one tail-call per selector, so we can have up to 5 selectors.
-#define MIN_FILTER_TAILCALL 6
-#define MAX_FILTER_TAILCALL 10
-#define MAX_SELECTORS	    (MAX_FILTER_TAILCALL - MIN_FILTER_TAILCALL + 1)
-
 static inline __attribute__((always_inline)) long
 filter_read_arg(void *ctx, int index, struct bpf_map_def *heap,
 		struct bpf_map_def *filter, struct bpf_map_def *tailcalls,

pkg/selectors/kernel.go

@@ -632,30 +632,30 @@ func ParseMatchCapabilityChanges(k *KernelSelectorState, actions []v1alpha1.Capa
 	return nil
 }
 
-func ParseMatchBinary(k *KernelSelectorState, b *v1alpha1.BinarySelector) error {
+func ParseMatchBinary(k *KernelSelectorState, b *v1alpha1.BinarySelector, selIdx int) error {
 	op, err := SelectorOp(b.Operator)
 	if err != nil {
 		return fmt.Errorf("matchBinary error: %w", err)
 	}
 	if op != SelectorOpIn && op != SelectorOpNotIn {
 		return fmt.Errorf("matchBinary error: Only In and NotIn operators are supported")
 	}
-	k.SetBinaryOp(op)
+	k.SetBinaryOp(selIdx, op)
 	for _, s := range b.Values {
 		if len(s) > 255 {
 			return fmt.Errorf("matchBinary error: Binary names > 255 chars do not supported")
 		}
-		k.AddBinaryName(s)
+		k.AddBinaryName(selIdx, s)
 	}
 	return nil
 }
 
-func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelector) error {
+func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelector, selIdx int) error {
 	if len(binarys) > 1 {
 		return fmt.Errorf("Only support single binary selector")
 	}
 	for _, s := range binarys {
-		if err := ParseMatchBinary(k, &s); err != nil {
+		if err := ParseMatchBinary(k, &s, selIdx); err != nil {
 			return err
 		}
 	}
@@ -665,6 +665,7 @@ func ParseMatchBinaries(k *KernelSelectorState, binarys []v1alpha1.BinarySelecto
 func parseSelector(
 	k *KernelSelectorState,
 	selectors *v1alpha1.KProbeSelector,
+	selIdx int,
 	args []v1alpha1.KProbeArg,
 	actionArgTable *idtable.Table) error {
 	if err := ParseMatchPids(k, selectors.MatchPIDs); err != nil {
@@ -682,7 +683,7 @@ func parseSelector(
 	if err := ParseMatchCapabilityChanges(k, selectors.MatchCapabilityChanges); err != nil {
 		return fmt.Errorf("parseMatchCapabilityChanges error: %w", err)
 	}
-	if err := ParseMatchBinaries(k, selectors.MatchBinaries); err != nil {
+	if err := ParseMatchBinaries(k, selectors.MatchBinaries, selIdx); err != nil {
 		return fmt.Errorf("parseMatchBinaries error: %w", err)
 	}
 	if err := ParseMatchArgs(k, selectors.MatchArgs, args); err != nil {
@@ -739,7 +740,7 @@ func InitKernelSelectors(selectors []v1alpha1.KProbeSelector, args []v1alpha1.KP
 
 func InitKernelSelectorState(selectors []v1alpha1.KProbeSelector, args []v1alpha1.KProbeArg,
 	actionArgTable *idtable.Table) (*KernelSelectorState, error) {
-	kernelSelectors := &KernelSelectorState{}
+	kernelSelectors := newKernelSelectorState()
 
 	WriteSelectorUint32(kernelSelectors, uint32(len(selectors)))
 	soff := make([]uint32, len(selectors))
@@ -749,7 +750,7 @@ func InitKernelSelectorState(selectors []v1alpha1.KProbeSelector, args []v1alpha
 	for i, s := range selectors {
 		WriteSelectorLength(kernelSelectors, soff[i])
 		loff := AdvanceSelectorLength(kernelSelectors)
-		if err := parseSelector(kernelSelectors, &s, args, actionArgTable); err != nil {
+		if err := parseSelector(kernelSelectors, &s, i, args, actionArgTable); err != nil {
 			return nil, err
 		}
 		WriteSelectorLength(kernelSelectors, loff)

pkg/selectors/selectors.go

@@ -17,55 +17,69 @@ var (
 	binVals = make(map[string]uint32)
 )
 
+type MatchBinariesMappings struct {
+	op          uint32
+	selNamesMap map[uint32]uint32 // these will be used for the sel_names_map
+}
+
+func (k *MatchBinariesMappings) GetBinSelNamesMap() map[uint32]uint32 {
+	return k.selNamesMap
+}
+
 type KernelSelectorState struct {
 	off uint32     // offset into encoding
 	e   [4096]byte // kernel encoding of selectors
 
 	// valueMaps are used to populate value maps for InMap and NotInMap operators
 	valueMaps []map[[8]byte]struct{}
 
-	// matchBinaries mappings
-	op          uint32
-	newBinVals  map[uint32]string // these should be added in the names_map
-	selNamesMap map[uint32]uint32 // these will be used for the sel_names_map
-	initOnce    sync.Once
+	matchBinaries map[int]*MatchBinariesMappings // matchBinaries mappings (one per selector)
+	newBinVals    map[uint32]string              // these should be added in the names_map
 }
 
-func (k *KernelSelectorState) SetBinaryOp(op uint32) {
-	k.op = op
+func newKernelSelectorState() *KernelSelectorState {
+	return &KernelSelectorState{
+		matchBinaries: make(map[int]*MatchBinariesMappings),
+		newBinVals:    make(map[uint32]string),
+	}
 }
 
-func (k *KernelSelectorState) GetBinaryOp() uint32 {
-	return k.op
+func (k *KernelSelectorState) SetBinaryOp(selIdx int, op uint32) {
+	// init a new entry (if needed)
+	if _, ok := k.matchBinaries[selIdx]; !ok {
+		k.matchBinaries[selIdx] = &MatchBinariesMappings{
+			selNamesMap: make(map[uint32]uint32),
+		}
+	}
+	k.matchBinaries[selIdx].op = op
 }
 
-func (k *KernelSelectorState) AddBinaryName(binary string) {
-	k.initOnce.Do(func() {
-		k.newBinVals = make(map[uint32]string)
-		k.selNamesMap = make(map[uint32]uint32)
-	})
+func (k *KernelSelectorState) GetBinaryOp(selIdx int) uint32 {
+	return k.matchBinaries[selIdx].op
+}
 
+func (k *KernelSelectorState) AddBinaryName(selIdx int, binary string) {
 	binMu.Lock()
 	defer binMu.Unlock()
 	idx, ok := binVals[binary]
 	if ok {
-		k.selNamesMap[idx] = 1
+		k.matchBinaries[selIdx].selNamesMap[idx] = 1
 		return
 	}
 
 	idx = binIdx
 	binIdx++
-	binVals[binary] = idx      // global map of all names_map entries
-	k.newBinVals[idx] = binary // new names_map entries that we should add
-	k.selNamesMap[idx] = 1     // value in the per-selector names_map (we ignore the value)
+	binVals[binary] = idx                        // global map of all names_map entries
+	k.newBinVals[idx] = binary                   // new names_map entries that we should add
+	k.matchBinaries[selIdx].selNamesMap[idx] = 1 // value in the per-selector names_map (we ignore the value)
 }
 
 func (k *KernelSelectorState) GetNewBinaryMappings() map[uint32]string {
 	return k.newBinVals
 }
 
-func (k *KernelSelectorState) GetBinSelNamesMap() map[uint32]uint32 {
-	return k.selNamesMap
+func (k *KernelSelectorState) GetBinSelNamesMap() map[int]*MatchBinariesMappings {
+	return k.matchBinaries
 }
 
 func (k *KernelSelectorState) Buffer() [4096]byte {

pkg/sensors/tracing/genericuprobe.go

@@ -125,9 +125,8 @@ func (k *observerUprobeSensor) LoadProbe(args sensors.LoadProbeArgs) error {
 			Index: 0,
 			Name:  "sel_names_map",
 			Load: func(m *ebpf.Map, index uint32) error {
-				// add a special entry (key == UINT32_MAX) that has as a value the number of matchBinaries entry
-				// if this is zero we don't have any matchBinaries selectors
-				if err := m.Update(uint32(0xffffffff), uprobeEntry.selectors.GetBinaryOp(), ebpf.UpdateAny); err != nil {
+				// add a special entry (key == UINT32_MAX) that has as a value the operation (In or NotIn)
+				if err := m.Update(uint32(0xffffffff), uprobeEntry.selectors.GetBinaryOp(0), ebpf.UpdateAny); err != nil {
 					return err
 				}
 				for idx, val := range uprobeEntry.selectors.GetBinSelNamesMap() {

pkg/sensors/tracing/selectors.go

@@ -63,13 +63,9 @@ func updateSelectors(
 		return fmt.Errorf("failed to open sel_names_map map %s: %w", selNamesMapName, err)
 	}
 	defer selNamesMap.Close()
-	if err := selNamesMap.Update(uint32(0xffffffff), ks.GetBinaryOp(), ebpf.UpdateAny); err != nil {
-		return err
-	}
-	for idx, val := range ks.GetBinSelNamesMap() {
-		if err := selNamesMap.Update(idx, val, ebpf.UpdateAny); err != nil {
-			return err
-		}
+
+	if err := populateBinariesMaps(ks, pinPathPrefix, selNamesMap); err != nil {
+		return fmt.Errorf("failed to populate sel_names_map: %w", err)
 	}
 
 	return nil
@@ -93,18 +89,8 @@ func selectorsMaploads(ks *selectors.KernelSelectorState, pinPathPrefix string,
 		}, {
 			Index: 0,
 			Name:  "sel_names_map",
-			Load: func(m *ebpf.Map, index uint32) error {
-				// add a special entry (key == UINT32_MAX) that has as a value the number of matchBinaries entry
-				// if this is zero we don't have any matchBinaries selectors
-				if err := m.Update(uint32(0xffffffff), ks.GetBinaryOp(), ebpf.UpdateAny); err != nil {
-					return err
-				}
-				for idx, val := range ks.GetBinSelNamesMap() {
-					if err := m.Update(idx, val, ebpf.UpdateAny); err != nil {
-						return err
-					}
-				}
-				return nil
+			Load: func(outerMap *ebpf.Map, index uint32) error {
+				return populateBinariesMaps(ks, pinPathPrefix, outerMap)
 			},
 		},
 	}
@@ -160,3 +146,43 @@ func populateArgFilterMap(
 
 	return nil
 }
+
+func populateBinariesMaps(
+	ks *selectors.KernelSelectorState,
+	pinPathPrefix string,
+	outerMap *ebpf.Map,
+) error {
+	for innerID, sel := range ks.GetBinSelNamesMap() {
+		innerName := fmt.Sprintf("sel_names_map_%d", innerID)
+		innerSpec := &ebpf.MapSpec{
+			Name:       innerName,
+			Type:       ebpf.Hash,
+			KeySize:    4, // uint32
+			ValueSize:  4, // uint32
+			MaxEntries: 256,
+		}
+		innerMap, err := ebpf.NewMapWithOptions(innerSpec, ebpf.MapOptions{
+			PinPath: sensors.PathJoin(pinPathPrefix, innerName),
+		})
+		if err != nil {
+			return fmt.Errorf("creating innerMap %s failed: %w", innerName, err)
+		}
+		defer innerMap.Close()
+
+		// add a special entry (key == UINT32_MAX) that has as a value the operator (In or NotIn)
+		if err := innerMap.Update(uint32(0xffffffff), ks.GetBinaryOp(innerID), ebpf.UpdateAny); err != nil {
+			return err
+		}
+
+		for idx, val := range sel.GetBinSelNamesMap() {
+			if err := innerMap.Update(idx, val, ebpf.UpdateAny); err != nil {
+				return err
+			}
+		}
+
+		if err := outerMap.Update(uint32(innerID), uint32(innerMap.FD()), 0); err != nil {
+			return fmt.Errorf("failed to insert %s: %w", innerName, err)
+		}
+	}
+	return nil
+}