From patchwork Thu Jun 29 06:42:42 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Han Zhou X-Patchwork-Id: 1801387 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp2.osuosl.org (smtp2.osuosl.org [IPv6:2605:bc80:3010::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4Qs8670mDjz1yhT for ; Thu, 29 Jun 2023 16:43:09 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 4324940382; Thu, 29 Jun 2023 06:43:03 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 4324940382 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Tm8nfw04wrVD; Thu, 29 Jun 2023 06:43:02 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp2.osuosl.org (Postfix) with ESMTPS id 32DD1400DA; Thu, 29 Jun 2023 06:43:01 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 32DD1400DA Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 00047C007C; Thu, 29 Jun 2023 06:43:00 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 59D31C0037 for ; Thu, 29 Jun 2023 06:42:59 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 2CFAB400FD for ; Thu, 29 Jun 2023 06:42:59 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 2CFAB400FD X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NhNftLRVqLPe for ; Thu, 29 Jun 2023 06:42:58 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org D5E56400DA Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::228]) by smtp2.osuosl.org (Postfix) with ESMTPS id D5E56400DA for ; Thu, 29 Jun 2023 06:42:57 +0000 (UTC) X-GND-Sasl: hzhou@ovn.org X-GND-Sasl: hzhou@ovn.org X-GND-Sasl: hzhou@ovn.org Received: by mail.gandi.net (Postfix) with ESMTPSA id B2D311BF204; Thu, 29 Jun 2023 06:42:53 +0000 (UTC) From: Han Zhou To: dev@openvswitch.org Date: Wed, 28 Jun 2023 23:42:42 -0700 Message-Id: <20230629064242.190176-1-hzhou@ovn.org> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn] mirror.c: Fix ovn-controller crash when mirror port is deleted from ovs. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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=, sb_mirror_table=, br_int=0xbf5020, local_bindings=) at ../controller/mirror.c:174 3 0x0000000000409e8e in main (argc=, argv=) 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 Acked-by: Dumitru Ceara --- controller/mirror.c | 86 ++++++++++++++++++++++++++++----------------- tests/ovn.at | 14 ++++++++ 2 files changed, 67 insertions(+), 33 deletions(-) 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 ])