[ovs-dev] [PATCH v3] odp-util: Fix clearing match mask if set action is partially unnecessary.
Adrian Moreno
amorenoz at redhat.com
Wed Jul 29 13:47:57 UTC 2020
Verified that v3 also solves the issue reported.
Tested-by: Adrián Moreno <amorenoz at redhat.com>
On 7/29/20 12:37 PM, Eli Britstein wrote:
> Thanks!
>
> Acked-by: Eli Britstein <elibr at mellanox.com>
>
> On 7/29/2020 12:13 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 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1854376&data=02%7C01%7Celibr%40mellanox.com%7C7d70b87d6084436b6ff608d8339fb47b%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637316108313661481&sdata=pa3tutGAU3%2BRuRy8y2rjb7AdBouIZrbX4iAE87YXrTY%3D&reserved=0
>>
>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>> ---
>>
>> Version 3:
>> - Implemented more accurate solution with OR-ing of the original
>> and resulted masks to avoid any dependencies from the internals
>> of commit() function.
>>
>> Version 2:
>> - Added simple unit test for this issue.
>> - Added 'Tested-by' tag.
>> - Refined comments.
>>
>>
>> lib/odp-util.c | 67 ++++++++++++++++++++++++++++++++-----------
>> lib/util.c | 13 +++++++++
>> lib/util.h | 1 +
>> tests/ofproto-dpif.at | 23 +++++++++++++++
>> 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"])])
>
More information about the dev
mailing list