[PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[Daniel Xu]

Previously, trying to insert an ip or ip6 only rule 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 only running l4proto validation
when mask is set.

Fixes: 9ba0e56199e3 ("bnxt_en: Enhance ethtool ntuple support for ip flows besides TCP/UDP")
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index f71cc8188b4e..1c97ee406bd7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1289,10 +1289,13 @@ 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_RAW && ip_spec->proto != IPPROTO_ICMP))
+	    (mproto && mproto != BNXT_IP_PROTO_FULL_MASK) ||
+	    (mproto && sproto != IPPROTO_RAW && sproto != IPPROTO_ICMP))
 		return false;
 	return true;
 }
@@ -1300,10 +1303,12 @@ 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_RAW &&
-	     ip_spec->l4_proto != IPPROTO_ICMPV6))
+	    (mproto && mproto != BNXT_IP_PROTO_FULL_MASK) ||
+	    (mproto && sproto != IPPROTO_RAW && sproto != IPPROTO_ICMPV6))
 		return false;
 	return true;
 }
-- 
2.46.0

Re: [PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[Michael Chan]

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

On Thu, Oct 31, 2024 at 9:59 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> Previously, trying to insert an ip or ip6 only rule 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 only running l4proto validation
> when mask is set.
>
> Fixes: 9ba0e56199e3 ("bnxt_en: Enhance ethtool ntuple support for ip flows besides TCP/UDP")
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>

Thanks for the patch.  I think the original author Vikas intended the
user to do this for ip only filters:

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

But your patch makes sense and simplifies the usage for the user.  I
just need to check that FW can accept 0 for the ip_protocol field to
mean wildcard when it receives the FW message to create the filter.

I will reply when I get the answer from the FW team.  If FW requires
0xff, then we just need to make a small change to your patch.

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

Re: [PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[Daniel Xu]

Hi Michael,

Thanks for taking a look.

On Fri, Nov 01, 2024 at 12:20:44PM GMT, Michael Chan wrote:
> On Thu, Oct 31, 2024 at 9:59 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> >
> > Previously, trying to insert an ip or ip6 only rule 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 only running l4proto validation
> > when mask is set.
> >
> > Fixes: 9ba0e56199e3 ("bnxt_en: Enhance ethtool ntuple support for ip flows besides TCP/UDP")
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> 
> Thanks for the patch.  I think the original author Vikas intended the
> user to do this for ip only filters:
> 
> ethtool -N eth0 flow-type ip6 dst-ip $IP6 l4_proto 0xff context 1
> 
> But your patch makes sense and simplifies the usage for the user.  I
> just need to check that FW can accept 0 for the ip_protocol field to
> mean wildcard when it receives the FW message to create the filter.
> 
> I will reply when I get the answer from the FW team.  If FW requires
> 0xff, then we just need to make a small change to your patch.

FWIW at least my HW/FW seems to behave correctly with my patch. I did
some quick tracing last night w/ a UDP traffic generator running to
confirm redirection occurs.

I tested on:

    driver: bnxt_en
    version: 6.9.5-<redacted>
    firmware-version: 229.0.154.1/pkg 229.1.123.1
    expansion-rom-version:
    bus-info: 0000:01:00.0
    supports-statistics: yes
    supports-test: yes
    supports-eeprom-access: yes
    supports-register-dump: yes
    supports-priv-flags: no

By the way, I noticed after I sent this patch that the get codepath
needs to be correspondingly updated. I will send a v2 with it.

Thanks,
Daniel

Re: [PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[Jakub Kicinski]

On Thu, 31 Oct 2024 22:58:30 -0600 Daniel Xu wrote:
> The reason was that all the l4proto validation was being run despite the
> l4proto mask being set to 0x0.  Fix by only running l4proto validation
> when mask is set.

Limitation is odd, but it's not a regression nor does it violate 
the uAPI so I think net-next would be appropriate, no Fixes tag
(you can say "commit xyz ("..") added.." as the reference).

Re: [PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[Michael Chan]

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

On Fri, Nov 1, 2024 at 3:42 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
> On Fri, Nov 01, 2024 at 12:20:44PM GMT, Michael Chan wrote:
> > Thanks for the patch.  I think the original author Vikas intended the
> > user to do this for ip only filters:
> >
> > ethtool -N eth0 flow-type ip6 dst-ip $IP6 l4_proto 0xff context 1
> >
> > But your patch makes sense and simplifies the usage for the user.  I
> > just need to check that FW can accept 0 for the ip_protocol field to
> > mean wildcard when it receives the FW message to create the filter.
> >
> > I will reply when I get the answer from the FW team.  If FW requires
> > 0xff, then we just need to make a small change to your patch.
>
> FWIW at least my HW/FW seems to behave correctly with my patch. I did
> some quick tracing last night w/ a UDP traffic generator running to
> confirm redirection occurs.
>
The FW team has confirmed that ip_protocol 0 will work as a wild card
on all FW supporting this feature.  So the patch will work.

But I think I want to eliminate the l4_proto 0xff usage.  It is
non-standard and non-intuitive.  So we should only support l4_proto to
be TCP, UDP, ICMP, or unspecified for any protocol.  Thanks.

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

Re: [PATCH net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification

[Daniel Xu]

Hi Michael,

On Tue, Nov 5, 2024, at 2:21 AM, Michael Chan wrote:
> On Fri, Nov 1, 2024 at 3:42 PM Daniel Xu <dxu@dxuuu.xyz> wrote:
>> On Fri, Nov 01, 2024 at 12:20:44PM GMT, Michael Chan wrote:
>> > Thanks for the patch.  I think the original author Vikas intended the
>> > user to do this for ip only filters:
>> >
>> > ethtool -N eth0 flow-type ip6 dst-ip $IP6 l4_proto 0xff context 1
>> >
>> > But your patch makes sense and simplifies the usage for the user.  I
>> > just need to check that FW can accept 0 for the ip_protocol field to
>> > mean wildcard when it receives the FW message to create the filter.
>> >
>> > I will reply when I get the answer from the FW team.  If FW requires
>> > 0xff, then we just need to make a small change to your patch.
>>
>> FWIW at least my HW/FW seems to behave correctly with my patch. I did
>> some quick tracing last night w/ a UDP traffic generator running to
>> confirm redirection occurs.
>>
> The FW team has confirmed that ip_protocol 0 will work as a wild card
> on all FW supporting this feature.  So the patch will work.
>
> But I think I want to eliminate the l4_proto 0xff usage.  It is
> non-standard and non-intuitive.  So we should only support l4_proto to
> be TCP, UDP, ICMP, or unspecified for any protocol.  Thanks.
>

Thanks for looking. I will send v3 that rolls up all the feedback
from this thread.

Thanks,
Daniel