| Message ID | 20240104-tcp_hash_fail-logs-v1-1-ff3e1f6f9e72@arista.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4c8530dc7d7da4abe97d65e8e038ce9852491369 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/tcp: Only produce AO/MD5 logs if there are any keys | expand |
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,
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.
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
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 :)
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>
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!
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
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;