netdev - Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<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