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

[ntop] add support for arrays and IPv6 #566

Merged
merged 1 commit into from Apr 26, 2019

Conversation

@mmarchini
Copy link
Member

commented Apr 24, 2019

  • Added ntop support for array types of sizes 4 and 16
  • Added ntop support for IPv6
  • Optimized number of instructions generated for ntop and memory used, both in the BPF stack and maps
  • Make first argument of ntop optional, inferring the af_type from the address type
  • Updated tools to handle IPv6 and use AF_* macros
@@ -110,6 +110,9 @@ llvm::Type *IRBuilderBPF::GetType(const SizedType &stype)
{
switch (stype.size)
{
case 16:
ty = getInt128Ty();

This comment has been minimized.

Copy link
@dalehamel

dalehamel Apr 24, 2019

Contributor

Nice trick, this is what I was missing I think.

I think i did something similar when I tried to fix #284

@mmarchini

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@dalehamel @brendangregg how do you feel about changing the ntop signature to make af_type optional (ntop([int af_type], inet inet)? It should be easy to detect the correct address type from it's size:

  • If an integer is given, we're dealing with a IPv4 address (since we don't support 128-bits integers yet)
  • If an uint8 array of size 4 is given, we're dealing with IPv4 address
  • If an uint8 array of size 16 is given, we're dealing with IPv6 address
@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Making it option is definitely preferred 👍 it was only required before because it needed to be explicit that only AF_INET was supported at the time.

@mmarchini mmarchini force-pushed the ntop_array_and_ipv6 branch from f2957f0 to 8f47db2 Apr 25, 2019

@mmarchini

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

Updated with:

  • Make first parameter of ntop optional and infer the AF type from the type and size of our address paramenter.
  • Some optimizations:
    • Only allocate what we need on the stack (8 bytes for IPv4, 20 bytes for IPv6)
    • Avoid copying things back and forth on the stack, LLVM doesn't seem smart enough to optimize this.

I'll update docs and tests now.

@mmarchini

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

For literal integers, ntop will be reduced to essentially two operations:

#: (18) r1 = 0x[address][af_type]
#: (7b) *(u64 *)(r10 -x) = r1

For example:

$ sudo ./src/bpftrace -ve 'BEGIN { printf("%s\n", ntop(0x0100007f));}'
Attaching 1 probe...

Program ID: 405

Bytecode: 
0: (bf) r6 = r1
1: (18) r1 = 0x100007f00000002
3: (7b) *(u64 *)(r10 -8) = r1
4: (b7) r1 = 0
5: (7b) *(u64 *)(r10 -16) = r1
6: (18) r7 = 0xffff8b6d7e5f3100
8: (85) call bpf_get_smp_processor_id#8
9: (bf) r4 = r10
10: (07) r4 += -16
11: (bf) r1 = r6
12: (bf) r2 = r7
13: (bf) r3 = r0
14: (b7) r5 = 16
15: (85) call bpf_perf_event_output#25
16: (b7) r0 = 0
17: (95) exit
processed 16 insns, stack depth 16

Attaching BEGIN
Running...
127.0.0.1
^C

Not particularly useful, because we'll fetch the address using probe_read on most cases. But I'm happy with this result :)

@mmarchini mmarchini force-pushed the ntop_array_and_ipv6 branch from 8f47db2 to ac9c50a Apr 26, 2019

@mmarchini mmarchini changed the title rewrite! inet support array and IPv6 [ntop] add support for arrays and IPv6 Apr 26, 2019

@mmarchini mmarchini marked this pull request as ready for review Apr 26, 2019

@mmarchini mmarchini force-pushed the ntop_array_and_ipv6 branch 2 times, most recently from 665bc69 to f749b6d Apr 26, 2019

[ntop] add support for arrays and IPv6
- Added ntop support for array types of sizes 4 and 16
- Added ntop support for IPv6
- Optimized number of instructions generated for ntop and memory used,
both in the BPF stack and maps
- Make first argument of ntop optional, inferring the af_type from the
address type
- Updated tools to handle IPv6 and use AF_* macros

Fixes: #30

@mmarchini mmarchini force-pushed the ntop_array_and_ipv6 branch from f749b6d to c9dd10f Apr 26, 2019

@brendangregg

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

thanks!

@brendangregg brendangregg merged commit 12995fc into master Apr 26, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ajor ajor deleted the ntop_array_and_ipv6 branch Apr 27, 2019

@ajor

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

This patch has broken some stuff related to reading in structs.
e.g. if you try running the test in tests/codegen/struct_save_nested.cpp on a real program, it will refuse to load now.

@ajor

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Fixed in #579

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.