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

chore: add event_config alignment check #200

Merged
merged 1 commit into from
Jun 30, 2022

Conversation

Forsworns
Copy link
Contributor

Take the issue #108 . Have tested locally.


BTW, the alignment-checker reminds me that

C and Go structs alignment check failed: execvemap.ExecveValue(48) size does not match execve_map_value(112)

Is this expected? I found that they are defined as

// bpf/lib/process.h
struct execve_map_value {
	struct msg_execve_key key;
	struct msg_execve_key pkey;
	__u32 flags;
	__u32 nspid;
	__u32 binary;
	__u32 pad;
	struct msg_ns ns;
	struct msg_capabilities caps;
} __attribute__((packed)) __attribute__((aligned(8)));

and

// pkg/sensors/exec/execvemap/execve.go
type ExecveValue struct {
	Process processapi.MsgExecveKey
	Parent  processapi.MsgExecveKey
	Flags   uint32
	Nspid   uint32
	Buffer  uint64
}

Apparently, they are not the same...
And another related ExecveValueL struct in pkg/sensors/exec/execvemap/execve.go seems deprectated.

@Forsworns Forsworns requested a review from a team as a code owner June 28, 2022 16:38
@Forsworns Forsworns requested a review from tpapagian June 28, 2022 16:38
@Forsworns Forsworns changed the title Alignchecker event config chore: add event_config alignment check Jun 28, 2022
Copy link
Member

@tpapagian tpapagian left a comment

Choose a reason for hiding this comment

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

Thanks @Forsworns! Can you please rebase and drop the second commit (i.e. "Merge branch 'cilium:main' into alignchecker_event_config").

bpf/alignchecker/bpf_alignchecker.c Show resolved Hide resolved
@tpapagian
Copy link
Member

Additionally, in EventConfig struct you may also add align directives similar to:

type MsgExitEvent struct {
Common MsgCommon `align:"common"`
ProcessKey MsgExecveKey `align:"current"`
Info MsgExitInfo `align:"info"`
}

Using this, the alignchecker does not only check the size of the struct but also the fields inside.

@tpapagian
Copy link
Member

Regarding the check between execvemap.ExecveValue and execve_map_value, I believe that these may not be the same. execve_map_value is not shared with the user-space. So we can remove that check.

@Forsworns
Copy link
Contributor Author

these may not be the same. execve_map_value is not shared with the user-space.

In fact, they are shared. In pkg/sensors/exec/procevents/proc_reader.go, writeExecveMap will open write to the execve_map at the ebpf side. It collects the information all of the running processes when Tetragon is initialized.

See

func GetRunningProcs(write, push bool) []Procs {

and

func writeExecveMap(procs []Procs) {

Maybe it's better to append the missing items to ExecveValue.

@tpapagian
Copy link
Member

these may not be the same. execve_map_value is not shared with the user-space.

In fact, they are shared. In pkg/sensors/exec/procevents/proc_reader.go, writeExecveMap will open write to the execve_map at the ebpf side. It collects the information all of the running processes when Tetragon is initialized.

See

func GetRunningProcs(write, push bool) []Procs {

and

func writeExecveMap(procs []Procs) {

Maybe it's better to append the missing items to ExecveValue.

I agree, maybe it is not enough to add new fields there, but we should also initialise them properly. I propose to keep that to a separate PR, so feel free to create a new one to solve this. Thanks!

@tpapagian
Copy link
Member

tpapagian commented Jun 29, 2022

Created #205 to keep track of execve_map_value issue.

@Forsworns Forsworns force-pushed the alignchecker_event_config branch 2 times, most recently from 2a0b9e7 to 04262a5 Compare June 29, 2022 09:13
@tpapagian
Copy link
Member

tpapagian commented Jun 29, 2022

Regarding the CI failure maybe you should also add "Signed-off-by" on the PR message (except from the commits where you already did it).

declare a dummy event_config struct in bpf_alignchecker and check it in alignchecker.go

Close cilium#108

Signed-off-by: Peihao Yang <peihao.young@gmail.com>
Copy link
Member

@tpapagian tpapagian left a comment

Choose a reason for hiding this comment

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

Thanks

@jrfastab
Copy link
Contributor

Thanks @Forsworns nice set of work.

@jrfastab jrfastab merged commit 8a81618 into cilium:main Jun 30, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants