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