[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

[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

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

[kernel test robot]

:::::: 
:::::: Manual check reason: "low confidence bisect report"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72@arista.com>
References: <20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72@arista.com>
TO: Dmitry Safonov <dima@arista.com>

Hi Dmitry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ac865f00af293d081356bec56eea90815094a60e]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Safonov/net-tcp-Only-produce-AO-MD5-logs-if-there-are-any-keys/20240104-214510
base:   ac865f00af293d081356bec56eea90815094a60e
patch link:    https://lore.kernel.org/r/20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72%40arista.com
patch subject: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202401061952.k6UqFm0j-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> include/net/tcp_ao.h: linux/jump_label.h is included more than once.

vim +131 include/net/tcp_ao.h

   129	
   130	#ifdef CONFIG_TCP_MD5SIG
 > 131	#include <linux/jump_label.h>
   132	extern struct static_key_false_deferred tcp_md5_needed;
   133	#define static_branch_tcp_md5()	static_branch_unlikely(&tcp_md5_needed.key)
   134	#else
   135	#define static_branch_tcp_md5()	false
   136	#endif
   137	#ifdef CONFIG_TCP_AO
   138	/* TCP-AO structures and functions */
 > 139	#include <linux/jump_label.h>
   140	extern struct static_key_false_deferred tcp_ao_needed;
   141	#define static_branch_tcp_ao()	static_branch_unlikely(&tcp_ao_needed.key)
   142	#else
   143	#define static_branch_tcp_ao()	false
   144	#endif
   145	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Liu, Yujie]

not_report

duplicate include under different #ifdef macro

-----Original Message-----
From: lkp <lkp@intel.com> 
Sent: Saturday, January 6, 2024 20:12
To: oe-kbuild@lists.linux.dev
Cc: lkp <lkp@intel.com>
Subject: Re: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys

:::::: 
:::::: Manual check reason: "low confidence bisect report"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72@arista.com>
References: <20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72@arista.com>
TO: Dmitry Safonov <dima@arista.com>

Hi Dmitry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on ac865f00af293d081356bec56eea90815094a60e]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Safonov/net-tcp-Only-produce-AO-MD5-logs-if-there-are-any-keys/20240104-214510
base:   ac865f00af293d081356bec56eea90815094a60e
patch link:    https://lore.kernel.org/r/20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72%40arista.com
patch subject: [PATCH] net/tcp: Only produce AO/MD5 logs if there are any keys
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202401061952.k6UqFm0j-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> include/net/tcp_ao.h: linux/jump_label.h is included more than once.

vim +131 include/net/tcp_ao.h

   129	
   130	#ifdef CONFIG_TCP_MD5SIG
 > 131	#include <linux/jump_label.h>
   132	extern struct static_key_false_deferred tcp_md5_needed;
   133	#define static_branch_tcp_md5()	static_branch_unlikely(&tcp_md5_needed.key)
   134	#else
   135	#define static_branch_tcp_md5()	false
   136	#endif
   137	#ifdef CONFIG_TCP_AO
   138	/* TCP-AO structures and functions */
 > 139	#include <linux/jump_label.h>
   140	extern struct static_key_false_deferred tcp_ao_needed;
   141	#define static_branch_tcp_ao()	static_branch_unlikely(&tcp_ao_needed.key)
   142	#else
   143	#define static_branch_tcp_ao()	false
   144	#endif
   145	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki