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](#1989 (comment)))

Signed-off-by: Dom Del Nano <[email protected]>

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);

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