From: Alexander Bluhm <bluhm@openbsd.org>
Subject: tcp softlro refactor check
To: tech@openbsd.org
Date: Tue, 22 Apr 2025 00:30:24 +0200

Hi,

I would like to do all checks that affect only a single packet
before looping over the enqueued packets.  Fragment and TCP protocol
checks can be removed, ether_extract_headers() already does that.

After the check is successful, store the payload length in ph_mss.
This is done later anyway.  If this mbuf field contains a positive
length, we know that the check has already been done.

ok?

bluhm

Index: netinet/tcp_input.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
diff -u -p -r1.437 tcp_input.c
--- netinet/tcp_input.c	21 Apr 2025 09:54:53 -0000	1.437
+++ netinet/tcp_input.c	21 Apr 2025 20:49:36 -0000
@@ -4246,6 +4246,42 @@ syn_cache_respond(struct syn_cache *sc, 
 	return (error);
 }
 
+static int
+tcp_softlro_check(struct mbuf *m, struct ether_extracted *ext)
+{
+	/* Don't merge packets with invalid TCP checksum. */
+	if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_IN_OK))
+		return 0;
+
+	if (ext->ip4) {
+		/* Don't merge packets with invalid IP header checksum. */
+		if (!ISSET(m->m_pkthdr.csum_flags, M_IPV4_CSUM_IN_OK))
+			return 0;
+
+		/* Don't merge IPv4 packets with IP options. */
+		if (ext->iphlen != sizeof(struct ip))
+			return 0;
+	}
+
+	/* Check TCP protocol and header. */
+	if (!ext->tcp)
+		return 0;
+
+	/* Don't merge empty segments. */
+	if (ext->paylen == 0)
+		return 0;
+
+	/* Just ACK and PUSH TCP flags are allowed. */
+	if (ISSET(ext->tcp->th_flags, TH_ACK|TH_PUSH) != ext->tcp->th_flags)
+		return 0;
+
+	/* TCP ACK flag has to be set. */
+	if (!ISSET(ext->tcp->th_flags, TH_ACK))
+		return 0;
+
+	return 1;
+}
+
 int
 tcp_softlro(struct mbuf *mhead, struct mbuf *mtail)
 {
@@ -4278,34 +4314,14 @@ tcp_softlro(struct mbuf *mhead, struct m
 
 	/* Check IP header. */
 	if (head.ip4 && tail.ip4) {
-		/* Don't merge packets with invalid header checksum. */
-		if (!ISSET(mhead->m_pkthdr.csum_flags, M_IPV4_CSUM_IN_OK) ||
-		    !ISSET(mtail->m_pkthdr.csum_flags, M_IPV4_CSUM_IN_OK))
-			return 0;
-
 		/* Check IPv4 addresses. */
 		if (head.ip4->ip_src.s_addr != tail.ip4->ip_src.s_addr ||
 		    head.ip4->ip_dst.s_addr != tail.ip4->ip_dst.s_addr)
 			return 0;
 
-		/* Don't merge IPv4 fragments. */
-		if (ISSET(head.ip4->ip_off, htons(IP_OFFMASK | IP_MF)) ||
-		    ISSET(tail.ip4->ip_off, htons(IP_OFFMASK | IP_MF)))
-			return 0;
-
 		/* Check max. IPv4 length. */
 		if (head.iplen + tail.iplen > IP_MAXPACKET)
 			return 0;
-
-		/* Don't merge IPv4 packets with option headers. */
-		if (head.iphlen != sizeof(struct ip) ||
-		    tail.iphlen != sizeof(struct ip))
-			return 0;
-
-		/* Don't non-TCP packets. */
-		if (head.ip4->ip_p != IPPROTO_TCP ||
-		    tail.ip4->ip_p != IPPROTO_TCP)
-			return 0;
 	} else if (head.ip6 && tail.ip6) {
 		/* Check IPv6 addresses. */
 		if (!IN6_ARE_ADDR_EQUAL(&head.ip6->ip6_src, &tail.ip6->ip6_src) ||
@@ -4316,19 +4332,10 @@ tcp_softlro(struct mbuf *mhead, struct m
 		if ((head.iplen - head.iphlen) +
 		    (tail.iplen - tail.iphlen) > IPV6_MAXPACKET)
 			return 0;
-
-		/* Don't merge IPv6 packets with option headers nor non-TCP. */
-		if (head.ip6->ip6_nxt != IPPROTO_TCP ||
-		    tail.ip6->ip6_nxt != IPPROTO_TCP)
-			return 0;
 	} else {
 		return 0;
 	}
 
-	/* Check TCP header. */
-	if (!head.tcp || !tail.tcp)
-		return 0;
-
 	/* Check TCP ports. */
 	if (head.tcp->th_sport != tail.tcp->th_sport ||
 	    head.tcp->th_dport != tail.tcp->th_dport)
@@ -4342,16 +4349,6 @@ tcp_softlro(struct mbuf *mhead, struct m
 	if (ntohl(head.tcp->th_seq) + head.paylen != ntohl(tail.tcp->th_seq))
 		return 0;
 
-	/* Just ACK and PUSH TCP flags are allowed. */
-	if (ISSET(head.tcp->th_flags, ~(TH_ACK|TH_PUSH)) ||
-	    ISSET(tail.tcp->th_flags, ~(TH_ACK|TH_PUSH)))
-		return 0;
-
-	/* TCP ACK flag has to be set. */
-	if (!ISSET(head.tcp->th_flags, TH_ACK) ||
-	    !ISSET(tail.tcp->th_flags, TH_ACK))
-		return 0;
-
 	/* Ignore segments with different TCP options. */
 	if (head.tcphlen - sizeof(struct tcphdr) !=
 	    tail.tcphlen - sizeof(struct tcphdr))
@@ -4454,19 +4451,23 @@ tcp_softlro(struct mbuf *mhead, struct m
 void
 tcp_softlro_glue(struct mbuf_list *ml, struct mbuf *mtail, struct ifnet *ifp)
 {
+	struct ether_extracted	 tail;
 	struct mbuf *mhead;
 
 	if (!ISSET(ifp->if_xflags, IFXF_LRO))
 		goto out;
 
-	/* Don't merge packets with invalid header checksum. */
-	if (!ISSET(mtail->m_pkthdr.csum_flags, M_TCP_CSUM_IN_OK))
-		goto out;
+        ether_extract_headers(mtail, &tail);
+        if (!tcp_softlro_check(mtail, &tail)) {
+                mtail->m_pkthdr.ph_mss = 0;
+                goto out;
+        }
+        mtail->m_pkthdr.ph_mss = tail.paylen;
 
 	for (mhead = ml->ml_head; mhead != NULL; mhead = mhead->m_nextpkt) {
-		/* Don't merge packets with invalid header checksum. */
-		if (!ISSET(mhead->m_pkthdr.csum_flags, M_TCP_CSUM_IN_OK))
-			continue;
+                /* This packet has been checked and was not mergable before. */
+                if (mhead->m_pkthdr.ph_mss == 0)
+                        continue;
 
 		/* Use RSS hash to skip packets of different connections. */
 		if (ISSET(mhead->m_pkthdr.csum_flags, M_FLOWID) &&