diff mbox series

[ovs-dev] odp-util: Don't fail on OVS_KEY_ATTR_IPV6_EXTHDRS attribute

Message ID 20220414093458.1194176-1-vladbu@nvidia.com
State Changes Requested
Headers show
Series [ovs-dev] odp-util: Don't fail on OVS_KEY_ATTR_IPV6_EXTHDRS attribute | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/intel-ovs-compilation success test: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Vlad Buslov April 14, 2022, 9:34 a.m. UTC 
From: Roi Dayan <roid@nvidia.com>

Even though cited commit states that userspace declaration of
OVS_KEY_ATTR_IPV6_EXTHDRS is not required, without it TC offload of IPv6
flows fails with following error:

2022-04-08T19:47:12.405Z|00001|odp_util(handler1)|ERR|internal error parsing flow key recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),eth(src=e4:f9:05:08:00:02,dst=33:33:00:00:00:
16),eth_type(0x86dd),ipv6(src=fe80::e6f9:5ff:fe08:2,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no)(bad key length 2, expected -1)(40 00),icmpv6(type=143,code=0)

To fix the error until proper IPv6 extensions support is implemented, add
the attribute OVS_KEY_ATTR_IPV6_EXTHDRS and skip it with a warning during
parsing.

Issue: 3030660
Fixes: d96d14b14733 ("openvswitch.h: Align uAPI definition with the kernel.")
Signed-off-by: Roi Dayan <roid@nvidia.com>
Signed-off-by: Vlad Buslov <vladbu@nvidia.com>
Acked-by: Maor Dickman <maord@nvidia.com>
---
 datapath/flow_netlink.c                           |  2 +-
 datapath/linux/compat/include/linux/openvswitch.h |  6 ++++++
 lib/odp-execute.c                                 |  2 ++
 lib/odp-util.c                                    | 11 +++++++++++
 ofproto/ofproto-dpif-sflow.c                      |  1 +
 5 files changed, 21 insertions(+), 1 deletion(-)

Comments

Ilya Maximets April 25, 2022, 5:22 p.m. UTC | #1 
On 4/14/22 11:34, Vlad Buslov wrote:
> From: Roi Dayan <roid@nvidia.com>
> 
> Even though cited commit states that userspace declaration of
> OVS_KEY_ATTR_IPV6_EXTHDRS is not required, without it TC offload of IPv6
> flows fails with following error:
> 
> 2022-04-08T19:47:12.405Z|00001|odp_util(handler1)|ERR|internal error parsing flow key recirc_id(0),dp_hash(0),skb_priority(0),in_port(2),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),eth(src=e4:f9:05:08:00:02,dst=33:33:00:00:00:
> 16),eth_type(0x86dd),ipv6(src=fe80::e6f9:5ff:fe08:2,dst=ff02::16,label=0,proto=58,tclass=0,hlimit=1,frag=no)(bad key length 2, expected -1)(40 00),icmpv6(type=143,code=0)
> 
> To fix the error until proper IPv6 extensions support is implemented, add
> the attribute OVS_KEY_ATTR_IPV6_EXTHDRS and skip it with a warning during
> parsing.

Hi.  Thanks for looking at this issue, but I don't think that is a
correct solution.  Offloading will be broken again once anything new
is added to the kernel.  We need to make OVS to correctly handle
ODP_FIT_TOO_MUCH case instead.  In general, I think, it should be
safe to just ignore it.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index caed443863ed..3532c38f61e2 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -359,7 +359,7 @@  size_t ovs_key_attr_size(void)
 	/* Whenever adding new OVS_KEY_ FIELDS, we should consider
 	 * updating this function.
 	 */
-	BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 31);
+	BUILD_BUG_ON(OVS_KEY_ATTR_MAX != 32);
 
 	return    nla_total_size(4)   /* OVS_KEY_ATTR_PRIORITY */
 		+ nla_total_size(0)   /* OVS_KEY_ATTR_TUNNEL */
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 8bb5abdc8349..acd047fbfe9a 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -401,6 +401,7 @@  enum ovs_key_attr {
 	OVS_KEY_ATTR_TUNNEL_INFO,   /* struct ip_tunnel_info.
 				     * For in-kernel use only.
 				     */
+	OVS_KEY_ATTR_IPV6_EXTHDRS, /* struct ovs_key_ipv6_exthdr */
 	__OVS_KEY_ATTR_MAX
 };
 
@@ -500,6 +501,11 @@  struct ovs_key_ipv6 {
 	__u8   ipv6_frag;	/* One of OVS_FRAG_TYPE_*. */
 };
 
+/* separate structure to support backward compatibility with older user space */
+struct ovs_key_ipv6_exthdrs {
+    __u16  hdrs;
+};
+
 struct ovs_key_tcp {
 	__be16 tcp_src;
 	__be16 tcp_dst;
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 7da56793d0ff..93921acb7412 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -554,6 +554,7 @@  odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a)
     case OVS_KEY_ATTR_CT_MARK:
     case OVS_KEY_ATTR_CT_LABELS:
     case OVS_KEY_ATTR_TUNNEL_INFO:
+    case OVS_KEY_ATTR_IPV6_EXTHDRS:
     case __OVS_KEY_ATTR_MAX:
     default:
         OVS_NOT_REACHED();
@@ -667,6 +668,7 @@  odp_execute_masked_set_action(struct dp_packet *packet,
     case OVS_KEY_ATTR_ICMPV6:
     case OVS_KEY_ATTR_TCP_FLAGS:
     case OVS_KEY_ATTR_TUNNEL_INFO:
+    case OVS_KEY_ATTR_IPV6_EXTHDRS:
     case __OVS_KEY_ATTR_MAX:
     default:
         OVS_NOT_REACHED();
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 073fc20d9f7f..0964d5f3eb6b 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -192,6 +192,7 @@  ovs_key_attr_to_string(enum ovs_key_attr attr, char *namebuf, size_t bufsize)
     case OVS_KEY_ATTR_RECIRC_ID: return "recirc_id";
     case OVS_KEY_ATTR_PACKET_TYPE: return "packet_type";
     case OVS_KEY_ATTR_NSH: return "nsh";
+    case OVS_KEY_ATTR_IPV6_EXTHDRS: return "ipv6_exthdrs";
 
     case OVS_KEY_ATTR_TUNNEL_INFO: return "<error: kernel-only tunnel_info>";
     case __OVS_KEY_ATTR_MAX:
@@ -2747,6 +2748,7 @@  const struct attr_len_tbl ovs_flow_key_attr_lens[OVS_KEY_ATTR_MAX + 1] = {
     [OVS_KEY_ATTR_NSH]       = { .len = ATTR_LEN_NESTED,
                                  .next = ovs_nsh_key_attr_lens,
                                  .next_max = OVS_NSH_KEY_ATTR_MAX },
+    [OVS_KEY_ATTR_IPV6_EXTHDRS] = { .len = sizeof(struct ovs_key_ipv6_exthdrs) },
 };
 
 /* Returns the correct length of the payload for a flow key attribute of the
@@ -3263,6 +3265,7 @@  odp_mask_is_constant__(enum ovs_key_attr attr, const void *mask, size_t size,
     case OVS_KEY_ATTR_UNSPEC:
     case OVS_KEY_ATTR_ENCAP:
     case OVS_KEY_ATTR_TUNNEL_INFO:
+    case OVS_KEY_ATTR_IPV6_EXTHDRS:
     case __OVS_KEY_ATTR_MAX:
     default:
         return false;
@@ -4415,6 +4418,7 @@  format_odp_key_attr__(const struct nlattr *a, const struct nlattr *ma,
     }
     case OVS_KEY_ATTR_UNSPEC:
     case OVS_KEY_ATTR_TUNNEL_INFO:
+    case OVS_KEY_ATTR_IPV6_EXTHDRS:
     case __OVS_KEY_ATTR_MAX:
     default:
         format_generic_odp_key(a, ds);
@@ -6620,6 +6624,7 @@  odp_key_to_dp_packet(const struct nlattr *key, size_t key_len,
         case OVS_KEY_ATTR_PACKET_TYPE:
         case OVS_KEY_ATTR_NSH:
         case OVS_KEY_ATTR_TUNNEL_INFO:
+        case OVS_KEY_ATTR_IPV6_EXTHDRS:
         case __OVS_KEY_ATTR_MAX:
         default:
             break;
@@ -6979,6 +6984,12 @@  parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                 expected_bit = OVS_KEY_ATTR_IPV6;
             }
         }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV6_EXTHDRS)) {
+            if (!is_mask) {
+                *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV6_EXTHDRS;
+                VLOG_WARN_RL(&rl, "not parsing ipv6 exthdrs");
+            }
+        }
     } else if (src_flow->dl_type == htons(ETH_TYPE_ARP) ||
                src_flow->dl_type == htons(ETH_TYPE_RARP)) {
         if (!is_mask) {
diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
index ab4941364c0d..6a1ceadfa4dd 100644
--- a/ofproto/ofproto-dpif-sflow.c
+++ b/ofproto/ofproto-dpif-sflow.c
@@ -1067,6 +1067,7 @@  sflow_read_set_action(const struct nlattr *attr,
     case OVS_KEY_ATTR_PACKET_TYPE:
     case OVS_KEY_ATTR_NSH:
     case OVS_KEY_ATTR_TUNNEL_INFO:
+    case OVS_KEY_ATTR_IPV6_EXTHDRS:
     case __OVS_KEY_ATTR_MAX:
     default:
         break;