diff mbox series

[v2] tcp: Add listening address to SYN flood message

Message ID 7ccd58e8e26bcdd82e66993cbd53ff59eebe3949.1668139105.git.jamie.bainbridge@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2] tcp: Add listening address to SYN flood message | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/checkpatch warning CHECK: Alignment should match open parenthesis
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jamie Bainbridge Nov. 11, 2022, 3:59 a.m. UTC 
The SYN flood message prints the listening port number, but 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 in the "IP.port"
format like most other tools (eg: tcpdump).

Each protcol's "any" address and a host address now look like:

 Possible SYN flooding on port 0.0.0.0.9001.
 Possible SYN flooding on port 127.0.0.1.9001.
 Possible SYN flooding on port ::.9001.
 Possible SYN flooding on port fc00::1.9001.

Signed-off-by: Jamie Bainbridge <jamie.bainbridge@gmail.com>
---
v2: Place IS_ENABLED() inside if condition c/o Andrew Lunn.
    Change port printf to unsigned c/o Stephen Hemminger.
    Remove long and unhelpful "Check SNMP counters" c/o Stephen.
    Use IP.port format c/o Eric Dumazet.
---
 net/ipv4/tcp_input.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Stephen Hemminger Nov. 11, 2022, 5:20 p.m. UTC | #1 
On Fri, 11 Nov 2022 14:59:32 +1100
Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:

> +	    xchg(&queue->synflood_warned, 1) == 0) {
> +		if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> +			net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
> +					proto, &sk->sk_v6_rcv_saddr,
> +					sk->sk_num, msg);
> +		} else {
> +			net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
> +					proto, &sk->sk_rcv_saddr,
> +					sk->sk_num, msg);

Minor nit, the standard format for printing addresses would be to use colon seperator before port

		if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
			net_info_ratelimited("%s: Possible SYN flooding on [%pI6c]:%u. %s.\n",
					proto, &sk->sk_v6_rcv_saddr, sk->sk_num, msg);
		} else {
			net_info_ratelimited("%s: Possible SYN flooding on %pI4:%u. %s.\n",
					proto, &sk->sk_rcv_saddr, sk->sk_num, msg);
Jamie Bainbridge Nov. 11, 2022, 11:59 p.m. UTC | #2 
On Sat, 12 Nov 2022 at 04:20, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Fri, 11 Nov 2022 14:59:32 +1100
> Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
>
> > +         xchg(&queue->synflood_warned, 1) == 0) {
> > +             if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
> > +                                     proto, &sk->sk_v6_rcv_saddr,
> > +                                     sk->sk_num, msg);
> > +             } else {
> > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
> > +                                     proto, &sk->sk_rcv_saddr,
> > +                                     sk->sk_num, msg);
>
> Minor nit, the standard format for printing addresses would be to use colon seperator before port
>
>                 if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
>                         net_info_ratelimited("%s: Possible SYN flooding on [%pI6c]:%u. %s.\n",
>                                         proto, &sk->sk_v6_rcv_saddr, sk->sk_num, msg);
>                 } else {
>                         net_info_ratelimited("%s: Possible SYN flooding on %pI4:%u. %s.\n",
>                                         proto, &sk->sk_rcv_saddr, sk->sk_num, msg);

I considered this too, though Eric suggested "IP.port" to match tcpdump.

Please let me know which advice to follow?

Jamie
Eric Dumazet Nov. 12, 2022, 12:10 a.m. UTC | #3 
On Fri, Nov 11, 2022 at 4:00 PM Jamie Bainbridge
<jamie.bainbridge@gmail.com> wrote:
>
> On Sat, 12 Nov 2022 at 04:20, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Fri, 11 Nov 2022 14:59:32 +1100
> > Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
> >
> > > +         xchg(&queue->synflood_warned, 1) == 0) {
> > > +             if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
> > > +                                     proto, &sk->sk_v6_rcv_saddr,
> > > +                                     sk->sk_num, msg);
> > > +             } else {
> > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
> > > +                                     proto, &sk->sk_rcv_saddr,
> > > +                                     sk->sk_num, msg);
> >
> > Minor nit, the standard format for printing addresses would be to use colon seperator before port
> >
> >                 if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> >                         net_info_ratelimited("%s: Possible SYN flooding on [%pI6c]:%u. %s.\n",
> >                                         proto, &sk->sk_v6_rcv_saddr, sk->sk_num, msg);
> >                 } else {
> >                         net_info_ratelimited("%s: Possible SYN flooding on %pI4:%u. %s.\n",
> >                                         proto, &sk->sk_rcv_saddr, sk->sk_num, msg);
>
> I considered this too, though Eric suggested "IP.port" to match tcpdump.
>
> Please let me know which advice to follow?

IPv6 [address]:port is also a standard (and unambiguous) way.

https://www.rfc-editor.org/rfc/rfc5952#page-11
Stephen Hemminger Nov. 12, 2022, 12:11 a.m. UTC | #4 
On Sat, 12 Nov 2022 10:59:52 +1100
Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:

> On Sat, 12 Nov 2022 at 04:20, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Fri, 11 Nov 2022 14:59:32 +1100
> > Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
> >  
> > > +         xchg(&queue->synflood_warned, 1) == 0) {
> > > +             if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
> > > +                                     proto, &sk->sk_v6_rcv_saddr,
> > > +                                     sk->sk_num, msg);
> > > +             } else {
> > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
> > > +                                     proto, &sk->sk_rcv_saddr,
> > > +                                     sk->sk_num, msg);  
> >
> > Minor nit, the standard format for printing addresses would be to use colon seperator before port
> >
> >                 if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> >                         net_info_ratelimited("%s: Possible SYN flooding on [%pI6c]:%u. %s.\n",
> >                                         proto, &sk->sk_v6_rcv_saddr, sk->sk_num, msg);
> >                 } else {
> >                         net_info_ratelimited("%s: Possible SYN flooding on %pI4:%u. %s.\n",
> >                                         proto, &sk->sk_rcv_saddr, sk->sk_num, msg);  
> 
> I considered this too, though Eric suggested "IP.port" to match tcpdump.

That works, if it happens I doubt it matters.
Eric Dumazet Nov. 12, 2022, 12:14 a.m. UTC | #5 
On Fri, Nov 11, 2022 at 4:11 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Sat, 12 Nov 2022 10:59:52 +1100
> Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
>
> > On Sat, 12 Nov 2022 at 04:20, Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Fri, 11 Nov 2022 14:59:32 +1100
> > > Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
> > >
> > > > +         xchg(&queue->synflood_warned, 1) == 0) {
> > > > +             if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
> > > > +                                     proto, &sk->sk_v6_rcv_saddr,
> > > > +                                     sk->sk_num, msg);
> > > > +             } else {
> > > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
> > > > +                                     proto, &sk->sk_rcv_saddr,
> > > > +                                     sk->sk_num, msg);
> > >
> > > Minor nit, the standard format for printing addresses would be to use colon seperator before port
> > >
> > >                 if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > >                         net_info_ratelimited("%s: Possible SYN flooding on [%pI6c]:%u. %s.\n",
> > >                                         proto, &sk->sk_v6_rcv_saddr, sk->sk_num, msg);
> > >                 } else {
> > >                         net_info_ratelimited("%s: Possible SYN flooding on %pI4:%u. %s.\n",
> > >                                         proto, &sk->sk_rcv_saddr, sk->sk_num, msg);
> >
> > I considered this too, though Eric suggested "IP.port" to match tcpdump.
>
> That works, if it happens I doubt it matters.

Note that "ss dst" really needs the [] notation for IPv6

ss -t dst "[::1]"
State                  Recv-Q             Send-Q
    Local Address:Port                            Peer Address:Port
         Process
CLOSE-WAIT             1                  0
            [::1]:50584                                  [::1]:ipp

So we have inconsistency anyway...

As you said, no strong opinion.
Jamie Bainbridge Nov. 14, 2022, 12:37 a.m. UTC | #6 
On Sat, 12 Nov 2022 at 10:14, Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Nov 11, 2022 at 4:11 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Sat, 12 Nov 2022 10:59:52 +1100
> > Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
> >
> > > On Sat, 12 Nov 2022 at 04:20, Stephen Hemminger
> > > <stephen@networkplumber.org> wrote:
> > > >
> > > > On Fri, 11 Nov 2022 14:59:32 +1100
> > > > Jamie Bainbridge <jamie.bainbridge@gmail.com> wrote:
> > > >
> > > > > +         xchg(&queue->synflood_warned, 1) == 0) {
> > > > > +             if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > > > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
> > > > > +                                     proto, &sk->sk_v6_rcv_saddr,
> > > > > +                                     sk->sk_num, msg);
> > > > > +             } else {
> > > > > +                     net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
> > > > > +                                     proto, &sk->sk_rcv_saddr,
> > > > > +                                     sk->sk_num, msg);
> > > >
> > > > Minor nit, the standard format for printing addresses would be to use colon seperator before port
> > > >
> > > >                 if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> > > >                         net_info_ratelimited("%s: Possible SYN flooding on [%pI6c]:%u. %s.\n",
> > > >                                         proto, &sk->sk_v6_rcv_saddr, sk->sk_num, msg);
> > > >                 } else {
> > > >                         net_info_ratelimited("%s: Possible SYN flooding on %pI4:%u. %s.\n",
> > > >                                         proto, &sk->sk_rcv_saddr, sk->sk_num, msg);
> > >
> > > I considered this too, though Eric suggested "IP.port" to match tcpdump.
> >
> > That works, if it happens I doubt it matters.
>
> Note that "ss dst" really needs the [] notation for IPv6
>
> ss -t dst "[::1]"
> State                  Recv-Q             Send-Q
>     Local Address:Port                            Peer Address:Port
>          Process
> CLOSE-WAIT             1                  0
>             [::1]:50584                                  [::1]:ipp
>
> So we have inconsistency anyway...
>
> As you said, no strong opinion.

Following an RFC and ss filter paste is a good reason, I'll do a v3.

Jamie
diff mbox series

Patch

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 0640453fce54b6daae0861d948f3db075830daf6..5b156dfc13b3d45c20e4fe6a45af7c42f39b2c66 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6831,9 +6831,17 @@  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) && sk->sk_family == AF_INET6) {
+			net_info_ratelimited("%s: Possible SYN flooding on port %pI6c.%u. %s.\n",
+					proto, &sk->sk_v6_rcv_saddr,
+					sk->sk_num, msg);
+		} else {
+			net_info_ratelimited("%s: Possible SYN flooding on port %pI4.%u. %s.\n",
+					proto, &sk->sk_rcv_saddr,
+					sk->sk_num, msg);
+		}
+	}
 
 	return want_cookie;
 }