[PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[Nathan Chancellor]

Clang warns (or errors with CONFIG_WERROR=y) when CONFIG_TCP_AO is set:

  net/ipv4/tcp_output.c:663:2: error: label at end of compound statement is a C23 extension [-Werror,-Wc23-extensions]
    663 |         }
        |         ^
  1 error generated.

On earlier releases (such as clang-11, the current minimum supported
version for building the kernel) that do not support C23, this was a
hard error unconditionally:

  net/ipv4/tcp_output.c:663:2: error: expected statement
          }
          ^
  1 error generated.

While adding a semicolon after the label would resolve this, it is more
in line with the kernel as a whole to refactor this block into a
standalone function, which means the goto a label construct can just be
replaced with a return statement. Do so to resolve the warning.

Closes: https://github.com/ClangBuiltLinux/linux/issues/1953
Fixes: 1e03d32bea8e ("net/tcp: Add TCP-AO sign to outgoing packets")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
Changes in v3:
- Don't use a pointer to a pointer for ptr parameter to avoid the extra
  indirection in process_tcp_ao_options(), just return the modified ptr
  value back to the caller (Eric)
- Link to v2: https://lore.kernel.org/r/20231106-tcp-ao-fix-label-in-compound-statement-warning-v2-1-91eff6e1648c@kernel.org

Changes in v2:
- Break out problematic block into its own function so that goto can be
  replaced with a simple return, instead of the simple semicolon
  approach of v1 (Christoph)
- Link to v1: https://lore.kernel.org/r/20231031-tcp-ao-fix-label-in-compound-statement-warning-v1-1-c9731d115f17@kernel.org
---
 net/ipv4/tcp_output.c | 70 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 0d8dd5b7e2e5..eb13a55d660c 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -601,6 +601,44 @@ static void bpf_skops_write_hdr_opt(struct sock *sk, struct sk_buff *skb,
 }
 #endif
 
+static __be32 *process_tcp_ao_options(struct tcp_sock *tp,
+				      const struct tcp_request_sock *tcprsk,
+				      struct tcp_out_options *opts,
+				      struct tcp_key *key, __be32 *ptr)
+{
+#ifdef CONFIG_TCP_AO
+	u8 maclen = tcp_ao_maclen(key->ao_key);
+
+	if (tcprsk) {
+		u8 aolen = maclen + sizeof(struct tcp_ao_hdr);
+
+		*ptr++ = htonl((TCPOPT_AO << 24) | (aolen << 16) |
+			       (tcprsk->ao_keyid << 8) |
+			       (tcprsk->ao_rcv_next));
+	} else {
+		struct tcp_ao_key *rnext_key;
+		struct tcp_ao_info *ao_info;
+
+		ao_info = rcu_dereference_check(tp->ao_info,
+			lockdep_sock_is_held(&tp->inet_conn.icsk_inet.sk));
+		rnext_key = READ_ONCE(ao_info->rnext_key);
+		if (WARN_ON_ONCE(!rnext_key))
+			return ptr;
+		*ptr++ = htonl((TCPOPT_AO << 24) |
+			       (tcp_ao_len(key->ao_key) << 16) |
+			       (key->ao_key->sndid << 8) |
+			       (rnext_key->rcvid));
+	}
+	opts->hash_location = (__u8 *)ptr;
+	ptr += maclen / sizeof(*ptr);
+	if (unlikely(maclen % sizeof(*ptr))) {
+		memset(ptr, TCPOPT_NOP, sizeof(*ptr));
+		ptr++;
+	}
+#endif
+	return ptr;
+}
+
 /* Write previously computed TCP options to the packet.
  *
  * Beware: Something in the Internet is very sensitive to the ordering of
@@ -629,37 +667,7 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
 		opts->hash_location = (__u8 *)ptr;
 		ptr += 4;
 	} else if (tcp_key_is_ao(key)) {
-#ifdef CONFIG_TCP_AO
-		u8 maclen = tcp_ao_maclen(key->ao_key);
-
-		if (tcprsk) {
-			u8 aolen = maclen + sizeof(struct tcp_ao_hdr);
-
-			*ptr++ = htonl((TCPOPT_AO << 24) | (aolen << 16) |
-				       (tcprsk->ao_keyid << 8) |
-				       (tcprsk->ao_rcv_next));
-		} else {
-			struct tcp_ao_key *rnext_key;
-			struct tcp_ao_info *ao_info;
-
-			ao_info = rcu_dereference_check(tp->ao_info,
-				lockdep_sock_is_held(&tp->inet_conn.icsk_inet.sk));
-			rnext_key = READ_ONCE(ao_info->rnext_key);
-			if (WARN_ON_ONCE(!rnext_key))
-				goto out_ao;
-			*ptr++ = htonl((TCPOPT_AO << 24) |
-				       (tcp_ao_len(key->ao_key) << 16) |
-				       (key->ao_key->sndid << 8) |
-				       (rnext_key->rcvid));
-		}
-		opts->hash_location = (__u8 *)ptr;
-		ptr += maclen / sizeof(*ptr);
-		if (unlikely(maclen % sizeof(*ptr))) {
-			memset(ptr, TCPOPT_NOP, sizeof(*ptr));
-			ptr++;
-		}
-out_ao:
-#endif
+		ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
 	}
 	if (unlikely(opts->mss)) {
 		*ptr++ = htonl((TCPOPT_MSS << 24) |

---
base-commit: c1ed833e0b3b7b9edc82b97b73b2a8a10ceab241
change-id: 20231031-tcp-ao-fix-label-in-compound-statement-warning-ebd6c9978498

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[Dmitry Safonov]

On 11/6/23 21:14, Nathan Chancellor wrote:
> Clang warns (or errors with CONFIG_WERROR=y) when CONFIG_TCP_AO is set:
> 
>   net/ipv4/tcp_output.c:663:2: error: label at end of compound statement is a C23 extension [-Werror,-Wc23-extensions]
>     663 |         }
>         |         ^
>   1 error generated.
> 
> On earlier releases (such as clang-11, the current minimum supported
> version for building the kernel) that do not support C23, this was a
> hard error unconditionally:
> 
>   net/ipv4/tcp_output.c:663:2: error: expected statement
>           }
>           ^
>   1 error generated.
> 
> While adding a semicolon after the label would resolve this, it is more
> in line with the kernel as a whole to refactor this block into a
> standalone function, which means the goto a label construct can just be
> replaced with a return statement. Do so to resolve the warning.
> 
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1953
> Fixes: 1e03d32bea8e ("net/tcp: Add TCP-AO sign to outgoing packets")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Seems like exactly the fix that my git testing tree had, with an
exception to naming the helper tcp_ao_options_write().
But then I found* your patch-v1 and decided not to send an alternative
patch.

Thanks for fixing this,
Reviewed-by: Dmitry Safonov <dima@arista.com>

*had to fix my Gmail lkml filter to label not only emails with cc/to my
name, but also the raw email address (usually, I got them to/cc "Dmitry
Safonov", but this one didn't have the name and got lost in the lkml pile).

> ---
> Changes in v3:
> - Don't use a pointer to a pointer for ptr parameter to avoid the extra
>   indirection in process_tcp_ao_options(), just return the modified ptr
>   value back to the caller (Eric)
> - Link to v2: https://lore.kernel.org/r/20231106-tcp-ao-fix-label-in-compound-statement-warning-v2-1-91eff6e1648c@kernel.org
> 
> Changes in v2:
> - Break out problematic block into its own function so that goto can be
>   replaced with a simple return, instead of the simple semicolon
>   approach of v1 (Christoph)
> - Link to v1: https://lore.kernel.org/r/20231031-tcp-ao-fix-label-in-compound-statement-warning-v1-1-c9731d115f17@kernel.org
> ---
>  net/ipv4/tcp_output.c | 70 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 0d8dd5b7e2e5..eb13a55d660c 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -601,6 +601,44 @@ static void bpf_skops_write_hdr_opt(struct sock *sk, struct sk_buff *skb,
>  }
>  #endif
>  
> +static __be32 *process_tcp_ao_options(struct tcp_sock *tp,
> +				      const struct tcp_request_sock *tcprsk,
> +				      struct tcp_out_options *opts,
> +				      struct tcp_key *key, __be32 *ptr)
> +{
> +#ifdef CONFIG_TCP_AO
> +	u8 maclen = tcp_ao_maclen(key->ao_key);
> +
> +	if (tcprsk) {
> +		u8 aolen = maclen + sizeof(struct tcp_ao_hdr);
> +
> +		*ptr++ = htonl((TCPOPT_AO << 24) | (aolen << 16) |
> +			       (tcprsk->ao_keyid << 8) |
> +			       (tcprsk->ao_rcv_next));
> +	} else {
> +		struct tcp_ao_key *rnext_key;
> +		struct tcp_ao_info *ao_info;
> +
> +		ao_info = rcu_dereference_check(tp->ao_info,
> +			lockdep_sock_is_held(&tp->inet_conn.icsk_inet.sk));
> +		rnext_key = READ_ONCE(ao_info->rnext_key);
> +		if (WARN_ON_ONCE(!rnext_key))
> +			return ptr;
> +		*ptr++ = htonl((TCPOPT_AO << 24) |
> +			       (tcp_ao_len(key->ao_key) << 16) |
> +			       (key->ao_key->sndid << 8) |
> +			       (rnext_key->rcvid));
> +	}
> +	opts->hash_location = (__u8 *)ptr;
> +	ptr += maclen / sizeof(*ptr);
> +	if (unlikely(maclen % sizeof(*ptr))) {
> +		memset(ptr, TCPOPT_NOP, sizeof(*ptr));
> +		ptr++;
> +	}
> +#endif
> +	return ptr;
> +}
> +
>  /* Write previously computed TCP options to the packet.
>   *
>   * Beware: Something in the Internet is very sensitive to the ordering of
> @@ -629,37 +667,7 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
>  		opts->hash_location = (__u8 *)ptr;
>  		ptr += 4;
>  	} else if (tcp_key_is_ao(key)) {
> -#ifdef CONFIG_TCP_AO
> -		u8 maclen = tcp_ao_maclen(key->ao_key);
> -
> -		if (tcprsk) {
> -			u8 aolen = maclen + sizeof(struct tcp_ao_hdr);
> -
> -			*ptr++ = htonl((TCPOPT_AO << 24) | (aolen << 16) |
> -				       (tcprsk->ao_keyid << 8) |
> -				       (tcprsk->ao_rcv_next));
> -		} else {
> -			struct tcp_ao_key *rnext_key;
> -			struct tcp_ao_info *ao_info;
> -
> -			ao_info = rcu_dereference_check(tp->ao_info,
> -				lockdep_sock_is_held(&tp->inet_conn.icsk_inet.sk));
> -			rnext_key = READ_ONCE(ao_info->rnext_key);
> -			if (WARN_ON_ONCE(!rnext_key))
> -				goto out_ao;
> -			*ptr++ = htonl((TCPOPT_AO << 24) |
> -				       (tcp_ao_len(key->ao_key) << 16) |
> -				       (key->ao_key->sndid << 8) |
> -				       (rnext_key->rcvid));
> -		}
> -		opts->hash_location = (__u8 *)ptr;
> -		ptr += maclen / sizeof(*ptr);
> -		if (unlikely(maclen % sizeof(*ptr))) {
> -			memset(ptr, TCPOPT_NOP, sizeof(*ptr));
> -			ptr++;
> -		}
> -out_ao:
> -#endif
> +		ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
>  	}
>  	if (unlikely(opts->mss)) {
>  		*ptr++ = htonl((TCPOPT_MSS << 24) |
> 
> ---
> base-commit: c1ed833e0b3b7b9edc82b97b73b2a8a10ceab241
> change-id: 20231031-tcp-ao-fix-label-in-compound-statement-warning-ebd6c9978498

Thanks,
             Dmitry

Re: [PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[Nathan Chancellor]

On Mon, Nov 06, 2023 at 09:26:48PM +0000, Dmitry Safonov wrote:
> Seems like exactly the fix that my git testing tree had, with an
> exception to naming the helper tcp_ao_options_write().

Heh, not sure why I never considered that as an option... I am guessing
it does not matter enough for a v4 at this point but I could send a
net-next change later to update it if you so desire!

> But then I found* your patch-v1 and decided not to send an alternative
> patch.
> 
> Thanks for fixing this,

Thanks for taking a look!

> Reviewed-by: Dmitry Safonov <dima@arista.com>
> 
> *had to fix my Gmail lkml filter to label not only emails with cc/to my
> name, but also the raw email address (usually, I got them to/cc "Dmitry
> Safonov", but this one didn't have the name and got lost in the lkml pile).

Sorry about that, b4 used to have some interesting behavior around names
at one point (don't remember the details at the moment) and just using
emails avoided those issues. Maybe I should go back to names and emails
to see if I notice any problems again.

Cheers,
Nathan

Re: [PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[Dmitry Safonov]

On 11/6/23 21:34, Nathan Chancellor wrote:
> On Mon, Nov 06, 2023 at 09:26:48PM +0000, Dmitry Safonov wrote:
>> Seems like exactly the fix that my git testing tree had, with an
>> exception to naming the helper tcp_ao_options_write().
> 
> Heh, not sure why I never considered that as an option... I am guessing
> it does not matter enough for a v4 at this point but I could send a
> net-next change later to update it if you so desire!

It doesn't matter really, not worth another patch :-)

>> But then I found* your patch-v1 and decided not to send an alternative
>> patch.
>>
>> Thanks for fixing this,
> 
> Thanks for taking a look!
> 
>> Reviewed-by: Dmitry Safonov <dima@arista.com>
>>
>> *had to fix my Gmail lkml filter to label not only emails with cc/to my
>> name, but also the raw email address (usually, I got them to/cc "Dmitry
>> Safonov", but this one didn't have the name and got lost in the lkml pile).
> 
> Sorry about that, b4 used to have some interesting behavior around names
> at one point (don't remember the details at the moment) and just using
> emails avoided those issues. Maybe I should go back to names and emails
> to see if I notice any problems again.

No worries, should be fixed now. I preferred previously filtering by
full name, rather than email address as I send patches from both work
and home emails, but also sometimes people send patches/questions to
emails from the companies I previously worked for, regardless .mailmap
entries.

Probably, they look for author in lkml/mail archive, rather than use git
to get the current/proper email address.

Thanks,
             Dmitry

Re: [PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[Eric Dumazet]

On Mon, Nov 6, 2023 at 10:14 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Clang warns (or errors with CONFIG_WERROR=y) when CONFIG_TCP_AO is set:
>
>   net/ipv4/tcp_output.c:663:2: error: label at end of compound statement is a C23 extension [-Werror,-Wc23-extensions]
>     663 |         }
>         |         ^
>   1 error generated.
>
> On earlier releases (such as clang-11, the current minimum supported
> version for building the kernel) that do not support C23, this was a
> hard error unconditionally:
>
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1953
> Fixes: 1e03d32bea8e ("net/tcp: Add TCP-AO sign to outgoing packets")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> Changes in v3:
> - Don't use a pointer to a pointer for ptr parameter to avoid the extra
>   indirection in process_tcp_ao_options(), just return the modified ptr
>   value back to the caller (Eric)

SGTM thanks.
Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v3] tcp: Fix -Wc23-extensions in tcp_options_write()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 06 Nov 2023 14:14:16 -0700 you wrote:
> Clang warns (or errors with CONFIG_WERROR=y) when CONFIG_TCP_AO is set:
> 
>   net/ipv4/tcp_output.c:663:2: error: label at end of compound statement is a C23 extension [-Werror,-Wc23-extensions]
>     663 |         }
>         |         ^
>   1 error generated.
> 
> [...]

Here is the summary with links:
  - [net,v3] tcp: Fix -Wc23-extensions in tcp_options_write()
    https://git.kernel.org/netdev/net/c/7425627b2b2c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html