| Message ID | 20201011120607.3374.49659.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ofctrl: Add a predictable resolution for conflicting flow actions. | expand |
On Sun, Oct 11, 2020 at 5:06 AM Dumitru Ceara <dceara@redhat.com> 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 all desired flows refering the same > installed flow are partially sorted in the following way: > - first all flows with action "allow". > - then all flows with action "drop". > - then a single flow with action "conjunction" (resulting from merging all > flows with the same match and action conjunction). > > This ensures that "allow" flows have precedence over "drop" flows which in > turn have precedence over "conjunction" flows. Essentially less restrictive > flows are always preferred over more restrictive flows whenever a match > conflict happens. > > CC: Daniel Alvarez <dalvarez@redhat.com> > Reported-at: https://bugzilla.redhat.com/1871931 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/ofctrl.c | 95 +++++++++++++++++++--- > tests/ovn.at | 216 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 296 insertions(+), 15 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 74f98e3..30b5667 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -172,6 +172,18 @@ struct sb_flow_ref { > struct uuid sb_uuid; > }; > > +/* Desired flow reference type based on the action of the desired flow. > + * > + * This is used to maintain a partial ordering between multiple desired > + * flows that are linked to the same installed flow. > + */ > +enum desired_flow_ref_type { > + DFLOW_REF_ALLOW, > + DFLOW_REF_DROP, > + DFLOW_REF_CONJ, > + DFLOW_REF_MAX, > +}; > + > /* A installed flow, in static variable installed_flows. > * > * Installed flows are updated in ofctrl_put for maintaining the flow > @@ -188,6 +200,14 @@ struct sb_flow_ref { > * relationship is 1 to N. A link is added when a flow addition is processed. > * A link is removed when a flow deletion is processed, the desired flow > * table is cleared, or the installed flow table is cleared. > + * > + * To ensure predictable behavior, the list of desired flows is maintained > + * partially sorted in the following way (from least restrictive to most > + * restrictive wrt. match): > + * - allow flows without action conjunction. > + * - drop flows without action conjunction. > + * - a single flow with action conjunction. > + * > * The first desired_flow in the list is the active one, the one that is > * actually installed. > */ > @@ -195,12 +215,12 @@ struct installed_flow { > struct ovn_flow flow; > struct hmap_node match_hmap_node; /* For match based hashing. */ > > - /* A list of desired ovn_flow nodes (linked by > + /* Lists of desired ovn_flow nodes (linked by > * desired_flow.installed_ref_list_node), which reference this installed > * flow. (There are cases that multiple desired flows reference the same > * installed flow, e.g. when there are conflict/duplicated ACLs that > * generates same match conditions). */ > - struct ovs_list desired_refs; > + struct ovs_list desired_refs[DFLOW_REF_MAX]; > }; > > typedef bool > @@ -797,6 +817,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 +848,18 @@ static bool > 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); > + > + if (flow_action_has_conj(&d->flow)) { > + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); > + ovs_list_push_back(&i->desired_refs[DFLOW_REF_CONJ], > + &d->installed_ref_list_node); > + } else if (flow_action_has_drop(&d->flow)) { > + ovs_list_push_back(&i->desired_refs[DFLOW_REF_DROP], > + &d->installed_ref_list_node); > + } else { > + ovs_list_push_back(&i->desired_refs[DFLOW_REF_ALLOW], > + &d->installed_ref_list_node); > + } > return installed_flow_get_active(i) == d; > } > > @@ -854,15 +891,27 @@ 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); > d->installed_flow = NULL; > + > + /* If the flow we're removing was a conjunction flow, it should have been > + * the only one referring this installed flow. > + */ > + if (flow_action_has_conj(&d->flow)) { > + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); > + } > + > return old_active == d; > } > > static void > unlink_all_refs_for_installed_flow(struct installed_flow *i) > { > - struct desired_flow *d, *next; > - LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, &i->desired_refs) { > - unlink_installed_to_desired(i, d); > + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { > + struct desired_flow *d, *next; > + > + LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, > + &i->desired_refs[rt]) { > + unlink_installed_to_desired(i, d); > + } > } > } > > @@ -1253,7 +1302,10 @@ static struct installed_flow * > installed_flow_dup(struct desired_flow *src) > { > struct installed_flow *dst = xmalloc(sizeof *dst); > - ovs_list_init(&dst->desired_refs); > + > + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { > + ovs_list_init(&dst->desired_refs[rt]); > + } > dst->flow.table_id = src->flow.table_id; > dst->flow.priority = src->flow.priority; > minimatch_clone(&dst->flow.match, &src->flow.match); > @@ -1267,10 +1319,12 @@ installed_flow_dup(struct desired_flow *src) > static struct desired_flow * > installed_flow_get_active(struct installed_flow *f) > { > - if (!ovs_list_is_empty(&f->desired_refs)) { > - return CONTAINER_OF(ovs_list_front(&f->desired_refs), > - struct desired_flow, > - installed_ref_list_node); > + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { > + if (!ovs_list_is_empty(&f->desired_refs[rt])) { > + return CONTAINER_OF(ovs_list_front(&f->desired_refs[rt]), > + struct desired_flow, > + installed_ref_list_node); > + } > } > return NULL; > } > @@ -1790,8 +1844,13 @@ 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); Hi Dumitru, it seems ovn_flow_log() is missing here for the flow mod. > + } > } > } > } > @@ -1920,8 +1979,14 @@ 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 > + * active, e.g., it is less restrictive than the previous > + * active flow then modify the installed flow. > + */ > + if (link_installed_to_desired(i, f)) { > + ovn_flow_log(&i->flow, "updating installed (tracked)"); > + installed_flow_mod(&i->flow, &f->flow, msgs); > + } > } > /* 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 b/tests/ovn.at > index 6f1ab59..8202f38 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -13727,6 +13727,222 @@ 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 > +} > + > +as hv1 ovn-appctl -t ovn-controller vlog/set ofctrl:dbg > + > +# 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" 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" 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" 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 >
On 10/13/20 9:23 AM, Han Zhou wrote: > On Sun, Oct 11, 2020 at 5:06 AM Dumitru Ceara <dceara@redhat.com> 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 all desired flows refering the > same >> installed flow are partially sorted in the following way: >> - first all flows with action "allow". >> - then all flows with action "drop". >> - then a single flow with action "conjunction" (resulting from merging all >> flows with the same match and action conjunction). >> >> This ensures that "allow" flows have precedence over "drop" flows which in >> turn have precedence over "conjunction" flows. Essentially less > restrictive >> flows are always preferred over more restrictive flows whenever a match >> conflict happens. >> >> CC: Daniel Alvarez <dalvarez@redhat.com> >> Reported-at: https://bugzilla.redhat.com/1871931 >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> controller/ofctrl.c | 95 +++++++++++++++++++--- >> tests/ovn.at | 216 > +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 296 insertions(+), 15 deletions(-) >> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c >> index 74f98e3..30b5667 100644 >> --- a/controller/ofctrl.c >> +++ b/controller/ofctrl.c >> @@ -172,6 +172,18 @@ struct sb_flow_ref { >> struct uuid sb_uuid; >> }; >> >> +/* Desired flow reference type based on the action of the desired flow. >> + * >> + * This is used to maintain a partial ordering between multiple desired >> + * flows that are linked to the same installed flow. >> + */ >> +enum desired_flow_ref_type { >> + DFLOW_REF_ALLOW, >> + DFLOW_REF_DROP, >> + DFLOW_REF_CONJ, >> + DFLOW_REF_MAX, >> +}; >> + >> /* A installed flow, in static variable installed_flows. >> * >> * Installed flows are updated in ofctrl_put for maintaining the flow >> @@ -188,6 +200,14 @@ struct sb_flow_ref { >> * relationship is 1 to N. A link is added when a flow addition is > processed. >> * A link is removed when a flow deletion is processed, the desired flow >> * table is cleared, or the installed flow table is cleared. >> + * >> + * To ensure predictable behavior, the list of desired flows is > maintained >> + * partially sorted in the following way (from least restrictive to most >> + * restrictive wrt. match): >> + * - allow flows without action conjunction. >> + * - drop flows without action conjunction. >> + * - a single flow with action conjunction. >> + * >> * The first desired_flow in the list is the active one, the one that is >> * actually installed. >> */ >> @@ -195,12 +215,12 @@ struct installed_flow { >> struct ovn_flow flow; >> struct hmap_node match_hmap_node; /* For match based hashing. */ >> >> - /* A list of desired ovn_flow nodes (linked by >> + /* Lists of desired ovn_flow nodes (linked by >> * desired_flow.installed_ref_list_node), which reference this > installed >> * flow. (There are cases that multiple desired flows reference the > same >> * installed flow, e.g. when there are conflict/duplicated ACLs that >> * generates same match conditions). */ >> - struct ovs_list desired_refs; >> + struct ovs_list desired_refs[DFLOW_REF_MAX]; >> }; >> >> typedef bool >> @@ -797,6 +817,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 +848,18 @@ static bool >> 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); >> + >> + if (flow_action_has_conj(&d->flow)) { >> + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); >> + ovs_list_push_back(&i->desired_refs[DFLOW_REF_CONJ], >> + &d->installed_ref_list_node); >> + } else if (flow_action_has_drop(&d->flow)) { >> + ovs_list_push_back(&i->desired_refs[DFLOW_REF_DROP], >> + &d->installed_ref_list_node); >> + } else { >> + ovs_list_push_back(&i->desired_refs[DFLOW_REF_ALLOW], >> + &d->installed_ref_list_node); >> + } >> return installed_flow_get_active(i) == d; >> } >> >> @@ -854,15 +891,27 @@ 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); >> d->installed_flow = NULL; >> + >> + /* If the flow we're removing was a conjunction flow, it should have > been >> + * the only one referring this installed flow. >> + */ >> + if (flow_action_has_conj(&d->flow)) { >> + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); >> + } >> + >> return old_active == d; >> } >> >> static void >> unlink_all_refs_for_installed_flow(struct installed_flow *i) >> { >> - struct desired_flow *d, *next; >> - LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, > &i->desired_refs) { >> - unlink_installed_to_desired(i, d); >> + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { >> + struct desired_flow *d, *next; >> + >> + LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, >> + &i->desired_refs[rt]) { >> + unlink_installed_to_desired(i, d); >> + } >> } >> } >> >> @@ -1253,7 +1302,10 @@ static struct installed_flow * >> installed_flow_dup(struct desired_flow *src) >> { >> struct installed_flow *dst = xmalloc(sizeof *dst); >> - ovs_list_init(&dst->desired_refs); >> + >> + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { >> + ovs_list_init(&dst->desired_refs[rt]); >> + } >> dst->flow.table_id = src->flow.table_id; >> dst->flow.priority = src->flow.priority; >> minimatch_clone(&dst->flow.match, &src->flow.match); >> @@ -1267,10 +1319,12 @@ installed_flow_dup(struct desired_flow *src) >> static struct desired_flow * >> installed_flow_get_active(struct installed_flow *f) >> { >> - if (!ovs_list_is_empty(&f->desired_refs)) { >> - return CONTAINER_OF(ovs_list_front(&f->desired_refs), >> - struct desired_flow, >> - installed_ref_list_node); >> + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { >> + if (!ovs_list_is_empty(&f->desired_refs[rt])) { >> + return CONTAINER_OF(ovs_list_front(&f->desired_refs[rt]), >> + struct desired_flow, >> + installed_ref_list_node); >> + } >> } >> return NULL; >> } >> @@ -1790,8 +1844,13 @@ 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); > > Hi Dumitru, it seems ovn_flow_log() is missing here for the flow mod. > Hi Han, Ack, thanks for spotting this, I'll fix it in v6. Regards, Dumitru >> + } >> } >> } >> } >> @@ -1920,8 +1979,14 @@ 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 >> + * active, e.g., it is less restrictive than the previous >> + * active flow then modify the installed flow. >> + */ >> + if (link_installed_to_desired(i, f)) { >> + ovn_flow_log(&i->flow, "updating installed > (tracked)"); >> + installed_flow_mod(&i->flow, &f->flow, msgs); >> + } >> } >> /* 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 b/tests/ovn.at >> index 6f1ab59..8202f38 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -13727,6 +13727,222 @@ 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 >> +} >> + >> +as hv1 ovn-appctl -t ovn-controller vlog/set ofctrl:dbg >> + >> +# 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" 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" 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" 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 >> >
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 74f98e3..30b5667 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -172,6 +172,18 @@ struct sb_flow_ref { struct uuid sb_uuid; }; +/* Desired flow reference type based on the action of the desired flow. + * + * This is used to maintain a partial ordering between multiple desired + * flows that are linked to the same installed flow. + */ +enum desired_flow_ref_type { + DFLOW_REF_ALLOW, + DFLOW_REF_DROP, + DFLOW_REF_CONJ, + DFLOW_REF_MAX, +}; + /* A installed flow, in static variable installed_flows. * * Installed flows are updated in ofctrl_put for maintaining the flow @@ -188,6 +200,14 @@ struct sb_flow_ref { * relationship is 1 to N. A link is added when a flow addition is processed. * A link is removed when a flow deletion is processed, the desired flow * table is cleared, or the installed flow table is cleared. + * + * To ensure predictable behavior, the list of desired flows is maintained + * partially sorted in the following way (from least restrictive to most + * restrictive wrt. match): + * - allow flows without action conjunction. + * - drop flows without action conjunction. + * - a single flow with action conjunction. + * * The first desired_flow in the list is the active one, the one that is * actually installed. */ @@ -195,12 +215,12 @@ struct installed_flow { struct ovn_flow flow; struct hmap_node match_hmap_node; /* For match based hashing. */ - /* A list of desired ovn_flow nodes (linked by + /* Lists of desired ovn_flow nodes (linked by * desired_flow.installed_ref_list_node), which reference this installed * flow. (There are cases that multiple desired flows reference the same * installed flow, e.g. when there are conflict/duplicated ACLs that * generates same match conditions). */ - struct ovs_list desired_refs; + struct ovs_list desired_refs[DFLOW_REF_MAX]; }; typedef bool @@ -797,6 +817,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 +848,18 @@ static bool 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); + + if (flow_action_has_conj(&d->flow)) { + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); + ovs_list_push_back(&i->desired_refs[DFLOW_REF_CONJ], + &d->installed_ref_list_node); + } else if (flow_action_has_drop(&d->flow)) { + ovs_list_push_back(&i->desired_refs[DFLOW_REF_DROP], + &d->installed_ref_list_node); + } else { + ovs_list_push_back(&i->desired_refs[DFLOW_REF_ALLOW], + &d->installed_ref_list_node); + } return installed_flow_get_active(i) == d; } @@ -854,15 +891,27 @@ 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); d->installed_flow = NULL; + + /* If the flow we're removing was a conjunction flow, it should have been + * the only one referring this installed flow. + */ + if (flow_action_has_conj(&d->flow)) { + ovs_assert(ovs_list_is_empty(&i->desired_refs[DFLOW_REF_CONJ])); + } + return old_active == d; } static void unlink_all_refs_for_installed_flow(struct installed_flow *i) { - struct desired_flow *d, *next; - LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, &i->desired_refs) { - unlink_installed_to_desired(i, d); + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { + struct desired_flow *d, *next; + + LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node, + &i->desired_refs[rt]) { + unlink_installed_to_desired(i, d); + } } } @@ -1253,7 +1302,10 @@ static struct installed_flow * installed_flow_dup(struct desired_flow *src) { struct installed_flow *dst = xmalloc(sizeof *dst); - ovs_list_init(&dst->desired_refs); + + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { + ovs_list_init(&dst->desired_refs[rt]); + } dst->flow.table_id = src->flow.table_id; dst->flow.priority = src->flow.priority; minimatch_clone(&dst->flow.match, &src->flow.match); @@ -1267,10 +1319,12 @@ installed_flow_dup(struct desired_flow *src) static struct desired_flow * installed_flow_get_active(struct installed_flow *f) { - if (!ovs_list_is_empty(&f->desired_refs)) { - return CONTAINER_OF(ovs_list_front(&f->desired_refs), - struct desired_flow, - installed_ref_list_node); + for (enum desired_flow_ref_type rt = 0; rt < DFLOW_REF_MAX; rt++) { + if (!ovs_list_is_empty(&f->desired_refs[rt])) { + return CONTAINER_OF(ovs_list_front(&f->desired_refs[rt]), + struct desired_flow, + installed_ref_list_node); + } } return NULL; } @@ -1790,8 +1844,13 @@ 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); + } } } } @@ -1920,8 +1979,14 @@ 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 + * active, e.g., it is less restrictive than the previous + * active flow then modify the installed flow. + */ + if (link_installed_to_desired(i, f)) { + ovn_flow_log(&i->flow, "updating installed (tracked)"); + installed_flow_mod(&i->flow, &f->flow, msgs); + } } /* 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 b/tests/ovn.at index 6f1ab59..8202f38 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -13727,6 +13727,222 @@ 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 +} + +as hv1 ovn-appctl -t ovn-controller vlog/set ofctrl:dbg + +# 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" 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" 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" 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
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 all desired flows refering the same installed flow are partially sorted in the following way: - first all flows with action "allow". - then all flows with action "drop". - then a single flow with action "conjunction" (resulting from merging all flows with the same match and action conjunction). This ensures that "allow" flows have precedence over "drop" flows which in turn have precedence over "conjunction" flows. Essentially less restrictive flows are always preferred over more restrictive flows whenever a match conflict happens. CC: Daniel Alvarez <dalvarez@redhat.com> Reported-at: https://bugzilla.redhat.com/1871931 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/ofctrl.c | 95 +++++++++++++++++++--- tests/ovn.at | 216 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 296 insertions(+), 15 deletions(-)