netdev - Re: [PATCH net-next] ethtool: Block setting of symmetric RSS when non-symmetric rx-flow-hash is requested

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: <f7a63428-5b2e-47fe-a108-cdf93f732ea2@redhat.com>
Date: Mon, 17 Mar 2025 21:19:43 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Gal Pressman <gal@...dia.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Andrew Lunn <andrew+netdev@...n.ch>, netdev@...r.kernel.org
Cc: Simon Horman <horms@...nel.org>, Andrew Lunn <andrew@...n.ch>,
 Tariq Toukan <tariqt@...dia.com>
Subject: Re: [PATCH net-next] ethtool: Block setting of symmetric RSS when
 non-symmetric rx-flow-hash is requested

On 3/10/25 8:23 AM, Gal Pressman wrote:
> Symmetric RSS hash requires that:
> * No other fields besides IP src/dst and/or L4 src/dst
> * If src is set, dst must also be set
> 
> This restriction was only enforced when RXNFC was configured after
> symmetric hash was enabled. In the opposite order of operations (RXNFC
> then symmetric enablement) the check was not performed.
> 
> Perform the sanity check on set_rxfh as well, by iterating over all flow
> types hash fields and making sure they are all symmetric.
> 
> Introduce a function that returns whether a flow type is hashable (not
> spec only) and needs to be iterated over. To make sure that no one
> forgets to update the list of hashable flow types when adding new flow
> types, a static assert is added to draw the developer's attention.
> 
> Reviewed-by: Tariq Toukan <tariqt@...dia.com>
> Signed-off-by: Gal Pressman <gal@...dia.com>
> ---
>  include/uapi/linux/ethtool.h | 124 ++++++++++++++++++-----------------
>  net/ethtool/ioctl.c          |  99 +++++++++++++++++++++++++---
>  2 files changed, 153 insertions(+), 70 deletions(-)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 84833cca29fe..d36f8f4e3eef 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -2295,71 +2295,75 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>  #define	RXH_XFRM_SYM_OR_XOR	(1 << 1)
>  #define	RXH_XFRM_NO_CHANGE	0xff
>  
> -/* L2-L4 network traffic flow types */
> -#define	TCP_V4_FLOW	0x01	/* hash or spec (tcp_ip4_spec) */
> -#define	UDP_V4_FLOW	0x02	/* hash or spec (udp_ip4_spec) */
> -#define	SCTP_V4_FLOW	0x03	/* hash or spec (sctp_ip4_spec) */
> -#define	AH_ESP_V4_FLOW	0x04	/* hash only */
> -#define	TCP_V6_FLOW	0x05	/* hash or spec (tcp_ip6_spec; nfc only) */
> -#define	UDP_V6_FLOW	0x06	/* hash or spec (udp_ip6_spec; nfc only) */
> -#define	SCTP_V6_FLOW	0x07	/* hash or spec (sctp_ip6_spec; nfc only) */
> -#define	AH_ESP_V6_FLOW	0x08	/* hash only */
> -#define	AH_V4_FLOW	0x09	/* hash or spec (ah_ip4_spec) */
> -#define	ESP_V4_FLOW	0x0a	/* hash or spec (esp_ip4_spec) */
> -#define	AH_V6_FLOW	0x0b	/* hash or spec (ah_ip6_spec; nfc only) */
> -#define	ESP_V6_FLOW	0x0c	/* hash or spec (esp_ip6_spec; nfc only) */
> -#define	IPV4_USER_FLOW	0x0d	/* spec only (usr_ip4_spec) */
> -#define	IP_USER_FLOW	IPV4_USER_FLOW
> -#define	IPV6_USER_FLOW	0x0e	/* spec only (usr_ip6_spec; nfc only) */
> -#define	IPV4_FLOW	0x10	/* hash only */
> -#define	IPV6_FLOW	0x11	/* hash only */
> -#define	ETHER_FLOW	0x12	/* spec only (ether_spec) */
> +enum {
> +	/* L2-L4 network traffic flow types */
> +	TCP_V4_FLOW = 0x01, /* hash or spec (tcp_ip4_spec) */
> +	UDP_V4_FLOW = 0x02, /* hash or spec (udp_ip4_spec) */
> +	SCTP_V4_FLOW = 0x03, /* hash or spec (sctp_ip4_spec) */
> +	AH_ESP_V4_FLOW = 0x04, /* hash only */
> +	TCP_V6_FLOW = 0x05, /* hash or spec (tcp_ip6_spec; nfc only) */
> +	UDP_V6_FLOW = 0x06, /* hash or spec (udp_ip6_spec; nfc only) */
> +	SCTP_V6_FLOW = 0x07, /* hash or spec (sctp_ip6_spec; nfc only) */
> +	AH_ESP_V6_FLOW = 0x08, /* hash only */
> +	AH_V4_FLOW = 0x09, /* hash or spec (ah_ip4_spec) */
> +	ESP_V4_FLOW = 0x0a, /* hash or spec (esp_ip4_spec) */
> +	AH_V6_FLOW = 0x0b, /* hash or spec (ah_ip6_spec; nfc only) */
> +	ESP_V6_FLOW = 0x0c, /* hash or spec (esp_ip6_spec; nfc only) */
> +	IPV4_USER_FLOW = 0x0d, /* spec only (usr_ip4_spec) */
> +	IP_USER_FLOW = IPV4_USER_FLOW,
> +	IPV6_USER_FLOW = 0x0e, /* spec only (usr_ip6_spec; nfc only) */
> +	IPV4_FLOW = 0x10, /* hash only */
> +	IPV6_FLOW = 0x11, /* hash only */
> +	ETHER_FLOW = 0x12, /* spec only (ether_spec) */
>  
> -/* Used for GTP-U IPv4 and IPv6.
> - * The format of GTP packets only includes
> - * elements such as TEID and GTP version.
> - * It is primarily intended for data communication of the UE.
> - */
> -#define GTPU_V4_FLOW 0x13	/* hash only */
> -#define GTPU_V6_FLOW 0x14	/* hash only */
> +	/* Used for GTP-U IPv4 and IPv6.
> +	 * The format of GTP packets only includes
> +	 * elements such as TEID and GTP version.
> +	 * It is primarily intended for data communication of the UE.
> +	 */
> +	GTPU_V4_FLOW = 0x13, /* hash only */
> +	GTPU_V6_FLOW = 0x14, /* hash only */
>  
> -/* Use for GTP-C IPv4 and v6.
> - * The format of these GTP packets does not include TEID.
> - * Primarily expected to be used for communication
> - * to create sessions for UE data communication,
> - * commonly referred to as CSR (Create Session Request).
> - */
> -#define GTPC_V4_FLOW 0x15	/* hash only */
> -#define GTPC_V6_FLOW 0x16	/* hash only */
> +	/* Use for GTP-C IPv4 and v6.
> +	 * The format of these GTP packets does not include TEID.
> +	 * Primarily expected to be used for communication
> +	 * to create sessions for UE data communication,
> +	 * commonly referred to as CSR (Create Session Request).
> +	 */
> +	GTPC_V4_FLOW = 0x15, /* hash only */
> +	GTPC_V6_FLOW = 0x16, /* hash only */
>  
> -/* Use for GTP-C IPv4 and v6.
> - * Unlike GTPC_V4_FLOW, the format of these GTP packets includes TEID.
> - * After session creation, it becomes this packet.
> - * This is mainly used for requests to realize UE handover.
> - */
> -#define GTPC_TEID_V4_FLOW 0x17	/* hash only */
> -#define GTPC_TEID_V6_FLOW 0x18	/* hash only */
> +	/* Use for GTP-C IPv4 and v6.
> +	 * Unlike GTPC_V4_FLOW, the format of these GTP packets includes TEID.
> +	 * After session creation, it becomes this packet.
> +	 * This is mainly used for requests to realize UE handover.
> +	 */
> +	GTPC_TEID_V4_FLOW = 0x17, /* hash only */
> +	GTPC_TEID_V6_FLOW = 0x18, /* hash only */
>  
> -/* Use for GTP-U and extended headers for the PSC (PDU Session Container).
> - * The format of these GTP packets includes TEID and QFI.
> - * In 5G communication using UPF (User Plane Function),
> - * data communication with this extended header is performed.
> - */
> -#define GTPU_EH_V4_FLOW 0x19	/* hash only */
> -#define GTPU_EH_V6_FLOW 0x1a	/* hash only */
> +	/* Use for GTP-U and extended headers for the PSC (PDU Session Container).
> +	 * The format of these GTP packets includes TEID and QFI.
> +	 * In 5G communication using UPF (User Plane Function),
> +	 * data communication with this extended header is performed.
> +	 */
> +	GTPU_EH_V4_FLOW = 0x19, /* hash only */
> +	GTPU_EH_V6_FLOW = 0x1a, /* hash only */
>  
> -/* Use for GTP-U IPv4 and v6 PSC (PDU Session Container) extended headers.
> - * This differs from GTPU_EH_V(4|6)_FLOW in that it is distinguished by
> - * UL/DL included in the PSC.
> - * There are differences in the data included based on Downlink/Uplink,
> - * and can be used to distinguish packets.
> - * The functions described so far are useful when you want to
> - * handle communication from the mobile network in UPF, PGW, etc.
> - */
> -#define GTPU_UL_V4_FLOW 0x1b	/* hash only */
> -#define GTPU_UL_V6_FLOW 0x1c	/* hash only */
> -#define GTPU_DL_V4_FLOW 0x1d	/* hash only */
> -#define GTPU_DL_V6_FLOW 0x1e	/* hash only */
> +	/* Use for GTP-U IPv4 and v6 PSC (PDU Session Container) extended headers.
> +	 * This differs from GTPU_EH_V(4|6)_FLOW in that it is distinguished by
> +	 * UL/DL included in the PSC.
> +	 * There are differences in the data included based on Downlink/Uplink,
> +	 * and can be used to distinguish packets.
> +	 * The functions described so far are useful when you want to
> +	 * handle communication from the mobile network in UPF, PGW, etc.
> +	 */
> +	GTPU_UL_V4_FLOW = 0x1b, /* hash only */
> +	GTPU_UL_V6_FLOW = 0x1c, /* hash only */
> +	GTPU_DL_V4_FLOW = 0x1d, /* hash only */
> +	GTPU_DL_V6_FLOW = 0x1e, /* hash only */
> +
> +	FLOW_TYPE_MAX,
> +};

I fear we can't replace macros with enum in uAPI: existing application
could do weird thing leveraging the macro defintion and would break with
this change.

I think you could simply add:

#define FLOW_TYPE_MAX 0x1f

Thank,

Paolo