diff mbox series

[net-next,v6,2/7] net: ethtool: add support for symmetric-xor RSS hash

Message ID 20231120205614.46350-3-ahmed.zaki@intel.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Support symmetric-xor RSS hash | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3025 this patch: 3025
netdev/cc_maintainers warning 4 maintainers not CCed: keescook@chromium.org sudheer.mogilappagari@intel.com jdamato@fastly.com d-tatianin@yandex-team.ru
netdev/build_clang success Errors and warnings before: 1289 this patch: 1289
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3237 this patch: 3237
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 103 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ahmed Zaki Nov. 20, 2023, 8:56 p.m. UTC
Symmetric RSS hash functions are beneficial in applications that monitor
both Tx and Rx packets of the same flow (IDS, software firewalls, ..etc).
Getting all traffic of the same flow on the same RX queue results in
higher CPU cache efficiency.

A NIC that supports "symmetric-xor" can achieve this RSS hash symmetry
by XORing the source and destination fields and pass the values to the
RSS hash algorithm.

The user may request RSS hash symmetry for a specific algorithm, via:

    # ethtool -X eth0 hfunc <hash_alg> symmetric-xor

or turn symmetry off (asymmetric) by:

    # ethtool -X eth0 hfunc <hash_alg>

The specific fields for each flow type should then be specified as usual
via:
    # ethtool -N|-U eth0 rx-flow-hash <flow_type> s|d|f|n

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 Documentation/networking/scaling.rst | 15 +++++++++++++++
 include/uapi/linux/ethtool.h         | 12 +++++++++++-
 include/uapi/linux/ethtool_netlink.h |  1 +
 net/ethtool/ioctl.c                  |  4 ++--
 net/ethtool/rss.c                    |  5 +++++
 5 files changed, 34 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski Nov. 21, 2023, 11:33 p.m. UTC | #1
On Mon, 20 Nov 2023 13:56:09 -0700 Ahmed Zaki wrote:
> + * @data: Extension for the RSS hash function. Valid values are one of the
> + *	%RXH_HFUNC_*.

@data is way too generic. Can we call this key_xfrm? key_preproc?

> +/* RSS hash function data
> + * XOR the corresponding source and destination fields of each specified
> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
> + * calculation.
> + */
> +#define	RXH_HFUNC_SYM_XOR	(1 << 0)

We need to mention somewhere that sym-xor is unsafe, per Alex's
comments.

> +++ b/include/uapi/linux/ethtool_netlink.h

You need to fill in the details in:

Documentation/networking/ethtool-netlink.rst
and
Documentation/netlink/specs/ethtool.yaml

Last but not least please keep the field check you moved to the drivers
in the core. Nobody will remember to check that other drivers added the
check as well.
Gal Pressman Nov. 23, 2023, 1:33 p.m. UTC | #2
On 22/11/2023 1:33, Jakub Kicinski wrote:
> Last but not least please keep the field check you moved to the drivers
> in the core. Nobody will remember to check that other drivers added the
> check as well.

The wording of this sentence is a bit confusing, so I might be repeating
what you already said, but this patchset needs to make sure that drivers
that do not support the new symmetric flag return an error instead of
silently ignoring it.
Jakub Kicinski Nov. 23, 2023, 4:27 p.m. UTC | #3
On Thu, 23 Nov 2023 15:33:04 +0200 Gal Pressman wrote:
> On 22/11/2023 1:33, Jakub Kicinski wrote:
> > Last but not least please keep the field check you moved to the drivers
> > in the core. Nobody will remember to check that other drivers added the
> > check as well.  
> 
> The wording of this sentence is a bit confusing, so I might be repeating
> what you already said, but this patchset needs to make sure that drivers
> that do not support the new symmetric flag return an error instead of
> silently ignoring it.

Ah, good point! That, too.

I was referring to what changed in v5, from the change long:

  v5: move sanity checks from ethtool/ioctl.c to ice's and iavf's rxfnc
      drivers entries (patches 5 and 6).
Ahmed Zaki Nov. 27, 2023, 2:21 p.m. UTC | #4
On 2023-11-21 16:33, Jakub Kicinski wrote:
> On Mon, 20 Nov 2023 13:56:09 -0700 Ahmed Zaki wrote:
>> + * @data: Extension for the RSS hash function. Valid values are one of the
>> + *	%RXH_HFUNC_*.
> 
> @data is way too generic. Can we call this key_xfrm? key_preproc?

We manipulate the "input data" (protocol fields) not the key. I will 
rename to "input_xfrm".

> 
>> +/* RSS hash function data
>> + * XOR the corresponding source and destination fields of each specified
>> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
>> + * calculation.
>> + */
>> +#define	RXH_HFUNC_SYM_XOR	(1 << 0)
> 
> We need to mention somewhere that sym-xor is unsafe, per Alex's
> comments.

I already added the following in Documentation/networking/scaling.rst:

"The Symmetric-XOR" is a type of RSS algorithms that achieves this hash 
symmetry by XORing the input source and destination fields of the IP
and/or L4 protocols. This, however, results in reduced input entropy and
could potentially be exploited."


Or do you mean add it also to "uapi/linux/ethtool.h" ?


Will do the rest in the next version.
Jakub Kicinski Nov. 27, 2023, 4:57 p.m. UTC | #5
On Mon, 27 Nov 2023 07:21:47 -0700 Ahmed Zaki wrote:
> On 2023-11-21 16:33, Jakub Kicinski wrote:
> > On Mon, 20 Nov 2023 13:56:09 -0700 Ahmed Zaki wrote:  
> >> + * @data: Extension for the RSS hash function. Valid values are one of the
> >> + *	%RXH_HFUNC_*.  
> > 
> > @data is way too generic. Can we call this key_xfrm? key_preproc?  
> 
> We manipulate the "input data" (protocol fields) not the key. I will 
> rename to "input_xfrm".

Ugh, right!

> >> +/* RSS hash function data
> >> + * XOR the corresponding source and destination fields of each specified
> >> + * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
> >> + * calculation.
> >> + */
> >> +#define	RXH_HFUNC_SYM_XOR	(1 << 0)  
> > 
> > We need to mention somewhere that sym-xor is unsafe, per Alex's
> > comments.  
> 
> I already added the following in Documentation/networking/scaling.rst:
> 
> "The Symmetric-XOR" is a type of RSS algorithms that achieves this hash 
> symmetry by XORing the input source and destination fields of the IP
> and/or L4 protocols. This, however, results in reduced input entropy and
> could potentially be exploited."
> 
> Or do you mean add it also to "uapi/linux/ethtool.h" ?

Yes, a short mention in a comment next to the define.
There's a good chance person looking at this will not notice 
the documentation in scaling.
diff mbox series

Patch

diff --git a/Documentation/networking/scaling.rst b/Documentation/networking/scaling.rst
index 03ae19a689fc..4eb50bcb9d42 100644
--- a/Documentation/networking/scaling.rst
+++ b/Documentation/networking/scaling.rst
@@ -44,6 +44,21 @@  by masking out the low order seven bits of the computed hash for the
 packet (usually a Toeplitz hash), taking this number as a key into the
 indirection table and reading the corresponding value.
 
+Some NICs support symmetric RSS hashing where, if the IP (source address,
+destination address) and TCP/UDP (source port, destination port) tuples
+are swapped, the computed hash is the same. This is beneficial in some
+applications that monitor TCP/IP flows (IDS, firewalls, ...etc) and need
+both directions of the flow to land on the same Rx queue (and CPU). The
+"Symmetric-XOR" is a type of RSS algorithms that achieves this hash
+symmetry by XORing the input source and destination fields of the IP
+and/or L4 protocols. This, however, results in reduced input entropy and
+could potentially be exploited. Specifically, the algorithm XORs the input
+as follows::
+
+    # (SRC_IP ^ DST_IP, SRC_IP ^ DST_IP, SRC_PORT ^ DST_PORT, SRC_PORT ^ DST_PORT)
+
+The result is then fed to the underlying RSS algorithm.
+
 Some advanced NICs allow steering packets to queues based on
 programmable filters. For example, webserver bound TCP port 80 packets
 can be directed to their own receive queue. Such “n-tuple” filters can
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..5d629b7b2d55 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1266,6 +1266,8 @@  struct ethtool_rxfh_indir {
  *	hardware hash key.
  * @hfunc: Defines the current RSS hash function used by HW (or to be set to).
  *	Valid values are one of the %ETH_RSS_HASH_*.
+ * @data: Extension for the RSS hash function. Valid values are one of the
+ *	%RXH_HFUNC_*.
  * @rsvd8: Reserved for future use; see the note on reserved space.
  * @rsvd32: Reserved for future use; see the note on reserved space.
  * @rss_config: RX ring/queue index for each hash value i.e., indirection table
@@ -1285,7 +1287,8 @@  struct ethtool_rxfh {
 	__u32   indir_size;
 	__u32   key_size;
 	__u8	hfunc;
-	__u8	rsvd8[3];
+	__u8	data;
+	__u8	rsvd8[2];
 	__u32	rsvd32;
 	__u32   rss_config[];
 };
@@ -1992,6 +1995,13 @@  static inline int ethtool_validate_duplex(__u8 duplex)
 
 #define WOL_MODE_COUNT		8
 
+/* RSS hash function data
+ * XOR the corresponding source and destination fields of each specified
+ * protocol. Both copies of the XOR'ed fields are fed into the RSS and RXHASH
+ * calculation.
+ */
+#define	RXH_HFUNC_SYM_XOR	(1 << 0)
+
 /* 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) */
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..638fa7f0682f 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -908,6 +908,7 @@  enum {
 	ETHTOOL_A_RSS_HFUNC,		/* u32 */
 	ETHTOOL_A_RSS_INDIR,		/* binary */
 	ETHTOOL_A_RSS_HKEY,		/* binary */
+	ETHTOOL_A_RSS_HFUNC_DATA,	/* u32 */
 
 	__ETHTOOL_A_RSS_CNT,
 	ETHTOOL_A_RSS_MAX = (__ETHTOOL_A_RSS_CNT - 1),
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index f4e6067d200f..98726d1bef77 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1195,7 +1195,7 @@  static noinline_for_stack int ethtool_get_rxfh(struct net_device *dev,
 	user_key_size = rxfh.key_size;
 
 	/* Check that reserved fields are 0 for now */
-	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
+	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd32)
 		return -EINVAL;
 	/* Most drivers don't handle rss_context, check it's 0 as well */
 	if (rxfh.rss_context && !ops->get_rxfh_context)
@@ -1268,7 +1268,7 @@  static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		return -EFAULT;
 
 	/* Check that reserved fields are 0 for now */
-	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd8[2] || rxfh.rsvd32)
+	if (rxfh.rsvd8[0] || rxfh.rsvd8[1] || rxfh.rsvd32)
 		return -EINVAL;
 	/* Most drivers don't handle rss_context, check it's 0 as well */
 	if (rxfh.rss_context && !ops->set_rxfh_context)
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 2d11f881810d..99ee061e6582 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -13,6 +13,7 @@  struct rss_reply_data {
 	u32				indir_size;
 	u32				hkey_size;
 	u32				hfunc;
+	u32				hfunc_data;
 	u32				*indir_table;
 	u8				*hkey;
 };
@@ -97,6 +98,7 @@  rss_prepare_data(const struct ethnl_req_info *req_base,
 		goto out_ops;
 
 	data->hfunc = rxfh.hfunc;
+	data->hfunc_data = rxfh.data;
 out_ops:
 	ethnl_ops_complete(dev);
 	return ret;
@@ -110,6 +112,7 @@  rss_reply_size(const struct ethnl_req_info *req_base,
 	int len;
 
 	len = nla_total_size(sizeof(u32)) +	/* _RSS_HFUNC */
+	      nla_total_size(sizeof(u32)) +	/* _RSS_HFUNC_DATA */
 	      nla_total_size(sizeof(u32) * data->indir_size) + /* _RSS_INDIR */
 	      nla_total_size(data->hkey_size);	/* _RSS_HKEY */
 
@@ -124,6 +127,8 @@  rss_fill_reply(struct sk_buff *skb, const struct ethnl_req_info *req_base,
 
 	if ((data->hfunc &&
 	     nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC, data->hfunc)) ||
+	    (data->hfunc_data &&
+	     nla_put_u32(skb, ETHTOOL_A_RSS_HFUNC_DATA, data->hfunc_data)) ||
 	    (data->indir_size &&
 	     nla_put(skb, ETHTOOL_A_RSS_INDIR,
 		     sizeof(u32) * data->indir_size, data->indir_table)) ||