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

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

[Gustavo A. R. Silva]

Small patch series aimed at fixing thousands of -Wflex-array-member-not-at-end
warnings by creating a new tagged struct within a flexible structure. We then
use this new struct type to fix problematic middle-flex-array declarations in
multiple composite structs, as well as to update the type of some variables in
various functions.

Changes in v2:
 - Update changelog text in patch 2/2 to better reflect the changes
   made. (Jakub)
 - Adjust variable declarations to follow the reverse xmas tree
   convention. (Jakub)

v1:
 - Link: https://lore.kernel.org/linux-hardening/cover.1729536776.git.gustavoars@kernel.org

Gustavo A. R. Silva (2):
  UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings
  net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end
    warnings

 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  6 ++--
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    |  4 +--
 .../ethernet/chelsio/cxgb4vf/cxgb4vf_main.c   |  2 +-
 .../net/ethernet/cisco/enic/enic_ethtool.c    |  2 +-
 .../net/ethernet/qlogic/qede/qede_ethtool.c   |  4 +--
 include/linux/ethtool.h                       |  2 +-
 include/uapi/linux/ethtool.h                  | 33 ++++++++++---------
 net/ethtool/ioctl.c                           |  2 +-
 net/ethtool/linkinfo.c                        |  8 ++---
 net/ethtool/linkmodes.c                       | 18 ++++++----
 10 files changed, 44 insertions(+), 37 deletions(-)

-- 
2.43.0

[PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Gustavo A. R. Silva]

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

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];
-- 
2.43.0

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

[Gustavo A. R. Silva]

-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally.

Change the type of the middle struct member currently causing trouble from
`struct ethtool_link_settings` to `struct ethtool_link_settings_hdr`.

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`. These changes are needed because the
type of the conflicting middle members changed. So, those instances that
expect the type to be `struct ethtool_link_settings` should be adjusted to
the newly created type `struct ethtool_link_settings_hdr`.

Also, adjust variable declarations to follow the reverse xmas tree
convention.

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]

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Update changelog text in patch to better reflect the changes
   made. (Jakub)
 - Adjust variable declarations to follow the reverse xmas tree
   convention. (Jakub) 

v1:
 - Link: https://lore.kernel.org/linux-hardening/f4f8ca5cd7f039bcab816194342c7b6101e891fe.1729536776.git.gustavoars@kernel.org/

 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c  |  6 +++---
 .../net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c |  4 ++--
 .../ethernet/chelsio/cxgb4vf/cxgb4vf_main.c    |  2 +-
 drivers/net/ethernet/cisco/enic/enic_ethtool.c |  2 +-
 .../net/ethernet/qlogic/qede/qede_ethtool.c    |  4 ++--
 include/linux/ethtool.h                        |  2 +-
 net/ethtool/ioctl.c                            |  2 +-
 net/ethtool/linkinfo.c                         |  8 ++++----
 net/ethtool/linkmodes.c                        | 18 +++++++++++-------
 9 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index f71cc8188b4e..e0ebe69110bf 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2781,7 +2781,7 @@ u32 bnxt_fw_to_ethtool_speed(u16 fw_link_speed)
 static void bnxt_get_default_speeds(struct ethtool_link_ksettings *lk_ksettings,
 				    struct bnxt_link_info *link_info)
 {
-	struct ethtool_link_settings *base = &lk_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
 
 	if (link_info->link_state == BNXT_LINK_STATE_UP) {
 		base->speed = bnxt_fw_to_ethtool_speed(link_info->link_speed);
@@ -2800,7 +2800,7 @@ static void bnxt_get_default_speeds(struct ethtool_link_ksettings *lk_ksettings,
 static int bnxt_get_link_ksettings(struct net_device *dev,
 				   struct ethtool_link_ksettings *lk_ksettings)
 {
-	struct ethtool_link_settings *base = &lk_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
 	enum ethtool_link_mode_bit_indices link_mode;
 	struct bnxt *bp = netdev_priv(dev);
 	struct bnxt_link_info *link_info;
@@ -3023,9 +3023,9 @@ u16 bnxt_get_fw_auto_link_speeds(const unsigned long *mode)
 static int bnxt_set_link_ksettings(struct net_device *dev,
 			   const struct ethtool_link_ksettings *lk_ksettings)
 {
+	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
 	struct bnxt *bp = netdev_priv(dev);
 	struct bnxt_link_info *link_info = &bp->link_info;
-	const struct ethtool_link_settings *base = &lk_ksettings->base;
 	bool set_pause = false;
 	u32 speed, lanes = 0;
 	int rc = 0;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 7f3f5afa864f..45d28a65347e 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -662,8 +662,8 @@ static unsigned int lmm_to_fw_caps(const unsigned long *link_mode_mask)
 static int get_link_ksettings(struct net_device *dev,
 			      struct ethtool_link_ksettings *link_ksettings)
 {
+	struct ethtool_link_settings_hdr *base = &link_ksettings->base;
 	struct port_info *pi = netdev_priv(dev);
-	struct ethtool_link_settings *base = &link_ksettings->base;
 
 	/* For the nonce, the Firmware doesn't send up Port State changes
 	 * when the Virtual Interface attached to the Port is down.  So
@@ -717,9 +717,9 @@ static int get_link_ksettings(struct net_device *dev,
 static int set_link_ksettings(struct net_device *dev,
 			    const struct ethtool_link_ksettings *link_ksettings)
 {
+	const struct ethtool_link_settings_hdr *base = &link_ksettings->base;
 	struct port_info *pi = netdev_priv(dev);
 	struct link_config *lc = &pi->link_cfg;
-	const struct ethtool_link_settings *base = &link_ksettings->base;
 	struct link_config old_lc;
 	unsigned int fw_caps;
 	int ret = 0;
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index 2fbe0f059a0b..61d08547e3f9 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -1436,8 +1436,8 @@ static void fw_caps_to_lmm(enum fw_port_type port_type,
 static int cxgb4vf_get_link_ksettings(struct net_device *dev,
 				  struct ethtool_link_ksettings *link_ksettings)
 {
+	struct ethtool_link_settings_hdr *base = &link_ksettings->base;
 	struct port_info *pi = netdev_priv(dev);
-	struct ethtool_link_settings *base = &link_ksettings->base;
 
 	/* For the nonce, the Firmware doesn't send up Port State changes
 	 * when the Virtual Interface attached to the Port is down.  So
diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index f7986f2b6a17..4fe85780a950 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -129,8 +129,8 @@ static void enic_intr_coal_set_rx(struct enic *enic, u32 timer)
 static int enic_get_ksettings(struct net_device *netdev,
 			      struct ethtool_link_ksettings *ecmd)
 {
+	struct ethtool_link_settings_hdr *base = &ecmd->base;
 	struct enic *enic = netdev_priv(netdev);
-	struct ethtool_link_settings *base = &ecmd->base;
 
 	ethtool_link_ksettings_add_link_mode(ecmd, supported,
 					     10000baseT_Full);
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 97b059be1041..24ff154285ac 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -508,7 +508,7 @@ static int qede_get_link_ksettings(struct net_device *dev,
 				   struct ethtool_link_ksettings *cmd)
 {
 	typeof(cmd->link_modes) *link_modes = &cmd->link_modes;
-	struct ethtool_link_settings *base = &cmd->base;
+	struct ethtool_link_settings_hdr *base = &cmd->base;
 	struct qede_dev *edev = netdev_priv(dev);
 	struct qed_link_output current_link;
 
@@ -541,7 +541,7 @@ static int qede_get_link_ksettings(struct net_device *dev,
 static int qede_set_link_ksettings(struct net_device *dev,
 				   const struct ethtool_link_ksettings *cmd)
 {
-	const struct ethtool_link_settings *base = &cmd->base;
+	const struct ethtool_link_settings_hdr *base = &cmd->base;
 	const struct ethtool_forced_speed_map *map;
 	struct qede_dev *edev = netdev_priv(dev);
 	struct qed_link_output current_link;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12f6dc567598..1199e308c8dd 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -211,7 +211,7 @@ void ethtool_rxfh_context_lost(struct net_device *dev, u32 context_id);
  * fields, but they are allowed to overwrite them (will be ignored).
  */
 struct ethtool_link_ksettings {
-	struct ethtool_link_settings base;
+	struct ethtool_link_settings_hdr base;
 	struct {
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 5cc131cdb1bc..7da94e26ced6 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -425,7 +425,7 @@ convert_link_ksettings_to_legacy_settings(
 
 /* layout of the struct passed from/to userland */
 struct ethtool_link_usettings {
-	struct ethtool_link_settings base;
+	struct ethtool_link_settings_hdr base;
 	struct {
 		__u32 supported[__ETHTOOL_LINK_MODE_MASK_NU32];
 		__u32 advertising[__ETHTOOL_LINK_MODE_MASK_NU32];
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index 30b8ce275159..2d5bc57160be 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -8,9 +8,9 @@ struct linkinfo_req_info {
 };
 
 struct linkinfo_reply_data {
-	struct ethnl_reply_data		base;
-	struct ethtool_link_ksettings	ksettings;
-	struct ethtool_link_settings	*lsettings;
+	struct ethnl_reply_data			base;
+	struct ethtool_link_ksettings		ksettings;
+	struct ethtool_link_settings_hdr	*lsettings;
 };
 
 #define LINKINFO_REPDATA(__reply_base) \
@@ -98,7 +98,7 @@ static int
 ethnl_set_linkinfo(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct ethtool_link_ksettings ksettings = {};
-	struct ethtool_link_settings *lsettings;
+	struct ethtool_link_settings_hdr *lsettings;
 	struct net_device *dev = req_info->dev;
 	struct nlattr **tb = info->attrs;
 	bool mod = false;
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 259cd9ef1f2a..17e49cf89f03 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -11,10 +11,10 @@ struct linkmodes_req_info {
 };
 
 struct linkmodes_reply_data {
-	struct ethnl_reply_data		base;
-	struct ethtool_link_ksettings	ksettings;
-	struct ethtool_link_settings	*lsettings;
-	bool				peer_empty;
+	struct ethnl_reply_data			base;
+	struct ethtool_link_ksettings		ksettings;
+	struct ethtool_link_settings_hdr	*lsettings;
+	bool					peer_empty;
 };
 
 #define LINKMODES_REPDATA(__reply_base) \
@@ -62,10 +62,12 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 {
 	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
 	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
-	const struct ethtool_link_settings *lsettings = &ksettings->base;
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
+	const struct ethtool_link_settings_hdr *lsettings;
 	int len, ret;
 
+	lsettings = &ksettings->base;
+
 	len = nla_total_size(sizeof(u8)) /* LINKMODES_AUTONEG */
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_LANES */
@@ -103,10 +105,12 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
 {
 	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
 	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
-	const struct ethtool_link_settings *lsettings = &ksettings->base;
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
+	const struct ethtool_link_settings_hdr *lsettings;
 	int ret;
 
+	lsettings = &ksettings->base;
+
 	if (nla_put_u8(skb, ETHTOOL_A_LINKMODES_AUTONEG, lsettings->autoneg))
 		return -EMSGSIZE;
 
@@ -237,7 +241,7 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 				  struct ethtool_link_ksettings *ksettings,
 				  bool *mod, const struct net_device *dev)
 {
-	struct ethtool_link_settings *lsettings = &ksettings->base;
+	struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
 	bool req_speed, req_lanes, req_duplex;
 	const struct nlattr *master_slave_cfg, *lanes_cfg;
 	int ret;
-- 
2.43.0

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 29 Oct 2024 15:55:02 -0600 you wrote:
> Small patch series aimed at fixing thousands of -Wflex-array-member-not-at-end
> warnings by creating a new tagged struct within a flexible structure. We then
> use this new struct type to fix problematic middle-flex-array declarations in
> multiple composite structs, as well as to update the type of some variables in
> various functions.
> 
> Changes in v2:
>  - Update changelog text in patch 2/2 to better reflect the changes
>    made. (Jakub)
>  - Adjust variable declarations to follow the reverse xmas tree
>    convention. (Jakub)
> 
> [...]

Here is the summary with links:
  - [v2,1/2,next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings
    https://git.kernel.org/netdev/net-next/c/43d3487035e9
  - [v2,2/2,next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings
    https://git.kernel.org/netdev/net-next/c/3bd9b9abdf15

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Jakub Kicinski]

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.

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Jakub Kicinski]

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) 

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Gustavo A. R. Silva]

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

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Gustavo A. R. Silva]

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
> 

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Nick Desaulniers]

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

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Jakub Kicinski]

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?

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Gustavo A. R. Silva]

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

Re: [PATCH v2 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Kees Cook]

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[];
 };
 


-- 
Kees Cook