Re: [PATCH] tcp: Add listening address to SYN flood message

Hi Andrew,

On Thu, Nov 10, 2022 at 11:42 PM Andrew Lunn [off-list ref] wrote:

On Fri, Nov 11, 2022 at 08:20:18AM +1100, Jamie Bainbridge wrote:

quoted

On Fri, 11 Nov 2022 at 00:51, Andrew Lunn [off-list ref] wrote:

quoted

On Thu, Nov 10, 2022 at 09:21:06PM +1100, Jamie Bainbridge wrote:

quoted

The SYN flood message prints the listening port number, but on a system
with many processes bound to the same port on different IPs, it's
impossible to tell which socket is the problem.

Add the listen IP address to the SYN flood message. It might have been
nicer to print the address first, but decades of monitoring tools are
watching for the string "SYN flooding on port" so don't break that.

Tested with each protcol's "any" address and a host address:

 Possible SYN flooding on port 9001. IP 0.0.0.0.
 Possible SYN flooding on port 9001. IP 127.0.0.1.
 Possible SYN flooding on port 9001. IP ::.
 Possible SYN flooding on port 9001. IP fc00::1.

Signed-off-by: Jamie Bainbridge <redacted>
---
 net/ipv4/tcp_input.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0640453fce54b6daae0861d948f3db075830daf6..fb86056732266fedc8ad574bbf799dbdd7a425a3 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -6831,9 +6831,19 @@ static bool tcp_syn_flood_action(const struct sock *sk, const char *proto)
              __NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPREQQFULLDROP);

      if (!queue->synflood_warned && syncookies != 2 &&
-         xchg(&queue->synflood_warned, 1) == 0)
-             net_info_ratelimited("%s: Possible SYN flooding on port %d. %s.  Check SNMP counters.\n",
-                                  proto, sk->sk_num, msg);
+         xchg(&queue->synflood_warned, 1) == 0) {
+#if IS_ENABLED(CONFIG_IPV6)
+             if (sk->sk_family == AF_INET6) {

Can the IS_ENABLED() go inside the if? You get better build testing
that way.

     Andrew

Are you sure? Why would the IS_ENABLED() be inside of a condition
which isn't compiled in? If IPv6 isn't compiled in then the condition
would never evaluate as true, so seems pointless a pointless
comparison to make? People not compiling in IPv6 have explicitly asked
*not* to have their kernel filled with a bunch of "if (family ==
AF_INET6)" haven't they?

There are many other examples of this pattern of "IS_ENABLED()" first
and "if (family == AF_INET6)" inside it, but I can't see any of the
inverse which I think you're suggesting, see:

 grep -C1 -ERHn "IS_ENABLED\(CONFIG_IPV6\)" net | grep -C1 "family == AF_INET6"

Please let me know if I've misunderstood?

So what i'm suggesting is

               if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
                       net_info_ratelimited("%s: Possible SYN flooding on port %d. IP %pI6c. %s.  Check SNMP counters.\n",
                                       proto, sk->sk_num,
                                       &sk->sk_v6_rcv_saddr, msg);
                }

Unfortunately the IPv6-specific members are not defined if
CONFIG_IPV6=n. Patch sent.

https://lore.kernel.org/netdev/d1ecf500f07e063d4e8e34f4045ddca55416c686.1668507036.git.geert+renesas@glider.be (local)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds