UAPI: ethtool: Avoid flex-array in struct ethtool_link_settings
Subsystems (8)
cover UAPI: ethtool: Avoid flex-array in struct ethtool_link_settings
Hi,
This reverts the tagged struct group in struct ethtool_link_settings and instead just removes the flexible array member from Linux's view as it is entirely unused.
-Kees
Kees Cook (3):
Revert "net: ethtool: Avoid thousands of
-Wflex-array-member-not-at-end warnings"
Revert "UAPI: ethtool: Use __struct_group() in struct
ethtool_link_settings"
UAPI: ethtool: Avoid flex-array in struct ethtool_link_settings
.../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 | 40 ++++++++++--------- net/ethtool/ioctl.c | 2 +- net/ethtool/linkinfo.c | 8 ++-- net/ethtool/linkmodes.c | 18 ++++----- 10 files changed, 44 insertions(+), 44 deletions(-)
Signature · 1 line
2.34.1
PATCH 1/3 Revert "net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings"
This reverts commit 3bd9b9abdf1563a22041b7255baea6d449902f1a. We cannot use the new tagged struct group because it throws C++ errors even under "extern C".
---
Cc: "Gustavo A. R. Silva" <[email protected]>
--- .../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, 22 insertions(+), 26 deletions(-)
drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +3 -3
@@ -2780,7 +2780,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_hdr *base = &lk_ksettings->base; +struct ethtool_link_settings *base = &lk_ksettings->base;if (link_info->link_state ==BNXT_LINK_STATE_UP ) { base->speed =bnxt_fw_to_ethtool_speed (link_info->link_speed);
@@ -2799,7 +2799,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_hdr *base = &lk_ksettings->base; +struct ethtool_link_settings *base = &lk_ksettings->base;enum ethtool_link_mode_bit_indices link_mode;struct bnxt *bp =netdev_priv (dev);struct bnxt_link_info *link_info;
@@ -3022,9 +3022,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 ;
drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c +2 -2
@@ -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 ;
drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c +1 -1
@@ -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
drivers/net/ethernet/cisco/enic/enic_ethtool.c +1 -1
@@ -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);
drivers/net/ethernet/qlogic/qede/qede_ethtool.c +2 -2
@@ -504,7 +504,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_hdr *base = &cmd->base; +struct ethtool_link_settings *base = &cmd->base;struct qede_dev *edev =netdev_priv (dev);struct qed_link_output current_link;
@@ -537,7 +537,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_hdr *base = &cmd->base; +const struct ethtool_link_settings *base = &cmd->base;const struct ethtool_forced_speed_map *map;struct qede_dev *edev =netdev_priv (dev);struct qed_link_output current_link;
include/linux/ethtool.h +1 -1
@@ -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_hdr base; +struct ethtool_link_settings base;struct {__ETHTOOL_DECLARE_LINK_MODE_MASK (supported);__ETHTOOL_DECLARE_LINK_MODE_MASK (advertising);
net/ethtool/ioctl.c +1 -1
@@ -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_hdr base; +struct ethtool_link_settings base;struct {__u32 supported[__ETHTOOL_LINK_MODE_MASK_NU32];__u32 advertising[__ETHTOOL_LINK_MODE_MASK_NU32];
net/ethtool/linkinfo.c +4 -4
@@ -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_hdr *lsettings; +struct ethnl_reply_data base; +struct ethtool_link_ksettings ksettings; +struct ethtool_link_settings *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_hdr *lsettings; +struct ethtool_link_settings *lsettings;struct net_device *dev = req_info->dev;struct nlattr **tb = info->attrs;bool mod = false;
net/ethtool/linkmodes.c +7 -11
@@ -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_hdr *lsettings; -bool peer_empty; +struct ethnl_reply_data base; +struct ethtool_link_ksettings ksettings; +struct ethtool_link_settings *lsettings; +bool peer_empty; };#define LINKMODES_REPDATA (__reply_base) \
@@ -62,12 +62,10 @@ 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 ÐTOOL_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 */
@@ -105,12 +103,10 @@ 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 ÐTOOL_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 ;
@@ -241,7 +237,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_hdr *lsettings = &ksettings->base; + struct ethtool_link_settings *lsettings = &ksettings->base;bool req_speed, req_lanes, req_duplex;const struct nlattr *master_slave_cfg, *lanes_cfg;int ret;
Signature · 1 line
2.34.1
PATCH 2/3 Revert "UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings"
In reply to Kees CookThis reverts commit 43d3487035e9a86fad952de4240a518614240d43. We cannot use tagged struct groups in UAPI because C++ will throw syntax errors even under "extern C".
---
Cc: "Gustavo A. R. Silva" <[email protected]>
--- include/uapi/linux/ethtool.h | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-)
include/uapi/linux/ethtool.h +15 -18
@@ -2511,24 +2511,21 @@ enum ethtool_reset_flags {* autonegotiation; 0 if unknown or not applicable. Read-only. */ struct ethtool_link_settings { -/* 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 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];
Signature · 1 line
2.34.1
PATCH 3/3 UAPI: ethtool: Avoid flex-array in struct ethtool_link_settings
In reply to Kees Cookstruct ethtool_link_settings tends to be used as a header for other structures that have trailing bytes[1], but has a trailing flexible array itself. Using this overlapped with other structures leads to ambiguous object sizing in the compiler, so we want to avoid such situations (which have caused real bugs in the past). Detecting this can be done with -Wflex-array-member-not-at-end, which will need to be enabled globally.
Using a tagged struct_group() to create a new ethtool_link_settings_hdr structure isn't possible as it seems we cannot use the tagged variant of struct_group() due to syntax issues from C++'s perspective (even within "extern C")[2]. Instead, we can just leave the offending member defined in UAPI and remove it from the kernel's view of the structure, as Linux doesn't actually use this member at all. There is also no change in size since it was already a flexible array that didn't contribute to size returned by any use of sizeof().
---
Cc: Jakub Kicinski <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: "Kory Maincent (Dent Project)" <[email protected]>
--- include/uapi/linux/ethtool.h | 7 +++++++ 1 file changed, 7 insertions(+)
include/uapi/linux/ethtool.h +7 -0
@@ -2526,12 +2526,19 @@ struct ethtool_link_settings {__u8 master_slave_state;__u8 rate_matching;__u32 reserved[7 ]; +#ifndef __KERNEL__ +/* Linux builds with -Wflex-array-member-not-at-end but does +* not use the "link_mode_masks" member. Leave it defined for +* userspace for now, and when userspace wants to start using +* -Wfamnae, we'll need a new solution. +*/ __u32 link_mode_masks[];/* layout of link_mode_masks fields: * __u32 map_supported[link_mode_masks_nwords]; * __u32 map_advertising[link_mode_masks_nwords]; * __u32 map_lp_advertising[link_mode_masks_nwords]; */ +#endif };/**
Signature · 1 line
2.34.1
Quoted 7 lines · On Fri, 15 Nov 2024 12:43:05 -0800 Kees Cook wrote:
struct
ethtool_link_settingstends to be used as a header for other structures that have trailing bytes[1], but has a trailing flexible array itself. Using this overlapped with other structures leads to ambiguous object sizing in the compiler, so we want to avoid such situations (which have caused real bugs in the past). Detecting this can be done with -Wflex-array-member-not-at-end, which will need to be enabled globally.
Quoted 8 lines
Using a tagged
struct_group()to create a newethtool_link_settings_hdrstructure isn't possible as it seems we cannot use the tagged variant ofstruct_group()due to syntax issues from C++'s perspective (even within "extern C")[2]. Instead, we can just leave the offending member defined in UAPI and remove it from the kernel's view of the structure, as Linux doesn't actually use this member at all. There is also no change in size since it was already a flexible array that didn't contribute to size returned by any use ofsizeof().
Perfect. I was starting to doubt if user space needs the member, ethtool CLI doesn't but looks like NetworkManager does. So as you say we'll cross that bridge...
Thanks!
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Quoted 9 lines · On Fri, 15 Nov 2024 12:43:02 -0800 you wrote:
Hi,
This reverts the tagged struct group in struct
ethtool_link_settingsand instead just removes the flexible array member from Linux's view as it is entirely unused.-Kees
[...]
Here is the summary with links:
- [1/3] Revert "net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings"
https://git.kernel.org/netdev/net-next/c/1cfb5e57886a - [2/3] Revert "UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings" https://git.kernel.org/netdev/net-next/c/ebda123fe703 - [3/3] UAPI: ethtool: Avoid flex-array in struct ethtool_link_settings https://git.kernel.org/netdev/net-next/c/96c677fca54a
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html