[PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Geert Uytterhoeven]

If CONFIG_IPV6=n:

    net/ipv4/tcp_input.c: In function ‘tcp_syn_flood_action’:
    include/net/sock.h:387:37: error: ‘const struct sock_common’ has no member named ‘skc_v6_rcv_saddr’; did you mean ‘skc_rcv_saddr’?
      387 | #define sk_v6_rcv_saddr __sk_common.skc_v6_rcv_saddr
	  |                                     ^~~~~~~~~~~~~~~~
    include/linux/printk.h:429:19: note: in definition of macro ‘printk_index_wrap’
      429 |   _p_func(_fmt, ##__VA_ARGS__);    \
	  |                   ^~~~~~~~~~~
    include/linux/printk.h:530:2: note: in expansion of macro ‘printk’
      530 |  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
	  |  ^~~~~~
    include/linux/net.h:272:3: note: in expansion of macro ‘pr_info’
      272 |   function(__VA_ARGS__);    \
	  |   ^~~~~~~~
    include/linux/net.h:288:2: note: in expansion of macro ‘net_ratelimited_function’
      288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
	  |  ^~~~~~~~~~~~~~~~~~~~~~~~
    include/linux/net.h:288:43: note: in expansion of macro ‘sk_v6_rcv_saddr’
      288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
	  |                                           ^~~~~~~~~~~
    net/ipv4/tcp_input.c:6847:4: note: in expansion of macro ‘net_info_ratelimited’
     6847 |    net_info_ratelimited("%s: Possible SYN flooding on port [%pI6c]:%u. %s.\n",
	  |    ^~~~~~~~~~~~~~~~~~~~

Fix this by using "#if" instead of "if", like is done for all other
checks for CONFIG_IPV6.

Fixes: d9282e48c6088105 ("tcp: Add listening address to SYN flood message")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 net/ipv4/tcp_input.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 94024fdc2da1b28a..e5d7a33fac6666bb 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6843,11 +6843,14 @@ static bool tcp_syn_flood_action(const struct sock *sk, const char *proto)
 
 	if (!queue->synflood_warned && syncookies != 2 &&
 	    xchg(&queue->synflood_warned, 1) == 0) {
-		if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
+#if IS_ENABLED(CONFIG_IPV6)
+		if (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 {
+		} else
+#endif
+		{
 			net_info_ratelimited("%s: Possible SYN flooding on port %pI4:%u. %s.\n",
 					proto, &sk->sk_rcv_saddr,
 					sk->sk_num, msg);
-- 
2.25.1

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Matthieu Baerts]

Hi Geert,

On 15/11/2022 11:12, Geert Uytterhoeven wrote:
> If CONFIG_IPV6=n:
> 
>     net/ipv4/tcp_input.c: In function ‘tcp_syn_flood_action’:
>     include/net/sock.h:387:37: error: ‘const struct sock_common’ has no member named ‘skc_v6_rcv_saddr’; did you mean ‘skc_rcv_saddr’?
>       387 | #define sk_v6_rcv_saddr __sk_common.skc_v6_rcv_saddr
> 	  |                                     ^~~~~~~~~~~~~~~~
>     include/linux/printk.h:429:19: note: in definition of macro ‘printk_index_wrap’
>       429 |   _p_func(_fmt, ##__VA_ARGS__);    \
> 	  |                   ^~~~~~~~~~~
>     include/linux/printk.h:530:2: note: in expansion of macro ‘printk’
>       530 |  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> 	  |  ^~~~~~
>     include/linux/net.h:272:3: note: in expansion of macro ‘pr_info’
>       272 |   function(__VA_ARGS__);    \
> 	  |   ^~~~~~~~
>     include/linux/net.h:288:2: note: in expansion of macro ‘net_ratelimited_function’
>       288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> 	  |  ^~~~~~~~~~~~~~~~~~~~~~~~
>     include/linux/net.h:288:43: note: in expansion of macro ‘sk_v6_rcv_saddr’
>       288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> 	  |                                           ^~~~~~~~~~~
>     net/ipv4/tcp_input.c:6847:4: note: in expansion of macro ‘net_info_ratelimited’
>      6847 |    net_info_ratelimited("%s: Possible SYN flooding on port [%pI6c]:%u. %s.\n",
> 	  |    ^~~~~~~~~~~~~~~~~~~~
> 
> Fix this by using "#if" instead of "if", like is done for all other
> checks for CONFIG_IPV6.

Thank you for the patch!

Our CI validating MPTCP also found the issue. I was going to suggest a
similar one before I saw yours :)

Everything is fixed on my side after having applied the patch!

Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net>

> Fixes: d9282e48c6088105 ("tcp: Add listening address to SYN flood message")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  net/ipv4/tcp_input.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 94024fdc2da1b28a..e5d7a33fac6666bb 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6843,11 +6843,14 @@ static bool tcp_syn_flood_action(const struct sock *sk, const char *proto)
>  
>  	if (!queue->synflood_warned && syncookies != 2 &&
>  	    xchg(&queue->synflood_warned, 1) == 0) {
> -		if (IS_ENABLED(CONFIG_IPV6) && sk->sk_family == AF_INET6) {
> +#if IS_ENABLED(CONFIG_IPV6)
> +		if (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 {
> +		} else
> +#endif
> +		{

I was going to suggest to remove the unneeded braces here and just
before + eventually fix the indentation under net_info_ratelimited()
while at it but that's just some details not directly linked to the fix
here.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Jakub Kicinski]

On Tue, 15 Nov 2022 11:12:16 +0100 Geert Uytterhoeven wrote:
> If CONFIG_IPV6=n:
> 
>     net/ipv4/tcp_input.c: In function ‘tcp_syn_flood_action’:
>     include/net/sock.h:387:37: error: ‘const struct sock_common’ has no member named ‘skc_v6_rcv_saddr’; did you mean ‘skc_rcv_saddr’?
>       387 | #define sk_v6_rcv_saddr __sk_common.skc_v6_rcv_saddr
> 	  |                                     ^~~~~~~~~~~~~~~~
>     include/linux/printk.h:429:19: note: in definition of macro ‘printk_index_wrap’
>       429 |   _p_func(_fmt, ##__VA_ARGS__);    \
> 	  |                   ^~~~~~~~~~~
>     include/linux/printk.h:530:2: note: in expansion of macro ‘printk’
>       530 |  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> 	  |  ^~~~~~
>     include/linux/net.h:272:3: note: in expansion of macro ‘pr_info’
>       272 |   function(__VA_ARGS__);    \
> 	  |   ^~~~~~~~
>     include/linux/net.h:288:2: note: in expansion of macro ‘net_ratelimited_function’
>       288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> 	  |  ^~~~~~~~~~~~~~~~~~~~~~~~
>     include/linux/net.h:288:43: note: in expansion of macro ‘sk_v6_rcv_saddr’
>       288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> 	  |                                           ^~~~~~~~~~~
>     net/ipv4/tcp_input.c:6847:4: note: in expansion of macro ‘net_info_ratelimited’
>      6847 |    net_info_ratelimited("%s: Possible SYN flooding on port [%pI6c]:%u. %s.\n",
> 	  |    ^~~~~~~~~~~~~~~~~~~~
> 
> Fix this by using "#if" instead of "if", like is done for all other
> checks for CONFIG_IPV6.
> 
> Fixes: d9282e48c6088105 ("tcp: Add listening address to SYN flood message")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Sorry for the late reaction, this now conflicts with bf36267e3ad3df8

I was gonna hand edit but perhaps we can do better with the ifdef
formation.

Instead of 

#ifdef v6
	if (v6) {
		expensive_call6();
	} else    //  d k
#endif            //  o i
	{         //  o e
		expensive_call4();
	}

Can we go with:

#ifdef v6
	if (v6)
		expensive_call6();
	else 
#endif
		expensive_call4();

or

	if (v4) {
		expensive_call4();
#ifdef v6
	} else {
		expensive_call6();
#endif
	}

or

	if (v6) {
#ifdef v6
		expensive_call6();
#endif
	} else {
		expensive_call6();
	}


I know you're just going with the most obviously correct / smallest diff
way, but the broken up else bracket gives me flashbacks of looking at
vendor code :S

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Jamie Bainbridge]

On Thu, 17 Nov 2022 at 07:31, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 15 Nov 2022 11:12:16 +0100 Geert Uytterhoeven wrote:
> > If CONFIG_IPV6=n:
> >
> >     net/ipv4/tcp_input.c: In function ‘tcp_syn_flood_action’:
> >     include/net/sock.h:387:37: error: ‘const struct sock_common’ has no member named ‘skc_v6_rcv_saddr’; did you mean ‘skc_rcv_saddr’?
> >       387 | #define sk_v6_rcv_saddr __sk_common.skc_v6_rcv_saddr
> >         |                                     ^~~~~~~~~~~~~~~~
> >     include/linux/printk.h:429:19: note: in definition of macro ‘printk_index_wrap’
> >       429 |   _p_func(_fmt, ##__VA_ARGS__);    \
> >         |                   ^~~~~~~~~~~
> >     include/linux/printk.h:530:2: note: in expansion of macro ‘printk’
> >       530 |  printk(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> >         |  ^~~~~~
> >     include/linux/net.h:272:3: note: in expansion of macro ‘pr_info’
> >       272 |   function(__VA_ARGS__);    \
> >         |   ^~~~~~~~
> >     include/linux/net.h:288:2: note: in expansion of macro ‘net_ratelimited_function’
> >       288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> >         |  ^~~~~~~~~~~~~~~~~~~~~~~~
> >     include/linux/net.h:288:43: note: in expansion of macro ‘sk_v6_rcv_saddr’
> >       288 |  net_ratelimited_function(pr_info, fmt, ##__VA_ARGS__)
> >         |                                           ^~~~~~~~~~~
> >     net/ipv4/tcp_input.c:6847:4: note: in expansion of macro ‘net_info_ratelimited’
> >      6847 |    net_info_ratelimited("%s: Possible SYN flooding on port [%pI6c]:%u. %s.\n",
> >         |    ^~~~~~~~~~~~~~~~~~~~
> >
> > Fix this by using "#if" instead of "if", like is done for all other
> > checks for CONFIG_IPV6.
> >
> > Fixes: d9282e48c6088105 ("tcp: Add listening address to SYN flood message")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Sorry for the late reaction, this now conflicts with bf36267e3ad3df8
>
> I was gonna hand edit but perhaps we can do better with the ifdef
> formation.
>
> Instead of
>
> #ifdef v6
>         if (v6) {
>                 expensive_call6();
>         } else    //  d k
> #endif            //  o i
>         {         //  o e
>                 expensive_call4();
>         }

I actually started off using this way in my v1. I did that
intentionally because that pattern already exists in other places,
discussed at:

 https://lore.kernel.org/netdev/CAAvyFNg1F8ixrgy0YeL-TT5xLmk8N7dD=ZMLQ6VxsjHb_PU9bg@mail.gmail.com/

or just see:

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

Geert's patch adheres to existing style, which seems the better option?

> Can we go with:
>
> #ifdef v6
>         if (v6)
>                 expensive_call6();
>         else
> #endif
>                 expensive_call4();

This is a nested if, so it really should have braces to prevent
dangling else, both now and in the future.

> or
>
>         if (v4) {
>                 expensive_call4();
> #ifdef v6
>         } else {
>                 expensive_call6();
> #endif
>         }
> or
>
>         if (v6) {
> #ifdef v6
>                 expensive_call6();
> #endif
>         } else {
>                 expensive_call6();
>         }

These should work, but I expect they cause a comparison which can't be
optimised out at compile time. This is probably why the first style
exists.

In this SYN flood codepath optimisation doesn't matter because we're
doing ratelimited logging anyway. But if we're breaking with existing
style, then wouldn't the others also have to change to this style? I
haven't reviewed all the other usage to tell if they're in an oft-used
fastpath where such a thing might matter.

It seems Geert's patch style is the best way.

Jamie

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Jakub Kicinski]

On Thu, 17 Nov 2022 08:39:43 +1100 Jamie Bainbridge wrote:
> >         if (v6) {
> > #ifdef v6
> >                 expensive_call6();
> > #endif
> >         } else {
> >                 expensive_call6();
> >         }  
> 
> These should work, but I expect they cause a comparison which can't be
> optimised out at compile time. This is probably why the first style
> exists.
> 
> In this SYN flood codepath optimisation doesn't matter because we're
> doing ratelimited logging anyway. But if we're breaking with existing
> style, then wouldn't the others also have to change to this style? I
> haven't reviewed all the other usage to tell if they're in an oft-used
> fastpath where such a thing might matter.

I think the word style already implies subjectivity.

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Jamie Bainbridge]

On Thu, 17 Nov 2022 at 08:15, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 17 Nov 2022 08:39:43 +1100 Jamie Bainbridge wrote:
> > >         if (v6) {
> > > #ifdef v6
> > >                 expensive_call6();
> > > #endif
> > >         } else {
> > >                 expensive_call6();
> > >         }
> >
> > These should work, but I expect they cause a comparison which can't be
> > optimised out at compile time. This is probably why the first style
> > exists.
> >
> > In this SYN flood codepath optimisation doesn't matter because we're
> > doing ratelimited logging anyway. But if we're breaking with existing
> > style, then wouldn't the others also have to change to this style? I
> > haven't reviewed all the other usage to tell if they're in an oft-used
> > fastpath where such a thing might matter.
>
> I think the word style already implies subjectivity.

You are right. Looking further, there are many other ways
IF_ENABLED(CONFIG_IPV6) is used, including similar to the ways you
have suggested.

I don't mind Geert's original patch, but if you want a different
style, I like your suggestion with v4 first:

        if (v4) {
                expensive_call4();
#ifdef v6
        } else {
                expensive_call6();
#endif
        }

Jamie

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Geert Uytterhoeven]

Hi Jamie,

On Fri, Nov 18, 2022 at 2:50 AM Jamie Bainbridge
<jamie.bainbridge@gmail.com> wrote:
> On Thu, 17 Nov 2022 at 08:15, Jakub Kicinski <kuba@kernel.org> wrote:
> > On Thu, 17 Nov 2022 08:39:43 +1100 Jamie Bainbridge wrote:
> > > >         if (v6) {
> > > > #ifdef v6
> > > >                 expensive_call6();
> > > > #endif
> > > >         } else {
> > > >                 expensive_call6();
> > > >         }
> > >
> > > These should work, but I expect they cause a comparison which can't be
> > > optimised out at compile time. This is probably why the first style
> > > exists.
> > >
> > > In this SYN flood codepath optimisation doesn't matter because we're
> > > doing ratelimited logging anyway. But if we're breaking with existing
> > > style, then wouldn't the others also have to change to this style? I
> > > haven't reviewed all the other usage to tell if they're in an oft-used
> > > fastpath where such a thing might matter.
> >
> > I think the word style already implies subjectivity.
>
> You are right. Looking further, there are many other ways
> IF_ENABLED(CONFIG_IPV6) is used, including similar to the ways you
> have suggested.
>
> I don't mind Geert's original patch, but if you want a different
> style, I like your suggestion with v4 first:
>
>         if (v4) {
>                 expensive_call4();
> #ifdef v6
>         } else {
>                 expensive_call6();
> #endif
>         }

IMHO this is worse, as the #ifdef/#endif is spread across the two branches
of an if-conditional.

Hence this is usually written as:

            if (cond1) {
                    expensive_call1();
            }
    #ifdef cond2_enabled
           else {
                    expensive_call1();
            }
    #endif

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

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Jakub Kicinski]

On Fri, 18 Nov 2022 09:29:13 +0100 Geert Uytterhoeven wrote:
> IMHO this is worse, as the #ifdef/#endif is spread across the two branches
> of an if-conditional.
> 
> Hence this is usually written as:
> 
>             if (cond1) {
>                     expensive_call1();
>             }
>     #ifdef cond2_enabled
>            else {
>                     expensive_call1();
>             }
>     #endif

Alright, good enough for me.

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Saeed Mahameed]

On 18 Nov 09:29, Geert Uytterhoeven wrote:
>Hi Jamie,
>
>On Fri, Nov 18, 2022 at 2:50 AM Jamie Bainbridge
><jamie.bainbridge@gmail.com> wrote:
>> On Thu, 17 Nov 2022 at 08:15, Jakub Kicinski <kuba@kernel.org> wrote:
>> > On Thu, 17 Nov 2022 08:39:43 +1100 Jamie Bainbridge wrote:
>> > > >         if (v6) {
>> > > > #ifdef v6
>> > > >                 expensive_call6();
>> > > > #endif
>> > > >         } else {
>> > > >                 expensive_call6();
>> > > >         }
>> > >
>> > > These should work, but I expect they cause a comparison which can't be
>> > > optimised out at compile time. This is probably why the first style
>> > > exists.
>> > >
>> > > In this SYN flood codepath optimisation doesn't matter because we're
>> > > doing ratelimited logging anyway. But if we're breaking with existing
>> > > style, then wouldn't the others also have to change to this style? I
>> > > haven't reviewed all the other usage to tell if they're in an oft-used
>> > > fastpath where such a thing might matter.
>> >
>> > I think the word style already implies subjectivity.
>>
>> You are right. Looking further, there are many other ways
>> IF_ENABLED(CONFIG_IPV6) is used, including similar to the ways you
>> have suggested.
>>
>> I don't mind Geert's original patch, but if you want a different
>> style, I like your suggestion with v4 first:
>>
>>         if (v4) {
>>                 expensive_call4();
>> #ifdef v6
>>         } else {
>>                 expensive_call6();
>> #endif
>>         }
>
>IMHO this is worse, as the #ifdef/#endif is spread across the two branches
>of an if-conditional.
>
>Hence this is usually written as:
>
>            if (cond1) {
>                    expensive_call1();
>            }
>    #ifdef cond2_enabled
>           else {
>                    expensive_call1();
>            }
>    #endif
>

I don't think any of this complication is needed, 

there's a macro inet6_rcv_saddr(sk), we can use it instead of directly
referencing &sk->sk_v6_rcv_saddr, it already handles the case where 
CONFIG_IPV6=n

--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6845,7 +6845,7 @@ static bool tcp_syn_flood_action(const struct sock *sk, const char *proto)
             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,
+                                       proto, inet6_rcv_saddr(sk),

Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n

[Jakub Kicinski]

On Mon, 21 Nov 2022 14:44:19 -0800 Saeed Mahameed wrote:
> there's a macro inet6_rcv_saddr(sk), we can use it instead of directly
> referencing &sk->sk_v6_rcv_saddr, it already handles the case where 
> CONFIG_IPV6=n
> 
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6845,7 +6845,7 @@ static bool tcp_syn_flood_action(const struct sock *sk, const char *proto)
>              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,
> +                                       proto, inet6_rcv_saddr(sk),

Great, could you post a full patch? I haven't seen v2, now it's almost
Thanksgiving..