From patchwork Wed Jun 12 15:59:02 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1114661 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45PBS35b3Bz9s9y for ; Thu, 13 Jun 2019 02:01:06 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 0F2781E33; Wed, 12 Jun 2019 16:01:02 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 35F8F1E2A for ; Wed, 12 Jun 2019 15:59:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 3F067E6 for ; Wed, 12 Jun 2019 15:59:29 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8D92730821EC; Wed, 12 Jun 2019 15:59:12 +0000 (UTC) Received: from dceara.remote.csb (ovpn-117-241.ams2.redhat.com [10.36.117.241]) by smtp.corp.redhat.com (Postfix) with ESMTP id 52917196B1; Wed, 12 Jun 2019 15:59:09 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Wed, 12 Jun 2019 17:59:02 +0200 Message-Id: <1560355142-11622-1-git-send-email-dceara@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Wed, 12 Jun 2019 15:59:12 +0000 (UTC) X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH v4] ovn-controller: Fix parsing of OVN tunnel IDs X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Encap tunnel-ids are of the form: . In physical_run we were checking if a tunnel-id corresponds to the local chassis-id by searching if the chassis-id string is included in the tunnel-id (strstr). This can break quite easily, for example, if the local chassis-id is a substring of a remote chassis-id. In that case we were wrongfully skipping the tunnel creation. To fix that new tunnel-id creation and parsing functions are added in encaps.[ch]. These functions are now used everywhere where applicable. Acked-by: Venu Iyer Reported-at: https://bugzilla.redhat.com/1708131 Reported-by: Haidong Li Fixes: b520ca7 ("Support for multiple VTEP in OVN") Signed-off-by: Dumitru Ceara Signed-off-by: Ben Pfaff --- v4: Add Ben's incremental patch v3: Rebase v2: Update commit message --- ovn/controller/bfd.c | 31 +++++++++-------- ovn/controller/encaps.c | 74 +++++++++++++++++++++++++++++++++++++++-- ovn/controller/encaps.h | 6 ++++ ovn/controller/ovn-controller.h | 7 ---- ovn/controller/physical.c | 51 +++++++++++----------------- ovn/controller/pinctrl.c | 4 ++- 6 files changed, 117 insertions(+), 56 deletions(-) diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c index a2f590d..22db00a 100644 --- a/ovn/controller/bfd.c +++ b/ovn/controller/bfd.c @@ -15,6 +15,7 @@ #include #include "bfd.h" +#include "encaps.h" #include "lport.h" #include "ovn-controller.h" @@ -72,14 +73,16 @@ bfd_calculate_active_tunnels(const struct ovsrec_bridge *br_int, const char *id = smap_get(&port_rec->external_ids, "ovn-chassis-id"); if (id) { - char *chassis_name; - char *save_ptr = NULL; - char *tokstr = xstrdup(id); - chassis_name = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, &save_ptr); - if (chassis_name && !sset_contains(active_tunnels, chassis_name)) { - sset_add(active_tunnels, chassis_name); + char *chassis_name = NULL; + + if (encaps_tunnel_id_parse(id, &chassis_name, + NULL)) { + if (!sset_contains(active_tunnels, + chassis_name)) { + sset_add(active_tunnels, chassis_name); + } + free(chassis_name); } - free(tokstr); } } } @@ -193,17 +196,17 @@ bfd_run(const struct ovsrec_interface_table *interface_table, const char *tunnel_id = smap_get(&br_int->ports[k]->external_ids, "ovn-chassis-id"); if (tunnel_id) { - char *chassis_name; - char *save_ptr = NULL; - char *tokstr = xstrdup(tunnel_id); + char *chassis_name = NULL; char *port_name = br_int->ports[k]->name; sset_add(&tunnels, port_name); - chassis_name = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, &save_ptr); - if (chassis_name && sset_contains(&bfd_chassis, chassis_name)) { - sset_add(&bfd_ifaces, port_name); + + if (encaps_tunnel_id_parse(tunnel_id, &chassis_name, NULL)) { + if (sset_contains(&bfd_chassis, chassis_name)) { + sset_add(&bfd_ifaces, port_name); + } + free(chassis_name); } - free(tokstr); } } diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c index 7bd52d1..3b3921a 100644 --- a/ovn/controller/encaps.c +++ b/ovn/controller/encaps.c @@ -26,6 +26,13 @@ VLOG_DEFINE_THIS_MODULE(encaps); +/* + * Given there could be multiple tunnels with different IPs to the same + * chassis we annotate the ovn-chassis-id with + * OVN_MVTEP_CHASSISID_DELIM. + */ +#define OVN_MVTEP_CHASSISID_DELIM '@' + void encaps_register_ovs_idl(struct ovsdb_idl *ovs_idl) { @@ -78,6 +85,70 @@ tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id) return NULL; } +/* + * Returns a tunnel-id of the form 'chassis_id'-delimiter-'encap_ip'. + */ +char * +encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip) +{ + return xasprintf("%s%c%s", chassis_id, OVN_MVTEP_CHASSISID_DELIM, + encap_ip); +} + +/* + * Parses a 'tunnel_id' of the form . + * If the 'chassis_id' argument is not NULL the function will allocate memory + * and store the chassis-id part of the tunnel-id at '*chassis_id'. + * If the 'encap_ip' argument is not NULL the function will allocate memory + * and store the encapsulation IP part of the tunnel-id at '*encap_ip'. + */ +bool +encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id, + char **encap_ip) +{ + /* Find the delimiter. Fail if there is no delimiter or if + * or is the empty string.*/ + const char *d = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM); + if (d == tunnel_id || !d || !d[1]) { + return false; + } + + if (chassis_id) { + *chassis_id = xmemdup0(tunnel_id, d - tunnel_id); + } + if (encap_ip) { + *encap_ip = xstrdup(d + 1); + } + return true; +} + +/* + * Returns true if 'tunnel_id' contains 'chassis_id' and, if specified, the + * given 'encap_ip'. Returns false otherwise. + */ +bool +encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id, + const char *encap_ip) +{ + while (*tunnel_id == *chassis_id) { + if (!*tunnel_id) { + /* 'tunnel_id' and 'chassis_id' are equal strings. This is a + * mismatch because 'tunnel_id' is missing the delimiter and IP. */ + return false; + } + tunnel_id++; + chassis_id++; + } + + /* We found the first byte that disagrees between 'tunnel_id' and + * 'chassis_id'. If we consumed all of 'chassis_id' and arrived at the + * delimiter in 'tunnel_id' (and if 'encap_ip' is correct, if it was + * supplied), it's a match. */ + return (*tunnel_id == OVN_MVTEP_CHASSISID_DELIM + && *chassis_id == '\0' + && (!encap_ip || !strcmp(tunnel_id + 1, encap_ip))); +} + static void tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, const char *new_chassis_id, const struct sbrec_encap *encap) @@ -94,8 +165,7 @@ tunnel_add(struct tunnel_ctx *tc, const struct sbrec_sb_global *sbg, * combination of the chassis_name and the encap-ip to identify * a specific tunnel to the chassis. */ - tunnel_entry_id = xasprintf("%s%s%s", new_chassis_id, - OVN_MVTEP_CHASSISID_DELIM, encap->ip); + tunnel_entry_id = encaps_tunnel_id_create(new_chassis_id, encap->ip); if (csum && (!strcmp(csum, "true") || !strcmp(csum, "false"))) { smap_add(&options, "csum", csum); } diff --git a/ovn/controller/encaps.h b/ovn/controller/encaps.h index 7ed3e09..afa4183 100644 --- a/ovn/controller/encaps.h +++ b/ovn/controller/encaps.h @@ -39,4 +39,10 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn, bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_bridge *br_int); +char *encaps_tunnel_id_create(const char *chassis_id, const char *encap_ip); +bool encaps_tunnel_id_parse(const char *tunnel_id, char **chassis_id, + char **encap_ip); +bool encaps_tunnel_id_match(const char *tunnel_id, const char *chassis_id, + const char *encap_ip); + #endif /* ovn/encaps.h */ diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h index 6afd727..2c224c8 100644 --- a/ovn/controller/ovn-controller.h +++ b/ovn/controller/ovn-controller.h @@ -81,11 +81,4 @@ enum chassis_tunnel_type { uint32_t get_tunnel_type(const char *name); -/* - * Given there could be multiple tunnels with different IPs to the same - * chassis we annotate the ovn-chassis-id with - * OVN_MVTEP_CHASSISID_DELIM. - */ -#define OVN_MVTEP_CHASSISID_DELIM "@" - #endif /* ovn/ovn-controller.h */ diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index c8dc282..dd455dd 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -16,6 +16,7 @@ #include #include "binding.h" #include "byte-order.h" +#include "encaps.h" #include "flow.h" #include "ha-chassis.h" #include "lflow.h" @@ -80,38 +81,27 @@ struct chassis_tunnel { /* * This function looks up the list of tunnel ports (provided by * ovn-chassis-id ports) and returns the tunnel for the given chassid-id and - * encap-ip. The ovn-chassis-id is formed using the chassis-id and encap-ip as - * OVN_MVTEP_CHASSISID_DELIM. The list is hashed using - * the chassis-id. If the encap-ip is not specified, it means we'll just - * return a tunnel for that chassis-id, i.e. we just check for chassis-id and - * if there is a match, we'll return the tunnel. If encap-ip is also provided we - * use OVN_MVTEP_CHASSISID_DELIM to do a more specific - * lookup. + * encap-ip. The ovn-chassis-id is formed using the chassis-id and encap-ip. + * The list is hashed using the chassis-id. If the encap-ip is not specified, + * it means we'll just return a tunnel for that chassis-id, i.e. we just check + * for chassis-id and if there is a match, we'll return the tunnel. + * If encap-ip is also provided we use both chassis-id and encap-ip to do + * a more specific lookup. */ static struct chassis_tunnel * chassis_tunnel_find(const char *chassis_id, char *encap_ip) { - char *chassis_tunnel_entry; - /* * If the specific encap_ip is given, look for the chassisid_ip entry, * else return the 1st found entry for the chassis. */ - if (encap_ip != NULL) { - chassis_tunnel_entry = xasprintf("%s%s%s", chassis_id, - OVN_MVTEP_CHASSISID_DELIM, encap_ip); - } else { - chassis_tunnel_entry = xasprintf("%s", chassis_id); - } struct chassis_tunnel *tun = NULL; HMAP_FOR_EACH_WITH_HASH (tun, hmap_node, hash_string(chassis_id, 0), &tunnels) { - if (strstr(tun->chassis_id, chassis_tunnel_entry) != NULL) { - free (chassis_tunnel_entry); + if (encaps_tunnel_id_match(tun->chassis_id, chassis_id, encap_ip)) { return tun; } } - free (chassis_tunnel_entry); return NULL; } @@ -1064,8 +1054,9 @@ physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name, } const char *tunnel_id = smap_get(&port_rec->external_ids, - "ovn-chassis-id"); - if (tunnel_id && strstr(tunnel_id, chassis->name)) { + "ovn-chassis-id"); + if (tunnel_id && + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL)) { continue; } @@ -1121,16 +1112,10 @@ physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name, * where we need to tunnel to the chassis, but won't * have the encap-ip specifically. */ - char *tokstr = xstrdup(tunnel_id); - char *save_ptr = NULL; - char *hash_id = strtok_r(tokstr, OVN_MVTEP_CHASSISID_DELIM, - &save_ptr); - char *ip = strtok_r(NULL, "", &save_ptr); - /* - * If the value has morphed into something other than - * chassis-id>delim>encap-ip, ignore. - */ - if (!hash_id || !ip) { + char *hash_id = NULL; + char *ip = NULL; + + if (!encaps_tunnel_id_parse(tunnel_id, &hash_id, &ip)) { continue; } struct chassis_tunnel *tun = chassis_tunnel_find(hash_id, ip); @@ -1151,7 +1136,8 @@ physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name, tun->type = tunnel_type; physical_map_changed = true; } - free(tokstr); + free(hash_id); + free(ip); break; } else { const char *iface_id = smap_get(&iface_rec->external_ids, @@ -1256,7 +1242,8 @@ physical_run(struct ovsdb_idl_index *sbrec_port_binding_by_name, struct match match = MATCH_CATCHALL_INITIALIZER; if (!binding->chassis || - strstr(tun->chassis_id, binding->chassis->name) == NULL) { + !encaps_tunnel_id_match(tun->chassis_id, + binding->chassis->name, NULL)) { continue; } diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c index b7bb4c9..798bd34 100644 --- a/ovn/controller/pinctrl.c +++ b/ovn/controller/pinctrl.c @@ -22,6 +22,7 @@ #include "csum.h" #include "dirs.h" #include "dp-packet.h" +#include "encaps.h" #include "flow.h" #include "ha-chassis.h" #include "lport.h" @@ -2725,7 +2726,8 @@ get_localnet_vifs_l3gwports( } const char *tunnel_id = smap_get(&port_rec->external_ids, "ovn-chassis-id"); - if (tunnel_id && strstr(tunnel_id, chassis->name)) { + if (tunnel_id && + encaps_tunnel_id_match(tunnel_id, chassis->name, NULL)) { continue; } const char *localnet = smap_get(&port_rec->external_ids,