[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <158eb222-d875-4f96-b027-83854e5f4275@embeddedor.com>
Date: Mon, 28 Oct 2024 17:32:53 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Jakub Kicinski <kuba@...nel.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: Michael Chan <michael.chan@...adcom.com>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Potnuri Bharat Teja <bharat@...lsio.com>,
Christian Benvenuti <benve@...co.com>, Satish Kharat <satishkh@...co.com>,
Manish Chopra <manishc@...vell.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of
-Wflex-array-member-not-at-end warnings
On 28/10/24 17:21, Jakub Kicinski wrote:
> On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
>> Fix 3338 of the following -Wflex-array-member-not-at-end warnings:
>>
>> include/linux/ethtool.h:214:38: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>
> I don't see any change in the number of warnings with W=1:
> gcc (GCC) 14.2.1 20240912 (Red Hat 14.2.1-3)
> Is it only enabled with W=2?
-Wfamnae is not currently part of any upstream build. We are working
to have it enabled. So, these warnings are the ones I see in my local
build with the following patch applied:
diff --git a/Makefile b/Makefile
index d4a41c44e0fc..08d18b5d01f5 100644
--- a/Makefile
+++ b/Makefile
@@ -1002,6 +1002,9 @@ NOSTDINC_FLAGS += -nostdinc
# perform bounds checking.
KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)
+# Avoid flexible-array members not at the end of composite structure.
+KBUILD_CFLAGS += $(call cc-option, -Wflex-array-member-not-at-end)
+
#Currently, disable -Wstringop-overflow for GCC 11, globally.
KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow)
KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow)
>
>> Additionally, update the type of some variables in various functions
>> that don't access the flexible-array member, changing them to the
>> newly created `struct ethtool_link_settings_hdr`.
>
> Why? Please avoid unnecessary code changes.
This is actually necessary. As the type of the conflicting middle members
changed, those instances that expect the type to be `struct ethtool_link_settings`
should be adjusted to the new type. Another option is to leave the type
unchanged and instead use container_of. See below.
So, instead of this:
- struct ethtool_link_settings *base = &link_ksettings->base;
+ struct ethtool_link_settings_hdr *base = &link_ksettings->base;
we would do something like this:
- struct ethtool_link_settings *base = &link_ksettings->base;
+ struct ethtool_link_settings *base = container_of(&link_ksettings->base,
+ struct struct ethtool_link_settings, hdr);
I think that in this case, we could avoid using `container_of()`, but
if you prefer that, I can update the patch.
>
>> include/linux/ethtool.h | 2 +-
>
> This is probably where most of the warnings come from.
> Please split the changes to this header file as a separate patch
> for ease of review / validation.
>
Sure thing!
Thanks
--
Gustavo
Powered by blists - more mailing lists