Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bug] matchPIDs is using first pid only #3255

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bpf/process/pfilter.h
Original file line number Diff line number Diff line change
Expand Up @@ -342,15 +342,19 @@ selector_match(__u32 *f, struct selector_filter *sel,
four:
res4 = process_filter(sel, f, enter, &msg->ns, &msg->caps);
index = next_pid_value(index, f, ty);
sel->index = index;
three:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we do directly sel->index = next_pid_value(index, f, ty); ?

Copy link
Contributor Author

@arthur-zhang arthur-zhang Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not. If we do that, index is not changed. and sel->index will be 0 and 4(using first two pids), not more pids will be chosen. (I already tested this)

maybe we can remove index variable and use sel->index = next_pid_value(sel->index, f, ty) directly

res3 = process_filter(sel, f, enter, &msg->ns, &msg->caps);
index = next_pid_value(index, f, ty);
sel->index = index;
two:
res2 = process_filter(sel, f, enter, &msg->ns, &msg->caps);
index = next_pid_value(index, f, ty);
sel->index = index;
one:
res1 = process_filter(sel, f, enter, &msg->ns, &msg->caps);
index = next_pid_value(index, f, ty);
sel->index = index;

if (ty == op_filter_notin)
return res1 & res2 & res3 & res4;
Expand Down
2 changes: 1 addition & 1 deletion bpf/tests/Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
include ../Makefile.defs

TESTS = prepend_name_test.o
TESTS = prepend_name_test.o pid_match_test.o

OBJSDIR := objs/
OBJS := $(addprefix $(OBJSDIR),$(TESTS))
Expand Down
56 changes: 56 additions & 0 deletions bpf/tests/pid_match_test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
/* Copyright Authors of Cilium */

//go:build ignore

#include "vmlinux.h"
#include "compiler.h"
#include "bpf_core_read.h"
#include "bpf_helpers.h"
#include "process/retprobe_map.h"

#include "process/types/basic.h"
#include "process/generic_calls.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this include is needed and AFAICS will cause compilation fail now

#include "process/pfilter.h"

char _license[] __attribute__((section("license"), used)) = "Dual BSD/GPL";

struct filter_map_value {
unsigned char buf[FILTER_SIZE];
};

struct {
__uint(type, BPF_MAP_TYPE_ARRAY);
__uint(max_entries, 1);
__type(key, int);
__type(value, struct filter_map_value);
} test_filter_map SEC(".maps");

__attribute__((section("raw_tracepoint/test"), used)) int
test_pid_match()
{
__u32 *f;
int zero = 0, index = 0;
struct pid_filter *pid;
struct execve_map_value *enter;

f = map_lookup_elem(&test_filter_map, &zero);
if (!f)
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for {} in here


pid = (struct pid_filter *)((u64)f + index);
index += sizeof(struct pid_filter);

enter = map_lookup_elem(&execve_map, &zero);
if (!enter)
return 0;

struct selector_filter sel = {
.index = index,
.ty = pid->op,
.flags = pid->flags,
.len = pid->len,
};

return selector_match(f, &sel, enter, NULL, &process_filter_pid);
}
146 changes: 146 additions & 0 deletions bpf/tests/pid_match_test.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for taking the time to write such tests, I just wonder if there was a more straightforward way instead of adding a tests under bpf/tests. It's mostly because those are used for BPF unit tests and that would be maybe better to put the actual testing Go code under pkg/someappropriatepkg so that it gets tested along the other tests. Would you know a good place @olsajiri maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum, I dont mind it there.. it's testing the bpf selector_match function, which seems ok

Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright Authors of Tetragon

package bpf

import (
"errors"
"fmt"
"testing"

"github.com/cilium/ebpf"
"github.com/cilium/tetragon/pkg/api/processapi"
"github.com/cilium/tetragon/pkg/k8s/apis/cilium.io/v1alpha1"
"github.com/cilium/tetragon/pkg/selectors"
"github.com/cilium/tetragon/pkg/sensors/exec/execvemap"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const programNamePidMatch = "test_pid_match"

type testContext struct {
coll *ebpf.Collection
prog *ebpf.Program
execvMap *ebpf.Map
t *testing.T
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS there's no need to store t in testContext

}

// setupTest loads the test program and returns a test context.
func setupTest(t *testing.T) (*testContext, error) {
ctx := &testContext{t: t}
// load test program
coll, err := ebpf.LoadCollection("objs/pid_match_test.o")
if err != nil {
var ve *ebpf.VerifierError
if errors.As(err, &ve) {
return nil, fmt.Errorf("verifier error: %+v", ve)
}
return nil, err
}
ctx.coll = coll

var ok bool
ctx.prog, ok = coll.Programs[programNamePidMatch]
require.True(t, ok, "program %s not found", programNamePidMatch)

ctx.execvMap, ok = coll.Maps["execve_map"]
require.True(t, ok, "execve_map not found")

return ctx, nil
}

func (ctx *testContext) cleanup() {
if ctx.coll != nil {
ctx.coll.Close()
}
}

// runProg runs the test program with the given PID and returns the result.
func (ctx *testContext) runProg(selfPid uint32) (uint32, error) {
err := ctx.execvMap.Update(uint32(0), &execvemap.ExecveValue{
Process: processapi.MsgExecveKey{Pid: selfPid},
}, 0)
if err != nil {
return 0, fmt.Errorf("failed to update execve map: %w", err)
}
res, err := ctx.prog.Run(&ebpf.RunOptions{})
if err != nil {
return 0, fmt.Errorf("failed to run program: %w", err)
}
return res, nil
}

// initKernelStateData initializes the kernel state data with the given PIDs.
func (ctx *testContext) initKernelStateData(pids []uint32) error {
k := &selectors.KernelSelectorState{}
err := selectors.ParseMatchPid(k, &v1alpha1.PIDSelector{
Operator: "In",
Values: pids,
IsNamespacePID: false,
FollowForks: false,
})
if err != nil {
return fmt.Errorf("failed to parse PID selector: %w", err)
}

filterMap, ok := ctx.coll.Maps["test_filter_map"]
if !ok {
return errors.New("test_filter_map not found")
}

return filterMap.Update(uint32(0), k.Buffer(), 0)
}

func Test_PidMatch(t *testing.T) {
ctx, err := setupTest(t)
require.NoError(t, err)
defer ctx.cleanup()

tests := []struct {
name string
pids []uint32
testPid uint32
expected uint32
}{
{
name: "Match_1_PID",
pids: []uint32{1},
testPid: 1,
expected: 1,
},
{
name: "Match_2_PID",
pids: []uint32{1, 2},
testPid: 2,
expected: 1,
},
{
name: "Match_2_PID_NOT_IN_LIST",
pids: []uint32{1, 2},
testPid: 3,
expected: 0,
},
{
name: "Match_4_PID",
pids: []uint32{1, 2, 3, 4},
testPid: 4,
expected: 1,
},
{
name: "Match_4_PID_NOT_IN_LIST",
pids: []uint32{1, 2, 3, 4},
testPid: 5,
expected: 0,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require.NoError(t, ctx.initKernelStateData(tt.pids))
result, err := ctx.runProg(tt.testPid)
require.NoError(t, err)
assert.Equal(t, tt.expected, result)
})
}
}
Loading