Use a precision specifier in snprintf to ensure the generated
string fits within the ETH_GSTRING_LEN (32 bytes) buffer.
Signed-off-by: Sean Chang <seanwascoding@gmail.com>
---
v2:
- Split the original treewide patch into subsystem-specific commits.
- Added more detailed commit descriptions to satisfy checkpatch.
drivers/net/ethernet/cadence/macb_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 43cd013bb70e..26f9ccadd9f6 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3159,8 +3159,8 @@ static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
for (i = 0; i < QUEUE_STATS_LEN; i++, p += ETH_GSTRING_LEN) {
- snprintf(stat_string, ETH_GSTRING_LEN, "q%d_%s",
- q, queue_statistics[i].stat_string);
+ snprintf(stat_string, ETH_GSTRING_LEN, "q%u_%.19s",
+ q, queue_statistics[i].stat_string);
memcpy(p, stat_string, ETH_GSTRING_LEN);
}
}
--
2.34.1On Tue, Feb 17, 2026 at 01:49:50AM +0800, Sean Chang wrote:
> Use a precision specifier in snprintf to ensure the generated
> string fits within the ETH_GSTRING_LEN (32 bytes) buffer.
>
> Signed-off-by: Sean Chang <seanwascoding@gmail.com>
> ---
> v2:
> - Split the original treewide patch into subsystem-specific commits.
> - Added more detailed commit descriptions to satisfy checkpatch.
>
> drivers/net/ethernet/cadence/macb_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 43cd013bb70e..26f9ccadd9f6 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3159,8 +3159,8 @@ static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
>
> for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> for (i = 0; i < QUEUE_STATS_LEN; i++, p += ETH_GSTRING_LEN) {
> - snprintf(stat_string, ETH_GSTRING_LEN, "q%d_%s",
> - q, queue_statistics[i].stat_string);
> + snprintf(stat_string, ETH_GSTRING_LEN, "q%u_%.19s",
> + q, queue_statistics[i].stat_string);
These strings are special, in that they are fixed length, 32 bytes
long, and not \0 terminated. There are some helpers at the end of
linux/ethtool.h for dealing with these strings. You might want to use
them.
> memcpy(p, stat_string, ETH_GSTRING_LEN);
Also, it looks like stat_string might be one byte too
short. snprintf() will \0 terminate, so you need it big enough to hold
the \0, and then this memcpy() will discard it.
I also wounder, why is just one architecture complaining about this?
Andrew
On Tue, Feb 17, 2026 at 2:35 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 43cd013bb70e..26f9ccadd9f6 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -3159,8 +3159,8 @@ static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
> >
> > for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> > for (i = 0; i < QUEUE_STATS_LEN; i++, p += ETH_GSTRING_LEN) {
> > - snprintf(stat_string, ETH_GSTRING_LEN, "q%d_%s",
> > - q, queue_statistics[i].stat_string);
> > + snprintf(stat_string, ETH_GSTRING_LEN, "q%u_%.19s",
> > + q, queue_statistics[i].stat_string);
>
> These strings are special, in that they are fixed length, 32 bytes
> long, and not \0 terminated. There are some helpers at the end of
> linux/ethtool.h for dealing with these strings. You might want to use
> them.
>
Yes, I've switched to ethtool_sprintf() from linux/ethtool.h and
removed the manual
snprintf() and memcpy() calls. After testing, it behaves exactly as expected,
so I will send out [PATCH V3] shortly.
> I also wounder, why is just one architecture complaining about this?
Regarding the architecture-specific nature of the warning: I have verified that
this warning is not triggered on x86_64, even with W=1. It appears to
be specific
to the RISC-V toolchain's diagnostic analysis.
Regardless, converting to ethtool_sprintf() is the correct approach
which does not throw
any warning on the both platforms.
Best regards,
Sean
> Regarding the architecture-specific nature of the warning: I have verified that > this warning is not triggered on x86_64, even with W=1. It appears to > be specific > to the RISC-V toolchain's diagnostic analysis. Given the other patches in there series, i have to wounder, is the diagnostic analysis correct? Or is the RISC-V toolchain buggy? > Regardless, converting to ethtool_sprintf() is the correct approach > which does not throw any warning on the both platforms. I do agree ethtool_sprintf() is better. But we want the commit message to reflect why we are making this change. Is it because ethtool_sprintf() is better, or are we working around toolchain bugs? Andrew
On Wed, Feb 18, 2026 at 1:46 AM Andrew Lunn <andrew@lunn.ch> wrote: > > Given the other patches in there series, i have to wounder, is the > diagnostic analysis correct? Or is the RISC-V toolchain buggy? > I retry the different methods, I found that when I directly compile the macb_main.c, it can trigger the same error, and I already prove the different compiler will reach the same result, so it is not a bug of the compiler. I also found that the x86_64 defconfig does not enable CONFIG_MACB because it requires CONFIG_COMMON_CLK (which is also disabled by default on x86), whereas the RISC-V defconfig has both enabled. > > I do agree ethtool_sprintf() is better. But we want the commit message > to reflect why we are making this change. Is it because > ethtool_sprintf() is better, or are we working around toolchain bugs? > according to the content I mentioned upon, I think the commit will something like "ethtool_sprintf() is better to prevent data truncation and properly fill the 32-byte ethtool string boundary.'" Best regards, Sean
On Wed, Feb 18, 2026 at 03:14:08AM +0800, Sean Chang wrote: > On Wed, Feb 18, 2026 at 1:46 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > Given the other patches in there series, i have to wounder, is the > > diagnostic analysis correct? Or is the RISC-V toolchain buggy? > > > > I retry the different methods, I found that when I directly compile > the macb_main.c, it can trigger the same error, and I already prove > the different compiler will reach the same result, so it is not a bug > of the compiler. > > I also found that the x86_64 defconfig does not enable CONFIG_MACB > because it requires CONFIG_COMMON_CLK (which is also disabled by > default on x86), whereas the RISC-V defconfig has both enabled. arm and arm64 are much more similar to risc-v than x86. You should test with those compilers. However, make allmodconfig will get you the common clk code on x86. Most build testing is done with this configuration, including the netdev CI. > > I do agree ethtool_sprintf() is better. But we want the commit message > > to reflect why we are making this change. Is it because > > ethtool_sprintf() is better, or are we working around toolchain bugs? > > > > according to the content I mentioned upon, I think the commit will > something like "ethtool_sprintf() is better to prevent data truncation > and properly fill the 32-byte ethtool string boundary.'" O.K. Andrew
On Wed, Feb 18, 2026 at 4:33 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > Given the other patches in there series, i have to wounder, is the > > > diagnostic analysis correct? Or is the RISC-V toolchain buggy? > > > > > > > I retry the different methods, I found that when I directly compile > > the macb_main.c, it can trigger the same error, and I already prove > > the different compiler will reach the same result, so it is not a bug > > of the compiler. > > > > I also found that the x86_64 defconfig does not enable CONFIG_MACB > > because it requires CONFIG_COMMON_CLK (which is also disabled by > > default on x86), whereas the RISC-V defconfig has both enabled. > > arm and arm64 are much more similar to risc-v than x86. You should > test with those compilers. However, make allmodconfig will get you the > common clk code on x86. Most build testing is done with this > configuration, including the netdev CI. > Thanks for your suggestion to use the "allmodconfig" before the build. I running the command: "make ARCH=arm64 CROSS_COMPILE=aarch64- linux-gnu- KCFLAGS="-Wno-error" W=1 C=1 modules -j$(nproc) 2>&1 | tee build.log" and "make ARCH=arm CROSS_COMPILE=arm-linux- gnueabihf- KCFLAGS="-Wno-error" W=1 C=1 modules -j$(nproc) 2>&1 | tee build.log", it throws the same warning -> [-Wformat-truncation=], so It proves that even if I switch to a different platform, like arm, arm32, x86_64, riscv64, it still shows the same warning. Best regards, Sean
© 2016 - 2026 Red Hat, Inc.