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

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

[Gal Pressman]

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@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.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,
+};
 
 /* Flag to enable additional fields in struct ethtool_rx_flow_spec */
 #define	FLOW_EXT	0x80000000
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 77d714874eca..aa1473bbeaa8 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -977,6 +977,88 @@ static int ethtool_rxnfc_copy_to_user(void __user *useraddr,
 	return 0;
 }
 
+static bool flow_type_hashable(u32 flow_type)
+{
+	switch (flow_type) {
+	case TCP_V4_FLOW:
+	case UDP_V4_FLOW:
+	case SCTP_V4_FLOW:
+	case AH_ESP_V4_FLOW:
+	case TCP_V6_FLOW:
+	case UDP_V6_FLOW:
+	case SCTP_V6_FLOW:
+	case AH_ESP_V6_FLOW:
+	case AH_V4_FLOW:
+	case ESP_V4_FLOW:
+	case AH_V6_FLOW:
+	case ESP_V6_FLOW:
+	case IPV4_FLOW:
+	case IPV6_FLOW:
+	case GTPU_V4_FLOW:
+	case GTPU_V6_FLOW:
+	case GTPC_V4_FLOW:
+	case GTPC_V6_FLOW:
+	case GTPC_TEID_V4_FLOW:
+	case GTPC_TEID_V6_FLOW:
+	case GTPU_EH_V4_FLOW:
+	case GTPU_EH_V6_FLOW:
+	case GTPU_UL_V4_FLOW:
+	case GTPU_UL_V6_FLOW:
+	case GTPU_DL_V4_FLOW:
+	case GTPU_DL_V6_FLOW:
+		return true;
+	}
+
+	return false;
+}
+
+/* When adding a new type, update the assert and, if it's hashable, add it to
+ * the flow_type_hashable switch case.
+ */
+static_assert(GTPU_DL_V6_FLOW + 1 == FLOW_TYPE_MAX);
+
+static int ethtool_check_xfrm_rxfh(u32 input_xfrm, u64 rxfh)
+{
+	/* Sanity check: if symmetric-xor/symmetric-or-xor is set, then:
+	 * 1 - no other fields besides IP src/dst and/or L4 src/dst
+	 * 2 - If src is set, dst must also be set
+	 */
+	if ((input_xfrm != RXH_XFRM_NO_CHANGE &&
+	     input_xfrm & (RXH_XFRM_SYM_XOR | RXH_XFRM_SYM_OR_XOR)) &&
+	    ((rxfh & ~(RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
+	     (!!(rxfh & RXH_IP_SRC) ^ !!(rxfh & RXH_IP_DST)) ||
+	     (!!(rxfh & RXH_L4_B_0_1) ^ !!(rxfh & RXH_L4_B_2_3))))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ethtool_check_flow_types(struct net_device *dev, u32 input_xfrm)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_rxnfc info = {
+		.cmd = ETHTOOL_GRXFH,
+	};
+	int err;
+	u32 i;
+
+	for (i = 0; i < FLOW_TYPE_MAX; i++) {
+		if (!flow_type_hashable(i))
+			continue;
+
+		info.flow_type = i;
+		err = ops->get_rxnfc(dev, &info, NULL);
+		if (err)
+			continue;
+
+		err = ethtool_check_xfrm_rxfh(input_xfrm, info.data);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 						u32 cmd, void __user *useraddr)
 {
@@ -1011,16 +1093,9 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev,
 		if (rc)
 			return rc;
 
-		/* Sanity check: if symmetric-xor/symmetric-or-xor is set, then:
-		 * 1 - no other fields besides IP src/dst and/or L4 src/dst
-		 * 2 - If src is set, dst must also be set
-		 */
-		if ((rxfh.input_xfrm & (RXH_XFRM_SYM_XOR | RXH_XFRM_SYM_OR_XOR)) &&
-		    ((info.data & ~(RXH_IP_SRC | RXH_IP_DST |
-				    RXH_L4_B_0_1 | RXH_L4_B_2_3)) ||
-		     (!!(info.data & RXH_IP_SRC) ^ !!(info.data & RXH_IP_DST)) ||
-		     (!!(info.data & RXH_L4_B_0_1) ^ !!(info.data & RXH_L4_B_2_3))))
-			return -EINVAL;
+		rc = ethtool_check_xfrm_rxfh(rxfh.input_xfrm, info.data);
+		if (rc)
+			return rc;
 	}
 
 	rc = ops->set_rxnfc(dev, &info);
@@ -1412,6 +1487,10 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 	     rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))
 		return -EINVAL;
 
+	ret = ethtool_check_flow_types(dev, rxfh.input_xfrm);
+	if (ret)
+		return ret;
+
 	indir_bytes = dev_indir_size * sizeof(rxfh_dev.indir[0]);
 
 	/* Check settings which may be global rather than per RSS-context */
-- 
2.40.1

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

[Simon Horman]

On Mon, Mar 10, 2025 at 09:23:29AM +0200, Gal Pressman wrote:
> Symmetric RSS hash requires that:
> * No other fields besides IP src/dst and/or L4 src/dst

nit: I think it would read slightly better if the line above included a verb.
     Likewise for the code comment with the same text.

> * 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@nvidia.com>
> Signed-off-by: Gal Pressman <gal@nvidia.com>

The nit above not withstanding - take it or leave it - this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Gal Pressman]

On 17/03/2025 17:07, Simon Horman wrote:
> On Mon, Mar 10, 2025 at 09:23:29AM +0200, Gal Pressman wrote:
>> Symmetric RSS hash requires that:
>> * No other fields besides IP src/dst and/or L4 src/dst
> 
> nit: I think it would read slightly better if the line above included a verb.
>      Likewise for the code comment with the same text.
> 
>> * 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@nvidia.com>
>> Signed-off-by: Gal Pressman <gal@nvidia.com>
> 
> The nit above not withstanding - take it or leave it - this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

Will change, thanks for the review.

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

[Paolo Abeni]

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@nvidia.com>
> Signed-off-by: Gal Pressman <gal@nvidia.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

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

[Gal Pressman]

On 17/03/2025 22:19, Paolo Abeni wrote:
> 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 couldn't think of any issues, you got me curious, do you have an example?

> 
> I think you could simply add:
> 
> #define FLOW_TYPE_MAX 0x1f

I preferred the enum so it would be "automatic", anyway, will change,
thanks!

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

[Paolo Abeni]

On 3/18/25 8:28 AM, Gal Pressman wrote:
> On 17/03/2025 22:19, Paolo Abeni wrote:
>> 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 couldn't think of any issues, you got me curious, do you have an example?

I guess something alike the following could be quite common or at least
possible:

#ifdef AH_V4_FLOW

// kernel support AH flow, implement user-space side

#else

// fail on any AH-flow related code path

#endif

Cheers,

Paolo

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

[Gal Pressman]

On 18/03/2025 9:36, Paolo Abeni wrote:
> On 3/18/25 8:28 AM, Gal Pressman wrote:
>> On 17/03/2025 22:19, Paolo Abeni wrote:
>>> 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 couldn't think of any issues, you got me curious, do you have an example?
> 
> I guess something alike the following could be quite common or at least
> possible:
> 
> #ifdef AH_V4_FLOW
> 
> // kernel support AH flow, implement user-space side
> 
> #else
> 
> // fail on any AH-flow related code path
> 
> #endif

Right, thanks Paolo!

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

[Jakub Kicinski]

On Tue, 18 Mar 2025 11:00:10 +0200 Gal Pressman wrote:
> > I guess something alike the following could be quite common or at least
> > possible:
> > 
> > #ifdef AH_V4_FLOW
> > 
> > // kernel support AH flow, implement user-space side
> > 
> > #else
> > 
> > // fail on any AH-flow related code path
> > 
> > #endif  
> 
> Right, thanks Paolo!

I don't see a v2, so commenting here.

I believe that we have had this conversation in BPF context in the past.
BPF likes to have things in enums because enums are preserved in debug
info / BTF and defines (obviously) aren't. So we converted a bunch of
things in uAPI to enums in the past for BPF's benefit. While Paolo's
concern is correct (and I believe I voiced similar concerns myself),
in practice we have never encountered any issues.

No strong preference from my side, but FWIW I think there's significant
precedent for such conversions.

One nit if you decide to keep the enum, Gal, please keep the comments
aligned.

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

[Gal Pressman]

On 24/03/2025 16:35, Jakub Kicinski wrote:
> On Tue, 18 Mar 2025 11:00:10 +0200 Gal Pressman wrote:
>>> I guess something alike the following could be quite common or at least
>>> possible:
>>>
>>> #ifdef AH_V4_FLOW
>>>
>>> // kernel support AH flow, implement user-space side
>>>
>>> #else
>>>
>>> // fail on any AH-flow related code path
>>>
>>> #endif  
>>
>> Right, thanks Paolo!
> 
> I don't see a v2, so commenting here.
> 
> I believe that we have had this conversation in BPF context in the past.
> BPF likes to have things in enums because enums are preserved in debug
> info / BTF and defines (obviously) aren't. So we converted a bunch of
> things in uAPI to enums in the past for BPF's benefit. While Paolo's
> concern is correct (and I believe I voiced similar concerns myself),
> in practice we have never encountered any issues.
> 
> No strong preference from my side, but FWIW I think there's significant
> precedent for such conversions.
> 
> One nit if you decide to keep the enum, Gal, please keep the comments
> aligned.

I do prefer to keep the enum, will address the alignment.

Thanks for the review.