[ovs-dev] [PATCH v3] odp-util: Fix clearing match mask if set action is partially unnecessary.
Eli Britstein
elibr at mellanox.com
Wed Jul 29 10:37:45 UTC 2020
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