| Message ID | 20200727185848.4089473-1-i.maximets@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] odp-util: Fix clearing match mask if set action is partially unnecessary. | expand |
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' > 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". > 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'. > > 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@redhat.com> > Signed-off-by: Ilya Maximets <i.maximets@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")? > { > + *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"])])
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@redhat.com> >> Signed-off-by: Ilya Maximets <i.maximets@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.
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(). Right. My mistake before. > >>> match key and will never be matched, i.e. flows with any destination >>> >>> 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? That was my thought too when I did that work. For that, I introduced the generic 'struct offsetof_sizeof' array, and wildcarding the mask code is mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it through) we can leverage the generic "commit" function that already gets that array to do the "ORs". This way we will do only for the fields and not tough the "holes". >>> >>> Best regards, Ilya Maximets.
On 7/28/20 4:16 PM, Eli Britstein wrote: > > 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(). > Right. My mistake before. >> >>>> match key and will never be matched, i.e. flows with any destination >>>> >>>> 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? > That was my thought too when I did that work. For that, I introduced the generic 'struct offsetof_sizeof' array, and wildcarding the mask code is mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it through) we can leverage the generic "commit" function that already gets that array to do the "ORs". This way we will do only for the fields and not tough the "holes". Yes, that a good point. What about incremental change like this (incremental to the previous diff I sent): --- diff --git a/lib/odp-util.c b/lib/odp-util.c index 364a6fbe1..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. @@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow, &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); + or_masks(&mask, &orig_mask, ovs_key_ethernet_offsetof_sizeof_arr); put_ethernet_key(&mask, &wc->masks); } } --- And the same for all other callers of or_bytes(). What do you think? Best regards, Ilya Maximets.
On 7/28/2020 7:32 PM, Ilya Maximets wrote: > On 7/28/20 4:16 PM, Eli Britstein wrote: >> 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(). >> Right. My mistake before. >>>>> match key and will never be matched, i.e. flows with any destination >>>>> >>>>> 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? >> That was my thought too when I did that work. For that, I introduced the generic 'struct offsetof_sizeof' array, and wildcarding the mask code is mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it through) we can leverage the generic "commit" function that already gets that array to do the "ORs". This way we will do only for the fields and not tough the "holes". > Yes, that a good point. What about incremental change like this > (incremental to the previous diff I sent): > > --- > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 364a6fbe1..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. > @@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow, > &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); > + or_masks(&mask, &orig_mask, ovs_key_ethernet_offsetof_sizeof_arr); > put_ethernet_key(&mask, &wc->masks); > } > } > --- > > And the same for all other callers of or_bytes(). > > What do you think? Seems OK. Can you please post v3? > > Best regards, Ilya Maximets.
On 7/29/20 6:55 AM, Eli Britstein wrote: > > On 7/28/2020 7:32 PM, Ilya Maximets wrote: >> On 7/28/20 4:16 PM, Eli Britstein wrote: >>> 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(). >>> Right. My mistake before. >>>>>> match key and will never be matched, i.e. flows with any destination >>>>>> >>>>>> 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? >>> That was my thought too when I did that work. For that, I introduced the generic 'struct offsetof_sizeof' array, and wildcarding the mask code is mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it through) we can leverage the generic "commit" function that already gets that array to do the "ORs". This way we will do only for the fields and not tough the "holes". >> Yes, that a good point. What about incremental change like this >> (incremental to the previous diff I sent): >> >> --- >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index 364a6fbe1..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. >> @@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow, struct flow *base_flow, >> &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); >> + or_masks(&mask, &orig_mask, ovs_key_ethernet_offsetof_sizeof_arr); >> put_ethernet_key(&mask, &wc->masks); >> } >> } >> --- >> >> And the same for all other callers of or_bytes(). >> >> What do you think? > Seems OK. Can you please post v3? Sure. Sent. >> >> Best regards, Ilya Maximets.
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"])])
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 <amorenoz@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@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(-)