From patchwork Mon Jul 27 18:58:48 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1337233 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.136; helo=silver.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BFpxn6nYJz9sR4 for ; Tue, 28 Jul 2020 04:59:09 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 6464F2076B; Mon, 27 Jul 2020 18:59:08 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nswDNTApcll3; Mon, 27 Jul 2020 18:59:04 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 5B83F1FEF0; Mon, 27 Jul 2020 18:59:04 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 4D747C004F; Mon, 27 Jul 2020 18:59:04 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 79A00C004D for ; Mon, 27 Jul 2020 18:59:03 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 68BEE81B19 for ; Mon, 27 Jul 2020 18:59:03 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id H6UG1uXh5kMn for ; Mon, 27 Jul 2020 18:59:02 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [217.70.183.194]) by fraxinus.osuosl.org (Postfix) with ESMTPS id 96177858F7 for ; Mon, 27 Jul 2020 18:59:01 +0000 (UTC) X-Originating-IP: 90.177.210.238 Received: from im-t490s.redhat.com (238.210.broadband10.iol.cz [90.177.210.238]) (Authenticated sender: i.maximets@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id ACC4940007; Mon, 27 Jul 2020 18:58:54 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org, Ben Pfaff Date: Mon, 27 Jul 2020 20:58:48 +0200 Message-Id: <20200727185848.4089473-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 Cc: Eli Britstein , Ilya Maximets Subject: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" While committing set() actions, commit() could wildcard all the fields that are same in match key and in the set action. This leads to situation where mask after commit could actually contain less bits than it was before. And if set action was partially committed, all the fields that was the same will be cleared out from the matching key resulting in the incorrect (too wide) flow. For example, for the flow that matches on both src and dst mac addresses, if the dst mac is the same and only src should be changed by the set() action, destination address will be wildcarded in the match key and will never be matched, i.e. flows with any destination mac will match, which is not correct. Setting OF rule: in_port=1,dl_src=50:54:00:00:00:09 actions=mod_dl_dst(50:54:00:00:00:0a),output(2) Sendidng following packets on port 1: 1. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800) 2. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),eth_type(0x0800) 3. eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800) Resulted datapath flows: <...> eth(dst=50:54:00:00:00:0c),<...>, actions:set(eth(dst=50:54:00:00:00:0a)),2 <...> eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2 The first flow doesn't have any match on source MAC address and the third packet successfully matched on it while it mast be dropped. Fix that by updating matching mask only if mask was expanded, but not in other cases. With fix applied, resulted correct flows are: eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2 eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),<...>, actions:set(eth(dst=50:54:00:00:00:0a)),2 eth(src=50:54:00:00:00:0b),<...>, actions:drop The code before commit dbf4a92800d0 was not able to reduce the mask, it was only possible to expand it to exact match, so it was OK to update original matching mask with the new value in all cases. Fixes: dbf4a92800d0 ("odp-util: Do not rewrite fields with the same values as matched") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1854376 Tested-by: Adrián Moreno Signed-off-by: Ilya Maximets --- Version 2: - Added simple unit test for this issue. - Added 'Tested-by' tag. - Refined comments. lib/odp-util.c | 82 ++++++++++++++++++++++++++++++++----------- tests/ofproto-dpif.at | 23 ++++++++++++ 2 files changed, 84 insertions(+), 21 deletions(-) diff --git a/lib/odp-util.c b/lib/odp-util.c index 011db9ebb..4c001c75b 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -7736,8 +7736,9 @@ static bool commit(enum ovs_key_attr attr, bool use_masked_set, const void *key, void *base, void *mask, size_t size, struct offsetof_sizeof *offsetof_sizeof_arr, - struct ofpbuf *odp_actions) + struct ofpbuf *odp_actions, bool *mask_expanded) { + *mask_expanded = false; if (keycmp_mask(key, base, offsetof_sizeof_arr, mask)) { bool fully_masked = odp_mask_is_exact(attr, mask, size); @@ -7746,6 +7747,7 @@ commit(enum ovs_key_attr attr, bool use_masked_set, } else { if (!fully_masked) { memset(mask, 0xff, size); + *mask_expanded = true; } commit_set_action(odp_actions, attr, key, size); } @@ -7782,6 +7784,8 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow, struct ovs_key_ethernet key, base, mask; struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] = OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR; + bool expanded; + if (flow->packet_type != htonl(PT_ETH)) { return; } @@ -7792,9 +7796,12 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow, if (commit(OVS_KEY_ATTR_ETHERNET, use_masked, &key, &base, &mask, sizeof key, - ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) { + ovs_key_ethernet_offsetof_sizeof_arr, odp_actions, &expanded)) { put_ethernet_key(&base, base_flow); - put_ethernet_key(&mask, &wc->masks); + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + put_ethernet_key(&mask, &wc->masks); + } } } @@ -7920,6 +7927,7 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow, struct ovs_key_ipv4 key, mask, base; struct offsetof_sizeof ovs_key_ipv4_offsetof_sizeof_arr[] = OVS_KEY_IPV4_OFFSETOF_SIZEOF_ARR; + bool expanded; /* Check that nw_proto and nw_frag remain unchanged. */ ovs_assert(flow->nw_proto == base_flow->nw_proto && @@ -7937,9 +7945,10 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow, } if (commit(OVS_KEY_ATTR_IPV4, use_masked, &key, &base, &mask, sizeof key, - ovs_key_ipv4_offsetof_sizeof_arr, odp_actions)) { + ovs_key_ipv4_offsetof_sizeof_arr, odp_actions, &expanded)) { put_ipv4_key(&base, base_flow, false); - if (mask.ipv4_proto != 0) { /* Mask was changed by commit(). */ + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ put_ipv4_key(&mask, &wc->masks, true); } } @@ -7977,6 +7986,7 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow, struct ovs_key_ipv6 key, mask, base; struct offsetof_sizeof ovs_key_ipv6_offsetof_sizeof_arr[] = OVS_KEY_IPV6_OFFSETOF_SIZEOF_ARR; + bool expanded; /* Check that nw_proto and nw_frag remain unchanged. */ ovs_assert(flow->nw_proto == base_flow->nw_proto && @@ -7995,9 +8005,10 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow, } if (commit(OVS_KEY_ATTR_IPV6, use_masked, &key, &base, &mask, sizeof key, - ovs_key_ipv6_offsetof_sizeof_arr, odp_actions)) { + ovs_key_ipv6_offsetof_sizeof_arr, odp_actions, &expanded)) { put_ipv6_key(&base, base_flow, false); - if (mask.ipv6_proto != 0) { /* Mask was changed by commit(). */ + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ put_ipv6_key(&mask, &wc->masks, true); } } @@ -8034,15 +8045,19 @@ commit_set_arp_action(const struct flow *flow, struct flow *base_flow, struct ovs_key_arp key, mask, base; struct offsetof_sizeof ovs_key_arp_offsetof_sizeof_arr[] = OVS_KEY_ARP_OFFSETOF_SIZEOF_ARR; + bool expanded; get_arp_key(flow, &key); get_arp_key(base_flow, &base); get_arp_key(&wc->masks, &mask); if (commit(OVS_KEY_ATTR_ARP, true, &key, &base, &mask, sizeof key, - ovs_key_arp_offsetof_sizeof_arr, odp_actions)) { + ovs_key_arp_offsetof_sizeof_arr, odp_actions, &expanded)) { put_arp_key(&base, base_flow); - put_arp_key(&mask, &wc->masks); + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + put_arp_key(&mask, &wc->masks); + } return SLOW_ACTION; } return 0; @@ -8072,6 +8087,7 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow, struct offsetof_sizeof ovs_key_icmp_offsetof_sizeof_arr[] = OVS_KEY_ICMP_OFFSETOF_SIZEOF_ARR; enum ovs_key_attr attr; + bool expanded; if (is_icmpv4(flow, NULL)) { attr = OVS_KEY_ATTR_ICMP; @@ -8086,9 +8102,12 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow, get_icmp_key(&wc->masks, &mask); if (commit(attr, false, &key, &base, &mask, sizeof key, - ovs_key_icmp_offsetof_sizeof_arr, odp_actions)) { + ovs_key_icmp_offsetof_sizeof_arr, odp_actions, &expanded)) { put_icmp_key(&base, base_flow); - put_icmp_key(&mask, &wc->masks); + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + put_icmp_key(&mask, &wc->masks); + } return SLOW_ACTION; } return 0; @@ -8138,15 +8157,19 @@ commit_set_nd_action(const struct flow *flow, struct flow *base_flow, struct ovs_key_nd key, mask, base; struct offsetof_sizeof ovs_key_nd_offsetof_sizeof_arr[] = OVS_KEY_ND_OFFSETOF_SIZEOF_ARR; + bool expanded; get_nd_key(flow, &key); get_nd_key(base_flow, &base); get_nd_key(&wc->masks, &mask); if (commit(OVS_KEY_ATTR_ND, use_masked, &key, &base, &mask, sizeof key, - ovs_key_nd_offsetof_sizeof_arr, odp_actions)) { + ovs_key_nd_offsetof_sizeof_arr, odp_actions, &expanded)) { put_nd_key(&base, base_flow); - put_nd_key(&mask, &wc->masks); + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + put_nd_key(&mask, &wc->masks); + } return SLOW_ACTION; } @@ -8162,6 +8185,7 @@ commit_set_nd_extensions_action(const struct flow *flow, struct ovs_key_nd_extensions key, mask, base; struct offsetof_sizeof ovs_key_nd_extensions_offsetof_sizeof_arr[] = OVS_KEY_ND_EXTENSIONS_OFFSETOF_SIZEOF_ARR; + bool expanded; get_nd_extensions_key(flow, &key); get_nd_extensions_key(base_flow, &base); @@ -8169,9 +8193,12 @@ commit_set_nd_extensions_action(const struct flow *flow, if (commit(OVS_KEY_ATTR_ND_EXTENSIONS, use_masked, &key, &base, &mask, sizeof key, ovs_key_nd_extensions_offsetof_sizeof_arr, - odp_actions)) { + odp_actions, &expanded)) { put_nd_extensions_key(&base, base_flow); - put_nd_extensions_key(&mask, &wc->masks); + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + put_nd_extensions_key(&mask, &wc->masks); + } return SLOW_ACTION; } return 0; @@ -8388,6 +8415,7 @@ commit_set_port_action(const struct flow *flow, struct flow *base_flow, union ovs_key_tp key, mask, base; struct offsetof_sizeof ovs_key_tp_offsetof_sizeof_arr[] = OVS_KEY_TCP_OFFSETOF_SIZEOF_ARR; + bool expanded; /* Check if 'flow' really has an L3 header. */ if (!flow->nw_proto) { @@ -8413,9 +8441,12 @@ commit_set_port_action(const struct flow *flow, struct flow *base_flow, get_tp_key(&wc->masks, &mask); if (commit(key_type, use_masked, &key, &base, &mask, sizeof key, - ovs_key_tp_offsetof_sizeof_arr, odp_actions)) { + ovs_key_tp_offsetof_sizeof_arr, odp_actions, &expanded)) { put_tp_key(&base, base_flow); - put_tp_key(&mask, &wc->masks); + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + put_tp_key(&mask, &wc->masks); + } } } @@ -8430,15 +8461,20 @@ commit_set_priority_action(const struct flow *flow, struct flow *base_flow, {0, sizeof(uint32_t)}, {0, 0} }; + bool expanded; key = flow->skb_priority; base = base_flow->skb_priority; mask = wc->masks.skb_priority; if (commit(OVS_KEY_ATTR_PRIORITY, use_masked, &key, &base, &mask, - sizeof key, ovs_key_prio_offsetof_sizeof_arr, odp_actions)) { + sizeof key, ovs_key_prio_offsetof_sizeof_arr, + odp_actions, &expanded)) { base_flow->skb_priority = base; - wc->masks.skb_priority = mask; + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + wc->masks.skb_priority = mask; + } } } @@ -8453,6 +8489,7 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow, {0, sizeof(uint32_t)}, {0, 0} }; + bool expanded; key = flow->pkt_mark; base = base_flow->pkt_mark; @@ -8460,9 +8497,12 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow, if (commit(OVS_KEY_ATTR_SKB_MARK, use_masked, &key, &base, &mask, sizeof key, ovs_key_pkt_mark_offsetof_sizeof_arr, - odp_actions)) { + odp_actions, &expanded)) { base_flow->pkt_mark = base; - wc->masks.pkt_mark = mask; + if (expanded) { + /* Mask expanded by commit(), need to match new fields. */ + wc->masks.pkt_mark = mask; + } } } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index feabb7380..d63ef237a 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -8914,6 +8914,29 @@ recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(dst=50:54:00:00:00:0c),eth_ty OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif megaflow - set dl_dst with match on dl_src]) +OVS_VSWITCHD_START +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg]) +add_of_ports br0 1 2 +AT_DATA([flows.txt], [dnl +table=0 in_port=1,dl_src=50:54:00:00:00:09 actions=mod_dl_dst(50:54:00:00:00:0a),output(2) +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.6,dst=10.0.0.5,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) +sleep 1 +dnl The first packet is essentially a no-op, as the new destination MAC is the +dnl same as the original. The second entry actually updates the destination +dnl MAC. The last one must be dropped as it doesn't match with dl_src. +AT_CHECK([strip_ufid < ovs-vswitchd.log | filter_flow_install | strip_used], [0], [dnl +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(frag=no), actions:2 +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(frag=no), actions:set(eth(dst=50:54:00:00:00:0a)),2 +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:0b),eth_type(0x0800),ipv4(frag=no), actions:drop +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + m4_define([OFPROTO_DPIF_MEGAFLOW_DISABLED], [AT_SETUP([ofproto-dpif megaflow - disabled$1]) OVS_VSWITCHD_START([], [], [], [m4_if([$1], [], [], [--dummy-numa="0,0,0,0,1,1,1,1"])])