[ovs-dev,ovs-discuss] a problem the userspace datapath failed to create a new datapath flow for the QinQ packet

Message ID 34EFBCA9F01B0748BEB6B629CE643AE60C9DA075@DGGEMM533-MBX.china.huawei.com
State New
Headers show
Series
  • [ovs-dev,ovs-discuss] a problem the userspace datapath failed to create a new datapath flow for the QinQ packet
Related show

Commit Message

wangyunjian Jan. 14, 2019, 12:12 p.m.
Hi,

I found a problem the userspace datapath failed to create a 
new datapath flow for the QinQ packet. When getting mask from
nlattrs with the mask include ip,udp,etc, it parsed 802.1Q
header wrongly.

This patch fixes it.  Any ideas?  

Regards
Yunjian

err log:
log_odp_key_attributes[4709]|93701|odp_util(handler18)|: present but not expected: ipv4 icmp: skb_priority(0),tunnel(ttl=0,flags(0)),skb_mark(0xffffffff),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0xffffffff),dp_hash(0),in_port(4294967295),eth(src=ff:ff:ff:ff:ff:ff,dst=ff:ff:ff:ff:ff:ff),eth_type(0xffff),vlan(vid=4095,pcp=7),encap(eth_type(0xffff),ipv4(src=255.255.0.0,dst=0.0.0.0,proto=0,tos=0,ttl=0,frag=<error>),icmp(type=0,code=0)

lib/odp-util.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 238 insertions(+), 1 deletion(-)

Comments

0-day Robot Jan. 15, 2019, 1:57 p.m. | #1
Bleep bloop.  Greetings wangyunjian, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
fatal: corrupt patch at line 7
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 a problem the userspace datapath failed to create a new datapath flow for the QinQ packet
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Ben Pfaff Jan. 15, 2019, 5:09 p.m. | #2
On Mon, Jan 14, 2019 at 12:12:34PM +0000, wangyunjian wrote:
> Hi,
> 
> I found a problem the userspace datapath failed to create a 
> new datapath flow for the QinQ packet. When getting mask from
> nlattrs with the mask include ip,udp,etc, it parsed 802.1Q
> header wrongly.
> 
> This patch fixes it.  Any ideas?  

Thanks for working to fix the problem.  This introduces a lot of
redundant code.  Would you mind refactoring so that there is less code
duplication?

Patch

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 1e8c5f1..7ad8cc6 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6322,6 +6322,240 @@  parse_ethertype(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
     return true;
}

+static bool
+parse_l2_5(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
+                  uint64_t present_attrs, uint64_t *expected_attrs,
+                  struct flow *flow, const struct flow *src_flow)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    bool is_mask = src_flow != flow;
+    const void *check_start = NULL;
+    size_t check_len = 0;
+    enum ovs_key_attr expected_bit = 0xff;
+
+    if (eth_type_mpls(src_flow->dl_type)) {
+        if (!is_mask || present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS)) {
+            *expected_attrs |= (UINT64_C(1) << OVS_KEY_ATTR_MPLS);
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_MPLS)) {
+            size_t size = nl_attr_get_size(attrs[OVS_KEY_ATTR_MPLS]);
+            const ovs_be32 *mpls_lse = nl_attr_get(attrs[OVS_KEY_ATTR_MPLS]);
+            int n = size / sizeof(ovs_be32);
+            int i;
+
+            if (!size || size % sizeof(ovs_be32)) {
+                return false;
+            }
+            if (flow->mpls_lse[0] && flow->dl_type != htons(0xffff)) {
+                return false;
+            }
+
+            for (i = 0; i < n && i < FLOW_MAX_MPLS_LABELS; i++) {
+                flow->mpls_lse[i] = mpls_lse[i];
+            }
+            if (n > FLOW_MAX_MPLS_LABELS) {
+                return false;
+            }
+
+            if (!is_mask) {
+                /* BOS may be set only in the innermost label. */
+                for (i = 0; i < n - 1; i++) {
+                    if (flow->mpls_lse[i] & htonl(MPLS_BOS_MASK)) {
+                        return false;
+                    }
+                }
+
+                /* BOS must be set in the innermost label. */
+                if (n < FLOW_MAX_MPLS_LABELS
+                    && !(flow->mpls_lse[n - 1] & htonl(MPLS_BOS_MASK))) {
+                    return false;
+                }
+            }
+        }
+
+        return true;
+    } else if (src_flow->dl_type == htons(ETH_TYPE_IP)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV4;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV4)) {
+            const struct ovs_key_ipv4 *ipv4_key;
+
+            ipv4_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV4]);
+            put_ipv4_key(ipv4_key, flow, is_mask);
+            if (flow->nw_frag > FLOW_NW_FRAG_MASK) {
+                return false;
+            }
+            if (is_mask) {
+                check_start = ipv4_key;
+                check_len = sizeof *ipv4_key;
+                expected_bit = OVS_KEY_ATTR_IPV4;
+            }
+        }
+    } else if (src_flow->dl_type == htons(ETH_TYPE_IPV6)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_IPV6;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_IPV6)) {
+            const struct ovs_key_ipv6 *ipv6_key;
+
+            ipv6_key = nl_attr_get(attrs[OVS_KEY_ATTR_IPV6]);
+            put_ipv6_key(ipv6_key, flow, is_mask);
+            if (flow->nw_frag > FLOW_NW_FRAG_MASK) {
+                return false;
+            }
+            if (is_mask) {
+                check_start = ipv6_key;
+                check_len = sizeof *ipv6_key;
+                expected_bit = OVS_KEY_ATTR_IPV6;
+            }
+        }
+    } else if (src_flow->dl_type == htons(ETH_TYPE_ARP) ||
+               src_flow->dl_type == htons(ETH_TYPE_RARP)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ARP;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ARP)) {
+            const struct ovs_key_arp *arp_key;
+
+            arp_key = nl_attr_get(attrs[OVS_KEY_ATTR_ARP]);
+            if (!is_mask && (arp_key->arp_op & htons(0xff00))) {
+                VLOG_ERR_RL(&rl, "unsupported ARP opcode %"PRIu16" in flow "
+                            "key", ntohs(arp_key->arp_op));
+                return false;
+            }
+            put_arp_key(arp_key, flow);
+            if (is_mask) {
+                check_start = arp_key;
+                check_len = sizeof *arp_key;
+                expected_bit = OVS_KEY_ATTR_ARP;
+            }
+        }
+    } else {
+        return true;
+    }
+    if (check_len > 0) { /* Happens only when 'is_mask'. */
+        if (!is_all_zeros(check_start, check_len) &&
+            flow->dl_type != htons(0xffff)) {
+            return false;
+        } else {
+            *expected_attrs |= UINT64_C(1) << expected_bit;
+        }
+    }
+
+    expected_bit = OVS_KEY_ATTR_UNSPEC;
+    if (src_flow->nw_proto == IPPROTO_TCP
+        && (src_flow->dl_type == htons(ETH_TYPE_IP) ||
+            src_flow->dl_type == htons(ETH_TYPE_IPV6))
+        && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TCP;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_TCP)) {
+            const union ovs_key_tp *tcp_key;
+
+            tcp_key = nl_attr_get(attrs[OVS_KEY_ATTR_TCP]);
+            put_tp_key(tcp_key, flow);
+            expected_bit = OVS_KEY_ATTR_TCP;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_TCP_FLAGS)) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_TCP_FLAGS;
+            flow->tcp_flags = nl_attr_get_be16(attrs[OVS_KEY_ATTR_TCP_FLAGS]);
+        }
+    } else if (src_flow->nw_proto == IPPROTO_UDP
+               && (src_flow->dl_type == htons(ETH_TYPE_IP) ||
+                   src_flow->dl_type == htons(ETH_TYPE_IPV6))
+               && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_UDP;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_UDP)) {
+            const union ovs_key_tp *udp_key;
+
+            udp_key = nl_attr_get(attrs[OVS_KEY_ATTR_UDP]);
+            put_tp_key(udp_key, flow);
+            expected_bit = OVS_KEY_ATTR_UDP;
+        }
+    } else if (src_flow->nw_proto == IPPROTO_SCTP
+               && (src_flow->dl_type == htons(ETH_TYPE_IP) ||
+                   src_flow->dl_type == htons(ETH_TYPE_IPV6))
+               && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_SCTP;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_SCTP)) {
+            const union ovs_key_tp *sctp_key;
+
+            sctp_key = nl_attr_get(attrs[OVS_KEY_ATTR_SCTP]);
+            put_tp_key(sctp_key, flow);
+            expected_bit = OVS_KEY_ATTR_SCTP;
+        }
+    } else if (src_flow->nw_proto == IPPROTO_ICMP
+               && src_flow->dl_type == htons(ETH_TYPE_IP)
+               && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMP;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ICMP)) {
+            const struct ovs_key_icmp *icmp_key;
+
+            icmp_key = nl_attr_get(attrs[OVS_KEY_ATTR_ICMP]);
+            flow->tp_src = htons(icmp_key->icmp_type);
+            flow->tp_dst = htons(icmp_key->icmp_code);
+            expected_bit = OVS_KEY_ATTR_ICMP;
+        }
+    } else if (src_flow->nw_proto == IPPROTO_ICMPV6
+               && src_flow->dl_type == htons(ETH_TYPE_IPV6)
+               && !(src_flow->nw_frag & FLOW_NW_FRAG_LATER)) {
+        if (!is_mask) {
+            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ICMPV6;
+        }
+        if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ICMPV6)) {
+            const struct ovs_key_icmpv6 *icmpv6_key;
+
+            icmpv6_key = nl_attr_get(attrs[OVS_KEY_ATTR_ICMPV6]);
+            flow->tp_src = htons(icmpv6_key->icmpv6_type);
+            flow->tp_dst = htons(icmpv6_key->icmpv6_code);
+            expected_bit = OVS_KEY_ATTR_ICMPV6;
+            if (is_nd(src_flow, NULL)) {
+                if (!is_mask) {
+                    *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND;
+                }
+                if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ND)) {
+                    const struct ovs_key_nd *nd_key;
+
+                    nd_key = nl_attr_get(attrs[OVS_KEY_ATTR_ND]);
+                    flow->nd_target = nd_key->nd_target;
+                    flow->arp_sha = nd_key->nd_sll;
+                    flow->arp_tha = nd_key->nd_tll;
+                    if (is_mask) {
+                        /* Even though 'tp_src' and 'tp_dst' are 16 bits wide,
+                         * ICMP type and code are 8 bits wide.  Therefore, an
+                         * exact match looks like htons(0xff), not
+                         * htons(0xffff).  See xlate_wc_finish() for details.
+                         * */
+                        if (!is_all_zeros(nd_key, sizeof *nd_key) &&
+                            (flow->tp_src != htons(0xff) ||
+                             flow->tp_dst != htons(0xff))) {
+                            return false;
+                        } else {
+                            *expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ND;
+                        }
+                    }
+                }
+            }
+        }
+    }
+    if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) {
+        if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) {
+            return false;
+        } else {
+            *expected_attrs |= UINT64_C(1) << expected_bit;
+        }
+    }
+    return true;
+}
+
static enum odp_key_fitness
parse_l2_5_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                   uint64_t present_attrs, int out_of_range_attr,
@@ -6658,7 +6892,10 @@  parse_8021q_onward(const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1],
                              flow, src_flow)) {
             return ODP_FIT_ERROR;
         }
-
+        if (!parse_l2_5(attrs, present_attrs, &expected_attrs,
+                             flow, src_flow)) {
+            return ODP_FIT_ERROR;
+        }
         encaps++;
     }