[PATCH net-next v3 0/2] bnxt_en: ethtool: Improve wildcard l4proto on ip4/ip6 ntuple rules

[PATCH net-next v3 0/2] bnxt_en: ethtool: Improve wildcard l4proto on ip4/ip6 ntuple rules

[Daniel Xu]

This patchset improves wildcarding over l4proto on ip4 and ip6 nutple
rules. Previous support required setting l4proto explicitly to 0xFF if
you wanted wildcard, which ethtool (naturally) did not do. For example,
this command would fail with -EOPNOSUPP:

    ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1

After this patchset, only TCP, UDP, ICMP, and unset will be supported
for l4proto.

Changes from v2:
* Target net-next
* Remove Fixes: tag
* Remove support for IPPROTO_RAW (0xFF) l4proto

Changes from v1:
* Set underlying l4proto to IPPROTO_RAW to fix get path

Daniel Xu (2):
  bnxt_en: ethtool: Remove ip4/ip6 ntuple support for IPPROTO_RAW
  bnxt_en: ethtool: Support unset l4proto on ip4/ip6 ntuple rules

 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 44 +++++++++++--------
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  1 +
 2 files changed, 26 insertions(+), 19 deletions(-)

-- 
2.46.0

[PATCH net-next v3 1/2] bnxt_en: ethtool: Remove ip4/ip6 ntuple support for IPPROTO_RAW

[Daniel Xu]

Commit 9ba0e56199e3 ("bnxt_en: Enhance ethtool ntuple support for ip
flows besides TCP/UDP") added support for ip4/ip6 ntuple rules.
However, if you wanted to wildcard over l4proto, you had to provide
0xFF.

The choice of 0xFF is non-standard and non-intuitive. Delete support for
it in this commit. Next commit we will introduce a cleaner way to
wildcard l4proto.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 21 ++++++-------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 6ef06579df53..41160aed9476 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1124,14 +1124,10 @@ static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 	fkeys = &fltr->fkeys;
 	fmasks = &fltr->fmasks;
 	if (fkeys->basic.n_proto == htons(ETH_P_IP)) {
-		if (fkeys->basic.ip_proto == IPPROTO_ICMP ||
-		    fkeys->basic.ip_proto == IPPROTO_RAW) {
+		if (fkeys->basic.ip_proto == IPPROTO_ICMP) {
 			fs->flow_type = IP_USER_FLOW;
 			fs->h_u.usr_ip4_spec.ip_ver = ETH_RX_NFC_IP4;
-			if (fkeys->basic.ip_proto == IPPROTO_ICMP)
-				fs->h_u.usr_ip4_spec.proto = IPPROTO_ICMP;
-			else
-				fs->h_u.usr_ip4_spec.proto = IPPROTO_RAW;
+			fs->h_u.usr_ip4_spec.proto = IPPROTO_ICMP;
 			fs->m_u.usr_ip4_spec.proto = BNXT_IP_PROTO_FULL_MASK;
 		} else if (fkeys->basic.ip_proto == IPPROTO_TCP) {
 			fs->flow_type = TCP_V4_FLOW;
@@ -1153,13 +1149,9 @@ static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 			fs->m_u.tcp_ip4_spec.pdst = fmasks->ports.dst;
 		}
 	} else {
-		if (fkeys->basic.ip_proto == IPPROTO_ICMPV6 ||
-		    fkeys->basic.ip_proto == IPPROTO_RAW) {
+		if (fkeys->basic.ip_proto == IPPROTO_ICMPV6) {
 			fs->flow_type = IPV6_USER_FLOW;
-			if (fkeys->basic.ip_proto == IPPROTO_ICMPV6)
-				fs->h_u.usr_ip6_spec.l4_proto = IPPROTO_ICMPV6;
-			else
-				fs->h_u.usr_ip6_spec.l4_proto = IPPROTO_RAW;
+			fs->h_u.usr_ip6_spec.l4_proto = IPPROTO_ICMPV6;
 			fs->m_u.usr_ip6_spec.l4_proto = BNXT_IP_PROTO_FULL_MASK;
 		} else if (fkeys->basic.ip_proto == IPPROTO_TCP) {
 			fs->flow_type = TCP_V6_FLOW;
@@ -1285,7 +1277,7 @@ static bool bnxt_verify_ntuple_ip4_flow(struct ethtool_usrip4_spec *ip_spec,
 	if (ip_mask->l4_4_bytes || ip_mask->tos ||
 	    ip_spec->ip_ver != ETH_RX_NFC_IP4 ||
 	    ip_mask->proto != BNXT_IP_PROTO_FULL_MASK ||
-	    (ip_spec->proto != IPPROTO_RAW && ip_spec->proto != IPPROTO_ICMP))
+	    ip_spec->proto != IPPROTO_ICMP)
 		return false;
 	return true;
 }
@@ -1295,8 +1287,7 @@ static bool bnxt_verify_ntuple_ip6_flow(struct ethtool_usrip6_spec *ip_spec,
 {
 	if (ip_mask->l4_4_bytes || ip_mask->tclass ||
 	    ip_mask->l4_proto != BNXT_IP_PROTO_FULL_MASK ||
-	    (ip_spec->l4_proto != IPPROTO_RAW &&
-	     ip_spec->l4_proto != IPPROTO_ICMPV6))
+	    ip_spec->l4_proto != IPPROTO_ICMPV6)
 		return false;
 	return true;
 }
-- 
2.46.0

Re: [PATCH net-next v3 1/2] bnxt_en: ethtool: Remove ip4/ip6 ntuple support for IPPROTO_RAW

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Mon, Nov 4, 2024 at 8:13 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Commit 9ba0e56199e3 ("bnxt_en: Enhance ethtool ntuple support for ip
> flows besides TCP/UDP") added support for ip4/ip6 ntuple rules.
> However, if you wanted to wildcard over l4proto, you had to provide
> 0xFF.
>
> The choice of 0xFF is non-standard and non-intuitive. Delete support for
> it in this commit. Next commit we will introduce a cleaner way to
> wildcard l4proto.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

[PATCH net-next v3 2/2] bnxt_en: ethtool: Support unset l4proto on ip4/ip6 ntuple rules

[Daniel Xu]

Previously, trying to insert an ip4/ip6 ntuple rule with an unset
l4proto would get rejected with -EOPNOTSUPP. For example, the following
would fail:

    ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1

The reason was that all the l4proto validation was being run despite the
l4proto mask being set to 0x0. Fix by respecting the mask on l4proto and
treating a mask of 0x0 as wildcard l4proto.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 31 ++++++++++++++-----
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  1 +
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 41160aed9476..cfd2c65b1c90 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1124,7 +1124,12 @@ static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 	fkeys = &fltr->fkeys;
 	fmasks = &fltr->fmasks;
 	if (fkeys->basic.n_proto == htons(ETH_P_IP)) {
-		if (fkeys->basic.ip_proto == IPPROTO_ICMP) {
+		if (fkeys->basic.ip_proto == BNXT_IP_PROTO_WILDCARD) {
+			fs->flow_type = IP_USER_FLOW;
+			fs->h_u.usr_ip4_spec.ip_ver = ETH_RX_NFC_IP4;
+			fs->h_u.usr_ip4_spec.proto = BNXT_IP_PROTO_WILDCARD;
+			fs->m_u.usr_ip4_spec.proto = 0;
+		} else if (fkeys->basic.ip_proto == IPPROTO_ICMP) {
 			fs->flow_type = IP_USER_FLOW;
 			fs->h_u.usr_ip4_spec.ip_ver = ETH_RX_NFC_IP4;
 			fs->h_u.usr_ip4_spec.proto = IPPROTO_ICMP;
@@ -1149,7 +1154,11 @@ static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 			fs->m_u.tcp_ip4_spec.pdst = fmasks->ports.dst;
 		}
 	} else {
-		if (fkeys->basic.ip_proto == IPPROTO_ICMPV6) {
+		if (fkeys->basic.ip_proto == BNXT_IP_PROTO_WILDCARD) {
+			fs->flow_type = IPV6_USER_FLOW;
+			fs->h_u.usr_ip6_spec.l4_proto = BNXT_IP_PROTO_WILDCARD;
+			fs->m_u.usr_ip6_spec.l4_proto = 0;
+		} else if (fkeys->basic.ip_proto == IPPROTO_ICMPV6) {
 			fs->flow_type = IPV6_USER_FLOW;
 			fs->h_u.usr_ip6_spec.l4_proto = IPPROTO_ICMPV6;
 			fs->m_u.usr_ip6_spec.l4_proto = BNXT_IP_PROTO_FULL_MASK;
@@ -1274,10 +1283,12 @@ static int bnxt_add_l2_cls_rule(struct bnxt *bp,
 static bool bnxt_verify_ntuple_ip4_flow(struct ethtool_usrip4_spec *ip_spec,
 					struct ethtool_usrip4_spec *ip_mask)
 {
+	u8 mproto = ip_mask->proto;
+	u8 sproto = ip_spec->proto;
+
 	if (ip_mask->l4_4_bytes || ip_mask->tos ||
 	    ip_spec->ip_ver != ETH_RX_NFC_IP4 ||
-	    ip_mask->proto != BNXT_IP_PROTO_FULL_MASK ||
-	    ip_spec->proto != IPPROTO_ICMP)
+	    (mproto && (mproto != BNXT_IP_PROTO_FULL_MASK || sproto != IPPROTO_ICMP)))
 		return false;
 	return true;
 }
@@ -1285,9 +1296,11 @@ static bool bnxt_verify_ntuple_ip4_flow(struct ethtool_usrip4_spec *ip_spec,
 static bool bnxt_verify_ntuple_ip6_flow(struct ethtool_usrip6_spec *ip_spec,
 					struct ethtool_usrip6_spec *ip_mask)
 {
+	u8 mproto = ip_mask->l4_proto;
+	u8 sproto = ip_spec->l4_proto;
+
 	if (ip_mask->l4_4_bytes || ip_mask->tclass ||
-	    ip_mask->l4_proto != BNXT_IP_PROTO_FULL_MASK ||
-	    ip_spec->l4_proto != IPPROTO_ICMPV6)
+	    (mproto && (mproto != BNXT_IP_PROTO_FULL_MASK || sproto != IPPROTO_ICMPV6)))
 		return false;
 	return true;
 }
@@ -1341,7 +1354,8 @@ static int bnxt_add_ntuple_cls_rule(struct bnxt *bp,
 		struct ethtool_usrip4_spec *ip_spec = &fs->h_u.usr_ip4_spec;
 		struct ethtool_usrip4_spec *ip_mask = &fs->m_u.usr_ip4_spec;
 
-		fkeys->basic.ip_proto = ip_spec->proto;
+		fkeys->basic.ip_proto = ip_mask->proto ? ip_spec->proto
+						       : BNXT_IP_PROTO_WILDCARD;
 		fkeys->basic.n_proto = htons(ETH_P_IP);
 		fkeys->addrs.v4addrs.src = ip_spec->ip4src;
 		fmasks->addrs.v4addrs.src = ip_mask->ip4src;
@@ -1372,7 +1386,8 @@ static int bnxt_add_ntuple_cls_rule(struct bnxt *bp,
 		struct ethtool_usrip6_spec *ip_spec = &fs->h_u.usr_ip6_spec;
 		struct ethtool_usrip6_spec *ip_mask = &fs->m_u.usr_ip6_spec;
 
-		fkeys->basic.ip_proto = ip_spec->l4_proto;
+		fkeys->basic.ip_proto = ip_mask->l4_proto ? ip_spec->l4_proto
+							  : BNXT_IP_PROTO_WILDCARD;
 		fkeys->basic.n_proto = htons(ETH_P_IPV6);
 		fkeys->addrs.v6addrs.src = *(struct in6_addr *)&ip_spec->ip6src;
 		fmasks->addrs.v6addrs.src = *(struct in6_addr *)&ip_mask->ip6src;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
index e2ee030237d4..33b86ede1ce5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
@@ -44,6 +44,7 @@ struct bnxt_led_cfg {
 #define BNXT_PXP_REG_LEN	0x3110
 
 #define BNXT_IP_PROTO_FULL_MASK	0xFF
+#define BNXT_IP_PROTO_WILDCARD	0x0
 
 extern const struct ethtool_ops bnxt_ethtool_ops;
 
-- 
2.46.0

Re: [PATCH net-next v3 2/2] bnxt_en: ethtool: Support unset l4proto on ip4/ip6 ntuple rules

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Mon, Nov 4, 2024 at 8:13 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Previously, trying to insert an ip4/ip6 ntuple rule with an unset
> l4proto would get rejected with -EOPNOTSUPP. For example, the following
> would fail:
>
>     ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1
>
> The reason was that all the l4proto validation was being run despite the
> l4proto mask being set to 0x0. Fix by respecting the mask on l4proto and
> treating a mask of 0x0 as wildcard l4proto.
>
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

I just realized that we don't allow l4_proto 6 and 17 for TCP and UDP.
It's been this way since the feature was added.  I can add it later to
make it more complete.  Thanks.

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next v3 0/2] bnxt_en: ethtool: Improve wildcard l4proto on ip4/ip6 ntuple rules

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  4 Nov 2024 21:13:18 -0700 you wrote:
> This patchset improves wildcarding over l4proto on ip4 and ip6 nutple
> rules. Previous support required setting l4proto explicitly to 0xFF if
> you wanted wildcard, which ethtool (naturally) did not do. For example,
> this command would fail with -EOPNOSUPP:
> 
>     ethtool -N eth0 flow-type ip6 dst-ip $IP6 context 1
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/2] bnxt_en: ethtool: Remove ip4/ip6 ntuple support for IPPROTO_RAW
    https://git.kernel.org/netdev/net-next/c/050eb2cebb9e
  - [net-next,v3,2/2] bnxt_en: ethtool: Support unset l4proto on ip4/ip6 ntuple rules
    https://git.kernel.org/netdev/net-next/c/5f143efd3804

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html