From 6fc0f1834b40abd8e3ab027be96f507fb8843ccc Mon Sep 17 00:00:00 2001 From: Dom Delnano Date: Mon, 16 Sep 2024 16:25:10 -0700 Subject: [PATCH] Use correct byte ordering function for kernel struct member (skc_num) (#2002) Summary: Use correct byte ordering function for kernel struct member (skc_num) This change doesn't result in a functional difference since `ntohs` and `htons` are inverses of each other on little endian machines (and noops for big endian machines). This field's byte order caused me confusion in #1989, so I wanted to make this struct access consistent. Relevant Issues: N/A Type of change: /kind cleanup Test Plan: Testing during #1989 (details [here](https://github.com/pixie-io/pixie/pull/1989#discussion_r1743081397)) Signed-off-by: Dom Del Nano --- .../tcp_stats/bcc_bpf/tcp_stats.c | 14 ++++++++------ .../tcp_stats/tcp_stats_bpf_test.cc | 3 +++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/stirling/source_connectors/tcp_stats/bcc_bpf/tcp_stats.c b/src/stirling/source_connectors/tcp_stats/bcc_bpf/tcp_stats.c index 4194af39869..8d78e4579c7 100644 --- a/src/stirling/source_connectors/tcp_stats/bcc_bpf/tcp_stats.c +++ b/src/stirling/source_connectors/tcp_stats/bcc_bpf/tcp_stats.c @@ -79,12 +79,12 @@ static int tcp_sendstat(struct pt_regs* ctx, uint32_t tgid, uint32_t id, int siz event.upid.start_time_ticks = get_tgid_start_time(); if (family == AF_INET) { - event.local_addr.in4.sin_port = ntohs(lport); + event.local_addr.in4.sin_port = htons(lport); event.remote_addr.in4.sin_port = rport; BPF_PROBE_READ_KERNEL_VAR(event.local_addr.in4.sin_addr.s_addr, &sk_common->skc_rcv_saddr); BPF_PROBE_READ_KERNEL_VAR(event.remote_addr.in4.sin_addr.s_addr, &sk_common->skc_daddr); } else if (family == AF_INET6) { - event.local_addr.in6.sin6_port = ntohs(lport); + event.local_addr.in6.sin6_port = htons(lport); event.remote_addr.in6.sin6_port = rport; BPF_PROBE_READ_KERNEL_VAR(event.local_addr.in6.sin6_addr, &sk_common->skc_v6_rcv_saddr); BPF_PROBE_READ_KERNEL_VAR(event.remote_addr.in6.sin6_addr, &sk_common->skc_v6_daddr); @@ -158,12 +158,12 @@ int probe_entry_tcp_cleanup_rbuf(struct pt_regs* ctx, struct sock* sk, int copie event.local_addr.sa.sa_family = family; if (family == AF_INET) { - event.local_addr.in4.sin_port = ntohs(lport); + event.local_addr.in4.sin_port = htons(lport); event.remote_addr.in4.sin_port = rport; BPF_PROBE_READ_KERNEL_VAR(event.local_addr.in4.sin_addr.s_addr, &sk->__sk_common.skc_rcv_saddr); BPF_PROBE_READ_KERNEL_VAR(event.remote_addr.in4.sin_addr.s_addr, &sk->__sk_common.skc_daddr); } else if (family == AF_INET6) { - event.local_addr.in6.sin6_port = ntohs(lport); + event.local_addr.in6.sin6_port = htons(lport); event.remote_addr.in6.sin6_port = rport; BPF_PROBE_READ_KERNEL_VAR(event.local_addr.in6.sin6_addr, &sk->__sk_common.skc_v6_rcv_saddr); BPF_PROBE_READ_KERNEL_VAR(event.remote_addr.in6.sin6_addr, &sk->__sk_common.skc_v6_daddr); @@ -196,6 +196,8 @@ int probe_entry_tcp_retransmit_skb(struct pt_regs* ctx, struct sock* skp, struct int lport = -1; int rport = -1; + // skc_num is stored in host byte order. The rest of our user space processing + // assumes network byte order so convert it here. BPF_PROBE_READ_KERNEL_VAR(lport, &skp->__sk_common.skc_num); BPF_PROBE_READ_KERNEL_VAR(rport, &skp->__sk_common.skc_dport); @@ -204,13 +206,13 @@ int probe_entry_tcp_retransmit_skb(struct pt_regs* ctx, struct sock* skp, struct event.local_addr.sa.sa_family = family; if (family == AF_INET) { - event.local_addr.in4.sin_port = ntohs(lport); + event.local_addr.in4.sin_port = htons(lport); event.remote_addr.in4.sin_port = rport; BPF_PROBE_READ_KERNEL_VAR(event.local_addr.in4.sin_addr.s_addr, &skp->__sk_common.skc_rcv_saddr); BPF_PROBE_READ_KERNEL_VAR(event.remote_addr.in4.sin_addr.s_addr, &skp->__sk_common.skc_daddr); } else if (family == AF_INET6) { - event.local_addr.in6.sin6_port = ntohs(lport); + event.local_addr.in6.sin6_port = htons(lport); event.remote_addr.in6.sin6_port = rport; BPF_PROBE_READ_KERNEL_VAR(event.local_addr.in6.sin6_addr, &skp->__sk_common.skc_v6_rcv_saddr); BPF_PROBE_READ_KERNEL_VAR(event.remote_addr.in6.sin6_addr, &skp->__sk_common.skc_v6_daddr); diff --git a/src/stirling/source_connectors/tcp_stats/tcp_stats_bpf_test.cc b/src/stirling/source_connectors/tcp_stats/tcp_stats_bpf_test.cc index b1bd46052c2..24411525f75 100644 --- a/src/stirling/source_connectors/tcp_stats/tcp_stats_bpf_test.cc +++ b/src/stirling/source_connectors/tcp_stats/tcp_stats_bpf_test.cc @@ -123,6 +123,9 @@ TEST_F(TcpTraceTest, Capture) { EXPECT_THAT(records, IsSupersetOf(expected)); // TODO(RagalahariP): Explore options for testing retransmissions in a unit test case, // as retransmissions are blocking calls without known timeout value. + + // TODO(ddelnano): Use a test case that verifies that local address of the socket is correct. + // The current implementation is correct, but other source connectors have had bugs here. } } // namespace stirling