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

tetragon: Adding support to store full binary path and arguments #116

Merged
merged 14 commits into from
Jun 9, 2022

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Jun 1, 2022

Adding support to store full binary path and full arguments
for execve events.

Signed-off-by: Jiri Olsa jolsa@kernel.org

@olsajiri olsajiri force-pushed the exec_path_fix branch 4 times, most recently from 1933f6d to 443091f Compare June 2, 2022 07:13
@olsajiri olsajiri marked this pull request as ready for review June 2, 2022 07:25
@olsajiri olsajiri requested a review from a team as a code owner June 2, 2022 07:26
@olsajiri olsajiri requested a review from tixxdz June 2, 2022 07:26
@olsajiri
Copy link
Contributor Author

olsajiri commented Jun 2, 2022

I left in some of the static check 'failures', because I don't thik it's real issue

@olsajiri olsajiri marked this pull request as draft June 3, 2022 13:40
@olsajiri olsajiri changed the title Fix execve path and arguments for binaries with long path tetragon: Adding support to store full binary path and arguments Jun 3, 2022
@olsajiri olsajiri force-pushed the exec_path_fix branch 3 times, most recently from ea8250b to 967ce6d Compare June 6, 2022 09:59
@olsajiri olsajiri marked this pull request as ready for review June 6, 2022 10:14
@willfindlay
Copy link
Contributor

Yeah let's add OPEN_ENDED_LINE to the ignore list... But what about PREFER_KERNEL_TYPES?

@olsajiri
Copy link
Contributor Author

olsajiri commented Jun 7, 2022

Yeah let's add OPEN_ENDED_LINE to the ignore list... But what about PREFER_KERNEL_TYPES?

I'd ignore PREFER_KERNEL_TYPES as well, kernel and our program use them a lot

@willfindlay
Copy link
Contributor

@olsajiri #134

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Looks great!

Just a couple of nits from my side.

pkg/sensors/exec/exec_test.go Outdated Show resolved Hide resolved
bpf/process/bpf_execve_event.c Outdated Show resolved Hide resolved
Adding exec event test that runs observer, executes binary
and checks we received proper exec/process event.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Currently we received mangled (not just shortened) binary
path for big path binaries (over 1024). Adding test case
for that, fix is coming in following changes.

We create directory path under /tmp and copy tester-progs/nop
in there. Each directory has maximum size.

We can't test 'full path size' on kernels < v5.15 because
the tracepoint won't trigger for data bigger that 2k. It's
fixed with:
  e531e90b5ab0 tracing: Increase PERF_MAX_TRACE_SIZE to handle Sentinel1 and docker together

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding test for big size arguments (512) bytes that we
do not support at the moment. The support is coming in
following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding test for big binary name together with big size arguments
(512) bytes that we do not support at the moment. The support is
coming in following changes.

It's combination of 2 previous tests for big path and args.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Moving event_args_builder function to bpf_execve_event.c
object, because it's not called from anywhere else and it
will be more convenient in following changes to use new
heap map directly instead of passing it as an argument.

It's not pure copy I fixed some lint complains.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Moving event_filename_builder function to bpf_execve_event.c
object, because it's not called from anywhere else and it
will be more convenient in following changes to use new
heap map directly instead of passing it as an argument.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
The probe_arg_read function skips the first argument, which is
the binary's path, only up to the size of 255.

If the pathname is longer, we will still skip just 255 bytes
and screw up following argument retrieval.

Using new heap map to skip maximum path size (4096). This way
we can remove the PROBE_PAST_CMD macro.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Using heap map data for pathname instead of the stack
as suggested in the comment. The probe_read_str will
copy NULL terminated string, so we don't need to
initialize it to zero.

It should help with adding debug code which complains
a lot about the program crossing the stack limit.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding support to receive extra data messages from sent
bpf code. The data message is simple byte array with
unique ID.

The data is grouped by this ID and stored in map so it can
be retrieved later by kprobe user side code.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding support to send extra data messages from bpf code.

The data_event_bytes function has following prototype:

  data_event_bytes(void *ctx, struct data_event_desc *desc,
                   unsigned long uptr, size_t size,
                   struct bpf_map_def *heap)

The function sends data message to user space with data copied
from 'uptr' with size 'size'.

The 'out' argument is pointer where we write the result of the
data event copy/send operation in following struct:

  struct data_event_desc {
        __s32 error;
        __u32 leftover;
        struct data_event_id id;
  } __attribute__((packed));

This will be received by the user side kprobe printers and they
can use the info to retrieve the data.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding data_event_str function that works in a same way
as data_event_bytes but reads full string instead of data
from pointer with size.

The string is sent as data event the same way as for
data_event_bytes interface.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Using processapi instead of api prefix in exec.go,
so we don't mix with other api packages that are
coming in following changes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding support to send full binary path for exec event.

We use the data_event_str function on bpf side and retrieve
the data from data events with data.Get on the user
space side.

We can remove the size restriction in the test.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding support to send full args for exec event.

We use the data_event_bytes function on bpf side and retrieve
the data from data events with data.Get on the user
space side.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
* filename so its redundant lets walk past it. Do we still need end check?
* Left for now until we analyze a bit.
*/
#define PROBE_PAST_CMD \
Copy link
Contributor

Choose a reason for hiding this comment

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

nice deleting more asm.

@jrfastab jrfastab merged commit 7a8c2a3 into cilium:main Jun 9, 2022
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.

4 participants