lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Wed,  3 Apr 2024 15:24:56 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>
Cc: Alexander Lobakin <aleksander.lobakin@...el.com>,
	Dmitry Safonov <dima@...sta.com>,
	Francesco Ruggeri <fruggeri@...sta.com>,
	Salam Noureddine <noureddine@...sta.com>,
	David Ahern <dsahern@...nel.org>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com,
	netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: [PATCH net-next] net/tcp: move TCP hash fail messages out of line

tcp_hash_fail() is used multiple times on hotpath, including static
inlines. It contains a couple branches and a lot of code, which all
gets inlined into the call sites. For example, one call emits two
calls to net_ratelimit() etc.
Move as much as we can out of line to a new global function. Use enum
to determine the type of failure. Check for net_ratelimit() only once,
format common fields only once as well to pass only unique strings to
pr_info().
The result for vmlinux with Clang 19:

add/remove: 2/0 grow/shrink: 0/4 up/down: 773/-4908 (-4135)
Function                                     old     new   delta
__tcp_hash_fail                                -     757    +757
__pfx___tcp_hash_fail                          -      16     +16
tcp_inbound_ao_hash                         1819    1062    -757
tcp_ao_verify_hash                          1217     451    -766
tcp_inbound_md5_hash                        1591     374   -1217
tcp_inbound_hash                            3566    1398   -2168

Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
---
 include/net/tcp.h    |  15 ++---
 include/net/tcp_ao.h |  58 ++++++++-----------
 net/ipv4/tcp.c       | 129 +++++++++++++++++++++++++++++++++++++------
 net/ipv4/tcp_ao.c    |  13 ++---
 4 files changed, 148 insertions(+), 67 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 9ab5b37e9d53..fa2303c788cf 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2809,17 +2809,14 @@ tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
 
 	/* Invalid option or two times meet any of auth options */
 	if (tcp_parse_auth_options(th, &md5_location, &aoh)) {
-		tcp_hash_fail("TCP segment has incorrect auth options set",
-			      family, skb, "");
+		tcp_hash_fail(TCP_HASH_FAIL_OPTS, family, skb);
 		return SKB_DROP_REASON_TCP_AUTH_HDR;
 	}
 
 	if (req) {
 		if (tcp_rsk_used_ao(req) != !!aoh) {
 			NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
-			tcp_hash_fail("TCP connection can't start/end using TCP-AO",
-				      family, skb, "%s",
-				      !aoh ? "missing AO" : "AO signed");
+			tcp_hash_fail(TCP_HASH_FAIL_CONN, family, skb, !aoh);
 			return SKB_DROP_REASON_TCP_AOFAILURE;
 		}
 	}
@@ -2837,14 +2834,14 @@ tcp_inbound_hash(struct sock *sk, const struct request_sock *req,
 		 * always at least one current_key.
 		 */
 		if (tcp_ao_required(sk, saddr, family, l3index, true)) {
-			tcp_hash_fail("AO hash is required, but not found",
-					family, skb, "L3 index %d", l3index);
+			tcp_hash_fail(TCP_HASH_FAIL_NOAO, family, skb,
+				      l3index);
 			return SKB_DROP_REASON_TCP_AONOTFOUND;
 		}
 		if (unlikely(tcp_md5_do_lookup(sk, l3index, saddr, family))) {
 			NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5NOTFOUND);
-			tcp_hash_fail("MD5 Hash not found",
-				      family, skb, "L3 index %d", l3index);
+			tcp_hash_fail(TCP_HASH_FAIL_NOMD5, family, skb,
+				      l3index);
 			return SKB_DROP_REASON_TCP_MD5NOTFOUND;
 		}
 		return SKB_NOT_DROPPED_YET;
diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index 471e177362b4..a00d765be30b 100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -143,42 +143,32 @@ extern struct static_key_false_deferred tcp_ao_needed;
 #define static_branch_tcp_ao()	false
 #endif
 
-static inline bool tcp_hash_should_produce_warnings(void)
-{
-	return static_branch_tcp_md5() || static_branch_tcp_ao();
-}
+enum {
+	TCP_HASH_FAIL_OPTS,
+	TCP_HASH_FAIL_CONN,
+	TCP_HASH_FAIL_NOAO,
+	TCP_HASH_FAIL_NOMD5,
+	TCP_HASH_FAIL_UNEXP,
+	TCP_HASH_FAIL_INET,
+	TCP_HASH_FAIL_LEN,
+	TCP_HASH_FAIL_MIS,
+	TCP_HASH_FAIL_NOKEY,
+	TCP_HASH_FAIL_NOID,
+};
+
+void __tcp_hash_fail(u8 reason, u8 family, const struct sk_buff *skb,
+		     int arg0, int arg1, int arg2);
 
-#define tcp_hash_fail(msg, family, skb, fmt, ...)			\
-do {									\
-	const struct tcphdr *th = tcp_hdr(skb);				\
-	char hdr_flags[6];						\
-	char *f = hdr_flags;						\
-									\
-	if (!tcp_hash_should_produce_warnings())			\
-		break;							\
-	if (th->fin)							\
-		*f++ = 'F';						\
-	if (th->syn)							\
-		*f++ = 'S';						\
-	if (th->rst)							\
-		*f++ = 'R';						\
-	if (th->psh)							\
-		*f++ = 'P';						\
-	if (th->ack)							\
-		*f++ = '.';						\
-	*f = 0;								\
-	if ((family) == AF_INET) {					\
-		net_info_ratelimited("%s for %pI4.%d->%pI4.%d [%s] " fmt "\n", \
-				msg, &ip_hdr(skb)->saddr, ntohs(th->source), \
-				&ip_hdr(skb)->daddr, ntohs(th->dest),	\
-				hdr_flags, ##__VA_ARGS__);		\
-	} else {							\
-		net_info_ratelimited("%s for [%pI6c].%d->[%pI6c].%d [%s]" fmt "\n", \
-				msg, &ipv6_hdr(skb)->saddr, ntohs(th->source), \
-				&ipv6_hdr(skb)->daddr, ntohs(th->dest),	\
-				hdr_flags, ##__VA_ARGS__);		\
-	}								\
+#define _tcp_hash_fail(reason, family, skb, ua, ...) do {		    \
+	if (static_branch_tcp_md5() || static_branch_tcp_ao()) {	    \
+		int ua[] = { __VA_ARGS__ + 0, 0, 0, 0, };		    \
+									    \
+		__tcp_hash_fail(reason, family, skb, ua[0], ua[1], ua[2]);  \
+	}								    \
 } while (0)
+#define tcp_hash_fail(reason, family, skb, ...)				    \
+	_tcp_hash_fail(reason, family, skb, __UNIQUE_ID(args_),		    \
+		       ##__VA_ARGS__)
 
 #ifdef CONFIG_TCP_AO
 /* TCP-AO structures and functions */
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index e767721b3a58..a8a9e0f7f768 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4379,6 +4379,116 @@ int tcp_getsockopt(struct sock *sk, int level, int optname, char __user *optval,
 }
 EXPORT_SYMBOL(tcp_getsockopt);
 
+#if defined(CONFIG_TCP_MD5SIG) || defined(CONFIG_TCP_AO)
+static const char *tcp_hash_fail_common(u8 family, const struct sk_buff *skb)
+{
+	const struct tcphdr *th = tcp_hdr(skb);
+	char hdr_flags[6];
+	char *f = hdr_flags;
+
+	if (th->fin)
+		*f++ = 'F';
+	if (th->syn)
+		*f++ = 'S';
+	if (th->rst)
+		*f++ = 'R';
+	if (th->psh)
+		*f++ = 'P';
+	if (th->ack)
+		*f++ = '.';
+	*f = '\0';
+
+	if (family == AF_INET)
+		return kasprintf(GFP_ATOMIC,
+				 " for %pI4.%d->%pI4.%d [%s] ",
+				 &ip_hdr(skb)->saddr, ntohs(th->source),
+				 &ip_hdr(skb)->daddr, ntohs(th->dest),
+				 hdr_flags);
+	else
+		return kasprintf(GFP_ATOMIC,
+				 " for [%pI6c].%d->[%pI6c].%d [%s] ",
+				 &ipv6_hdr(skb)->saddr, ntohs(th->source),
+				 &ipv6_hdr(skb)->daddr, ntohs(th->dest),
+				 hdr_flags);
+}
+
+void __tcp_hash_fail(u8 reason, u8 family, const struct sk_buff *skb,
+		     int arg0, int arg1, int arg2)
+{
+	const char *common __free(kfree) = NULL;
+
+	if (!net_ratelimit())
+		return;
+
+	common = tcp_hash_fail_common(family, skb);
+	if (!common)
+		return;
+
+#define tcp_hash_fail_msg(msg, common, fmt, ...)		\
+	pr_info(msg "%s" fmt "\n", common, ##__VA_ARGS__)
+	switch (reason) {
+	case TCP_HASH_FAIL_OPTS:
+		tcp_hash_fail_msg("TCP segment has incorrect auth options set",
+				  common, "");
+		break;
+	case TCP_HASH_FAIL_CONN:
+		tcp_hash_fail_msg("TCP connection can't start/end using TCP-AO",
+				  common, "%s",
+				  arg0 ? "missing AO" : "AO signed");
+		break;
+	case TCP_HASH_FAIL_NOAO:
+		tcp_hash_fail_msg("AO hash is required, but not found",
+				  common, "L3 index %d", arg0);
+		break;
+	case TCP_HASH_FAIL_NOMD5:
+		tcp_hash_fail_msg("MD5 Hash not found", common, "L3 index %d",
+				  arg0);
+		break;
+	case TCP_HASH_FAIL_UNEXP:
+		tcp_hash_fail_msg("Unexpected MD5 Hash found", common, "");
+		break;
+	case TCP_HASH_FAIL_INET:
+		if (family == AF_INET) {
+			tcp_hash_fail_msg("MD5 Hash failed", common,
+					  "%s L3 index %d",
+					  arg0 ?
+					  "tcp_v4_calc_md5_hash failed" : "",
+					  arg1);
+		} else {
+			if (arg0)
+				tcp_hash_fail_msg("MD5 Hash failed",
+						  common, "L3 index %d",
+						  arg1);
+			else
+				tcp_hash_fail_msg("MD5 Hash mismatch",
+						  common, "L3 index %d",
+						  arg1);
+		}
+		break;
+	case TCP_HASH_FAIL_LEN:
+		tcp_hash_fail_msg("AO hash wrong length", common,
+				  "%u != %d L3 index: %d", arg0, arg1, arg2);
+		break;
+	case TCP_HASH_FAIL_MIS:
+		tcp_hash_fail_msg("AO hash mismatch", common, "L3 index: %d",
+				  arg0);
+		break;
+	case TCP_HASH_FAIL_NOKEY:
+		tcp_hash_fail_msg("AO key not found", common,
+				  "keyid: %u L3 index: %d", arg0, arg1);
+		break;
+	case TCP_HASH_FAIL_NOID:
+		tcp_hash_fail_msg("Requested by the peer AO key id not found",
+				  common, "L3 index: %d", arg0);
+		break;
+	default:
+		break;
+	}
+#undef tcp_hash_fail_msg
+}
+EXPORT_SYMBOL(__tcp_hash_fail);
+#endif /* CONFIG_TCP_MD5SIG || CONFIG_TCP_AO */
+
 #ifdef CONFIG_TCP_MD5SIG
 int tcp_md5_sigpool_id = -1;
 EXPORT_SYMBOL_GPL(tcp_md5_sigpool_id);
@@ -4450,7 +4560,7 @@ tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
 
 	if (!key && hash_location) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5UNEXPECTED);
-		tcp_hash_fail("Unexpected MD5 Hash found", family, skb, "");
+		tcp_hash_fail(TCP_HASH_FAIL_UNEXP, family, skb);
 		return SKB_DROP_REASON_TCP_MD5UNEXPECTED;
 	}
 
@@ -4465,21 +4575,8 @@ tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
 							 NULL, skb);
 	if (genhash || memcmp(hash_location, newhash, 16) != 0) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE);
-		if (family == AF_INET) {
-			tcp_hash_fail("MD5 Hash failed", AF_INET, skb, "%s L3 index %d",
-				      genhash ? "tcp_v4_calc_md5_hash failed"
-				      : "", l3index);
-		} else {
-			if (genhash) {
-				tcp_hash_fail("MD5 Hash failed",
-					      AF_INET6, skb, "L3 index %d",
-					      l3index);
-			} else {
-				tcp_hash_fail("MD5 Hash mismatch",
-					      AF_INET6, skb, "L3 index %d",
-					      l3index);
-			}
-		}
+		tcp_hash_fail(TCP_HASH_FAIL_INET, family, skb, genhash,
+			      l3index);
 		return SKB_DROP_REASON_TCP_MD5FAILURE;
 	}
 	return SKB_NOT_DROPPED_YET;
diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
index 3afeeb68e8a7..f4dbb23e549b 100644
--- a/net/ipv4/tcp_ao.c
+++ b/net/ipv4/tcp_ao.c
@@ -892,8 +892,7 @@ tcp_ao_verify_hash(const struct sock *sk, const struct sk_buff *skb,
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
 		atomic64_inc(&info->counters.pkt_bad);
 		atomic64_inc(&key->pkt_bad);
-		tcp_hash_fail("AO hash wrong length", family, skb,
-			      "%u != %d L3index: %d", maclen,
+		tcp_hash_fail(TCP_HASH_FAIL_LEN, family, skb, maclen,
 			      tcp_ao_maclen(key), l3index);
 		return SKB_DROP_REASON_TCP_AOFAILURE;
 	}
@@ -909,8 +908,7 @@ tcp_ao_verify_hash(const struct sock *sk, const struct sk_buff *skb,
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOBAD);
 		atomic64_inc(&info->counters.pkt_bad);
 		atomic64_inc(&key->pkt_bad);
-		tcp_hash_fail("AO hash mismatch", family, skb,
-			      "L3index: %d", l3index);
+		tcp_hash_fail(TCP_HASH_FAIL_MIS, family, skb, l3index);
 		kfree(hash_buf);
 		return SKB_DROP_REASON_TCP_AOFAILURE;
 	}
@@ -938,8 +936,8 @@ tcp_inbound_ao_hash(struct sock *sk, const struct sk_buff *skb,
 	info = rcu_dereference(tcp_sk(sk)->ao_info);
 	if (!info) {
 		NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOKEYNOTFOUND);
-		tcp_hash_fail("AO key not found", family, skb,
-			      "keyid: %u L3index: %d", aoh->keyid, l3index);
+		tcp_hash_fail(TCP_HASH_FAIL_NOKEY, family, skb, aoh->keyid,
+			      l3index);
 		return SKB_DROP_REASON_TCP_AOUNEXPECTED;
 	}
 
@@ -1041,8 +1039,7 @@ tcp_inbound_ao_hash(struct sock *sk, const struct sk_buff *skb,
 key_not_found:
 	NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAOKEYNOTFOUND);
 	atomic64_inc(&info->counters.key_not_found);
-	tcp_hash_fail("Requested by the peer AO key id not found",
-		      family, skb, "L3index: %d", l3index);
+	tcp_hash_fail(TCP_HASH_FAIL_NOID, family, skb, l3index);
 	return SKB_DROP_REASON_TCP_AOKEYNOTFOUND;
 }
 
-- 
2.44.0


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ