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

add inet_ntop() call: IPv6 support #30

Closed
brendangregg opened this issue Sep 1, 2018 · 15 comments

Comments

@brendangregg
Copy link
Member

commented Sep 1, 2018

This will be useful for all networking one-liners/scripts. See the man page for inet_ntop(3), but maybe we should just call it ntop() for short (or something else).

Both of these should work, and print "127.0.0.1" and "::1":

bpftrace -e 'BEGIN { print("%s\n", ntop(AF_INET, 1)); }'
bpftrace -e 'BEGIN { print("%s\n", ntop(AF_INET6, 1)); }'

IPv4 should be shown as a dotted-quad, and IPV6 as a most-shortened address representation: https://tools.ietf.org/html/rfc5952

@brendangregg brendangregg added the llvm label Sep 12, 2018

@brendangregg

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2018

I imagine ntop() would work like usym(): there's a new type, Type::inet and Type::inet6, and just the address bytes are passed to user-space via the llvm BPF code, and then user-space bpftrace.cpp/map.cpp etc turns the Type::inet and Type::inet6 into the human readable strings.

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

With #262 merged, I now have a practical reason to implement this support so I'm going to take a crack at it. My early attempts are crashing, I suspect a bug in codegen. Specific error is that the probe doesn't load, so might be related to bcc. Down the rabbit hole I go 🐰 .

I noticed that bpftrace supports bitwise operations, but not hex literals (as far as I can tell). A quick and dirty way to translate the existing (now parsable) struct members such as sk->__sk_common.skc_rcv_addr which contain AF_INET addresses would be to just leverage bit-shifting to extract each byte of at least at ipv4 address for printing as a decimal. As a work-around, I'm just printing hex values for now, though ports are thankfully simple integers and are easier to work with.

The implementation specified above would also probably need to specify the literal decimal 2 instead of AF_INET, as the current header parsing doesn't parse #define macros (though I suspect the support is there in clang to do so). I suspect that based on the buffer size, we could just infer whether an address is ipv4 or ipv6 though, so perhaps specifying the address type is an unnecessary part of the API.

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

I noticed in #59 that hex literals are permitted, I guess they just have to be declared as a variable explicitly.

I was able to put together a way to print ipv4 addresses, but uses up 4 printf fields, so it'll mean using multiple printfs per line, but it'll get the job done in the meantime as a reasonable workaround:

  $sk->__sk_common.skc_rcv_daddr
  $mask1 = 0x000000FF;
  $mask2 = 0x0000FF00;
  $mask3 = 0x00FF0000;
  $mask4 = 0xFF000000;

  $q1 = ($daddr & $mask1) >> 0;
  $q2 = ($daddr & $mask2) >> 8;
  $q3 = ($daddr & $mask3) >> 16;
  $q4 = ($daddr & $mask4) >> 24;
  printf("%d.%d.%d.%d\n", $q1, $q2, $q3, $q4);

Sample output:

bpftrace -e 'BEGIN { $daddr = 0x08B2940A; $mask1 = 0x000000FF;  $mask2 = 0x0000FF00; $mask3 = 0x00FF0000; $mask4 = 0xFF000000; $q1 = ($daddr & $mask1) >> 0; $q2 = ($daddr & $mask2) >> 8; $q3 = ($daddr & $mask3) >> 16; $q4 = ($daddr & $mask4) >> 24; printf("%d.%d.%d.%d\n", $q1, $q2, $q3, $q4);}'
Attaching 1 probe...
10.148.178.8
@ajor

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

Hex literals are meant to be fully supported in bpftrace - this program works for me without first assigning to variables:
BEGIN { printf("%x\n", 0x4567 & 0xff) }

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2018

@ajor hmm yes you seem to be right. I was getting a syntax error earlier but it might have due to the syntax I used for the bitwise operators, and when I switched to variables it was fine. Perhaps I was on an old branch of bpftrace?

Either way, yes you're correct, hex literals seem to be working just fine.

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

Accessing ipv6 addresses in the kernel is going to be a bit tricky #268 (comment)

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

I've completed an initial implementation of this in my PR #269, which I think is now ready for review.

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

In #275 I indicate a possible approach for handling array types, which is the issue with ipv6 addresses right now.

This would also allow for accepting Type::array as the second field to the ntop function in the semantic analyzer, which could then just copy the bytes down to resolve_inet directly, and I think is the cleanest way to get ipv6 support.

@brendangregg

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

About IPv6: passing a 128 bit integer is one way to do it, but did you try passing it as 2 x 64 bit integers? I did such packing with usym(), for example:

  else if (call.func == "usym")
  {
    // store uint64_t[2] with: [0]: (uint64_t)addr, [1]: (uint64_t)pid
    AllocaInst *buf = b_.CreateAllocaBPF(call.type, "usym");
    b_.CreateMemSet(buf, b_.getInt8(0), call.type.size, 1);
    Value *pid = b_.CreateLShr(b_.CreateGetPidTgid(), 32);
    Value *addr_offset = b_.CreateGEP(buf, b_.getInt64(0));
    Value *pid_offset = b_.CreateGEP(buf, {b_.getInt64(0), b_.getInt64(8)});
    call.vargs->front()->accept(*this);
    b_.CreateStore(expr_, addr_offset);
    b_.CreateStore(pid, pid_offset);
    expr_ = buf;
  }
@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@brendangregg yes that should work and how i would intend to implement the codegen (it already does basically this, by packing the address family into the first 64 bit word, and ipv4 in the second 64 bit word, even expecting the remaining bytes to be in the third 64 bit word once ipv6 is supported, just changing getInt64 to getInt128 I think should make this work), but that's not where I think the problem is.

It's actually pretty tricky to access the bytes of an ipv6 address, as it's stored as a constant array which is not supported by bpftrace (though it is with #285, but I'm not sure if array indexing is a language feature that's wanted/needed - in my opinion, it is for use cases such as this). Aside from this, You can access the field and cast it to a 64 bit type, and define a custom struct to access the upper and lower 64 bits as two separate variables as another workaround.

Through either method, I'm able to access all of the necessary data for an ipv4 address, but there is no way to combine it into a single 128 bit variable as all binops only work on 64 bit types.

I prefer to keep the existing call signature (which is more complicated and in my opinion a better long-term fix and is more consistent with inet_ntop library function) but a couple of easy workarounds would be:

  • accept a third argument to ntop, hopefully allowing it to be a 'default argument' for backwards compatibility.
  • Make a second function ntop6 which takes an ipv6 address as two variables, in which case we would probably want to have both ntop4 (one arg) and ntop6 (two args)

My preferred implementation would be to allow reading 128 bit types through something like #285 (which is fairly easy, we just add support for uint_128 in a few places). bpftrace would still treat this as Type::integer, which would allow for adding support for more 128 bit operations over time, but limiting the initial change to just storing and passing 128 bit types. I haven't written the code to try this yet, as it requires working with a few unmerged branches, but this is how I intended to add it if there is consensus around #284 and #275.

@brendangregg brendangregg changed the title add inet_ntop() call add inet_ntop() call: IPv6 support Apr 17, 2019

@brendangregg

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Changed title to adding IPv6 support.

Was just talking to @jasonk000 who might try coding it.

User space needs to take the bytes and format it. I did the code for bcc: iovisor/bcc@9e0b087

I think there's at least two approaches:

  • A) pass by value: this probably needs #284, so we can have a 128-bit integer as the argument.
  • B) pass by reference: this probably needs #275, so merging of #285.

Here's an example address (needs Linux 4.16+ for this tracepoint):

# bpftrace -lv t:sock:inet_sock_set_state
tracepoint:sock:inet_sock_set_state
    const void * skaddr;
    int oldstate;
    int newstate;
    __u16 sport;
    __u16 dport;
    __u16 family;
    __u8 protocol;
    __u8 saddr[4];
    __u8 daddr[4];
    __u8 saddr_v6[16];
    __u8 daddr_v6[16];

Hmmm. How would the code to either (A) or (B) look? I'm not sure. Maybe for (A):

bpftrace -e 'BEGIN { print("%s\n", ntop(AF_INET6, args->saddr_v6[0])); }'
# or
bpftrace -e 'BEGIN { print("%s\n", ntop(AF_INET6, (u128)args->saddr_v6[0])); }'

The first may not work since ntop() may only see the u8 and not the address. The second might be a way to cast it as a 128-bit int, but we'd need int casts first (#554) and it just may not make sense with the parser anyway -- since it may end up casing the u8 as a u128 and not getting the rest of the bits.

Hence (B):

bpftrace -e 'BEGIN { print("%s\n", ntop(AF_INET6, &args->saddr_v6[0])); }'

This will need #553. It probably makes the most sense.

It begs the question: do we change the ntop() API? For AF_INET, do we switch that to pass-by-reference for consistency, or leave it as pass-by-value since it's a 32-bit? I don't feel strongly. Consistency sounds good, if it works.

Fixing should fix #521 as a side effect.

@mmarchini

This comment was marked as outdated.

Copy link
Member

commented Apr 24, 2019

While working on #553 I ended up implementing this (sort of) and #521. The only problem is, inet_ntop doesn't seem to support IPv6 yet:

$ bpftrace -e 'tracepoint:tcp:tcp_set_state { printf("%s\n", ntop(10, args->saddr_v6))  }'
Attaching 1 probe...
ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future.

ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future.

ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future.

ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future.

ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future.

But maybe it's my Kernel version? I'll try with a newer kernel soon.

EDIT: Nevermind, I see what I did wrong 😛

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

@mmarchini that message is internal to bpftrace

bpftrace/src/bpftrace.cpp

Lines 1618 to 1631 in 48619f4

std::string BPFtrace::resolve_inet(int af, uint64_t inet)
{
// FIXME ipv6 is a 128 bit type as an array, how to pass as argument?
if(af != AF_INET)
{
std::cerr << "ntop() currently only supports AF_INET (IPv4); IPv6 will be supported in the future." << std::endl;
return std::string("");
}
char addr_cstr[INET_ADDRSTRLEN];
inet_ntop(af, &inet, addr_cstr, INET_ADDRSTRLEN);
std::string addrstr(addr_cstr);
return addrstr;
}

it is hardcoded to not work with the IPV6 addr family as there is currently no way to pass 128 bit types by value (or at least, there wasn't when we added the ipv4 functionality)

What you are passing there I think is stored as a constant array, and we need to either:

  • Cast it to a 128 bit type to pass to ntop (there is already space allocated for this in the call, we just have no way to load it). This is the "by-value" approach @brendangregg mentions
  • Get the address of the constant array, and store it in its own variable to pass by reference to ntop, changing the ntop api. This is the "by-reference" approach.
@mmarchini

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Cast it to a 128 bit type to pass to ntop (there is already space allocated for this in the call, we just have no way to load it). This is the "by-value" approach @brendangregg mentions

I went essentially with this approach. #566 should fix this and #521.

@dalehamel

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Awesome!!! Reading through it now, it looks great on first glance

mmarchini added a commit that referenced this issue Apr 25, 2019

mmarchini added a commit that referenced this issue Apr 26, 2019

mmarchini added a commit that referenced this issue 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 added a commit that referenced this issue 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
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.