On 10/13/20 8:06 PM, Han Zhou wrote: > > > On Tue, Oct 13, 2020 at 1:52 AM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: >> >> Until now, in case the ACL configuration generates openflows that have >> the same match but different actions, ovn-controller was using the >> following approach: >> 1. If the flow being added contains conjunctive actions, merge its >> actions with the already existing flow. >> 2. Otherwise, if the flow is being added incrementally >> (update_installed_flows_by_track), don't install the new flow but >> instead keep the old one. >> 3. Otherwise, (update_installed_flows_by_compare), don't install the >> new flow but instead keep the old one. >> >> Even though one can argue that having an ACL with a match that includes >> the match of another ACL is a misconfiguration, it can happen that the >> users provision OVN like this. Depending on the order of reading and >> installing the logical flows, the above operations can yield >> unpredictable results, e.g., allow specific traffic but then after >> ovn-controller is restarted (or a recompute happens) that specific >> traffic starts being dropped. >> >> A simple example of ACL configuration is: >> ovn-nbctl acl-add ls to-lport 3 '(ip4.src==10.0.0.1 || >> ip4.src==10.0.0.2) && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow >> ovn-nbctl acl-add ls to-lport 3 'ip4.src==10.0.0.1' allow >> ovn-nbctl acl-add ls to-lport 2 'arp' allow >> ovn-nbctl acl-add ls to-lport 1 'ip4' drop >> >> This is a pattern used by most CMSs: >> - define a default deny policy. >> - punch holes in the default deny policy based on user specific >> configurations. >> >> Without this commit the behavior for traffic from 10.0.0.1 to 10.0.0.5 >> is unpredictable. Depending on the order of operations traffic might be >> dropped or allowed. >> >> It's also quite hard to force the CMS to ensure that such match overlaps >> never occur. >> >> To address this issue we now ensure that when selecting the active flow >> from all desired flows refering the same installed flow we give >> precedence to "allow" over "drop" over "conjunction" flows. >> >> Essentially, less restrictive flows are always preferred over more >> restrictive flows whenever a match conflict happens. >> >> CC: Daniel Alvarez <[email protected] <mailto:[email protected]>> >> Reported-at: https://bugzilla.redhat.com/1871931 >> Signed-off-by: Dumitru Ceara <[email protected] <mailto:[email protected]>> >> --- >> v6: >> - First 3 patches of the v5 series were already applied. >> - Send patch 4 as v6 addressing Han's comments: >> - Instead of maintaining a sorted list, select active flow by walking >> the list of desired flows. >> - Add missing ovn_flow_log() call. >> v5: >> - Turn the patch into a series. >> - Address Han's comments. >> - Ensure predictable flow ordering in all cases (during incremental >> procesing >> or full recomputation). >> v4: >> - Address Han's comments: >> - make sure only flows with action conjunction are combined. >> v3: >> - Add Mark's ack. >> - Add last missing pcap check in the test. >> v2: >> - Address Han's comments: >> - Do not delete desired flow that cannot be merged, it might be >> installed later. >> - Fix typos in the commit log. >> - Update the test to check the OVS flows. >> --- >> controller/ofctrl.c | 96 +++++++++++++++++++++-- >> tests/ovn.at <http://ovn.at> | 214 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 305 insertions(+), 5 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index ba0c61c..0edb2b9 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -229,6 +229,8 @@ static struct installed_flow *installed_flow_lookup( >> static void installed_flow_destroy(struct installed_flow *); >> static struct installed_flow *installed_flow_dup(struct desired_flow *); >> static struct desired_flow *installed_flow_get_active(struct installed_flow >> *); >> +static struct desired_flow *installed_flow_select_active( >> + struct installed_flow *); >> >> static uint32_t ovn_flow_match_hash(const struct ovn_flow *); >> static char *ovn_flow_to_string(const struct ovn_flow *); >> @@ -796,6 +798,12 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype >> type) >> } >> >> static bool >> +flow_action_has_drop(const struct ovn_flow *f) >> +{ >> + return f->ofpacts_len == 0; >> +} >> + >> +static bool >> flow_action_has_conj(const struct ovn_flow *f) >> { >> const struct ofpact *a = NULL; >> @@ -822,7 +830,7 @@ link_installed_to_desired(struct installed_flow *i, > struct desired_flow *d) >> { >> d->installed_flow = i; >> ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node); >> - return installed_flow_get_active(i) == d; >> + return installed_flow_select_active(i) == d; >> } >> > Hi Dumitru, I am really sorry that I didn't make it clear enough in my last > comments. I meant that I prefer maintaining the order in the desired_refs list > in the function link_installed_to_desired() when the item is inserted, which > makes the "first item is active" more useful. I didn't want to you drop the > ordering (but just wanted to avoid the array). Otherwise I would not merge the > patch 2 & 3 but directly use the temporary patch you created. To be more > clear, > it could be something like below: >
Hi Han, I guess I misunderstood your comments indeed. I thought you didn't want to keep the list partially sorted. However, I'm now curious what your concerns were regarding my original suggestion, apart from the missing ovn_flow_log(): http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ > /* Returns true if flow a is prefered over flow b. */ > static bool > flow_compare(struct ovn_flow *a, struct ovn_flow *b) > { > ... > if (b_has_allow) { > return false; > } > if (a_has_allow) { > return true; > } > if (b_has_drop) { > return false; > } > if (a_has_drop) { > return true; > } > /* both has conjunction only, assert failure */ > OVS_NOT_REACHED(); > } > > static bool > link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) > { > d->installed = i; > bool inserted = false; > LIST_FOR_EACH(f, &i->desired_refs) { > if (flow_compare(&d->flow, &f->flow)) { > /* Insert d before f */ > ovs_list_insert(&f->installed_ref_list_node, > &d->installed_ref_list_node); > inserted = true; > break; > } > } > if (!inserted) { > ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node); > } > } > >> /* Replaces 'old_desired' with 'new_desired' in the list of desired flows >> @@ -852,6 +860,7 @@ unlink_installed_to_desired(struct installed_flow *i, > struct desired_flow *d) >> >> ovs_assert(d && d->installed_flow == i); >> ovs_list_remove(&d->installed_ref_list_node); >> + installed_flow_select_active(i); > > This is not needed if the order is always maintained in > link_installed_to_desired like above example. > >> d->installed_flow = NULL; >> return old_active == d; >> } >> @@ -1274,6 +1283,70 @@ installed_flow_get_active(struct installed_flow *f) >> return NULL; >> } >> >> +/* Walks the list of desired flows that refer 'f' and selects the least >> + * restrictive one to be used as active flow, moving it to the front of >> + * the list. >> + * >> + * Returns the new active flow. >> + */ >> +static struct desired_flow * >> +installed_flow_select_active(struct installed_flow *f) >> +{ >> + struct desired_flow *old_active = installed_flow_get_active(f); >> + struct desired_flow *active = NULL; >> + >> + /* If there is at most one desired_flow, no selection needed, return >> + * early. >> + */ >> + if (!old_active || ovs_list_is_short(&f->desired_refs)) { >> + return old_active; >> + } >> + >> + struct desired_flow *d_allow = NULL; >> + struct desired_flow *d_drop = NULL; >> + struct desired_flow *d_conj = NULL; >> + struct desired_flow *d; >> + >> + LIST_FOR_EACH (d, installed_ref_list_node, &f->desired_refs) { >> + if (flow_action_has_drop(&d->flow)) { >> + if (!d_drop) { >> + d_drop = d; >> + } >> + } else if (flow_action_has_conj(&d->flow)) { >> + /* Conjunction flows are always merged together into a single >> + * flow. There should never be more than one referring the same >> + * installed flow. >> + */ >> + ovs_assert(!d_conj); >> + d_conj = d; >> + } else { >> + if (!d_allow) { >> + d_allow = d; >> + } >> + } >> + } >> + >> + /* Give precedence to "allow" over "drop" over "conjunction" action. */ >> + if (d_allow) { >> + active = d_allow; >> + } else if (d_drop) { >> + active = d_drop; >> + } else { >> + active = d_conj; >> + } > > If there are multiple flows in the same category (e.g. allow) in the list, > this > function will select a different flow everytime, and move it to the front, No. The flows are always pushed to the back of the list. So if there already was a flow in the same category this function will still find it as "active" and won't replace it. > resulting in an unnecessary flow-mod. (But never mind, because this function > is > not needed with the above suggested change) > Right, this is not needed if we maintain a sorted list. Thanks, Dumitru >> + >> + /* There must be an active flow otherwise we should have returned >> early. */ >> + ovs_assert(active); >> + >> + /* Move the newly selected active flow to the front of the list. */ >> + if (active != old_active) { >> + ovs_list_remove(&active->installed_ref_list_node); >> + ovs_list_push_front(&f->desired_refs, >> + &active->installed_ref_list_node); >> + } >> + return active; >> +} >> + >> static struct desired_flow * >> desired_flow_lookup__(struct ovn_desired_flow_table *flow_table, >> const struct ovn_flow *target, >> @@ -1789,8 +1862,14 @@ update_installed_flows_by_compare(struct > ovn_desired_flow_table *flow_table, >> link_installed_to_desired(i, d); >> } else if (!d->installed_flow) { >> /* This is a desired_flow that conflicts with one installed >> - * previously but not linked yet. */ >> - link_installed_to_desired(i, d); >> + * previously but not linked yet. However, if this flow becomes >> + * active, e.g., it is less restrictive than the previous active >> + * flow then modify the installed flow. >> + */ >> + if (link_installed_to_desired(i, d)) { >> + installed_flow_mod(&i->flow, &d->flow, msgs); >> + ovn_flow_log(&i->flow, "updating installed (conflict)"); >> + } >> } >> } >> } >> @@ -1919,8 +1998,15 @@ update_installed_flows_by_track(struct > ovn_desired_flow_table *flow_table, >> installed_flow_mod(&i->flow, &f->flow, msgs); >> } else { >> /* Adding a new flow that conflicts with an existing >> installed >> - * flow, so just add it to the link. */ >> - link_installed_to_desired(i, f); >> + * flow, so add it to the link. If this flow becomes >> active, >> + * e.g., it is less restrictive than the previous active >> flow >> + * then modify the installed flow. >> + */ >> + if (link_installed_to_desired(i, f)) { >> + installed_flow_mod(&i->flow, &f->flow, msgs); >> + ovn_flow_log(&i->flow, >> + "updating installed (tracked conflict)"); >> + } >> } >> /* The track_list_node emptyness is used to check if the node is >> * already added to track list, so initialize it again here. */ >> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at <http://ovn.at> >> index 488fd11..9420b1e 100644 >> --- a/tests/ovn.at <http://ovn.at> >> +++ b/tests/ovn.at <http://ovn.at> >> @@ -13727,6 +13727,220 @@ grep conjunction.*conjunction.*conjunction | wc >> -l`]) >> OVN_CLEANUP([hv1]) >> AT_CLEANUP >> >> +AT_SETUP([ovn -- Superseeding ACLs with conjunction]) >> +ovn_start >> + >> +ovn-nbctl ls-add ls1 >> + >> +ovn-nbctl lsp-add ls1 ls1-lp1 \ >> +-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" >> + >> +ovn-nbctl lsp-add ls1 ls1-lp2 \ >> +-- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02" >> + >> +net_add n1 >> +sim_add hv1 >> + >> +as hv1 >> +ovs-vsctl add-br br-phys >> +ovn_attach n1 br-phys 192.168.0.1 >> +ovs-vsctl -- add-port br-int hv1-vif1 -- \ >> + set interface hv1-vif1 external-ids:iface-id=ls1-lp1 \ >> + options:tx_pcap=hv1/vif1-tx.pcap \ >> + options:rxq_pcap=hv1/vif1-rx.pcap \ >> + ofport-request=1 >> + >> +ovs-vsctl -- add-port br-int hv1-vif2 -- \ >> + set interface hv1-vif2 external-ids:iface-id=ls1-lp2 \ >> + options:tx_pcap=hv1/vif2-tx.pcap \ >> + options:rxq_pcap=hv1/vif2-rx.pcap \ >> + ofport-request=2 >> + >> +# test_ip INPORT SRC_MAC DST_MAC SRC_IP DST_IP OUTPORT... >> +# >> +# This shell function causes an ip packet to be received on INPORT. >> +# The packet's content has Ethernet destination DST and source SRC >> +# (each exactly 12 hex digits) and Ethernet type ETHTYPE (4 hex digits). >> +# The OUTPORTs (zero or more) list the VIFs on which the packet should >> +# be received. INPORT and the OUTPORTs are specified as logical switch >> +# port numbers, e.g. 11 for vif11. >> +test_ip() { >> + # This packet has bad checksums but logical L3 routing doesn't check. >> + local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 >> + local packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}\ >> +${dst_ip}0035111100080000 >> + shift; shift; shift; shift; shift >> + as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet >> + for outport; do >> + echo $packet >> $outport.expected >> + done >> +} >> + >> +ip_to_hex() { >> + printf "%02x%02x%02x%02x" "$@" >> +} >> + >> +reset_pcap_file() { >> + local iface=$1 >> + local pcap_file=$2 >> + ovs-vsctl -- set Interface $iface options:tx_pcap=dummy-tx.pcap \ >> +options:rxq_pcap=dummy-rx.pcap >> + rm -f ${pcap_file}*.pcap >> + ovs-vsctl -- set Interface $iface options:tx_pcap=${pcap_file}-tx.pcap \ >> +options:rxq_pcap=${pcap_file}-rx.pcap >> +} >> + >> +# Add a default deny ACL and an allow ACL for specific IP traffic. >> +ovn-nbctl acl-add ls1 to-lport 2 'arp' allow >> +ovn-nbctl acl-add ls1 to-lport 1 'ip4' drop >> +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) > && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow >> +ovn-nbctl acl-add ls1 to-lport 3 '(ip4.src==10.0.0.1 || ip4.src==10.0.0.42) > && (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow >> +ovn-nbctl --wait=hv sync >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. >> +for src in `seq 1 2`; do >> + for dst in `seq 3 4`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + dip=`ip_to_hex 10 0 0 $dst` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + done >> +done >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. >> +dip=`ip_to_hex 10 0 0 5` >> +for src in `seq 1 2`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip >> +done >> + >> +cat 2.expected > expout >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in <http://ovs-pcap.in>" > hv1/vif2-tx.pcap > 2.packets >> +AT_CHECK([cat 2.packets], [0], [expout]) >> +reset_pcap_file hv1-vif2 hv1/vif2 >> +rm -f 2.packets >> +> 2.expected >> + >> +# Add two less restrictive allow ACLs for src IP 10.0.0.1. >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' >> allow >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow >> +ovn-nbctl --wait=hv sync >> + >> +# Check OVS flows, the less restrictive flows should have been installed. >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. >> +for src in `seq 1 2`; do >> + for dst in `seq 3 4`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + dip=`ip_to_hex 10 0 0 $dst` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + done >> +done >> + >> +# Traffic 10.0.0.2 -> 10.0.0.5 should be dropped. >> +sip=`ip_to_hex 10 0 0 2` >> +dip=`ip_to_hex 10 0 0 5` >> +test_ip 1 f00000000001 f00000000002 $sip $dip >> + >> +# Traffic 10.0.0.1 -> 10.0.0.5 should be allowed. >> +sip=`ip_to_hex 10 0 0 1` >> +dip=`ip_to_hex 10 0 0 5` >> +test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + >> +cat 2.expected > expout >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in <http://ovs-pcap.in>" > hv1/vif2-tx.pcap > 2.packets >> +AT_CHECK([cat 2.packets], [0], [expout]) >> +reset_pcap_file hv1-vif2 hv1/vif2 >> +rm -f 2.packets >> +> 2.expected >> + >> +#sleep infinity >> + >> +# Remove the first less restrictive allow ACL. >> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1 || ip4.src==10.0.0.1' >> +ovn-nbctl --wait=hv sync >> + >> +# Check OVS flows, the second less restrictive allow ACL should have been > installed. >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +# Remove the less restrictive allow ACL. >> +ovn-nbctl acl-del ls1 to-lport 3 'ip4.src==10.0.0.1' >> +ovn-nbctl --wait=hv sync >> + >> +# Check OVS flows, the 10.0.0.1 conjunction should have been reinstalled. >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 > actions=conjunction(2,2/2),conjunction(3,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.3, 10.0.0.4 should be allowed. >> +for src in `seq 1 2`; do >> + for dst in `seq 3 4`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + dip=`ip_to_hex 10 0 0 $dst` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip 2 >> + done >> +done >> + >> +# Traffic 10.0.0.1, 10.0.0.2 -> 10.0.0.5 should be dropped. >> +dip=`ip_to_hex 10 0 0 5` >> +for src in `seq 1 2`; do >> + sip=`ip_to_hex 10 0 0 $src` >> + >> + test_ip 1 f00000000001 f00000000002 $sip $dip >> +done >> + >> +cat 2.expected > expout >> +$PYTHON "$ovs_srcdir/utilities/ovs-pcap.in <http://ovs-pcap.in>" > hv1/vif2-tx.pcap > 2.packets >> +AT_CHECK([cat 2.packets], [0], [expout]) >> + >> +# Re-add the less restrictive allow ACL for src IP 10.0.0.1 >> +ovn-nbctl acl-add ls1 to-lport 3 'ip4.src==10.0.0.1' allow >> +ovn-nbctl --wait=hv sync >> + >> +# Check OVS flows, the less restrictive flows should have been installed. >> +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | \ >> + grep "priority=1003" | awk '{print $7 " " $8}' | sort], [0], [dnl >> +priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,1/2),conjunction(3,1/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=resubmit(,46) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2) >> +priority=1003,ip,metadata=0x1,nw_src=10.0.0.42 actions=conjunction(3,2/2) >> +]) >> + >> +OVN_CLEANUP([hv1]) >> +AT_CLEANUP >> + >> # 3 hypervisors, one logical switch, 3 logical ports per hypervisor >> AT_SETUP([ovn -- L2 Drop and Allow ACL w/ Stateful ACL]) >> ovn_start >> -- >> 1.8.3.1 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
