diff mbox series

[net-next] net/tcp: move TCP hash fail messages out of line

Message ID 20240403132456.41709-1-aleksander.lobakin@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net/tcp: move TCP hash fail messages out of line | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1847 this patch: 1847
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 5 maintainers not CCed: justinstitt@google.com ndesaulniers@google.com llvm@lists.linux.dev morbo@google.com nathan@kernel.org
netdev/build_clang success Errors and warnings before: 974 this patch: 974
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1883 this patch: 1883
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 287 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest warning net-next-2024-04-03--21-00 (tests: 951)

Commit Message

Alexander Lobakin April 3, 2024, 1:24 p.m. UTC
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@intel.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(-)

Comments

Dmitry Safonov April 3, 2024, 5 p.m. UTC | #1
Hi Alexander,

On Wed, Apr 3, 2024 at 2:26 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> 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

I can see that as an improvement, albeit that enum and the resulting switch
are quite gross, sorry.
I had patches to convert those messages to tracepoints (by Jakub's suggestion).
That seems to work quite nice and will remove this macro entirely:
https://lore.kernel.org/all/20240224-tcp-ao-tracepoints-v1-0-15f31b7f30a7@arista.com/

I need to send version 2 for that. Unfortunately, that got delayed by
me migrating
from my previous work laptop. That was not my choice, resulting in
little disruption.
I'm planning to send the new version optimistically by the end of this week,
at worst the next week.

Thanks,
            Dmitry
Alexander Lobakin April 4, 2024, 8:57 a.m. UTC | #2
From: Dmitry Safonov <dima@arista.com>
Date: Wed, 3 Apr 2024 18:00:49 +0100

> Hi Alexander,
> 
> On Wed, Apr 3, 2024 at 2:26 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> 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
> 
> I can see that as an improvement, albeit that enum and the resulting switch
> are quite gross, sorry.

I know, but that's the only way to move that print out of line.

> I had patches to convert those messages to tracepoints (by Jakub's suggestion).
> That seems to work quite nice and will remove this macro entirely:
> https://lore.kernel.org/all/20240224-tcp-ao-tracepoints-v1-0-15f31b7f30a7@arista.com/

Oh sorry, I didn't search lore for the related patches before sending.
Sounds good! My solution can be dropped then. Just make sure you have
the same code size reduction in tcp_*_hash() :D

> 
> I need to send version 2 for that. Unfortunately, that got delayed by
> me migrating
> from my previous work laptop. That was not my choice, resulting in
> little disruption.
> I'm planning to send the new version optimistically by the end of this week,
> at worst the next week.
> 
> Thanks,
>             Dmitry

Thanks,
Olek
diff mbox series

Patch

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