netdev - Re: [PATCH ethtool] move variable-sized members to the end of structs

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: <20240216145752.aihdclrz6o53tgl2@lion.mk-sys.cz>
Date: Fri, 16 Feb 2024 15:57:52 +0100
From: Michal Kubecek <mkubecek@...e.cz>
To: Denis Kirjanov <kirjanov@...il.com>
Cc: netdev@...r.kernel.org, Denis Kirjanov <dkirjanov@...e.de>
Subject: Re: [PATCH ethtool] move variable-sized members to the end of structs

On Fri, Feb 16, 2024 at 09:08:53AM -0500, Denis Kirjanov wrote:
> The patch fixes the following clang warnings:
> 
> warning: field 'xxx' with variable sized type 'xxx' not at the end of a struct
>  or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> 
> Signed-off-by: Denis Kirjanov <dkirjanov@...e.de>

Have you checked if this does not break the ioctl interface? Many of
these structures (maybe even all of them) are directly passed to kernel
via ioctl so that rearranging them would break the data block format
expected by kernel.

AFAICS at least in the cases that I checked, the outer struct member is
exactly the data expected as variable part of the inner struct.

Michal


> ---
>  ethtool.c  | 18 +++++++++---------
>  internal.h |  2 +-
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 3ac15a7..32e79ae 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -1736,8 +1736,8 @@ get_stringset(struct cmd_context *ctx, enum ethtool_stringset set_id,
>  	      ptrdiff_t drvinfo_offset, int null_terminate)
>  {
>  	struct {
> -		struct ethtool_sset_info hdr;
>  		u32 buf[1];
> +		struct ethtool_sset_info hdr;
>  	} sset_info;
>  	struct ethtool_drvinfo drvinfo;
>  	u32 len, i;
> @@ -2683,8 +2683,8 @@ do_ioctl_glinksettings(struct cmd_context *ctx)
>  {
>  	int err;
>  	struct {
> -		struct ethtool_link_settings req;
>  		__u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
> +		struct ethtool_link_settings req;
>  	} ecmd;
>  	struct ethtool_link_usettings *link_usettings;
>  	unsigned int u32_offs;
> @@ -2752,8 +2752,8 @@ do_ioctl_slinksettings(struct cmd_context *ctx,
>  		       const struct ethtool_link_usettings *link_usettings)
>  {
>  	struct {
> -		struct ethtool_link_settings req;
>  		__u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
> +		struct ethtool_link_settings req;
>  	} ecmd;
>  	unsigned int u32_offs;
>  
> @@ -5206,8 +5206,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
>  
>  	if (!strcmp(argp[0], "downshift")) {
>  		struct {
> -			struct ethtool_tunable ds;
>  			u8 count;
> +			struct ethtool_tunable ds;
>  		} cont;
>  
>  		cont.ds.cmd = ETHTOOL_PHY_GTUNABLE;
> @@ -5224,8 +5224,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
>  			fprintf(stdout, "Downshift disabled\n");
>  	} else if (!strcmp(argp[0], "fast-link-down")) {
>  		struct {
> -			struct ethtool_tunable fld;
>  			u8 msecs;
> +			struct ethtool_tunable fld;
>  		} cont;
>  
>  		cont.fld.cmd = ETHTOOL_PHY_GTUNABLE;
> @@ -5246,8 +5246,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
>  				cont.msecs);
>  	} else if (!strcmp(argp[0], "energy-detect-power-down")) {
>  		struct {
> -			struct ethtool_tunable ds;
>  			u16 msecs;
> +			struct ethtool_tunable ds;
>  		} cont;
>  
>  		cont.ds.cmd = ETHTOOL_PHY_GTUNABLE;
> @@ -5494,8 +5494,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
>  	/* Do it */
>  	if (ds_changed) {
>  		struct {
> -			struct ethtool_tunable ds;
>  			u8 count;
> +			struct ethtool_tunable ds;
>  		} cont;
>  
>  		cont.ds.cmd = ETHTOOL_PHY_STUNABLE;
> @@ -5510,8 +5510,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
>  		}
>  	} else if (fld_changed) {
>  		struct {
> -			struct ethtool_tunable fld;
>  			u8 msecs;
> +			struct ethtool_tunable fld;
>  		} cont;
>  
>  		cont.fld.cmd = ETHTOOL_PHY_STUNABLE;
> @@ -5526,8 +5526,8 @@ static int do_set_phy_tunable(struct cmd_context *ctx)
>  		}
>  	} else if (edpd_changed) {
>  		struct {
> -			struct ethtool_tunable fld;
>  			u16 msecs;
> +			struct ethtool_tunable fld;
>  		} cont;
>  
>  		cont.fld.cmd = ETHTOOL_PHY_STUNABLE;
> diff --git a/internal.h b/internal.h
> index 4b994f5..e0beec6 100644
> --- a/internal.h
> +++ b/internal.h
> @@ -152,12 +152,12 @@ struct ethtool_link_usettings {
>  	struct {
>  		__u8 transceiver;
>  	} deprecated;
> -	struct ethtool_link_settings base;
>  	struct {
>  		ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
>  		ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
>  		ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
>  	} link_modes;
> +	struct ethtool_link_settings base;
>  };
>  
>  #define ethtool_link_mode_for_each_u32(index)			\
> -- 
> 2.30.2
> 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)