| Message ID | 219859e674ef7a9d8af9ab4f64a9095580f04bcc.1730436983.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] bnxt_en: ethtool: Fix ip[6] ntuple rule verification | expand |
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.
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
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).
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.
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
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; }
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(-)