| Message ID | 9e9fb0bd72e5ba1e916acbb4995b1e358b86a689.1730238285.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 43d3487035e9a86fad952de4240a518614240d43 |
| Headers | show |
| Series | UAPI: net/ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings | expand |
On Tue, 29 Oct 2024 15:55:35 -0600 Gustavo A. R. Silva wrote: > Use the `__struct_group()` helper to create a new tagged > `struct ethtool_link_settings_hdr`. This structure groups together > all the members of the flexible `struct ethtool_link_settings` > except the flexible array. As a result, the array is effectively > separated from the rest of the members without modifying the memory > layout of the flexible structure. > > This new tagged struct will be used to fix problematic declarations > of middle-flex-arrays in composite structs[1]. Possibly a very noob question, but I'm updating a C++ library with new headers and I think this makes it no longer compile. $ cat > /tmp/t.cpp<<EOF extern "C" { #include "include/uapi/linux/ethtool.h" } int func() { return 0; } EOF $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 In file included from /usr/include/linux/posix_types.h:5, from /usr/include/linux/types.h:9, from ../linux/include/uapi/linux/ethtool.h:18, from /tmp/t.cpp:2: ../linux/include/uapi/linux/ethtool.h:2515:24: error: ‘struct ethtool_link_settings::<unnamed union>::ethtool_link_settings_hdr’ invalid; an anonymous union may only have public non-static data members [-fpermissive] 2515 | __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, | ^~~~~~~~~~~~~~~~~~~~~~~~~ I don't know much about C++, tho, so quite possibly missing something obvious.
On Sat, 9 Nov 2024 10:02:13 -0800 Jakub Kicinski wrote:
> $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2
gcc version 14.2.1 20240912 (Red Hat 14.2.1-3) (GCC)
On 09/11/24 12:02, Jakub Kicinski wrote: > On Tue, 29 Oct 2024 15:55:35 -0600 Gustavo A. R. Silva wrote: >> Use the `__struct_group()` helper to create a new tagged >> `struct ethtool_link_settings_hdr`. This structure groups together >> all the members of the flexible `struct ethtool_link_settings` >> except the flexible array. As a result, the array is effectively >> separated from the rest of the members without modifying the memory >> layout of the flexible structure. >> >> This new tagged struct will be used to fix problematic declarations >> of middle-flex-arrays in composite structs[1]. > > Possibly a very noob question, but I'm updating a C++ library with > new headers and I think this makes it no longer compile. > > $ cat > /tmp/t.cpp<<EOF > extern "C" { > #include "include/uapi/linux/ethtool.h" > } > int func() { return 0; } > EOF > > $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 > In file included from /usr/include/linux/posix_types.h:5, > from /usr/include/linux/types.h:9, > from ../linux/include/uapi/linux/ethtool.h:18, > from /tmp/t.cpp:2: > ../linux/include/uapi/linux/ethtool.h:2515:24: error: ‘struct ethtool_link_settings::<unnamed union>::ethtool_link_settings_hdr’ invalid; an anonymous union may only have public non-static data members [-fpermissive] > 2515 | __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > I don't know much about C++, tho, so quite possibly missing something > obvious. We are in the same situation here. It seems C++ considers it ambiguous to define a struct with a tag such as `struct TAG { MEMBERS } ATTRS NAME;` within an anonymous union. Let me look into this further... -- Gustavo
On 11/11/24 16:22, Gustavo A. R. Silva wrote: > > > On 09/11/24 12:02, Jakub Kicinski wrote: >> On Tue, 29 Oct 2024 15:55:35 -0600 Gustavo A. R. Silva wrote: >>> Use the `__struct_group()` helper to create a new tagged >>> `struct ethtool_link_settings_hdr`. This structure groups together >>> all the members of the flexible `struct ethtool_link_settings` >>> except the flexible array. As a result, the array is effectively >>> separated from the rest of the members without modifying the memory >>> layout of the flexible structure. >>> >>> This new tagged struct will be used to fix problematic declarations >>> of middle-flex-arrays in composite structs[1]. >> >> Possibly a very noob question, but I'm updating a C++ library with >> new headers and I think this makes it no longer compile. >> >> $ cat > /tmp/t.cpp<<EOF >> extern "C" { >> #include "include/uapi/linux/ethtool.h" >> } >> int func() { return 0; } >> EOF >> >> $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 >> In file included from /usr/include/linux/posix_types.h:5, >> from /usr/include/linux/types.h:9, >> from ../linux/include/uapi/linux/ethtool.h:18, >> from /tmp/t.cpp:2: >> ../linux/include/uapi/linux/ethtool.h:2515:24: error: ‘struct ethtool_link_settings::<unnamed union>::ethtool_link_settings_hdr’ invalid; an anonymous union >> may only have public non-static data members [-fpermissive] >> 2515 | __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, >> | ^~~~~~~~~~~~~~~~~~~~~~~~~ >> >> This seems to work with Clang: $ clang++-18 -fms-extensions /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 However, `-fms-extensions` doesn't seem to work for this case with GCC: https://godbolt.org/z/1shsPhz3s -Gustavo >> I don't know much about C++, tho, so quite possibly missing something >> obvious. > > We are in the same situation here. > > It seems C++ considers it ambiguous to define a struct with a tag such > as `struct TAG { MEMBERS } ATTRS NAME;` within an anonymous union. > > Let me look into this further... > -- > Gustavo >
On Tue, Nov 12, 2024 at 07:08:32PM -0600, Gustavo A. R. Silva wrote: > > > On 11/11/24 16:22, Gustavo A. R. Silva wrote: > > > > > > On 09/11/24 12:02, Jakub Kicinski wrote: > > > On Tue, 29 Oct 2024 15:55:35 -0600 Gustavo A. R. Silva wrote: > > > > Use the `__struct_group()` helper to create a new tagged > > > > `struct ethtool_link_settings_hdr`. This structure groups together > > > > all the members of the flexible `struct ethtool_link_settings` > > > > except the flexible array. As a result, the array is effectively > > > > separated from the rest of the members without modifying the memory > > > > layout of the flexible structure. > > > > > > > > This new tagged struct will be used to fix problematic declarations > > > > of middle-flex-arrays in composite structs[1]. > > > > > > Possibly a very noob question, but I'm updating a C++ library with > > > new headers and I think this makes it no longer compile. > > > > > > $ cat > /tmp/t.cpp<<EOF > > > extern "C" { > > > #include "include/uapi/linux/ethtool.h" > > > } > > > int func() { return 0; } > > > EOF > > > > > > $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 > > > In file included from /usr/include/linux/posix_types.h:5, > > > from /usr/include/linux/types.h:9, > > > from ../linux/include/uapi/linux/ethtool.h:18, > > > from /tmp/t.cpp:2: > > > ../linux/include/uapi/linux/ethtool.h:2515:24: error: ‘struct > > > ethtool_link_settings::<unnamed union>::ethtool_link_settings_hdr’ > > > invalid; an anonymous union may only have public non-static data > > > members [-fpermissive] > > > 2515 | __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > This seems to work with Clang: > > $ clang++-18 -fms-extensions /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 > > However, `-fms-extensions` doesn't seem to work for this case with GCC: > > https://godbolt.org/z/1shsPhz3s Hm, we can't break UAPI even for C++, so even if we had compiler options that would make it work, it's really unfriendly to userspace to make all the projects there suddenly start needing to use it. I think this means we just can't use tagged struct groups in UAPI. :( I have what I think is a much simpler solution. Sending it now... -Kees
On Fri, Nov 15, 2024 at 12:38:55PM -0800, Kees Cook wrote: > On Tue, Nov 12, 2024 at 07:08:32PM -0600, Gustavo A. R. Silva wrote: > > > > > > On 11/11/24 16:22, Gustavo A. R. Silva wrote: > > > > > > > > > On 09/11/24 12:02, Jakub Kicinski wrote: > > > > On Tue, 29 Oct 2024 15:55:35 -0600 Gustavo A. R. Silva wrote: > > > > > Use the `__struct_group()` helper to create a new tagged > > > > > `struct ethtool_link_settings_hdr`. This structure groups together > > > > > all the members of the flexible `struct ethtool_link_settings` > > > > > except the flexible array. As a result, the array is effectively > > > > > separated from the rest of the members without modifying the memory > > > > > layout of the flexible structure. > > > > > > > > > > This new tagged struct will be used to fix problematic declarations > > > > > of middle-flex-arrays in composite structs[1]. > > > > > > > > Possibly a very noob question, but I'm updating a C++ library with > > > > new headers and I think this makes it no longer compile. > > > > > > > > $ cat > /tmp/t.cpp<<EOF > > > > extern "C" { > > > > #include "include/uapi/linux/ethtool.h" > > > > } > > > > int func() { return 0; } > > > > EOF > > > > > > > > $ g++ /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 > > > > In file included from /usr/include/linux/posix_types.h:5, > > > > from /usr/include/linux/types.h:9, > > > > from ../linux/include/uapi/linux/ethtool.h:18, > > > > from /tmp/t.cpp:2: > > > > ../linux/include/uapi/linux/ethtool.h:2515:24: error: ‘struct > > > > ethtool_link_settings::<unnamed union>::ethtool_link_settings_hdr’ > > > > invalid; an anonymous union may only have public non-static data > > > > members [-fpermissive] > > > > 2515 | __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > This seems to work with Clang: > > > > $ clang++-18 -fms-extensions /tmp/t.cpp -I../linux -o /dev/null -c -W -Wall -O2 > > > > However, `-fms-extensions` doesn't seem to work for this case with GCC: > > > > https://godbolt.org/z/1shsPhz3s > > Hm, we can't break UAPI even for C++, so even if we had compiler options > that would make it work, it's really unfriendly to userspace to make all > the projects there suddenly start needing to use it. > > I think this means we just can't use tagged struct groups in UAPI. :( > > I have what I think is a much simpler solution. Sending it now... What was the follow up? cferris@ is reporting something similar for linux/uapi/linux/pkt_cls.h in the structure tc_u32_sel definition, found while updating the kernel headers in Android. Maybe that file/definition needs the same follow up? ~Nick
On Mon, 9 Dec 2024 12:59:40 -0800 Christopher Ferris wrote: > It looks like the way this was fixed in the ethtool.h uapi header was to > revert the usage of __struct_group. Should something similar happen for > pkt_cls.h? Or would it be easier to simply remove the usage of the TAG in > the _struct_group macro? Just to state it explicitly - are you running into a compilation issue with existing user space after updating pkt_cls.h?
On 09/12/24 15:14, Christopher Ferris wrote: > Yes, when compiling Android, we have a C++ file that includes the pkt_cls.h > directly to get access to some of the structures from that file. It > currently gets the "types cannot be declared in an anonymous union" error > due to the TAG part of the __struct_group usage not being empty in that > file. (sigh) this should be reverted: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a9c60712d71ff07197b2982899b9db28ed548ded -- Gustavo > > Christopher > > On Mon, Dec 9, 2024 at 1:10 PM Jakub Kicinski <kuba@kernel.org> wrote: > >> On Mon, 9 Dec 2024 12:59:40 -0800 Christopher Ferris wrote: >>> It looks like the way this was fixed in the ethtool.h uapi header was to >>> revert the usage of __struct_group. Should something similar happen for >>> pkt_cls.h? Or would it be easier to simply remove the usage of the TAG in >>> the _struct_group macro? >> >> Just to state it explicitly - are you running into a compilation issue >> with existing user space after updating pkt_cls.h? >> >
On Mon, Dec 09, 2024 at 04:39:38PM -0800, Christopher Ferris wrote: > By the way, there are some places where __struct_group is used in other > uapi headers, the only difference is that the TAG field of the macro is > left empty. That compiles fine when used in C++ code. Does this fix the C++ inclusion for you? diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 2c32080416b5..aeff841c528d 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -245,20 +245,27 @@ struct tc_u32_key { int offmask; }; +#define tc_u32_sel_hdr_members \ + unsigned char flags; \ + unsigned char offshift; \ + unsigned char nkeys; \ + __be16 offmask; \ + __u16 off; \ + short offoff; \ + short hoff; + +struct tc_u32_sel_hdr { + tc_u32_sel_hdr_members +}; + struct tc_u32_sel { - /* New members MUST be added within the __struct_group() macro below. */ - __struct_group(tc_u32_sel_hdr, hdr, /* no attrs */, - unsigned char flags; - unsigned char offshift; - unsigned char nkeys; - - __be16 offmask; - __u16 off; - short offoff; - - short hoff; - __be32 hmask; - ); + /* Open-coded struct_group() to avoid C++ errors. */ + union { + struct tc_u32_sel_hdr hdr; + struct { + tc_u32_sel_hdr_members + }; + }; struct tc_u32_key keys[]; };
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index c405ed63acfa..fc1f54b065f9 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -2511,21 +2511,24 @@ enum ethtool_reset_flags { * autonegotiation; 0 if unknown or not applicable. Read-only. */ struct ethtool_link_settings { - __u32 cmd; - __u32 speed; - __u8 duplex; - __u8 port; - __u8 phy_address; - __u8 autoneg; - __u8 mdio_support; - __u8 eth_tp_mdix; - __u8 eth_tp_mdix_ctrl; - __s8 link_mode_masks_nwords; - __u8 transceiver; - __u8 master_slave_cfg; - __u8 master_slave_state; - __u8 rate_matching; - __u32 reserved[7]; + /* New members MUST be added within the __struct_group() macro below. */ + __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, + __u32 cmd; + __u32 speed; + __u8 duplex; + __u8 port; + __u8 phy_address; + __u8 autoneg; + __u8 mdio_support; + __u8 eth_tp_mdix; + __u8 eth_tp_mdix_ctrl; + __s8 link_mode_masks_nwords; + __u8 transceiver; + __u8 master_slave_cfg; + __u8 master_slave_state; + __u8 rate_matching; + __u32 reserved[7]; + ); __u32 link_mode_masks[]; /* layout of link_mode_masks fields: * __u32 map_supported[link_mode_masks_nwords];
Use the `__struct_group()` helper to create a new tagged `struct ethtool_link_settings_hdr`. This structure groups together all the members of the flexible `struct ethtool_link_settings` except the flexible array. As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. This new tagged struct will be used to fix problematic declarations of middle-flex-arrays in composite structs[1]. [1] https://git.kernel.org/linus/d88cabfd9abc Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - None. v1: - Link: https://lore.kernel.org/linux-hardening/e9ccb0cd7e490bfa270a7c20979e16ff84ac91e2.1729536776.git.gustavoars@kernel.org/ include/uapi/linux/ethtool.h | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)