Skip to content

tetragon: Allow to use data events for char_buf data#789

Merged
kkourt merged 4 commits into
cilium:mainfrom
olsajiri:char_buf_max
Apr 27, 2023
Merged

tetragon: Allow to use data events for char_buf data#789
kkourt merged 4 commits into
cilium:mainfrom
olsajiri:char_buf_max

Conversation

@olsajiri

@olsajiri olsajiri commented Mar 10, 2023

Copy link
Copy Markdown
Contributor

Adding support to specify 'maxData: true' flag for char_buf argument
type to retrieve maximum data.

Using data events (same as for exec sensor) to retrieve and send
data to the user space from bpf program.

It's available for large kernels. Kernels < 5.3 won't load the
generic kprobe with data events due to verifier size limit.

It's possible to use the maxData flag like:

  - call: "__x64_sys_write"
    return: false
    syscall: true
    args:
    - index: 0
      type: "int"
    - index: 1
      type: "char_buf"
      sizeArgIndex: 3
      maxData: true
    - index: 2
      type: "size_t"

The data events are used only if the data won't fit into standard
event size.

There's no return kprobe support at the moment. I'm adding this
for terminal sniff, where it's not needed.

The maximum size for char_buf argument with maxData flag is 327360 bytes.

@olsajiri

Copy link
Copy Markdown
Contributor Author

static check fails on:
CHECK:BIT_MACRO: Prefer using the BIT macro

can be ignored

@olsajiri
olsajiri marked this pull request as ready for review March 10, 2023 17:49
@olsajiri
olsajiri requested a review from a team as a code owner March 10, 2023 17:49
@olsajiri
olsajiri requested a review from mtardy March 10, 2023 17:49

@kkourt kkourt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

Can we add a warning for the cases where the configuration will be ignored from bpf side?

Comment thread pkg/sensors/tracing/generickprobe.go Outdated
@olsajiri
olsajiri force-pushed the char_buf_max branch 3 times, most recently from 882e71b to 24efc15 Compare March 15, 2023 15:56
@olsajiri
olsajiri requested a review from kkourt March 15, 2023 20:10
@olsajiri
olsajiri marked this pull request as draft March 20, 2023 08:13
@olsajiri
olsajiri marked this pull request as ready for review March 21, 2023 10:43
@cilium cilium deleted a comment from netlify Bot Mar 30, 2023

@mtardy mtardy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you think you could document this new feature in the TracingPolicy reference, in the argument paragraph, where we already talk about char_buf and char_iovec: https://tetragon.cilium.io/docs/reference/tracing-policy/#arguments ? Like how this thing interacts with returnCopy maybe as well, what's the difference.

Comment thread pkg/sensors/tracing/kprobe_test.go Outdated
name: "sys-write"
spec:
kprobes:
- call: "__x64_sys_write"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- call: "__x64_sys_write"
- call: "sys_write"

Please can you use portable syscall symbols for testing to run on arm64

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sry for late response, I was out... yep, will fix

Comment thread pkg/sensors/tracing/kprobe_test.go Outdated
name: "sys-write"
spec:
kprobes:
- call: "__x64_sys_write"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- call: "__x64_sys_write"
- call: "sys_write"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

Comment thread pkg/sensors/tracing/kprobe_test.go Outdated
name: "sys-write"
spec:
kprobes:
- call: "__x64_sys_write"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- call: "__x64_sys_write"
- call: "sys_write"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

Comment on lines +54 to +55
description: This field is used only for char_buf and
char_iovec types.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible in this description and all the following to explain what the max field does on top of the fact that you can only use it for char_buf and char_iovec? :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hum I copy pasted from previous flag.. need to remove the char_iovec for sure ;-) will add the description, thanks

@olsajiri

Copy link
Copy Markdown
Contributor Author

Do you think you could document this new feature in the TracingPolicy reference, in the argument paragraph, where we already talk about char_buf and char_iovec: https://tetragon.cilium.io/docs/reference/tracing-policy/#arguments ? Like how this thing interacts with returnCopy maybe as well, what's the difference.

yes, I will add that, thanks

@olsajiri

Copy link
Copy Markdown
Contributor Author

I rebased new version on top of #885

@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 19, 2023
@kkourt

kkourt commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

I rebased new version on top of #885

Thanks! It seems that it needs one more rebase now that #885 was merged :(

@olsajiri
olsajiri force-pushed the char_buf_max branch 2 times, most recently from befd652 to 7c3b462 Compare April 27, 2023 07:23
At the moment argument types like char_buf and cha_iovec will
retrieve only certain amount of data dues to the event size
limitations.

Adding 'maxData' boolean flag that instructs the code to get as
much as possible data of the argument.

The actual implementation comes in following changes, this is
just the tracing policy support.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding support to specify 'max: true' flag for char_buf argument
type to retrieve maximum data.

Using data events (same as for exec sensor) to retrieve and send
data to the user space from bpf program.

It's available for large kernels. Kernels < 5.3 won't load the
generic kprobe with data events due to verifier size limit.

It's possible to use the max flag like:

  - call: "__x64_sys_write"
    return: false
    syscall: true
    args:
    - index: 0
      type: "int"
    - index: 1
      type: "char_buf"
      sizeArgIndex: 3
      max: true
    - index: 2
      type: "size_t"

The data events are used only if the data won't fit into standard
event size.

There's no return kprobe support at the moment. I'm adding this
for terminal sniff, where it's not needed.

The maximum size for char_buf argument with max flag is 327360 bytes.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
The cnt value is passed directly to __copy_char_iovec,
there's no need to read it with probe_read.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding tests for maxData flag used in char_buf arg, testing that:
  - we get truncated data if it's over 4096 bytes and maxData is not used (TestKprobeWriteMaxTrunc)
  - we get all data over 4096 and maxData is used (TestKprobeWriteMax)
  - we get maximum data size 327360 bytes (TestKprobeWriteMaxFull)

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@kkourt
kkourt merged commit b03408d into cilium:main Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase This PR needs to be rebased because it has merge conflicts.

3 participants