[ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.

Ilya Maximets i.maximets at ovn.org
Tue Jul 28 12:26:27 UTC 2020


On 7/28/20 12:58 PM, Eli Britstein wrote:
> 
> On 7/27/2020 9:58 PM, Ilya Maximets wrote:
>> 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
> typo 'was' -> 'were'

OK.

>> 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
> it is not "wildcarded". "wildcarded" means it had a match and it was removed. the case here it was only not "expanded".

It actually had a match on a filed and that match was removed from
the original flow while committing set() action.  That is the bug that
this patch intended to fix.  See the example below.  There was a match
on source MAC, but it was removed by commit_set_ether_action().

>> 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.
> typo 'mast' -> 'must'.

OK.

>>
>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1854376&data=02%7C01%7Celibr%40mellanox.com%7C3714762423634df2a85808d8325f1e73%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637314731402645471&sdata=elvKx9rFPu8Z0b%2FVS1Rdtdq1e8B%2BJr%2FN%2BhcngcrVSxA%3D&reserved=0
>> Tested-by: Adrián Moreno <amorenoz at redhat.com>
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
>>
>> 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)
> maybe we can change the return value of "commit" to enum of 3 values instead of 2 bools (one in the return value and another in "mask_expanded")?

Adding a new argument just because it's the simplest and less intrusive
solution.  If we want to fix the issue in a completely right way we need
to have a bitwise OR between the old mask and the mew one after the
commit().  This way we will not need to have an extra argument for
commit() function.  Something like this:

------
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 011db9ebb..364a6fbe1 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7779,9 +7779,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 +7790,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_bytes(&mask, &orig_mask, sizeof mask);
         put_ethernet_key(&mask, &wc->masks);
     }
 }
@@ -7917,7 +7920,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 +7931,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 +7943,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_bytes(&mask, &orig_mask, sizeof mask);
+        put_ipv4_key(&mask, &wc->masks, true);
    }
 }
 
@@ -7974,7 +7977,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 +7988,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 +8001,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_bytes(&mask, &orig_mask, sizeof mask);
+        put_ipv6_key(&mask, &wc->masks, true);
     }
 }
 
@@ -8031,17 +8034,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_bytes(&mask, &orig_mask, sizeof mask);
         put_arp_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8068,7 +8073,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 +8089,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_bytes(&mask, &orig_mask, sizeof mask);
         put_icmp_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8135,17 +8142,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_bytes(&mask, &orig_mask, sizeof mask);
         put_nd_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8159,18 +8168,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_bytes(&mask, &orig_mask, sizeof mask);
         put_nd_extensions_key(&mask, &wc->masks);
         return SLOW_ACTION;
     }
@@ -8385,7 +8396,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 +8422,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_bytes(&mask, &orig_mask, sizeof mask);
         put_tp_key(&mask, &wc->masks);
     }
 }
@@ -8438,7 +8451,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 +8475,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);
------

I'm a bit warried about this solution since we're not clearing out all the
masks, so memory sanitizers might bite us on that in case where will be some
holes in the datastructures even though we're ORing them, but not actually
using these uninitialized bytes.  To not use the unconditional OR we will
need to introduce new functions like 'or_ethernet_mask()' and this will grow
the code size, which doesn't look very pretty.

What do you think?

Best regards, Ilya Maximets.


More information about the dev mailing list