odp-util: Fix clearing match mask if set action is partially unnecessary.

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 were 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)

Sending 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 must be dropped.

Fix that by updating the match mask with only the new bits set by
commit(), but keeping those that were cleared (OR operation).

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
Acked-by: Eli Britstein <elibr@mellanox.com>
Tested-by: Adrián Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

4 files changed, 88 insertions, 16 deletions

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 011db9ebb..e54a78b43 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7701,6 +7701,28 @@ struct offsetof_sizeof {
     int size;
 };
 
+
+/* Performs bitwise OR over the fields in 'dst_' and 'src_' specified in
+ * 'offsetof_sizeof_arr' array.  Result is stored in 'dst_'. */
+static void
+or_masks(void *dst_, const void *src_,
+         struct offsetof_sizeof *offsetof_sizeof_arr)
+{
+    int field, size, offset;
+    const uint8_t *src = src_;
+    uint8_t *dst = dst_;
+
+    for (field = 0; ; field++) {
+        size   = offsetof_sizeof_arr[field].size;
+        offset = offsetof_sizeof_arr[field].offset;
+
+        if (!size) {
+            return;
+        }
+        or_bytes(dst + offset, src + offset, size);
+    }
+}
+
 /* Compares each of the fields in 'key0' and 'key1'.  The fields are specified
  * in 'offsetof_sizeof_arr', which is an array terminated by a 0-size field.
  * Returns true if all of the fields are equal, false if at least one differs.
@@ -7779,9 +7801,10 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
                         struct flow_wildcards *wc,
                         bool use_masked)
 {
-    struct ovs_key_ethernet key, base, mask;
+    struct ovs_key_ethernet key, base, mask, orig_mask;
     struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] =
         OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR;
+
     if (flow->packet_type != htonl(PT_ETH)) {
         return;
     }
@@ -7789,11 +7812,13 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow,
     get_ethernet_key(flow, &key);
     get_ethernet_key(base_flow, &base);
     get_ethernet_key(&wc->masks, &mask);
+    memcpy(&orig_mask, &mask, sizeof mask);
 
     if (commit(OVS_KEY_ATTR_ETHERNET, use_masked,
                &key, &base, &mask, sizeof key,
                ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
         put_ethernet_key(&base, base_flow);
+        or_masks(&mask, &orig_mask, ovs_key_ethernet_offsetof_sizeof_arr);
         put_ethernet_key(&mask, &wc->masks);
     }
 }
@@ -7917,7 +7942,7 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow,
                        struct ofpbuf *odp_actions, struct flow_wildcards *wc,
                        bool use_masked)
 {
-    struct ovs_key_ipv4 key, mask, base;
+    struct ovs_key_ipv4 key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_ipv4_offsetof_sizeof_arr[] =
         OVS_KEY_IPV4_OFFSETOF_SIZEOF_ARR;
 
@@ -7928,6 +7953,7 @@ commit_set_ipv4_action(const struct flow *flow, struct flow *base_flow,
     get_ipv4_key(flow, &key, false);
     get_ipv4_key(base_flow, &base, false);
     get_ipv4_key(&wc->masks, &mask, true);
+    memcpy(&orig_mask, &mask, sizeof mask);
     mask.ipv4_proto = 0;        /* Not writeable. */
     mask.ipv4_frag = 0;         /* Not writable. */
 
@@ -7939,9 +7965,8 @@ 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)) {
         put_ipv4_key(&base, base_flow, false);
-        if (mask.ipv4_proto != 0) { /* Mask was changed by commit(). */
-            put_ipv4_key(&mask, &wc->masks, true);
-        }
+        or_masks(&mask, &orig_mask, ovs_key_ipv4_offsetof_sizeof_arr);
+        put_ipv4_key(&mask, &wc->masks, true);
    }
 }
 
@@ -7974,7 +7999,7 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow,
                        struct ofpbuf *odp_actions, struct flow_wildcards *wc,
                        bool use_masked)
 {
-    struct ovs_key_ipv6 key, mask, base;
+    struct ovs_key_ipv6 key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_ipv6_offsetof_sizeof_arr[] =
         OVS_KEY_IPV6_OFFSETOF_SIZEOF_ARR;
 
@@ -7985,6 +8010,7 @@ commit_set_ipv6_action(const struct flow *flow, struct flow *base_flow,
     get_ipv6_key(flow, &key, false);
     get_ipv6_key(base_flow, &base, false);
     get_ipv6_key(&wc->masks, &mask, true);
+    memcpy(&orig_mask, &mask, sizeof mask);
     mask.ipv6_proto = 0;        /* Not writeable. */
     mask.ipv6_frag = 0;         /* Not writable. */
     mask.ipv6_label &= htonl(IPV6_LABEL_MASK); /* Not writable. */
@@ -7997,9 +8023,8 @@ 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)) {
         put_ipv6_key(&base, base_flow, false);
-        if (mask.ipv6_proto != 0) { /* Mask was changed by commit(). */
-            put_ipv6_key(&mask, &wc->masks, true);
-        }
+        or_masks(&mask, &orig_mask, ovs_key_ipv6_offsetof_sizeof_arr);
+        put_ipv6_key(&mask, &wc->masks, true);
     }
 }
 
@@ -8031,17 +8056,19 @@ static enum slow_path_reason
 commit_set_arp_action(const struct flow *flow, struct flow *base_flow,
                       struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
-    struct ovs_key_arp key, mask, base;
+    struct ovs_key_arp key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_arp_offsetof_sizeof_arr[] =
         OVS_KEY_ARP_OFFSETOF_SIZEOF_ARR;
 
     get_arp_key(flow, &key);
     get_arp_key(base_flow, &base);
     get_arp_key(&wc->masks, &mask);
+    memcpy(&orig_mask, &mask, sizeof mask);
 
     if (commit(OVS_KEY_ATTR_ARP, true, &key, &base, &mask, sizeof key,
                ovs_key_arp_offsetof_sizeof_arr, odp_actions)) {
         put_arp_key(&base, base_flow);
+        or_masks(&mask, &orig_mask, ovs_key_arp_offsetof_sizeof_arr);
         put_arp_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8068,7 +8095,7 @@ static enum slow_path_reason
 commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
                        struct ofpbuf *odp_actions, struct flow_wildcards *wc)
 {
-    struct ovs_key_icmp key, mask, base;
+    struct ovs_key_icmp key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_icmp_offsetof_sizeof_arr[] =
         OVS_KEY_ICMP_OFFSETOF_SIZEOF_ARR;
     enum ovs_key_attr attr;
@@ -8084,10 +8111,12 @@ commit_set_icmp_action(const struct flow *flow, struct flow *base_flow,
     get_icmp_key(flow, &key);
     get_icmp_key(base_flow, &base);
     get_icmp_key(&wc->masks, &mask);
+    memcpy(&orig_mask, &mask, sizeof mask);
 
     if (commit(attr, false, &key, &base, &mask, sizeof key,
                ovs_key_icmp_offsetof_sizeof_arr, odp_actions)) {
         put_icmp_key(&base, base_flow);
+        or_masks(&mask, &orig_mask, ovs_key_icmp_offsetof_sizeof_arr);
         put_icmp_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8135,17 +8164,19 @@ commit_set_nd_action(const struct flow *flow, struct flow *base_flow,
                      struct ofpbuf *odp_actions,
                      struct flow_wildcards *wc, bool use_masked)
 {
-    struct ovs_key_nd key, mask, base;
+    struct ovs_key_nd key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_nd_offsetof_sizeof_arr[] =
         OVS_KEY_ND_OFFSETOF_SIZEOF_ARR;
 
     get_nd_key(flow, &key);
     get_nd_key(base_flow, &base);
     get_nd_key(&wc->masks, &mask);
+    memcpy(&orig_mask, &mask, sizeof mask);
 
     if (commit(OVS_KEY_ATTR_ND, use_masked, &key, &base, &mask, sizeof key,
                ovs_key_nd_offsetof_sizeof_arr, odp_actions)) {
         put_nd_key(&base, base_flow);
+        or_masks(&mask, &orig_mask, ovs_key_nd_offsetof_sizeof_arr);
         put_nd_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8159,18 +8190,20 @@ commit_set_nd_extensions_action(const struct flow *flow,
                                 struct ofpbuf *odp_actions,
                                 struct flow_wildcards *wc, bool use_masked)
 {
-    struct ovs_key_nd_extensions key, mask, base;
+    struct ovs_key_nd_extensions key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_nd_extensions_offsetof_sizeof_arr[] =
         OVS_KEY_ND_EXTENSIONS_OFFSETOF_SIZEOF_ARR;
 
     get_nd_extensions_key(flow, &key);
     get_nd_extensions_key(base_flow, &base);
     get_nd_extensions_key(&wc->masks, &mask);
+    memcpy(&orig_mask, &mask, sizeof mask);
 
     if (commit(OVS_KEY_ATTR_ND_EXTENSIONS, use_masked, &key, &base, &mask,
                sizeof key, ovs_key_nd_extensions_offsetof_sizeof_arr,
                odp_actions)) {
         put_nd_extensions_key(&base, base_flow);
+        or_masks(&mask, &orig_mask, ovs_key_nd_extensions_offsetof_sizeof_arr);
         put_nd_extensions_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8385,7 +8418,7 @@ commit_set_port_action(const struct flow *flow, struct flow *base_flow,
                        bool use_masked)
 {
     enum ovs_key_attr key_type;
-    union ovs_key_tp key, mask, base;
+    union ovs_key_tp key, mask, orig_mask, base;
     struct offsetof_sizeof ovs_key_tp_offsetof_sizeof_arr[] =
         OVS_KEY_TCP_OFFSETOF_SIZEOF_ARR;
 
@@ -8411,10 +8444,12 @@ commit_set_port_action(const struct flow *flow, struct flow *base_flow,
     get_tp_key(flow, &key);
     get_tp_key(base_flow, &base);
     get_tp_key(&wc->masks, &mask);
+    memcpy(&orig_mask, &mask, sizeof mask);
 
     if (commit(key_type, use_masked, &key, &base, &mask, sizeof key,
                ovs_key_tp_offsetof_sizeof_arr, odp_actions)) {
         put_tp_key(&base, base_flow);
+        or_masks(&mask, &orig_mask, ovs_key_tp_offsetof_sizeof_arr);
         put_tp_key(&mask, &wc->masks);
     }
 }
@@ -8438,7 +8473,7 @@ commit_set_priority_action(const struct flow *flow, struct flow *base_flow,
     if (commit(OVS_KEY_ATTR_PRIORITY, use_masked, &key, &base, &mask,
                sizeof key, ovs_key_prio_offsetof_sizeof_arr, odp_actions)) {
         base_flow->skb_priority = base;
-        wc->masks.skb_priority = mask;
+        wc->masks.skb_priority |= mask;
     }
 }
 
@@ -8462,7 +8497,7 @@ commit_set_pkt_mark_action(const struct flow *flow, struct flow *base_flow,
                sizeof key, ovs_key_pkt_mark_offsetof_sizeof_arr,
                odp_actions)) {
         base_flow->pkt_mark = base;
-        wc->masks.pkt_mark = mask;
+        wc->masks.pkt_mark |= mask;
     }
 }
 
diff --git a/lib/util.c b/lib/util.c
index 830e14516..25635b27f 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -1395,6 +1395,19 @@ is_all_ones(const void *p, size_t n)
     return is_all_byte(p, n, 0xff);
 }
 
+/* *dst |= *src for 'n' bytes. */
+void
+or_bytes(void *dst_, const void *src_, size_t n)
+{
+    const uint8_t *src = src_;
+    uint8_t *dst = dst_;
+    size_t i;
+
+    for (i = 0; i < n; i++) {
+        *dst++ |= *src++;
+    }
+}
+
 /* Copies 'n_bits' bits starting from bit 'src_ofs' in 'src' to the 'n_bits'
  * starting from bit 'dst_ofs' in 'dst'.  'src' is 'src_len' bytes long and
  * 'dst' is 'dst_len' bytes long.
diff --git a/lib/util.h b/lib/util.h
index 7ad8758fe..067dcad15 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -484,6 +484,7 @@ be64_is_superset(ovs_be64 super, ovs_be64 sub)
 bool is_all_zeros(const void *, size_t);
 bool is_all_ones(const void *, size_t);
 bool is_all_byte(const void *, size_t, uint8_t byte);
+void or_bytes(void *dst, const void *src, size_t n);
 void bitwise_copy(const void *src, unsigned int src_len, unsigned int src_ofs,
                   void *dst, unsigned int dst_len, unsigned int dst_ofs,
                   unsigned int n_bits);
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"])])