diff mbox series

[ovs-dev] mirror.c: Fix ovn-controller crash when mirror port is deleted from ovs.

Message ID 20230629064242.190176-1-hzhou@ovn.org
State Accepted
Headers show
Series [ovs-dev] mirror.c: Fix ovn-controller crash when mirror port is deleted from ovs. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_ovn-kubernetes success github build: passed
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Han Zhou June 29, 2023, 6:42 a.m. UTC 
If the ovs port used as output_port in mirror is deleted (either by
mistake or intentionally), ovn-controller would crash in the
check_and_update_interface_table() when trying to sync the mirror port
configuration. e.g.:

0  0x0000000000449d6e in check_and_update_interface_table (ovs_mirror=0xcc5110, ovs_mirror=0xcc5110, sb_mirror=0xcc64b0) at ../controller/mirror.c:351
1  sync_ovn_mirror (ovs_mirror_ports=0x7ffcf79b0280, br_int=0xbf5020, ovs_idl_txn=0xe13570, m=0xce86b0) at ../controller/mirror.c:351
2  mirror_run (ovs_idl_txn=0xe13570, ovs_mirror_table=<optimized out>, sb_mirror_table=<optimized out>, br_int=0xbf5020, local_bindings=<optimized out>) at ../controller/mirror.c:174
3  0x0000000000409e8e in main (argc=<optimized out>, argv=<optimized out>) at ../controller/ovn-controller.c:5357

Similarly, it can also crash in delete_ovs_mirror() if the mirror port is
deleted together with src/dst port (when should_delete_ovs_mirror returns
true).

Both cases are captured in the updated test case of this patch.

This patch fixes the problem by verifying the existance of the mirror port, and
creates it if needed.

Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to create ovs mirrors")
Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 controller/mirror.c | 86 ++++++++++++++++++++++++++++-----------------
 tests/ovn.at        | 14 ++++++++
 2 files changed, 67 insertions(+), 33 deletions(-)

Comments

Dumitru Ceara June 30, 2023, 8:07 a.m. UTC | #1 
On 6/29/23 08:42, Han Zhou wrote:
> If the ovs port used as output_port in mirror is deleted (either by
> mistake or intentionally), ovn-controller would crash in the
> check_and_update_interface_table() when trying to sync the mirror port
> configuration. e.g.:
> 
> 0  0x0000000000449d6e in check_and_update_interface_table (ovs_mirror=0xcc5110, ovs_mirror=0xcc5110, sb_mirror=0xcc64b0) at ../controller/mirror.c:351
> 1  sync_ovn_mirror (ovs_mirror_ports=0x7ffcf79b0280, br_int=0xbf5020, ovs_idl_txn=0xe13570, m=0xce86b0) at ../controller/mirror.c:351
> 2  mirror_run (ovs_idl_txn=0xe13570, ovs_mirror_table=<optimized out>, sb_mirror_table=<optimized out>, br_int=0xbf5020, local_bindings=<optimized out>) at ../controller/mirror.c:174
> 3  0x0000000000409e8e in main (argc=<optimized out>, argv=<optimized out>) at ../controller/ovn-controller.c:5357
> 
> Similarly, it can also crash in delete_ovs_mirror() if the mirror port is
> deleted together with src/dst port (when should_delete_ovs_mirror returns
> true).
> 
> Both cases are captured in the updated test case of this patch.
> 
> This patch fixes the problem by verifying the existance of the mirror port, and
> creates it if needed.
> 
> Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to create ovs mirrors")
> Signed-off-by: Han Zhou <hzhou@ovn.org>
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara <dceara@redhat.com>

Regards,
Dumitru
Han Zhou June 30, 2023, 7:01 p.m. UTC | #2 
On Fri, Jun 30, 2023 at 1:07 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 6/29/23 08:42, Han Zhou wrote:
> > If the ovs port used as output_port in mirror is deleted (either by
> > mistake or intentionally), ovn-controller would crash in the
> > check_and_update_interface_table() when trying to sync the mirror port
> > configuration. e.g.:
> >
> > 0  0x0000000000449d6e in check_and_update_interface_table
(ovs_mirror=0xcc5110, ovs_mirror=0xcc5110, sb_mirror=0xcc64b0) at
../controller/mirror.c:351
> > 1  sync_ovn_mirror (ovs_mirror_ports=0x7ffcf79b0280, br_int=0xbf5020,
ovs_idl_txn=0xe13570, m=0xce86b0) at ../controller/mirror.c:351
> > 2  mirror_run (ovs_idl_txn=0xe13570, ovs_mirror_table=<optimized out>,
sb_mirror_table=<optimized out>, br_int=0xbf5020, local_bindings=<optimized
out>) at ../controller/mirror.c:174
> > 3  0x0000000000409e8e in main (argc=<optimized out>, argv=<optimized
out>) at ../controller/ovn-controller.c:5357
> >
> > Similarly, it can also crash in delete_ovs_mirror() if the mirror port
is
> > deleted together with src/dst port (when should_delete_ovs_mirror
returns
> > true).
> >
> > Both cases are captured in the updated test case of this patch.
> >
> > This patch fixes the problem by verifying the existance of the mirror
port, and
> > creates it if needed.
> >
> > Fixes: ba8aa26e44cb ("OVN Remote Port Mirroring: controller changes to
create ovs mirrors")
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > ---
>
> Looks good to me, thanks!
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>
> Regards,
> Dumitru
>

Thanks Dumitru. I applied to main and backported down to branch-22.12.
diff mbox series

Patch

diff --git a/controller/mirror.c b/controller/mirror.c
index 815da22da9f1..ebe455968ef1 100644
--- a/controller/mirror.c
+++ b/controller/mirror.c
@@ -61,6 +61,9 @@  static void sync_ovn_mirror(struct ovn_mirror *, struct ovsdb_idl_txn *,
 static void create_ovs_mirror(struct ovn_mirror *, struct ovsdb_idl_txn *,
                               const struct ovsrec_bridge *,
                               struct shash *ovs_mirror_ports);
+static struct ovsrec_port *create_mirror_port(struct ovn_mirror *,
+                                              struct ovsdb_idl_txn *,
+                                              const struct ovsrec_bridge *);
 static void sync_ovs_mirror_ports(struct ovn_mirror *,
                                   const struct ovsrec_bridge *);
 static void delete_ovs_mirror(struct ovn_mirror *,
@@ -312,16 +315,23 @@  get_mirror_tunnel_type(const struct sbrec_mirror *sb_mirror)
 }
 
 static void
-check_and_update_interface_table(const struct sbrec_mirror *sb_mirror,
-                                 const struct ovsrec_mirror *ovs_mirror)
+check_and_update_interface_table(struct ovn_mirror *m,
+                                 struct ovsdb_idl_txn *ovs_idl_txn,
+                                 const struct ovsrec_bridge *br_int)
 {
-    struct ovsrec_interface *iface = ovs_mirror->output_port->interfaces[0];
-    char *type = get_mirror_tunnel_type(sb_mirror);
+    if (!m->ovs_mirror->output_port) {
+        const struct ovsrec_port *mirror_port =
+            create_mirror_port(m, ovs_idl_txn, br_int);
+        ovsrec_mirror_set_output_port(m->ovs_mirror, mirror_port);
+        return;
+    }
+    struct ovsrec_interface *iface = m->ovs_mirror->output_port->interfaces[0];
+    char *type = get_mirror_tunnel_type(m->sb_mirror);
 
     if (strcmp(type, iface->type)) {
         ovsrec_interface_set_type(iface, type);
     }
-    set_mirror_iface_options(iface, sb_mirror);
+    set_mirror_iface_options(iface, m->sb_mirror);
     free(type);
 }
 
@@ -348,7 +358,7 @@  sync_ovn_mirror(struct ovn_mirror *m, struct ovsdb_idl_txn *ovs_idl_txn,
             return;
         }
     } else if (strcmp(m->sb_mirror->type, "local")) {
-        check_and_update_interface_table(m->sb_mirror, m->ovs_mirror);
+        check_and_update_interface_table(m, ovs_idl_txn, br_int);
 
         /* For upgradability, set the "ovn-owned" in case it was not set when
          * the port was created. */
@@ -403,6 +413,33 @@  get_iface_port(const struct ovsrec_interface *iface,
     return NULL;
 }
 
+static struct ovsrec_port *
+create_mirror_port(struct ovn_mirror *m, struct ovsdb_idl_txn *ovs_idl_txn,
+                   const struct ovsrec_bridge *br_int)
+{
+    struct ovsrec_interface *iface = ovsrec_interface_insert(ovs_idl_txn);
+    char *port_name = xasprintf("ovn-%s", m->name);
+
+    ovsrec_interface_set_name(iface, port_name);
+
+    char *type = get_mirror_tunnel_type(m->sb_mirror);
+    ovsrec_interface_set_type(iface, type);
+    set_mirror_iface_options(iface, m->sb_mirror);
+    free(type);
+
+    struct ovsrec_port *port = ovsrec_port_insert(ovs_idl_txn);
+    ovsrec_port_set_name(port, port_name);
+    ovsrec_port_set_interfaces(port, &iface, 1);
+    ovsrec_bridge_update_ports_addvalue(br_int, port);
+
+    const struct smap port_external_ids =
+        SMAP_CONST1(&port_external_ids, "ovn-owned", "true");
+    ovsrec_port_set_external_ids(port, &port_external_ids);
+
+    free(port_name);
+    return port;
+}
+
 static void
 create_ovs_mirror(struct ovn_mirror *m, struct ovsdb_idl_txn *ovs_idl_txn,
                   const struct ovsrec_bridge *br_int,
@@ -415,27 +452,7 @@  create_ovs_mirror(struct ovn_mirror *m, struct ovsdb_idl_txn *ovs_idl_txn,
             return;
         }
     } else {
-        struct ovsrec_interface *iface = ovsrec_interface_insert(ovs_idl_txn);
-        char *port_name = xasprintf("ovn-%s", m->name);
-
-        ovsrec_interface_set_name(iface, port_name);
-
-        char *type = get_mirror_tunnel_type(m->sb_mirror);
-        ovsrec_interface_set_type(iface, type);
-        set_mirror_iface_options(iface, m->sb_mirror);
-        free(type);
-
-        struct ovsrec_port *port = ovsrec_port_insert(ovs_idl_txn);
-        ovsrec_port_set_name(port, port_name);
-        ovsrec_port_set_interfaces(port, &iface, 1);
-        ovsrec_bridge_update_ports_addvalue(br_int, port);
-
-        const struct smap port_external_ids =
-            SMAP_CONST1(&port_external_ids, "ovn-owned", "true");
-        ovsrec_port_set_external_ids(port, &port_external_ids);
-
-        free(port_name);
-        mirror_port = port;
+        mirror_port = create_mirror_port(m, ovs_idl_txn, br_int);
     }
 
     m->ovs_mirror = ovsrec_mirror_insert(ovs_idl_txn);
@@ -495,12 +512,15 @@  static void
 delete_ovs_mirror(struct ovn_mirror *m, const struct ovsrec_bridge *br_int)
 {
     ovsrec_bridge_update_mirrors_delvalue(br_int, m->ovs_mirror);
-    bool ovn_owned = smap_get_bool(&m->ovs_mirror->output_port->external_ids,
-                                   "ovn-owned", false);
-    if (ovn_owned) {
-        ovsrec_bridge_update_ports_delvalue(br_int,
-                                            m->ovs_mirror->output_port);
-        ovsrec_port_delete(m->ovs_mirror->output_port);
+    if (m->ovs_mirror->output_port) {
+        bool ovn_owned =
+            smap_get_bool(&m->ovs_mirror->output_port->external_ids,
+                          "ovn-owned", false);
+        if (ovn_owned) {
+            ovsrec_bridge_update_ports_delvalue(br_int,
+                                                m->ovs_mirror->output_port);
+            ovsrec_port_delete(m->ovs_mirror->output_port);
+        }
     }
     ovsrec_mirror_delete(m->ovs_mirror);
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index 544fba1876f2..53fd1c4951ca 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -17131,6 +17131,20 @@  check ovn-nbctl --wait=hv sync
 
 OVS_WAIT_UNTIL([test 0 = $(as hv1 ovs-vsctl list Mirror | wc -l)])
 
+# Create the vif1 again, and mirror should be recreated.
+ovs-vsctl -- add-port br-int vif1 -- \
+    set interface vif1 external-ids:iface-id=ls1-lp1
+OVS_WAIT_UNTIL([test $(as hv1 ovs-vsctl get Mirror mirror0 name) = "mirror0"])
+
+# Delete the mirror port, should recreate.
+check ovs-vsctl del-port ovn-mirror0
+OVS_WAIT_UNTIL([ovs-vsctl list port ovn-mirror0 | grep ovn-mirror0])
+
+# Delete the mirror port and vif1 at the same time, mirror should be deleted
+# properly (without crash)
+ovs-vsctl del-port ovn-mirror0 -- del-port vif1
+OVS_WAIT_UNTIL([test 0 = $(as hv1 ovs-vsctl list Mirror | wc -l)])
+
 OVN_CLEANUP([hv1], [hv2])
 AT_CLEANUP
 ])