| Message ID | d1ecf500f07e063d4e8e34f4045ddca55416c686.1668507036.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n | expand |
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
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
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
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.
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
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
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.
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),
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..
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);
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(-)