[PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Kees Cook]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Nathan Chancellor]

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;

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Alexander Lobakin]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Nathan Chancellor]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Alexander Lobakin]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Kees Cook]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Alexander Lobakin]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Kees Cook]

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

Re: [PATCH v2] kbuild: Re-enable -Wunterminated-string-initialization

[Alexander Lobakin]

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