Messages in this thread Patch in this message | | | Date | Mon, 21 Nov 2022 14:44:19 -0800 | From | Saeed Mahameed <> | Subject | Re: [PATCH net-next] tcp: Fix tcp_syn_flood_action() if CONFIG_IPV6=n |
| |
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),
| |