* [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
@ 2022-11-15 10:12 Geert Uytterhoeven
2022-11-15 11:37 ` Matthieu Baerts
2022-11-16 20:31 ` Jakub Kicinski
0 siblings, 2 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2022-11-15 10:12 UTC (permalink / raw)
To: Jamie Bainbridge, Eric Dumazet, David S . Miller,
Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski, Paolo Abeni,
Chris Down, Stephen Hemminger
Cc: netdev, linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-15 10:12 [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n Geert Uytterhoeven
@ 2022-11-15 11:37 ` Matthieu Baerts
2022-11-16 20:31 ` Jakub Kicinski
1 sibling, 0 replies; 10+ messages in thread
From: Matthieu Baerts @ 2022-11-15 11:37 UTC (permalink / raw)
To: Geert Uytterhoeven, Jamie Bainbridge, Eric Dumazet,
David S . Miller, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
Paolo Abeni, Chris Down, Stephen Hemminger
Cc: netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-15 10:12 [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n Geert Uytterhoeven
2022-11-15 11:37 ` Matthieu Baerts
@ 2022-11-16 20:31 ` Jakub Kicinski
2022-11-16 21:39 ` Jamie Bainbridge
1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2022-11-16 20:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jamie Bainbridge, Eric Dumazet, David S . Miller,
Hideaki YOSHIFUJI, David Ahern, Paolo Abeni, Chris Down,
Stephen Hemminger, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-16 20:31 ` Jakub Kicinski
@ 2022-11-16 21:39 ` Jamie Bainbridge
2022-11-16 22:15 ` Jakub Kicinski
0 siblings, 1 reply; 10+ messages in thread
From: Jamie Bainbridge @ 2022-11-16 21:39 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Geert Uytterhoeven, Eric Dumazet, David S . Miller,
Hideaki YOSHIFUJI, David Ahern, Paolo Abeni, Chris Down,
Stephen Hemminger, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-16 21:39 ` Jamie Bainbridge
@ 2022-11-16 22:15 ` Jakub Kicinski
2022-11-18 1:45 ` Jamie Bainbridge
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2022-11-16 22:15 UTC (permalink / raw)
To: Jamie Bainbridge
Cc: Geert Uytterhoeven, Eric Dumazet, David S . Miller,
Hideaki YOSHIFUJI, David Ahern, Paolo Abeni, Chris Down,
Stephen Hemminger, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-16 22:15 ` Jakub Kicinski
@ 2022-11-18 1:45 ` Jamie Bainbridge
2022-11-18 8:29 ` Geert Uytterhoeven
0 siblings, 1 reply; 10+ messages in thread
From: Jamie Bainbridge @ 2022-11-18 1:45 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Geert Uytterhoeven, Eric Dumazet, David S . Miller,
Hideaki YOSHIFUJI, David Ahern, Paolo Abeni, Chris Down,
Stephen Hemminger, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-18 1:45 ` Jamie Bainbridge
@ 2022-11-18 8:29 ` Geert Uytterhoeven
2022-11-18 16:19 ` Jakub Kicinski
2022-11-21 22:44 ` Saeed Mahameed
0 siblings, 2 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2022-11-18 8:29 UTC (permalink / raw)
To: Jamie Bainbridge
Cc: Jakub Kicinski, Geert Uytterhoeven, Eric Dumazet,
David S . Miller, Hideaki YOSHIFUJI, David Ahern, Paolo Abeni,
Chris Down, Stephen Hemminger, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-18 8:29 ` Geert Uytterhoeven
@ 2022-11-18 16:19 ` Jakub Kicinski
2022-11-21 22:44 ` Saeed Mahameed
1 sibling, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2022-11-18 16:19 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jamie Bainbridge, Geert Uytterhoeven, Eric Dumazet,
David S . Miller, Hideaki YOSHIFUJI, David Ahern, Paolo Abeni,
Chris Down, Stephen Hemminger, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-18 8:29 ` Geert Uytterhoeven
2022-11-18 16:19 ` Jakub Kicinski
@ 2022-11-21 22:44 ` Saeed Mahameed
2022-11-22 3:52 ` Jakub Kicinski
1 sibling, 1 reply; 10+ messages in thread
From: Saeed Mahameed @ 2022-11-21 22:44 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jamie Bainbridge, Jakub Kicinski, Geert Uytterhoeven,
Eric Dumazet, David S . Miller, Hideaki YOSHIFUJI, David Ahern,
Paolo Abeni, Chris Down, Stephen Hemminger, netdev, linux-kernel
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),
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n
2022-11-21 22:44 ` Saeed Mahameed
@ 2022-11-22 3:52 ` Jakub Kicinski
0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2022-11-22 3:52 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Geert Uytterhoeven, Jamie Bainbridge, Geert Uytterhoeven,
Eric Dumazet, David S . Miller, Hideaki YOSHIFUJI, David Ahern,
Paolo Abeni, Chris Down, Stephen Hemminger, netdev, linux-kernel
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..
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-11-22 3:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-15 10:12 [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n Geert Uytterhoeven
2022-11-15 11:37 ` Matthieu Baerts
2022-11-16 20:31 ` Jakub Kicinski
2022-11-16 21:39 ` Jamie Bainbridge
2022-11-16 22:15 ` Jakub Kicinski
2022-11-18 1:45 ` Jamie Bainbridge
2022-11-18 8:29 ` Geert Uytterhoeven
2022-11-18 16:19 ` Jakub Kicinski
2022-11-21 22:44 ` Saeed Mahameed
2022-11-22 3:52 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).