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 <[email protected]>
---
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
Hi Alexander,
On Wed, Apr 3, 2024 at 2:26 PM Alexander Lobakin
<[email protected]> 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/[email protected]/
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
From: Dmitry Safonov <[email protected]>
Date: Wed, 3 Apr 2024 18:00:49 +0100
> Hi Alexander,
>
> On Wed, Apr 3, 2024 at 2:26 PM Alexander Lobakin
> <[email protected]> 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/[email protected]/
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