* [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization @ 2025-08-02 18:43 Kees Cook 2025-08-03 17:32 ` Nathan Chancellor 0 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2025-08-02 18:43 UTC (permalink / raw) To: Masahiro Yamada Cc: Kees Cook, Nathan Chancellor, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening With the few remaining fixes now landed, we can re-enable the option -Wunterminated-string-initialization (via -Wextra). Both GCC and Clang have the required multi-dimensional nonstring attribute support. Build tested allmodconfig with: gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0 gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2) clang version 20.1.8 (Fedora 20.1.8-1.fc42) ClangBuiltLinux clang version 21.1.0-rc2 clang version 22.0.0git Signed-off-by: Kees Cook <kees@kernel.org> --- v2: Clang is fixed too! :) (Nathan) v1: https://lore.kernel.org/lkml/20250802002733.work.941-kees@kernel.org/ Cc: Masahiro Yamada <masahiroy@kernel.org> Cc: Nathan Chancellor <nathan@kernel.org> Cc: Nicolas Schier <nicolas.schier@linux.dev> Cc: <linux-kbuild@vger.kernel.org> --- scripts/Makefile.extrawarn | 3 --- 1 file changed, 3 deletions(-) diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index dca175fffcab..a1001377dcb2 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -78,9 +78,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wcast-function-type) KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow) KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow) -# Currently, disable -Wunterminated-string-initialization as broken -KBUILD_CFLAGS += $(call cc-option, -Wno-unterminated-string-initialization) - # The allocators already balk at large sizes, so silence the compiler # warnings for bounds checks involving those possible values. While # -Wno-alloc-size-larger-than would normally be used here, earlier versions -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-02 18:43 [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization Kees Cook @ 2025-08-03 17:32 ` Nathan Chancellor 2025-08-05 14:50 ` Alexander Lobakin 0 siblings, 1 reply; 9+ messages in thread From: Nathan Chancellor @ 2025-08-03 17:32 UTC (permalink / raw) To: Kees Cook Cc: Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening On Sat, Aug 02, 2025 at 11:43:32AM -0700, Kees Cook wrote: > With the few remaining fixes now landed, we can re-enable the option > -Wunterminated-string-initialization (via -Wextra). Both GCC and Clang > have the required multi-dimensional nonstring attribute support. > > Build tested allmodconfig with: > gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0 > gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2) > clang version 20.1.8 (Fedora 20.1.8-1.fc42) > ClangBuiltLinux clang version 21.1.0-rc2 > clang version 22.0.0git > > Signed-off-by: Kees Cook <kees@kernel.org> > --- > v2: Clang is fixed too! :) (Nathan) > v1: https://lore.kernel.org/lkml/20250802002733.work.941-kees@kernel.org/ > Cc: Masahiro Yamada <masahiroy@kernel.org> > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: Nicolas Schier <nicolas.schier@linux.dev> > Cc: <linux-kbuild@vger.kernel.org> > --- > scripts/Makefile.extrawarn | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn > index dca175fffcab..a1001377dcb2 100644 > --- a/scripts/Makefile.extrawarn > +++ b/scripts/Makefile.extrawarn > @@ -78,9 +78,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wcast-function-type) > KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow) > KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow) > > -# Currently, disable -Wunterminated-string-initialization as broken > -KBUILD_CFLAGS += $(call cc-option, -Wno-unterminated-string-initialization) > - > # The allocators already balk at large sizes, so silence the compiler > # warnings for bounds checks involving those possible values. While > # -Wno-alloc-size-larger-than would normally be used here, earlier versions > -- > 2.34.1 > I threw this into my local -next tree and my matrix finds the following remaining instances of this warning, mostly in driver code that may be used on specific architectures (I am guessing it is only x86_64 allmodconfig that is clean?). I verified GCC 15 seems a couple of them then I stopped double checking for time sake. ARCH=arm multi_v5_defconfig: drivers/cpufreq/kirkwood-cpufreq.c:98:10: warning: initializer-string for character array is too long, array size is 16 but initializer has size 17 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 98 | .name = "kirkwood-cpufreq", | ^~~~~~~~~~~~~~~~~~ ARCH=arm allmodconfig: drivers/soc/ti/pm33xx.c:77:22: warning: initializer-string for character array is too long, array size is 10 but initializer has size 11 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 77 | .irq_nr = 0, .src = "Ext wakeup", | ^~~~~~~~~~~~ drivers/cpuidle/cpuidle-mvebu-v7.c:69:13: warning: initializer-string for character array is too long, array size is 16 but initializer has size 17 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 69 | .name = "MV CPU DEEP IDLE", | ^~~~~~~~~~~~~~~~~~ drivers/firmware/efi/libstub/vsprintf.c:121:33: warning: initializer-string for character array is too long, array size is 16 but initializer has size 17 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 121 | static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */ | ^~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:1049:2: warning: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 1049 | GBENU_STATS_HOST(ale_unknown_ucast_bytes), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ | ^~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:1051:2: warning: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 1051 | GBENU_STATS_HOST(ale_unknown_mcast_bytes), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ | ^~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:1053:2: warning: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 1053 | GBENU_STATS_HOST(ale_unknown_bcast_bytes), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ | ^~~~~~~~~~~~~~~~~ ARCH=riscv allmodconfig: In file included from arch/riscv/kernel/kgdb.c:13: arch/riscv/include/asm/gdb_xml.h:9:46: warning: initializer-string for character array is too long, array size is 31 but initializer has size 32 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 9 | static const char gdb_xfer_read_target[31] = "qXfer:features:read:target.xml:"; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ arch/riscv/include/asm/gdb_xml.h:13:4: warning: initializer-string for character array is too long, array size is 39 but initializer has size 40 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Wunterminated-string-initialization] 13 | "qXfer:features:read:riscv-64bit-cpu.xml"; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ These seem like obvious fixes from my brief investigations but I am not sure about the other ones. The RISC-V instances seem problematic because kgdb_arch_handle_qxfer_pkt() uses strcpy() to copy those strings? Cheers, Nathan diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c index 71c71c222346..7f7bbdb2a507 100644 --- a/drivers/firmware/efi/libstub/vsprintf.c +++ b/drivers/firmware/efi/libstub/vsprintf.c @@ -118,7 +118,7 @@ char *number(char *end, unsigned long long num, int base, char locase) */ /* we are called with base 8, 10 or 16, only, thus don't need "G..." */ - static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */ + static const char digits[16] __nonstring = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */ switch (base) { case 10: diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c index 55a1a96cd834..05d4323c6a13 100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c +++ b/drivers/net/ethernet/ti/netcp_ethss.c @@ -771,7 +771,7 @@ static struct netcp_module xgbe_module; /* Statistic management */ struct netcp_ethtool_stat { - char desc[ETH_GSTRING_LEN]; + char desc[ETH_GSTRING_LEN] __nonstring; int type; u32 size; int offset; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-03 17:32 ` Nathan Chancellor @ 2025-08-05 14:50 ` Alexander Lobakin 2025-08-05 21:48 ` Nathan Chancellor 0 siblings, 1 reply; 9+ messages in thread From: Alexander Lobakin @ 2025-08-05 14:50 UTC (permalink / raw) To: Nathan Chancellor, Kees Cook Cc: Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening From: Nathan Chancellor <nathan@kernel.org> Date: Sun, 3 Aug 2025 10:32:35 -0700 > On Sat, Aug 02, 2025 at 11:43:32AM -0700, Kees Cook wrote: >> With the few remaining fixes now landed, we can re-enable the option >> -Wunterminated-string-initialization (via -Wextra). Both GCC and Clang >> have the required multi-dimensional nonstring attribute support. [...] > diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c > index 55a1a96cd834..05d4323c6a13 100644 > --- a/drivers/net/ethernet/ti/netcp_ethss.c > +++ b/drivers/net/ethernet/ti/netcp_ethss.c > @@ -771,7 +771,7 @@ static struct netcp_module xgbe_module; > > /* Statistic management */ > struct netcp_ethtool_stat { > - char desc[ETH_GSTRING_LEN]; > + char desc[ETH_GSTRING_LEN] __nonstring; Hmmm, ETH_GSTRING_LEN is the maximum length of the driver's statistics name to be reported to Ethtool and this *includes* \0 at the end. If this compilation flag triggers a warning here, the driver devs need to fix their code. There should always be \0 at the end, `desc` is a "proper" C 0-terminated string. > int type; > u32 size; > int offset; Thanks, Olek ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-05 14:50 ` Alexander Lobakin @ 2025-08-05 21:48 ` Nathan Chancellor 2025-08-06 15:29 ` Alexander Lobakin 0 siblings, 1 reply; 9+ messages in thread From: Nathan Chancellor @ 2025-08-05 21:48 UTC (permalink / raw) To: Alexander Lobakin Cc: Kees Cook, Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening On Tue, Aug 05, 2025 at 04:50:28PM +0200, Alexander Lobakin wrote: > From: Nathan Chancellor <nathan@kernel.org> > Date: Sun, 3 Aug 2025 10:32:35 -0700 > > > On Sat, Aug 02, 2025 at 11:43:32AM -0700, Kees Cook wrote: > >> With the few remaining fixes now landed, we can re-enable the option > >> -Wunterminated-string-initialization (via -Wextra). Both GCC and Clang > >> have the required multi-dimensional nonstring attribute support. > > [...] > > > diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c > > index 55a1a96cd834..05d4323c6a13 100644 > > --- a/drivers/net/ethernet/ti/netcp_ethss.c > > +++ b/drivers/net/ethernet/ti/netcp_ethss.c > > @@ -771,7 +771,7 @@ static struct netcp_module xgbe_module; > > > > /* Statistic management */ > > struct netcp_ethtool_stat { > > - char desc[ETH_GSTRING_LEN]; > > + char desc[ETH_GSTRING_LEN] __nonstring; > > > Hmmm, ETH_GSTRING_LEN is the maximum length of the driver's statistics > name to be reported to Ethtool and this *includes* \0 at the end. > If this compilation flag triggers a warning here, the driver devs need > to fix their code. There should always be \0 at the end, `desc` is a > "proper" C 0-terminated string. Ack, I had misunderstood a previous fix that Kees did for a similar but different instance of the warning in another Ethernet driver and I did not look much further than the driver copying these values around with memcpy(). This does trigger a warning, from the original message: drivers/net/ethernet/ti/netcp_ethss.c:1049:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization] 1049 | GBENU_STATS_HOST(ale_unknown_ucast_bytes), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ | ^~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:1051:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization] 1051 | GBENU_STATS_HOST(ale_unknown_mcast_bytes), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ | ^~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:1053:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization] 1053 | GBENU_STATS_HOST(ale_unknown_bcast_bytes), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ | ^~~~~~~~~~~~~~~~~ So it seems to me like this is a legitimate problem? Are these descriptions expected to be stable once they are released or are we able to adjust them? We could maybe shave an 'o' from 'unknown' to easily resolve this without losing much in the way of quick visual processing. Cheers, Nathan diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c index 55a1a96cd834..70590a04b6fd 100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c +++ b/drivers/net/ethernet/ti/netcp_ethss.c @@ -493,12 +493,12 @@ struct gbenu_hw_stats { u32 ale_vid_ingress_drop; u32 ale_da_eq_sa_drop; u32 __rsvd_0[3]; - u32 ale_unknown_ucast; - u32 ale_unknown_ucast_bytes; - u32 ale_unknown_mcast; - u32 ale_unknown_mcast_bytes; - u32 ale_unknown_bcast; - u32 ale_unknown_bcast_bytes; + u32 ale_unknwn_ucast; + u32 ale_unknwn_ucast_bytes; + u32 ale_unknwn_mcast; + u32 ale_unknwn_mcast_bytes; + u32 ale_unknwn_bcast; + u32 ale_unknwn_bcast_bytes; u32 ale_pol_match; u32 ale_pol_match_red; /* NU */ u32 ale_pol_match_yellow; /* NU */ @@ -953,7 +953,7 @@ static const struct netcp_ethtool_stat gbe13_et_stats[] = { #define GBENU_STATS_HOST(field) \ { \ - "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ + "GBE_HST:"#field, GBENU_STATS0_MODULE, \ sizeof_field(struct gbenu_hw_stats, field), \ offsetof(struct gbenu_hw_stats, field) \ } @@ -1045,12 +1045,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_HOST(ale_rate_limit_drop), GBENU_STATS_HOST(ale_vid_ingress_drop), GBENU_STATS_HOST(ale_da_eq_sa_drop), - GBENU_STATS_HOST(ale_unknown_ucast), - GBENU_STATS_HOST(ale_unknown_ucast_bytes), - GBENU_STATS_HOST(ale_unknown_mcast), - GBENU_STATS_HOST(ale_unknown_mcast_bytes), - GBENU_STATS_HOST(ale_unknown_bcast), - GBENU_STATS_HOST(ale_unknown_bcast_bytes), + GBENU_STATS_HOST(ale_unknwn_ucast), + GBENU_STATS_HOST(ale_unknwn_ucast_bytes), + GBENU_STATS_HOST(ale_unknwn_mcast), + GBENU_STATS_HOST(ale_unknwn_mcast_bytes), + GBENU_STATS_HOST(ale_unknwn_bcast), + GBENU_STATS_HOST(ale_unknwn_bcast_bytes), GBENU_STATS_HOST(ale_pol_match), GBENU_STATS_HOST(ale_pol_match_red), GBENU_STATS_HOST(ale_pol_match_yellow), @@ -1111,12 +1111,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P1(ale_rate_limit_drop), GBENU_STATS_P1(ale_vid_ingress_drop), GBENU_STATS_P1(ale_da_eq_sa_drop), - GBENU_STATS_P1(ale_unknown_ucast), - GBENU_STATS_P1(ale_unknown_ucast_bytes), - GBENU_STATS_P1(ale_unknown_mcast), - GBENU_STATS_P1(ale_unknown_mcast_bytes), - GBENU_STATS_P1(ale_unknown_bcast), - GBENU_STATS_P1(ale_unknown_bcast_bytes), + GBENU_STATS_P1(ale_unknwn_ucast), + GBENU_STATS_P1(ale_unknwn_ucast_bytes), + GBENU_STATS_P1(ale_unknwn_mcast), + GBENU_STATS_P1(ale_unknwn_mcast_bytes), + GBENU_STATS_P1(ale_unknwn_bcast), + GBENU_STATS_P1(ale_unknwn_bcast_bytes), GBENU_STATS_P1(ale_pol_match), GBENU_STATS_P1(ale_pol_match_red), GBENU_STATS_P1(ale_pol_match_yellow), @@ -1177,12 +1177,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P2(ale_rate_limit_drop), GBENU_STATS_P2(ale_vid_ingress_drop), GBENU_STATS_P2(ale_da_eq_sa_drop), - GBENU_STATS_P2(ale_unknown_ucast), - GBENU_STATS_P2(ale_unknown_ucast_bytes), - GBENU_STATS_P2(ale_unknown_mcast), - GBENU_STATS_P2(ale_unknown_mcast_bytes), - GBENU_STATS_P2(ale_unknown_bcast), - GBENU_STATS_P2(ale_unknown_bcast_bytes), + GBENU_STATS_P2(ale_unknwn_ucast), + GBENU_STATS_P2(ale_unknwn_ucast_bytes), + GBENU_STATS_P2(ale_unknwn_mcast), + GBENU_STATS_P2(ale_unknwn_mcast_bytes), + GBENU_STATS_P2(ale_unknwn_bcast), + GBENU_STATS_P2(ale_unknwn_bcast_bytes), GBENU_STATS_P2(ale_pol_match), GBENU_STATS_P2(ale_pol_match_red), GBENU_STATS_P2(ale_pol_match_yellow), @@ -1243,12 +1243,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P3(ale_rate_limit_drop), GBENU_STATS_P3(ale_vid_ingress_drop), GBENU_STATS_P3(ale_da_eq_sa_drop), - GBENU_STATS_P3(ale_unknown_ucast), - GBENU_STATS_P3(ale_unknown_ucast_bytes), - GBENU_STATS_P3(ale_unknown_mcast), - GBENU_STATS_P3(ale_unknown_mcast_bytes), - GBENU_STATS_P3(ale_unknown_bcast), - GBENU_STATS_P3(ale_unknown_bcast_bytes), + GBENU_STATS_P3(ale_unknwn_ucast), + GBENU_STATS_P3(ale_unknwn_ucast_bytes), + GBENU_STATS_P3(ale_unknwn_mcast), + GBENU_STATS_P3(ale_unknwn_mcast_bytes), + GBENU_STATS_P3(ale_unknwn_bcast), + GBENU_STATS_P3(ale_unknwn_bcast_bytes), GBENU_STATS_P3(ale_pol_match), GBENU_STATS_P3(ale_pol_match_red), GBENU_STATS_P3(ale_pol_match_yellow), @@ -1309,12 +1309,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P4(ale_rate_limit_drop), GBENU_STATS_P4(ale_vid_ingress_drop), GBENU_STATS_P4(ale_da_eq_sa_drop), - GBENU_STATS_P4(ale_unknown_ucast), - GBENU_STATS_P4(ale_unknown_ucast_bytes), - GBENU_STATS_P4(ale_unknown_mcast), - GBENU_STATS_P4(ale_unknown_mcast_bytes), - GBENU_STATS_P4(ale_unknown_bcast), - GBENU_STATS_P4(ale_unknown_bcast_bytes), + GBENU_STATS_P4(ale_unknwn_ucast), + GBENU_STATS_P4(ale_unknwn_ucast_bytes), + GBENU_STATS_P4(ale_unknwn_mcast), + GBENU_STATS_P4(ale_unknwn_mcast_bytes), + GBENU_STATS_P4(ale_unknwn_bcast), + GBENU_STATS_P4(ale_unknwn_bcast_bytes), GBENU_STATS_P4(ale_pol_match), GBENU_STATS_P4(ale_pol_match_red), GBENU_STATS_P4(ale_pol_match_yellow), @@ -1375,12 +1375,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P5(ale_rate_limit_drop), GBENU_STATS_P5(ale_vid_ingress_drop), GBENU_STATS_P5(ale_da_eq_sa_drop), - GBENU_STATS_P5(ale_unknown_ucast), - GBENU_STATS_P5(ale_unknown_ucast_bytes), - GBENU_STATS_P5(ale_unknown_mcast), - GBENU_STATS_P5(ale_unknown_mcast_bytes), - GBENU_STATS_P5(ale_unknown_bcast), - GBENU_STATS_P5(ale_unknown_bcast_bytes), + GBENU_STATS_P5(ale_unknwn_ucast), + GBENU_STATS_P5(ale_unknwn_ucast_bytes), + GBENU_STATS_P5(ale_unknwn_mcast), + GBENU_STATS_P5(ale_unknwn_mcast_bytes), + GBENU_STATS_P5(ale_unknwn_bcast), + GBENU_STATS_P5(ale_unknwn_bcast_bytes), GBENU_STATS_P5(ale_pol_match), GBENU_STATS_P5(ale_pol_match_red), GBENU_STATS_P5(ale_pol_match_yellow), @@ -1441,12 +1441,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P6(ale_rate_limit_drop), GBENU_STATS_P6(ale_vid_ingress_drop), GBENU_STATS_P6(ale_da_eq_sa_drop), - GBENU_STATS_P6(ale_unknown_ucast), - GBENU_STATS_P6(ale_unknown_ucast_bytes), - GBENU_STATS_P6(ale_unknown_mcast), - GBENU_STATS_P6(ale_unknown_mcast_bytes), - GBENU_STATS_P6(ale_unknown_bcast), - GBENU_STATS_P6(ale_unknown_bcast_bytes), + GBENU_STATS_P6(ale_unknwn_ucast), + GBENU_STATS_P6(ale_unknwn_ucast_bytes), + GBENU_STATS_P6(ale_unknwn_mcast), + GBENU_STATS_P6(ale_unknwn_mcast_bytes), + GBENU_STATS_P6(ale_unknwn_bcast), + GBENU_STATS_P6(ale_unknwn_bcast_bytes), GBENU_STATS_P6(ale_pol_match), GBENU_STATS_P6(ale_pol_match_red), GBENU_STATS_P6(ale_pol_match_yellow), @@ -1507,12 +1507,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P7(ale_rate_limit_drop), GBENU_STATS_P7(ale_vid_ingress_drop), GBENU_STATS_P7(ale_da_eq_sa_drop), - GBENU_STATS_P7(ale_unknown_ucast), - GBENU_STATS_P7(ale_unknown_ucast_bytes), - GBENU_STATS_P7(ale_unknown_mcast), - GBENU_STATS_P7(ale_unknown_mcast_bytes), - GBENU_STATS_P7(ale_unknown_bcast), - GBENU_STATS_P7(ale_unknown_bcast_bytes), + GBENU_STATS_P7(ale_unknwn_ucast), + GBENU_STATS_P7(ale_unknwn_ucast_bytes), + GBENU_STATS_P7(ale_unknwn_mcast), + GBENU_STATS_P7(ale_unknwn_mcast_bytes), + GBENU_STATS_P7(ale_unknwn_bcast), + GBENU_STATS_P7(ale_unknwn_bcast_bytes), GBENU_STATS_P7(ale_pol_match), GBENU_STATS_P7(ale_pol_match_red), GBENU_STATS_P7(ale_pol_match_yellow), @@ -1573,12 +1573,12 @@ static const struct netcp_ethtool_stat gbenu_et_stats[] = { GBENU_STATS_P8(ale_rate_limit_drop), GBENU_STATS_P8(ale_vid_ingress_drop), GBENU_STATS_P8(ale_da_eq_sa_drop), - GBENU_STATS_P8(ale_unknown_ucast), - GBENU_STATS_P8(ale_unknown_ucast_bytes), - GBENU_STATS_P8(ale_unknown_mcast), - GBENU_STATS_P8(ale_unknown_mcast_bytes), - GBENU_STATS_P8(ale_unknown_bcast), - GBENU_STATS_P8(ale_unknown_bcast_bytes), + GBENU_STATS_P8(ale_unknwn_ucast), + GBENU_STATS_P8(ale_unknwn_ucast_bytes), + GBENU_STATS_P8(ale_unknwn_mcast), + GBENU_STATS_P8(ale_unknwn_mcast_bytes), + GBENU_STATS_P8(ale_unknwn_bcast), + GBENU_STATS_P8(ale_unknwn_bcast_bytes), GBENU_STATS_P8(ale_pol_match), GBENU_STATS_P8(ale_pol_match_red), GBENU_STATS_P8(ale_pol_match_yellow), ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-05 21:48 ` Nathan Chancellor @ 2025-08-06 15:29 ` Alexander Lobakin 2025-08-06 19:05 ` Kees Cook 0 siblings, 1 reply; 9+ messages in thread From: Alexander Lobakin @ 2025-08-06 15:29 UTC (permalink / raw) To: Nathan Chancellor Cc: Kees Cook, Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening From: Nathan Chancellor <nathan@kernel.org> Date: Tue, 5 Aug 2025 14:48:23 -0700 > On Tue, Aug 05, 2025 at 04:50:28PM +0200, Alexander Lobakin wrote: >> From: Nathan Chancellor <nathan@kernel.org> >> Date: Sun, 3 Aug 2025 10:32:35 -0700 >> >>> On Sat, Aug 02, 2025 at 11:43:32AM -0700, Kees Cook wrote: >>>> With the few remaining fixes now landed, we can re-enable the option >>>> -Wunterminated-string-initialization (via -Wextra). Both GCC and Clang >>>> have the required multi-dimensional nonstring attribute support. >> >> [...] >> >>> diff --git a/drivers/net/ethernet/ti/netcp_ethss.c b/drivers/net/ethernet/ti/netcp_ethss.c >>> index 55a1a96cd834..05d4323c6a13 100644 >>> --- a/drivers/net/ethernet/ti/netcp_ethss.c >>> +++ b/drivers/net/ethernet/ti/netcp_ethss.c >>> @@ -771,7 +771,7 @@ static struct netcp_module xgbe_module; >>> >>> /* Statistic management */ >>> struct netcp_ethtool_stat { >>> - char desc[ETH_GSTRING_LEN]; >>> + char desc[ETH_GSTRING_LEN] __nonstring; >> >> >> Hmmm, ETH_GSTRING_LEN is the maximum length of the driver's statistics >> name to be reported to Ethtool and this *includes* \0 at the end. >> If this compilation flag triggers a warning here, the driver devs need >> to fix their code. There should always be \0 at the end, `desc` is a >> "proper" C 0-terminated string. > > Ack, I had misunderstood a previous fix that Kees did for a similar but > different instance of the warning in another Ethernet driver and I > did not look much further than the driver copying these values around > with memcpy(). This does trigger a warning, from the original message: > > drivers/net/ethernet/ti/netcp_ethss.c:1049:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization] > 1049 | GBENU_STATS_HOST(ale_unknown_ucast_bytes), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' > 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ > | ^~~~~~~~~~~~~~~~~ > drivers/net/ethernet/ti/netcp_ethss.c:1051:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization] > 1051 | GBENU_STATS_HOST(ale_unknown_mcast_bytes), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' > 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ > | ^~~~~~~~~~~~~~~~~ > drivers/net/ethernet/ti/netcp_ethss.c:1053:2: error: initializer-string for character array is too long, array size is 32 but initializer has size 33 (including the null terminating character); did you mean to use the 'nonstring' attribute? [-Werror,-Wunterminated-string-initialization] > 1053 | GBENU_STATS_HOST(ale_unknown_bcast_bytes), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/ti/netcp_ethss.c:956:2: note: expanded from macro 'GBENU_STATS_HOST' > 956 | "GBE_HOST:"#field, GBENU_STATS0_MODULE, \ > | ^~~~~~~~~~~~~~~~~ > > So it seems to me like this is a legitimate problem? Are these Yes, it's not a false-positive. I don't know how it worked at the end of the day, may either the kernel Ethtool core or the userspace Ethtool put \0 at the end (or maybe it was just luck or nobody tested Ethtool stats on this driver). > descriptions expected to be stable once they are released or are we able Ethtool private stats are not "ABI" at all. Moreover, if they result in incorrect code, this needs to be fixed no matter if someone already wrote scripts dependent on these names or not. > to adjust them? We could maybe shave an 'o' from 'unknown' to easily > resolve this without losing much in the way of quick visual processing. I've no idea why it's popular to define Ethtool stats names in drivers using a fixed array of ETH_GSTRING_LEN and then do memcpy(). I've been always using just `const char * const[]` + strscpy() (then switched the latter to ethtool_puts()/ethtool_sprintf() -- we even have special helpers for that). In case some name goes past ETH_GSTRING_LEN, it would just be truncated, but always have \0 at the end. Plus most of the names are shorter than 32, so defining such arrays of 32 just wastes space in .rodata. > > Cheers, > Nathan Thanks, Olek ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-06 15:29 ` Alexander Lobakin @ 2025-08-06 19:05 ` Kees Cook 2025-08-07 13:31 ` Alexander Lobakin 0 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2025-08-06 19:05 UTC (permalink / raw) To: Alexander Lobakin Cc: Nathan Chancellor, Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening On Wed, Aug 06, 2025 at 05:29:55PM +0200, Alexander Lobakin wrote: > From: Nathan Chancellor <nathan@kernel.org> Thank you for the fixes Nathan! I'll dig through these and get them sent out before I try to land this patch again -- "But COMPILE_TEST is never wrong!" ;) > > [...] > > descriptions expected to be stable once they are released or are we able > > Ethtool private stats are not "ABI" at all. Moreover, if they result in > incorrect code, this needs to be fixed no matter if someone already > wrote scripts dependent on these names or not. Internally there isn't an ABI, but the userspace interface effectively has an ABI: the strings are fixed-size and NUL-padded but not NUL-terminated. > > to adjust them? We could maybe shave an 'o' from 'unknown' to easily > > resolve this without losing much in the way of quick visual processing. > > I've no idea why it's popular to define Ethtool stats names in drivers > using a fixed array of ETH_GSTRING_LEN and then do memcpy(). The above is why: they are fixed-size, non-NUL-terminated strings, so many drivers use this memcpy pattern. But not all. > I've been always using just `const char * const[]` + strscpy() (then > switched the latter to ethtool_puts()/ethtool_sprintf() -- we even have > special helpers for that). In case some name goes past ETH_GSTRING_LEN, > it would just be truncated, but always have \0 at the end. Unfortunately this is not true: not all sources are NUL terminated. > Plus most of the names are shorter than 32, so defining such arrays of > 32 just wastes space in .rodata. That IS true, but many drivers just keep giant blocks of data they can memcpy. :( Regardless, I will double-check this and see what needs to happen here. I've fixed a lot of these already[1]. -Kees [1] https://lore.kernel.org/lkml/20250416010210.work.904-kees@kernel.org/ -- Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-06 19:05 ` Kees Cook @ 2025-08-07 13:31 ` Alexander Lobakin 2025-08-07 22:00 ` Kees Cook 0 siblings, 1 reply; 9+ messages in thread From: Alexander Lobakin @ 2025-08-07 13:31 UTC (permalink / raw) To: Kees Cook Cc: Nathan Chancellor, Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening From: Kees Cook <kees@kernel.org> Date: Wed, 6 Aug 2025 12:05:32 -0700 > On Wed, Aug 06, 2025 at 05:29:55PM +0200, Alexander Lobakin wrote: >> From: Nathan Chancellor <nathan@kernel.org> > > Thank you for the fixes Nathan! I'll dig through these and get them sent > out before I try to land this patch again -- "But COMPILE_TEST is never > wrong!" ;) > >>> [...] >>> descriptions expected to be stable once they are released or are we able >> >> Ethtool private stats are not "ABI" at all. Moreover, if they result in >> incorrect code, this needs to be fixed no matter if someone already >> wrote scripts dependent on these names or not. > > Internally there isn't an ABI, but the userspace interface effectively has > an ABI: the strings are fixed-size and NUL-padded but not NUL-terminated. Ooops, maybe I missed something. I mean I know that Ethtool passes one big array of 32-byte-sized strings, but is it fine if some of these strings won't be NUL-terminated? > >>> to adjust them? We could maybe shave an 'o' from 'unknown' to easily >>> resolve this without losing much in the way of quick visual processing. >> >> I've no idea why it's popular to define Ethtool stats names in drivers >> using a fixed array of ETH_GSTRING_LEN and then do memcpy(). > > The above is why: they are fixed-size, non-NUL-terminated strings, so > many drivers use this memcpy pattern. But not all. Sure, lots of drivers uses normal string copy functions etc. But Ethtool strings *must* be NUL-terminated, so this fixed-size + memcpy() only hurts. > >> I've been always using just `const char * const[]` + strscpy() (then >> switched the latter to ethtool_puts()/ethtool_sprintf() -- we even have >> special helpers for that). In case some name goes past ETH_GSTRING_LEN, >> it would just be truncated, but always have \0 at the end. > > Unfortunately this is not true: not all sources are NUL terminated. I meant the following: Imagine a driver defining its stats string: static const char * const arr[] = { ... "some_stat_that_goes_above_ETH_GSTRING_LEN", }; Then, if this driver copies that string using ethtool_puts() or just strscpy(ETH_GSTRING_LEN), the string will be truncated, but \0 at the end is guaranteed. > >> Plus most of the names are shorter than 32, so defining such arrays of >> 32 just wastes space in .rodata. > > That IS true, but many drivers just keep giant blocks of data they can > memcpy. :( > > Regardless, I will double-check this and see what needs to happen here. > I've fixed a lot of these already[1]. > > -Kees > > [1] https://lore.kernel.org/lkml/20250416010210.work.904-kees@kernel.org/ Thanks, Olek ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-07 13:31 ` Alexander Lobakin @ 2025-08-07 22:00 ` Kees Cook 2025-08-11 14:25 ` Alexander Lobakin 0 siblings, 1 reply; 9+ messages in thread From: Kees Cook @ 2025-08-07 22:00 UTC (permalink / raw) To: Alexander Lobakin Cc: Nathan Chancellor, Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening On Thu, Aug 07, 2025 at 03:31:05PM +0200, Alexander Lobakin wrote: > Sure, lots of drivers uses normal string copy functions etc. > But Ethtool strings *must* be NUL-terminated, so this fixed-size + > memcpy() only hurts. This is the misunderstanding: they're only NUL padded, but not strictly NUL terminated. You can see ethtool itself has to be careful with the strings, limiting the fprintf to their sizeof(): https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/ethtool.c#n1013 or using strncmp everywhere. -- Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization 2025-08-07 22:00 ` Kees Cook @ 2025-08-11 14:25 ` Alexander Lobakin 0 siblings, 0 replies; 9+ messages in thread From: Alexander Lobakin @ 2025-08-11 14:25 UTC (permalink / raw) To: Kees Cook Cc: Nathan Chancellor, Masahiro Yamada, Nicolas Schier, linux-kbuild, Nick Desaulniers, Bill Wendling, Justin Stitt, linux-kernel, llvm, linux-hardening From: Kees Cook <kees@kernel.org> Date: Thu, 7 Aug 2025 15:00:24 -0700 > On Thu, Aug 07, 2025 at 03:31:05PM +0200, Alexander Lobakin wrote: >> Sure, lots of drivers uses normal string copy functions etc. >> But Ethtool strings *must* be NUL-terminated, so this fixed-size + >> memcpy() only hurts. > > This is the misunderstanding: they're only NUL padded, but not strictly > NUL terminated. You can see ethtool itself has to be careful with the > strings, limiting the fprintf to their sizeof(): > > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/ethtool.c#n1013 > > or using strncmp everywhere. Maybe we should add a check to the Ethtool core that every 32-th array symbol == \0 to detect misbehaving drivers like this one :D Thanks, Olek ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-08-11 14:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-02 18:43 [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization Kees Cook 2025-08-03 17:32 ` Nathan Chancellor 2025-08-05 14:50 ` Alexander Lobakin 2025-08-05 21:48 ` Nathan Chancellor 2025-08-06 15:29 ` Alexander Lobakin 2025-08-06 19:05 ` Kees Cook 2025-08-07 13:31 ` Alexander Lobakin 2025-08-07 22:00 ` Kees Cook 2025-08-11 14:25 ` Alexander Lobakin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).