diff mbox series

tcp: Add listening address to SYN flood message

Message ID f847459dc0a0e2d8ffa1d290d06e0e4a226a6f39.1668075479.git.jamie.bainbridge@gmail.com
State New
Headers
Series tcp: Add listening address to SYN flood message |

Commit Message

Jamie Bainbridge Nov. 10, 2022, 10:21 a.m. UTC 
  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 <jamie.bainbridge@gmail.com>
---
 net/ipv4/tcp_input.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Andrew Lunn Nov. 10, 2022, 1:51 p.m. UTC | #1 
On Thu, Nov 10, 2022 at 09:21:06PM +1100, Jamie Bainbridge wrote:
> 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 <jamie.bainbridge@gmail.com>
> ---
>  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
Jamie Bainbridge Nov. 10, 2022, 9:20 p.m. UTC | #2 
On Fri, 11 Nov 2022 at 00:51, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Nov 10, 2022 at 09:21:06PM +1100, Jamie Bainbridge wrote:
> > 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 <jamie.bainbridge@gmail.com>
> > ---
> >  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?

Jamie
Andrew Lunn Nov. 10, 2022, 10:30 p.m. UTC | #3 
On Fri, Nov 11, 2022 at 08:20:18AM +1100, Jamie Bainbridge wrote:
> On Fri, 11 Nov 2022 at 00:51, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Thu, Nov 10, 2022 at 09:21:06PM +1100, Jamie Bainbridge wrote:
> > > 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 <jamie.bainbridge@gmail.com>
> > > ---
> > >  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);
		}

The IS_ENABLED(CONFIG_IPV6) will evaluate to 0 at compile time, and
the optimiser will throw away the whole lot since it can never be
true. However, before the code gets to the optimiser, it first needs
to compile. It will check you have the correct number of parameters
for the string format, do the types match, do the structure members
exist, etc. Anybody doing compile testing of a change, and they have
IPV6 turned off, has a chance off getting errors reported when they
have actually broken IPV6, but don't know it, because they are not
compiling it.

Now, IPV6 is one of those big options which i expect 0-day tests quite
regularly. Using IF_ENABLED() like this brings more benefit from less
used options which gets very little build testing, and so are often
broke until somebody like Arnd runs builds with lots of random
configs.

	Andrew
Stephen Hemminger Nov. 10, 2022, 11:39 p.m. UTC | #4 
On Thu, 10 Nov 2022 21:21:06 +1100
Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:

> +	    xchg(&queue->synflood_warned, 1) == 0) {
> +#if IS_ENABLED(CONFIG_IPV6)
> +		if (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);
> +		} else
> +#endif
> +		{
> +			net_info_ratelimited("%s: Possible SYN flooding on port %d. IP %pI4. %s.  Check SNMP counters.\n",
> +					proto, sk->sk_num, &sk->sk_rcv_saddr, msg);
> +		}
> +	}
>  

Port number is unsigned not signed.
Message also seems overly wordy to me.
Eric Dumazet Nov. 10, 2022, 11:46 p.m. UTC | #5 
On Thu, Nov 10, 2022 at 3:39 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 10 Nov 2022 21:21:06 +1100
> Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
>
> > +         xchg(&queue->synflood_warned, 1) == 0) {
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +             if (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);
> > +             } else
> > +#endif
> > +             {
> > +                     net_info_ratelimited("%s: Possible SYN flooding on port %d. IP %pI4. %s.  Check SNMP counters.\n",
> > +                                     proto, sk->sk_num, &sk->sk_rcv_saddr, msg);
> > +             }
> > +     }
> >
>
> Port number is unsigned not signed.
> Message also seems overly wordy to me.

Also, it is customary to use  IP.port format (like most tools, see tcpdump)
Jamie Bainbridge Nov. 11, 2022, 12:38 a.m. UTC | #6 
On Fri, 11 Nov 2022 at 09:39, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 10 Nov 2022 21:21:06 +1100
> Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
>
> > +         xchg(&queue->synflood_warned, 1) == 0) {
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +             if (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);
> > +             } else
> > +#endif
> > +             {
> > +                     net_info_ratelimited("%s: Possible SYN flooding on port %d. IP %pI4. %s.  Check SNMP counters.\n",
> > +                                     proto, sk->sk_num, &sk->sk_rcv_saddr, msg);
> > +             }
> > +     }
> >
>
> Port number is unsigned not signed.
> Message also seems overly wordy to me.

Thanks for bringing this up. I agree with you.

I'd like to remove "Check SNMP counters" as it's not helpful to users.

How do they do that? (note userspace has changed from net-tools
"netstat -s" to iproute "nstat" since this message was added). Check
counters for what? If they even figure out the LISTEN stats are
growing, there's still troubleshooting to determine if the SYNs are
genuine or malicious, check/increase somaxconn and the socket listen()
backlog, check/improve application accept() performance, etc...

This is way too much to describe in a kernel log message, and it's the
job of the log message to be "descriptive" of what happened, not
"prescriptive" of policy to follow and cover every troubleshooting
possibility.

I will re-submit with a second patch removing this phrase.

Jamie
Geert Uytterhoeven Nov. 15, 2022, 10:29 a.m. UTC | #7 
Hi Andrew,

On Thu, Nov 10, 2022 at 11:42 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Nov 11, 2022 at 08:20:18AM +1100, Jamie Bainbridge wrote:
> > On Fri, 11 Nov 2022 at 00:51, Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Thu, Nov 10, 2022 at 09:21:06PM +1100, Jamie Bainbridge wrote:
> > > > 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 <jamie.bainbridge@gmail.com>
> > > > ---
> > > >  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

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
diff mbox series

Patch

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) {
+			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);
+		} else
+#endif
+		{
+			net_info_ratelimited("%s: Possible SYN flooding on port %d. IP %pI4. %s.  Check SNMP counters.\n",
+					proto, sk->sk_num, &sk->sk_rcv_saddr, msg);
+		}
+	}
 
 	return want_cookie;
 }