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
[off-list ref] wrote:

quoted

On Thu, 17 Nov 2022 at 08:15, Jakub Kicinski [off-list ref] wrote:

quoted

On Thu, 17 Nov 2022 08:39:43 +1100 Jamie Bainbridge wrote:

quoted

quoted

        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),