[ovs-dev] [PATCH v4] ovn-controller: Fix parsing of OVN tunnel IDs
Dumitru Ceara
dceara at redhat.com
Wed Jun 12 15:59:02 UTC 2019
Encap tunnel-ids are of the form:
<chassis-id><OVN_MVTEP_CHASSISID_DELIM><encap-ip>.
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 <iyervl at ymail.com>
Reported-at: https://bugzilla.redhat.com/1708131
Reported-by: Haidong Li <haili at redhat.com>
Fixes: b520ca7 ("Support for multiple VTEP in OVN")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
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 <config.h>
#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
+ * <chassis_name>OVN_MVTEP_CHASSISID_DELIM<IP>.
+ */
+#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 <chassis_name><delimiter><IP>.
+ * 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 <chassis_name>
+ * or <IP> 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
- * <chassis_name>OVN_MVTEP_CHASSISID_DELIM<IP>.
- */
-#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 <config.h>
#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
- * <chassis-id>OVN_MVTEP_CHASSISID_DELIM<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 <chassis-id>OVN_MVTEP_CHASSISID_DELIM<encap-ip> 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,
--
1.8.3.1
More information about the dev
mailing list