| Message ID | 20231106-tcp-ao-fix-label-in-compound-statement-warning-v3-1-b54a64602a85@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7425627b2b2cd671d5bf6541ce50f7cba8a76ad6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] tcp: Fix -Wc23-extensions in tcp_options_write() | expand |
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
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
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
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>
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!
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) |
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(-) --- base-commit: c1ed833e0b3b7b9edc82b97b73b2a8a10ceab241 change-id: 20231031-tcp-ao-fix-label-in-compound-statement-warning-ebd6c9978498 Best regards,