diff mbox series

[ovs-dev,branch-2.12,3/4] ovn-controller: Fix potential segfault with "virtual" port bindings.

Message ID 20200407110352.7801.19403.stgit@dceara.remote.csb
State Not Applicable
Headers show
Series OVN: Backport virtual port related fixes. | expand

Commit Message

Dumitru Ceara April 7, 2020, 11:04 a.m. UTC
Even though ovn-controller tries to set port_binding->chassis to NULL
every time port_binding->virtual_parent is set to NULL for bindings of
type="virtual", there's no way to enforce that an operator doesn't
manually clear the "virtual_parent" column in the Southbound database.

In such scenario ovn-controller would crash because of trying to
dereference the NULL port_binding->virtual_parent column.

Add an extra check and release "virtual" port bindings that have
"virtual_parent" NULL.

Reported-at: https://bugzilla.redhat.com/1818844
CC: Numan Siddique <nusiddiq@redhat.com>
Fixes: 054f4c85c413 ("Add a new logical switch port type - 'virtual'")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>

(cherry picked from OVN commit 1d0c6732d0e49f7f89f5e6a00ac9cdf6d3117e8d)
---
 ovn/controller/binding.c |   26 +++++++++++++++-----------
 tests/ovn.at             |   18 ++++++++++++++++++
 2 files changed, 33 insertions(+), 11 deletions(-)

Comments

0-day Robot April 7, 2020, 1:09 p.m. UTC | #1
Bleep bloop.  Greetings Dumitru Ceara, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <numans@ovn.org>
Lines checked: 103, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index ef8445f..3f411f0 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -610,22 +610,26 @@  consider_local_virtual_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
                             const struct sbrec_chassis *chassis_rec,
                             const struct sbrec_port_binding *binding_rec)
 {
+    if (binding_rec->virtual_parent) {
+        const struct sbrec_port_binding *parent =
+            lport_lookup_by_name(sbrec_port_binding_by_name,
+                                 binding_rec->virtual_parent);
+        if (parent && parent->chassis == chassis_rec) {
+            return;
+        }
+    }
+
     /* pinctrl module takes care of binding the ports of type 'virtual'.
      * Release such ports if their virtual parents are no longer claimed by
      * this chassis.
      */
-    const struct sbrec_port_binding *parent =
-        lport_lookup_by_name(sbrec_port_binding_by_name,
-                             binding_rec->virtual_parent);
-    if (!parent || parent->chassis != chassis_rec) {
-        VLOG_INFO("Releasing lport %s from this chassis.",
-                  binding_rec->logical_port);
-        if (binding_rec->encap) {
-            sbrec_port_binding_set_encap(binding_rec, NULL);
-        }
-        sbrec_port_binding_set_chassis(binding_rec, NULL);
-        sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
+    VLOG_INFO("Releasing lport %s from this chassis.",
+              binding_rec->logical_port);
+    if (binding_rec->encap) {
+        sbrec_port_binding_set_encap(binding_rec, NULL);
     }
+    sbrec_port_binding_set_chassis(binding_rec, NULL);
+    sbrec_port_binding_set_virtual_parent(binding_rec, NULL);
 }
 
 static void
diff --git a/tests/ovn.at b/tests/ovn.at
index e8125cc..fc76927 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14593,6 +14593,24 @@  AT_CHECK([cat lflows.txt], [0], [dnl
   table=9 (lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
 ])
 
+# Forcibly clear virtual_parent. ovn-controller should release the binding
+# gracefully.
+pb_uuid=$(ovn-sbctl --bare --columns _uuid find port_binding logical_port=sw0-vir)
+ovn-sbctl clear port_binding $pb_uuid virtual_parent
+
+OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
+logical_port=sw0-vir) = x])
+
+# From sw0-p0 resend GARP for 10.0.0.10. hv1 should reclaim sw0-vir
+# and sw0-p1 should be its virtual_parent.
+send_garp 1 1 $eth_src $eth_dst $spa $tpa
+
+OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
+logical_port=sw0-vir) = x$hv1_ch_uuid], [0], [])
+
+AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \
+logical_port=sw0-vir) = xsw0-p1])
+
 # From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir
 # and sw0-p3 should be its virtual_parent.
 eth_src=505400000005