[PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Dmitry Safonov]

User won't care about inproper hash options in the TCP header if they
don't use neither TCP-AO nor TCP-MD5. Yet, those logs can add up in
syslog, while not being a real concern to the host admin:
> kernel: TCP: TCP segment has incorrect auth options set for XX.20.239.12.54681->XX.XX.90.103.80 [S]

Keep silent and avoid logging when there aren't any keys in the system.

Side-note: I also defined static_branch_tcp_*() helpers to avoid more
ifdeffery, going to remove more ifdeffery further with their help.

Reported-by: Christian Kujau <lists@nerdbynature.de>
Closes: https://lore.kernel.org/all/f6b59324-1417-566f-a976-ff2402718a8d@nerdbynature.de/
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 include/net/tcp.h    |  2 --
 include/net/tcp_ao.h | 26 +++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 144ba48bb07b..87f0e6c2e1f2 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1788,8 +1788,6 @@ struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk,
 					 const struct sock *addr_sk);
 
 #ifdef CONFIG_TCP_MD5SIG
-#include <linux/jump_label.h>
-extern struct static_key_false_deferred tcp_md5_needed;
 struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,
 					   const union tcp_md5_addr *addr,
 					   int family, bool any_l3index);
diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index 647781080613..b04afced4cc9 100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -127,12 +127,35 @@ struct tcp_ao_info {
 	struct rcu_head		rcu;
 };
 
+#ifdef CONFIG_TCP_MD5SIG
+#include <linux/jump_label.h>
+extern struct static_key_false_deferred tcp_md5_needed;
+#define static_branch_tcp_md5()	static_branch_unlikely(&tcp_md5_needed.key)
+#else
+#define static_branch_tcp_md5()	false
+#endif
+#ifdef CONFIG_TCP_AO
+/* TCP-AO structures and functions */
+#include <linux/jump_label.h>
+extern struct static_key_false_deferred tcp_ao_needed;
+#define static_branch_tcp_ao()	static_branch_unlikely(&tcp_ao_needed.key)
+#else
+#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();
+}
+
 #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)							\
@@ -159,9 +182,6 @@ do {									\
 
 #ifdef CONFIG_TCP_AO
 /* TCP-AO structures and functions */
-#include <linux/jump_label.h>
-extern struct static_key_false_deferred tcp_ao_needed;
-
 struct tcp4_ao_context {
 	__be32		saddr;
 	__be32		daddr;

---
base-commit: ac865f00af293d081356bec56eea90815094a60e
change-id: 20240104-tcp_hash_fail-logs-daa1a4dde694

Best regards,
-- 
Dmitry Safonov <dima@arista.com>

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Dmitry Safonov]

On 1/4/24 13:42, Dmitry Safonov wrote:
> User won't care about inproper hash options in the TCP header if they
> don't use neither TCP-AO nor TCP-MD5. Yet, those logs can add up in
> syslog, while not being a real concern to the host admin:
>> kernel: TCP: TCP segment has incorrect auth options set for XX.20.239.12.54681->XX.XX.90.103.80 [S]
> 
> Keep silent and avoid logging when there aren't any keys in the system.
> 
> Side-note: I also defined static_branch_tcp_*() helpers to avoid more
> ifdeffery, going to remove more ifdeffery further with their help.
> 
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> Closes: https://lore.kernel.org/all/f6b59324-1417-566f-a976-ff2402718a8d@nerdbynature.de/

Probably, it also can have

Fixes: 2717b5adea9e ("net/tcp: Add tcp_hash_fail() ratelimited logs")

> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  include/net/tcp.h    |  2 --
>  include/net/tcp_ao.h | 26 +++++++++++++++++++++++---
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 144ba48bb07b..87f0e6c2e1f2 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1788,8 +1788,6 @@ struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk,
>  					 const struct sock *addr_sk);
>  
>  #ifdef CONFIG_TCP_MD5SIG
> -#include <linux/jump_label.h>
> -extern struct static_key_false_deferred tcp_md5_needed;
>  struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,
>  					   const union tcp_md5_addr *addr,
>  					   int family, bool any_l3index);
> diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
> index 647781080613..b04afced4cc9 100644
> --- a/include/net/tcp_ao.h
> +++ b/include/net/tcp_ao.h
> @@ -127,12 +127,35 @@ struct tcp_ao_info {
>  	struct rcu_head		rcu;
>  };
>  
> +#ifdef CONFIG_TCP_MD5SIG
> +#include <linux/jump_label.h>
> +extern struct static_key_false_deferred tcp_md5_needed;
> +#define static_branch_tcp_md5()	static_branch_unlikely(&tcp_md5_needed.key)
> +#else
> +#define static_branch_tcp_md5()	false
> +#endif
> +#ifdef CONFIG_TCP_AO
> +/* TCP-AO structures and functions */
> +#include <linux/jump_label.h>
> +extern struct static_key_false_deferred tcp_ao_needed;
> +#define static_branch_tcp_ao()	static_branch_unlikely(&tcp_ao_needed.key)
> +#else
> +#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();
> +}
> +
>  #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)							\
> @@ -159,9 +182,6 @@ do {									\
>  
>  #ifdef CONFIG_TCP_AO
>  /* TCP-AO structures and functions */
> -#include <linux/jump_label.h>
> -extern struct static_key_false_deferred tcp_ao_needed;
> -
>  struct tcp4_ao_context {
>  	__be32		saddr;
>  	__be32		daddr;
> 
> ---
> base-commit: ac865f00af293d081356bec56eea90815094a60e
> change-id: 20240104-tcp_hash_fail-logs-daa1a4dde694
> 
> Best regards,

-- 
Dmitry

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Jakub Kicinski]

On Thu,  4 Jan 2024 13:42:39 +0000 Dmitry Safonov wrote:
> User won't care about inproper hash options in the TCP header if they
> don't use neither TCP-AO nor TCP-MD5. Yet, those logs can add up in
> syslog, while not being a real concern to the host admin:
> > kernel: TCP: TCP segment has incorrect auth options set for XX.20.239.12.54681->XX.XX.90.103.80 [S]  
> 
> Keep silent and avoid logging when there aren't any keys in the system.
> 
> Side-note: I also defined static_branch_tcp_*() helpers to avoid more
> ifdeffery, going to remove more ifdeffery further with their help.

Wouldn't we be better off converting the prints to trace points. 
The chances for hitting them due to malicious packets feels much
higher than dealing with a buggy implementation in the wild.

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Dmitry Safonov]

Hi Jakub,

On 1/4/24 15:57, Jakub Kicinski wrote:
> On Thu,  4 Jan 2024 13:42:39 +0000 Dmitry Safonov wrote:
>> User won't care about inproper hash options in the TCP header if they
>> don't use neither TCP-AO nor TCP-MD5. Yet, those logs can add up in
>> syslog, while not being a real concern to the host admin:
>>> kernel: TCP: TCP segment has incorrect auth options set for XX.20.239.12.54681->XX.XX.90.103.80 [S]  
>>
>> Keep silent and avoid logging when there aren't any keys in the system.
>>
>> Side-note: I also defined static_branch_tcp_*() helpers to avoid more
>> ifdeffery, going to remove more ifdeffery further with their help.
> 
> Wouldn't we be better off converting the prints to trace points. 
> The chances for hitting them due to malicious packets feels much
> higher than dealing with a buggy implementation in the wild.

Do you mean a proper stuff like in net/core/net-traces.c or just
lowering the loglevel to net_dbg_ratelimited() [like Christian
originally proposed], which in turns becomes runtime enabled/disabled?

Both seem fine to me, albeit I was a bit reluctant to change it without
a good reason as even pre- 2717b5adea9e TCP-MD5 messages were logged and
some userspace may expect them. I guess we can try and see if anyone
notices/complains over changes to these messages changes or not.

Thanks,
             Dmitry

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Jakub Kicinski]

On Thu, 4 Jan 2024 16:42:05 +0000 Dmitry Safonov wrote:
> >> Keep silent and avoid logging when there aren't any keys in the system.
> >>
> >> Side-note: I also defined static_branch_tcp_*() helpers to avoid more
> >> ifdeffery, going to remove more ifdeffery further with their help.  
> > 
> > Wouldn't we be better off converting the prints to trace points. 
> > The chances for hitting them due to malicious packets feels much
> > higher than dealing with a buggy implementation in the wild.  
> 
> Do you mean a proper stuff like in net/core/net-traces.c or just
> lowering the loglevel to net_dbg_ratelimited() [like Christian
> originally proposed], which in turns becomes runtime enabled/disabled?

I mean proper tracepoints.

> Both seem fine to me, albeit I was a bit reluctant to change it without
> a good reason as even pre- 2717b5adea9e TCP-MD5 messages were logged and
> some userspace may expect them. I guess we can try and see if anyone
> notices/complains over changes to these messages changes or not.

Hm. Perhaps we can do the conversion in net-next. Let me ping Eric :)

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Eric Dumazet]

On Thu, Jan 4, 2024 at 5:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 4 Jan 2024 16:42:05 +0000 Dmitry Safonov wrote:
> > >> Keep silent and avoid logging when there aren't any keys in the system.
> > >>
> > >> Side-note: I also defined static_branch_tcp_*() helpers to avoid more
> > >> ifdeffery, going to remove more ifdeffery further with their help.
> > >
> > > Wouldn't we be better off converting the prints to trace points.
> > > The chances for hitting them due to malicious packets feels much
> > > higher than dealing with a buggy implementation in the wild.
> >
> > Do you mean a proper stuff like in net/core/net-traces.c or just
> > lowering the loglevel to net_dbg_ratelimited() [like Christian
> > originally proposed], which in turns becomes runtime enabled/disabled?
>
> I mean proper tracepoints.
>
> > Both seem fine to me, albeit I was a bit reluctant to change it without
> > a good reason as even pre- 2717b5adea9e TCP-MD5 messages were logged and
> > some userspace may expect them. I guess we can try and see if anyone
> > notices/complains over changes to these messages changes or not.
>
> Hm. Perhaps we can do the conversion in net-next. Let me ping Eric :)

Sure, let's wait for the next release for a conversion, thanks !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  4 Jan 2024 13:42:39 +0000 you wrote:
> User won't care about inproper hash options in the TCP header if they
> don't use neither TCP-AO nor TCP-MD5. Yet, those logs can add up in
> syslog, while not being a real concern to the host admin:
> > kernel: TCP: TCP segment has incorrect auth options set for XX.20.239.12.54681->XX.XX.90.103.80 [S]
> 
> Keep silent and avoid logging when there aren't any keys in the system.
> 
> [...]

Here is the summary with links:
  - net/tcp: Only produce AO/MD5 logs if there are any keys
    https://git.kernel.org/netdev/net/c/4c8530dc7d7d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

[Dmitry Safonov]

On 1/4/24 16:59, Eric Dumazet wrote:
> On Thu, Jan 4, 2024 at 5:59 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Thu, 4 Jan 2024 16:42:05 +0000 Dmitry Safonov wrote:
>>>>> Keep silent and avoid logging when there aren't any keys in the system.
>>>>>
>>>>> Side-note: I also defined static_branch_tcp_*() helpers to avoid more
>>>>> ifdeffery, going to remove more ifdeffery further with their help.
>>>>
>>>> Wouldn't we be better off converting the prints to trace points.
>>>> The chances for hitting them due to malicious packets feels much
>>>> higher than dealing with a buggy implementation in the wild.
>>>
>>> Do you mean a proper stuff like in net/core/net-traces.c or just
>>> lowering the loglevel to net_dbg_ratelimited() [like Christian
>>> originally proposed], which in turns becomes runtime enabled/disabled?
>>
>> I mean proper tracepoints.
>>
>>> Both seem fine to me, albeit I was a bit reluctant to change it without
>>> a good reason as even pre- 2717b5adea9e TCP-MD5 messages were logged and
>>> some userspace may expect them. I guess we can try and see if anyone
>>> notices/complains over changes to these messages changes or not.

[to add up context]
I supposed it's only tests that grep for those messages, but I've looked
up the code-base and it's wired up to daemon's code to monitor messages
with a "filter" for rsyslogd. Certainly not an issue for arista as there
are people maintaining that (and AFAIK, rasdaemon is already used for
other traces), but I guess provides grounds for my concerns over other
projects.

>> Hm. Perhaps we can do the conversion in net-next. Let me ping Eric :)
> 
> Sure, let's wait for the next release for a conversion, thanks !
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks!

I'll do the conversion for net-next if you don't mind :-)

That will be pretty nice as it's going to be easy to exercise in tcp-ao
selftests. Grepping dmesg can't be selftested as reliably/non-flaky.

Thanks,
            Dmitry