diff mbox series

[ovs-dev] ofctrl.c: Fix duplicated flow handling in I-P while merging opposite changes.

Message ID 1601663136-19111-1-git-send-email-hzhou@ovn.org
State New
Headers show
Series [ovs-dev] ofctrl.c: Fix duplicated flow handling in I-P while merging opposite changes. | expand

Commit Message

Han Zhou Oct. 2, 2020, 6:25 p.m. UTC
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(-)

Comments

Dumitru Ceara Oct. 8, 2020, 2:46 p.m. UTC | #1
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
Han Zhou Oct. 8, 2020, 6:51 p.m. UTC | #2
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 mbox series

Patch

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