netstacklat: Add sanity checks for rcv_nxt and copied_seq

In addition to the out-of-order sequence number in the previous
commit, two more key members that are read from the tcp_sock (outside
of the socket lock) is rcv_nxt and copied_seq. Add sanity checks to
these two members that ensure that they are monotonically
increasing (in a u32 wrap-around space).

To enable this sanity check, track the last seen (sane) value for both
of them together with the other ooo-state in the socket storage
map. At each read, compare the recorded values from the last read with
their current values to determine if the current values are ahead of
the previously seen values or not. Unlike the ooo sequence number, do
not consider sequence 0 invalid for these checks. As they are direct
members of the tcp_sock their values should always be valid (although
possibly concurrently updated elsewhere), as long as the probe read
succeeds (and failure is directly detected from the return value of
bpf_core_read()).

Skip adding similar monotonic growth checks for rcv_wup field to avoid
also having to probe and update that value every time. For the rcv_wnd
field I am no aware of any simple validity checks than can be
performed.

Signed-off-by: Simon Sundberg <[email protected]>

Author: simosund

netstacklat/netstacklat.bpf.c

@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-or-later */
 #include "vmlinux_local.h"
 #include <linux/bpf.h>
+#include <linux/errno.h>
 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
@@ -46,6 +47,8 @@ struct sk_buff___old {
 struct tcp_sock_ooo_range {
 	u32 prev_n_ooopkts;
 	u32 ooo_seq_end;
+	u32 last_rcv_nxt;
+	u32 last_copied_seq;
 	/* indicates if ooo_seq_end is still valid (as 0 can be valid seq) */
 	bool active;
 };
@@ -361,18 +364,13 @@ static int get_current_rcv_wnd_seq(struct tcp_sock *tp, u32 rcv_nxt, u32 *seq)
 	return err;
 }
 
-static int current_max_possible_ooo_seq(struct tcp_sock *tp, u32 *seq)
+static int current_max_possible_ooo_seq(struct tcp_sock *tp, u32 rcv_nxt,
+					u32 *seq)
 {
-	u32 rcv_nxt, cur_rcv_window, max_seq = 0;
+	u32 cur_rcv_window, max_seq = 0;
 	struct tcp_skb_cb cb;
 	int err = 0;
 
-	err = bpf_core_read(&rcv_nxt, sizeof(rcv_nxt), &tp->rcv_nxt);
-	if (err) {
-		bpf_printk("failed reading tcp_sock->rcv_nxt, err=%d", err);
-		goto exit;
-	}
-
 	if (BPF_CORE_READ(tp, out_of_order_queue.rb_node) == NULL) {
 		/* No ooo-segments currently in ooo-queue
 		 * Any ooo-segments must already have been merged to the
@@ -413,35 +411,68 @@ static int current_max_possible_ooo_seq(struct tcp_sock *tp, u32 *seq)
 	return err;
 }
 
-static bool tcp_read_in_ooo_range(struct tcp_sock *tp,
+static bool tcp_read_in_ooo_range(struct tcp_sock *tp, u32 copied_seq,
 				  struct tcp_sock_ooo_range *ooo_range)
 {
-	u32 read_seq;
-	int err;
-
 	if (!ooo_range->active)
 		return false;
 
-	err = bpf_core_read(&read_seq, sizeof(read_seq), &tp->copied_seq);
-	if (err) {
-		bpf_printk("failed to read tcp_sock->copied_seq, err=%d", err);
-		return true; // Assume we may be in ooo-range
-	}
-
-	if (u32_lt(ooo_range->ooo_seq_end, read_seq)) {
+	if (u32_lt(ooo_range->ooo_seq_end, copied_seq)) {
 		ooo_range->active = false;
 		return false;
 	} else {
 		return true;
 	}
 }
 
+static int get_and_validate_rcvnxt(struct tcp_sock *tp,
+				   struct tcp_sock_ooo_range *ooo_range,
+				   u32 *rcvnxt)
+{
+	u32 rcv_nxt = 0;
+	int err;
+
+	err = bpf_core_read(&rcv_nxt, sizeof(rcv_nxt), &tp->rcv_nxt);
+	if (err || (ooo_range->last_rcv_nxt &&
+		    u32_lt(rcv_nxt, ooo_range->last_rcv_nxt))) {
+		bpf_printk("failed to read valid tcp_sock->rcv_nxt, err=%d",
+			   err);
+		err = err ?: -ERANGE;
+	} else {
+		ooo_range->last_rcv_nxt = rcv_nxt;
+	}
+
+	*rcvnxt = rcv_nxt;
+	return err;
+}
+
+static int get_and_validate_copiedseq(struct tcp_sock *tp,
+				      struct tcp_sock_ooo_range *ooo_range,
+				      u32 *copiedseq)
+{
+	u32 copied_seq = 0;
+	int err;
+
+	err = bpf_core_read(&copied_seq, sizeof(copied_seq), &tp->copied_seq);
+	if (err || (ooo_range->last_copied_seq &&
+		    u32_lt(copied_seq, ooo_range->last_copied_seq))) {
+		bpf_printk("failed to read valid tcp_sock->copied_seq, err=%d",
+			   err);
+		err = err ?: -ERANGE;
+	} else {
+		ooo_range->last_copied_seq = copied_seq;
+	}
+
+	*copiedseq = copied_seq;
+	return err;
+}
+
 static bool tcp_read_maybe_holblocked(struct sock *sk)
 {
+	u32 n_ooopkts, rcv_nxt, copied_seq, nxt_seq;
 	struct tcp_sock_ooo_range *ooo_range;
+	int err, err_rcvnxt, err_copiedseq;
 	struct tcp_sock *tp = tcp_sk(sk);
-	u32 n_ooopkts, nxt_seq;
-	int err;
 
 	err = bpf_core_read(&n_ooopkts, sizeof(n_ooopkts), &tp->rcv_ooopack);
 	if (err) {
@@ -461,18 +492,30 @@ static bool tcp_read_maybe_holblocked(struct sock *sk)
 		return true; // Assume we may be in ooo-range
 	}
 
+	/* rcv_nxt and copied_seq may not be needed, but to ensure we always
+	 * update our tracked state for them, read, sanity check and update
+	 * both their values here. Errors are only checked for in the paths
+	 * were the values are actually needed.
+	 */
+	err_rcvnxt = get_and_validate_rcvnxt(tp, ooo_range, &rcv_nxt);
+	err_copiedseq = get_and_validate_copiedseq(tp, ooo_range, &copied_seq);
+
 	// Increase in ooo-packets since last - figure out next safe seq
 	if (n_ooopkts > ooo_range->prev_n_ooopkts) {
 		ooo_range->prev_n_ooopkts = n_ooopkts;
-		err = current_max_possible_ooo_seq(tp, &nxt_seq);
+		err = err_rcvnxt ?:
+				   current_max_possible_ooo_seq(tp, rcv_nxt,
+								&nxt_seq);
 		if (!err) {
 			ooo_range->ooo_seq_end = nxt_seq;
 			ooo_range->active = true;
 		}
+
 		return true;
 	}
 
-	return tcp_read_in_ooo_range(tp, ooo_range);
+	return err_copiedseq ? true :
+			       tcp_read_in_ooo_range(tp, copied_seq, ooo_range);
 }
 
 static void record_socket_latency(struct sock *sk, struct sk_buff *skb,