| Message ID | 1601663136-19111-1-git-send-email-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ofctrl.c: Fix duplicated flow handling in I-P while merging opposite changes. | expand |
On 10/2/20 8:25 PM, Han Zhou wrote: > In a scenario in I-P when a desired flow is removed and an exactly same flow is > added back in the same iteration, the merge_tracked_flows() function will merge > the operation without firing any OpenFlow action. However, if there are > multiple desired flows (e.g. F1 and F2) matching the same installed flow, and > if the one being re-added (say, F1) is the one currently installed in OVS, the > current implementation will update the currently installed flow to F2 in the > data structure while the actual OVS flow is not updated (still F1). So far > there is still no problem, but the member desired_flow of the installed flow > is pointing to the wrong desired flow. > > Now there is only one place where the desired_flow member is consulted, in > update_installed_flows_by_track() for handling a tracked *modified* flow. In > reality flow modification happens only when conjunction flows need to be > combined, which would never happen together with the above scenario of > merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any > real problem yet. > > However, there is a place that can already utilize this active desired_flow > information, which is when handling a tracked flow deletion in > update_installed_flows_by_track(): we can check if the flow being deleted is > the currently active one installed in OVS. If not, we don't need to send and > OpenFlow modify to OVS. This optimization is added in this patch, apart from > fixing the problem of merge_tracked_flows(). Without the fix, this optimization > would cause the test case added in this patch fail. > > Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.") > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > controller/ofctrl.c | 28 +++++++++++++- > tests/ovn.at | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 134 insertions(+), 2 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 81a00c8..e725c00 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) > } > } > > +/* Returns true if a desired flow is active (the one currently installed > + * among the list of desired flows that are linked to the same installed > + * flow). */ > +static inline bool > +desired_flow_is_active(struct desired_flow *d) > +{ > + return (d->installed_flow && d->installed_flow->desired_flow == d); > +} > + > +/* Set a desired flow as the active one among the list of desired flows > + * that are linked to the same installed flow. */ > +static inline void > +desired_flow_set_active(struct desired_flow *d) > +{ > + ovs_assert(d->installed_flow); > + d->installed_flow->desired_flow = d; > +} > + > static void > link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) > { > @@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > /* del_f must have been installed, otherwise it should have been > * removed during track_flow_add_or_modify. */ > ovs_assert(del_f->installed_flow); > + > + bool del_f_was_active = desired_flow_is_active(del_f); > if (!f->installed_flow) { > /* f is not installed yet. */ > struct installed_flow *i = del_f->installed_flow; > @@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > ovs_assert(f->installed_flow == del_f->installed_flow); > unlink_installed_to_desired(del_f->installed_flow, del_f); > } > + if (del_f_was_active) { > + desired_flow_set_active(f); Nit: I think in this case the separate API makes it a bit more complicated to read the code. We already set "installed_flow->desired_flow" directly in 3 other places. I think it's fine to just: f->installed_flow->desired_flow = f; Except for this the rest looks good to me: Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru
On Thu, Oct 8, 2020 at 7:47 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 10/2/20 8:25 PM, Han Zhou wrote: > > In a scenario in I-P when a desired flow is removed and an exactly same flow is > > added back in the same iteration, the merge_tracked_flows() function will merge > > the operation without firing any OpenFlow action. However, if there are > > multiple desired flows (e.g. F1 and F2) matching the same installed flow, and > > if the one being re-added (say, F1) is the one currently installed in OVS, the > > current implementation will update the currently installed flow to F2 in the > > data structure while the actual OVS flow is not updated (still F1). So far > > there is still no problem, but the member desired_flow of the installed flow > > is pointing to the wrong desired flow. > > > > Now there is only one place where the desired_flow member is consulted, in > > update_installed_flows_by_track() for handling a tracked *modified* flow. In > > reality flow modification happens only when conjunction flows need to be > > combined, which would never happen together with the above scenario of > > merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any > > real problem yet. > > > > However, there is a place that can already utilize this active desired_flow > > information, which is when handling a tracked flow deletion in > > update_installed_flows_by_track(): we can check if the flow being deleted is > > the currently active one installed in OVS. If not, we don't need to send and > > OpenFlow modify to OVS. This optimization is added in this patch, apart from > > fixing the problem of merge_tracked_flows(). Without the fix, this optimization > > would cause the test case added in this patch fail. > > > > Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.") > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > controller/ofctrl.c | 28 +++++++++++++- > > tests/ovn.at | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 134 insertions(+), 2 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 81a00c8..e725c00 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) > > } > > } > > > > +/* Returns true if a desired flow is active (the one currently installed > > + * among the list of desired flows that are linked to the same installed > > + * flow). */ > > +static inline bool > > +desired_flow_is_active(struct desired_flow *d) > > +{ > > + return (d->installed_flow && d->installed_flow->desired_flow == d); > > +} > > + > > +/* Set a desired flow as the active one among the list of desired flows > > + * that are linked to the same installed flow. */ > > +static inline void > > +desired_flow_set_active(struct desired_flow *d) > > +{ > > + ovs_assert(d->installed_flow); > > + d->installed_flow->desired_flow = d; > > +} > > + > > static void > > link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) > > { > > @@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > > /* del_f must have been installed, otherwise it should have been > > * removed during track_flow_add_or_modify. */ > > ovs_assert(del_f->installed_flow); > > + > > + bool del_f_was_active = desired_flow_is_active(del_f); > > if (!f->installed_flow) { > > /* f is not installed yet. */ > > struct installed_flow *i = del_f->installed_flow; > > @@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) > > ovs_assert(f->installed_flow == del_f->installed_flow); > > unlink_installed_to_desired(del_f->installed_flow, del_f); > > } > > + if (del_f_was_active) { > > + desired_flow_set_active(f); > > Nit: I think in this case the separate API makes it a bit more > complicated to read the code. We already set > "installed_flow->desired_flow" directly in 3 other places. I think it's > fine to just: > > f->installed_flow->desired_flow = f; > > Except for this the rest looks good to me: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > Thanks, > Dumitru > Thanks Dumitru for the ack! I applied this to master. As agreed with Dumitru in a quick chat on IRC, I kept the patch as is, because "is_active()" made code more readable and a corresponding "set_active()" was necessary to make the interface consistent. Thanks, Han
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 81a00c8..e725c00 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -788,6 +788,24 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) } } +/* Returns true if a desired flow is active (the one currently installed + * among the list of desired flows that are linked to the same installed + * flow). */ +static inline bool +desired_flow_is_active(struct desired_flow *d) +{ + return (d->installed_flow && d->installed_flow->desired_flow == d); +} + +/* Set a desired flow as the active one among the list of desired flows + * that are linked to the same installed flow. */ +static inline void +desired_flow_set_active(struct desired_flow *d) +{ + ovs_assert(d->installed_flow); + d->installed_flow->desired_flow = d; +} + static void link_installed_to_desired(struct installed_flow *i, struct desired_flow *d) { @@ -1740,6 +1758,8 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) /* del_f must have been installed, otherwise it should have been * removed during track_flow_add_or_modify. */ ovs_assert(del_f->installed_flow); + + bool del_f_was_active = desired_flow_is_active(del_f); if (!f->installed_flow) { /* f is not installed yet. */ struct installed_flow *i = del_f->installed_flow; @@ -1751,6 +1771,9 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) ovs_assert(f->installed_flow == del_f->installed_flow); unlink_installed_to_desired(del_f->installed_flow, del_f); } + if (del_f_was_active) { + desired_flow_set_active(f); + } hmap_remove(&deleted_flows, &del_f->match_hmap_node); ovs_list_remove(&del_f->track_list_node); desired_flow_destroy(del_f); @@ -1778,6 +1801,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, /* The desired flow was deleted */ if (f->installed_flow) { struct installed_flow *i = f->installed_flow; + bool was_active = desired_flow_is_active(f); unlink_installed_to_desired(i, f); if (!i->desired_flow) { @@ -1786,7 +1810,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, hmap_remove(&installed_flows, &i->match_hmap_node); installed_flow_destroy(i); - } else { + } else if (was_active) { /* There are other desired flow(s) referencing this * installed flow, so update the OVS flow for the new * active flow (at least the cookie will be different, @@ -1810,7 +1834,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, hmap_insert(&installed_flows, &new_node->match_hmap_node, new_node->flow.hash); link_installed_to_desired(new_node, f); - } else if (i->desired_flow == f) { + } else if (desired_flow_is_active(f)) { /* The installed flow is installed for f, but f has change * tracked, so it must have been modified. */ ovn_flow_log(&i->flow, "updating installed (tracked)"); diff --git a/tests/ovn.at b/tests/ovn.at index 7769b69..10b2b5f 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -22208,6 +22208,114 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) OVN_CLEANUP([hv1]) AT_CLEANUP +# This test cases tests a scenario of ACL confliction with address set update. +# It is to cover a corner case when flows are re-processed in the I-P +# iteration, combined with the scenario of conflicting ACLs. +AT_SETUP([ovn -- conflict ACLs with address set]) +ovn_start + +ovn-nbctl ls-add ls1 + +ovn-nbctl lsp-add ls1 lsp1 \ +-- lsp-set-addresses lsp1 "f0:00:00:00:00:01 10.0.0.1" + +ovn-nbctl lsp-add ls1 lsp2 \ +-- lsp-set-addresses lsp2 "f0:00:00:00:00:02 10.0.0.2" + +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=lsp1 \ + 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=lsp2 \ + options:tx_pcap=hv1/vif2-tx.pcap \ + options:rxq_pcap=hv1/vif2-rx.pcap \ + ofport-request=2 + +# Default drop +ovn-nbctl acl-add ls1 to-lport 1000 \ +'outport == "lsp1" && ip4' drop + +# 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-vif$inport $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 +} + +# Create an address set +ovn-nbctl create Address_Set name=as1 \ +addresses=\"10.0.0.2\",\"10.0.0.3\" + +# Create overlapping ACLs resulting in conflict desired OVS flows +# Add ACL1 uses the address set +ovn-nbctl --wait=hv acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == $as1' allow + +# Add ACL2 which uses a single IP, which shouldn't take effect because +# when it is added incrementally there is already a conflict one installed. +ovn-nbctl --wait=hv acl-add ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == 10.0.0.2' drop + + +sip=`ip_to_hex 10 0 0 2` +dip=`ip_to_hex 10 0 0 1` +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Update the address set which causes flow reprocessing but the OVS flow +# for allowing 10.0.0.2 should keep unchanged +ovn-nbctl --wait=hv set Address_Set as1 addresses=\"10.0.0.2\" + +test_ip 2 f00000000002 f00000000001 $sip $dip 1 +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +# Delete the ACL1 that has "allow" action +ovn-nbctl acl-del ls1 to-lport 1001 \ +'outport == "lsp1" && ip4 && ip4.src == $as1' + +# ACL2 should take effect and packet should be dropped +test_ip 2 f00000000002 f00000000001 $sip $dip +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [1.expected]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + AT_SETUP([ovn -- port bind/unbind change handling with conj flows - IPv6]) ovn_start
In a scenario in I-P when a desired flow is removed and an exactly same flow is added back in the same iteration, the merge_tracked_flows() function will merge the operation without firing any OpenFlow action. However, if there are multiple desired flows (e.g. F1 and F2) matching the same installed flow, and if the one being re-added (say, F1) is the one currently installed in OVS, the current implementation will update the currently installed flow to F2 in the data structure while the actual OVS flow is not updated (still F1). So far there is still no problem, but the member desired_flow of the installed flow is pointing to the wrong desired flow. Now there is only one place where the desired_flow member is consulted, in update_installed_flows_by_track() for handling a tracked *modified* flow. In reality flow modification happens only when conjunction flows need to be combined, which would never happen together with the above scenario of merge_tracked_flows(). So this wrong desired_flow pointer doesn't cause any real problem yet. However, there is a place that can already utilize this active desired_flow information, which is when handling a tracked flow deletion in update_installed_flows_by_track(): we can check if the flow being deleted is the currently active one installed in OVS. If not, we don't need to send and OpenFlow modify to OVS. This optimization is added in this patch, apart from fixing the problem of merge_tracked_flows(). Without the fix, this optimization would cause the test case added in this patch fail. Fixes: f4e508dd7 ("ofctrl.c: Merge opposite changes of tracked flows before installing.") Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/ofctrl.c | 28 +++++++++++++- tests/ovn.at | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 2 deletions(-)