Message ID | 1557747059-15070-1-git-send-email-dceara@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev] ovn-controller: Fix parsing of OVN tunnel IDs | expand |
Thanks for fixing this! a comment below:
On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote:
Add tunnel-id creation and parsing functions in encaps.h.
Reported-at: https://bugzilla.redhat.com/1708131
Reported-by: Haidong Li <haili@redhat.com>
Fixes: b520ca7 ("Support for multiple VTEP in OVN")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
ovn/controller/bfd.c | 31 ++++++++-------
ovn/controller/encaps.c | 87 ++++++++++++++++++++++++++++++++++++++++-
ovn/controller/encaps.h | 6 +++
ovn/controller/ovn-controller.h | 7 ----
ovn/controller/physical.c | 51 +++++++++---------------
ovn/controller/pinctrl.c | 4 +-
6 files changed, 130 insertions(+), 56 deletions(-)
diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c
index d016e27..b6aef04 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 dcf7810..d467540 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,83 @@ 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)
+{
+ const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM);
+
+ if (!match) {
+ return false;
+ }
+
+ if (chassis_id) {
+ size_t chassis_id_len = (match - tunnel_id);
+
+ *chassis_id = xmemdup0(tunnel_id, chassis_id_len);
+ }
+
+ /* Consume the tunnel-id delimiter. */
+ match++;
+
+ if (encap_ip) {
+ /*
+ * If the value has morphed into something other than
+ * <chassis-id><delim><encap-ip>, fail and free already allocated
+ * memory (i.e., chassis_id).
+ */
+ if (*match == 0) {
If the OVN_..._DELIM happens to be the last char in the tunnel_id (i.e. if somehow itis corrupted), is this safe given we have gone beyond it?
rest looks good.
thanks,
-venu
+ if (chassis_id) {
+ free(*chassis_id);
+ }
+ return false;
+ }
+ *encap_ip = xstrdup(match);
+ }
+
+ return true;
+}
+
+/*
+ * Returns true if a given 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)
+{
+ if (strstr(tunnel_id, chassis_id) != tunnel_id) {
+ return false;
+ }
+
+ size_t chassis_id_len = strlen(chassis_id);
+
+ if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) {
+ return false;
+ }
+
+ if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) {
+ return false;
+ }
+
+ return true;
+}
+
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 +178,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 7386404..3e3f731 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"
@@ -77,38 +78,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;
}
@@ -998,8 +988,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;
}
@@ -1055,16 +1046,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);
@@ -1085,7 +1070,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,
@@ -1194,7 +1180,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 8ae1f9b..9d1b031 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"
@@ -2722,7 +2723,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,
On Wed, May 15, 2019 at 10:30 PM venugopal iyer <iyervl@ymail.com> wrote: > > Thanks for fixing this! a comment below: > > On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: > > > Add tunnel-id creation and parsing functions in encaps.h. > > Reported-at: https://bugzilla.redhat.com/1708131 > Reported-by: Haidong Li <haili@redhat.com> > Fixes: b520ca7 ("Support for multiple VTEP in OVN") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > ovn/controller/bfd.c | 31 ++++++++------- > ovn/controller/encaps.c | 87 ++++++++++++++++++++++++++++++++++++++++- > ovn/controller/encaps.h | 6 +++ > ovn/controller/ovn-controller.h | 7 ---- > ovn/controller/physical.c | 51 +++++++++--------------- > ovn/controller/pinctrl.c | 4 +- > 6 files changed, 130 insertions(+), 56 deletions(-) > > diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c > index d016e27..b6aef04 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 dcf7810..d467540 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,83 @@ 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) > +{ > + const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM); > + > + if (!match) { > + return false; > + } > + > + if (chassis_id) { > + size_t chassis_id_len = (match - tunnel_id); > + > + *chassis_id = xmemdup0(tunnel_id, chassis_id_len); > + } > + > + /* Consume the tunnel-id delimiter. */ > + match++; > + > + if (encap_ip) { > + /* > + * If the value has morphed into something other than > + * <chassis-id><delim><encap-ip>, fail and free already allocated > + * memory (i.e., chassis_id). > + */ > + if (*match == 0) { > > If the OVN_..._DELIM happens to be the last char in the tunnel_id (i.e. if somehow it > is corrupted), is this safe given we have gone beyond it? Hi Venu, Thanks for taking the time to review this change. Just to make sure I understood your comment properly, do you mean the case when the tunnel_id string gets somehow corrupted and there's no null terminator on it anymore? In that case I don't see how we could potentially protect ourselves. We could validate that the memory after OVN_MVTEP_CHASSISID_DELIM looks like the string representation of an IP address but there's no guarantee that we're not accessing memory we don't own triggering undefined behavior. Otherwise if the string is valid (i.e., null-terminated) the match++ above would have consumed the delimiter and in the worst case match now points to the address of the null string terminator so it's safe to do the *match == 0 comparison. Please let me know if you had other checks in mind. Thanks, Dumitru > > rest looks good. > > thanks, > > -venu > > > + if (chassis_id) { > + free(*chassis_id); > + } > + return false; > + } > + *encap_ip = xstrdup(match); > + } > + > + return true; > +} > + > +/* > + * Returns true if a given 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) > +{ > + if (strstr(tunnel_id, chassis_id) != tunnel_id) { > + return false; > + } > + > + size_t chassis_id_len = strlen(chassis_id); > + > + if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) { > + return false; > + } > + > + if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) { > + return false; > + } > + > + return true; > +} > + > 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 +178,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 7386404..3e3f731 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" > @@ -77,38 +78,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; > } > > @@ -998,8 +988,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; > } > > @@ -1055,16 +1046,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); > @@ -1085,7 +1070,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, > @@ -1194,7 +1180,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 8ae1f9b..9d1b031 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" > @@ -2722,7 +2723,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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi, Dumitru: On Thursday, May 16, 2019, 2:13:41 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: On Wed, May 15, 2019 at 10:30 PM venugopal iyer <iyervl@ymail.com> wrote: > > Thanks for fixing this! a comment below: > > On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: > > > Add tunnel-id creation and parsing functions in encaps.h. > > Reported-at: https://bugzilla.redhat.com/1708131 > Reported-by: Haidong Li <haili@redhat.com> > Fixes: b520ca7 ("Support for multiple VTEP in OVN") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > ovn/controller/bfd.c | 31 ++++++++------- > ovn/controller/encaps.c | 87 ++++++++++++++++++++++++++++++++++++++++- > ovn/controller/encaps.h | 6 +++ > ovn/controller/ovn-controller.h | 7 ---- > ovn/controller/physical.c | 51 +++++++++--------------- > ovn/controller/pinctrl.c | 4 +- > 6 files changed, 130 insertions(+), 56 deletions(-) > > diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c > index d016e27..b6aef04 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 dcf7810..d467540 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,83 @@ 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) > +{ > + const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM); > + > + if (!match) { > + return false; > + } > + > + if (chassis_id) { > + size_t chassis_id_len = (match - tunnel_id); > + > + *chassis_id = xmemdup0(tunnel_id, chassis_id_len); > + } > + > + /* Consume the tunnel-id delimiter. */ > + match++; > + > + if (encap_ip) { > + /* > + * If the value has morphed into something other than > + * <chassis-id><delim><encap-ip>, fail and free already allocated > + * memory (i.e., chassis_id). > + */ > + if (*match == 0) { > > If the OVN_..._DELIM happens to be the last char in the tunnel_id (i.e. if somehow it > is corrupted), is this safe given we have gone beyond it? Hi Venu, Thanks for taking the time to review this change. Just to make sure I understood your comment properly, do you mean the case when the tunnel_id string gets somehow corrupted and there's no null terminator on it anymore? In that case I don't see how we could potentially protect ourselves. We could validate that the memory after OVN_MVTEP_CHASSISID_DELIM looks like the string representation of an IP address but there's no guarantee that we're not accessing memory we don't own triggering undefined behavior. Otherwise if the string is valid (i.e., null-terminated) the match++ above would have consumed the delimiter and in the worst case match now points to the address of the null string terminator so it's safe to do the *match == 0 comparison. <vi>The latter is what i wanted to confirm; the former, as you mention, we probably<vi> can't do much. Please let me know if you had other checks in mind. <vi> No, I am fine with change. thanks! -venu Thanks, Dumitru > > rest looks good. > > thanks, > > -venu > > > + if (chassis_id) { > + free(*chassis_id); > + } > + return false; > + } > + *encap_ip = xstrdup(match); > + } > + > + return true; > +} > + > +/* > + * Returns true if a given 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) > +{ > + if (strstr(tunnel_id, chassis_id) != tunnel_id) { > + return false; > + } > + > + size_t chassis_id_len = strlen(chassis_id); > + > + if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) { > + return false; > + } > + > + if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) { > + return false; > + } > + > + return true; > +} > + > 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 +178,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 7386404..3e3f731 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" > @@ -77,38 +78,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; > } > > @@ -998,8 +988,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; > } > > @@ -1055,16 +1046,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); > @@ -1085,7 +1070,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, > @@ -1194,7 +1180,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 8ae1f9b..9d1b031 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" > @@ -2722,7 +2723,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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, May 16, 2019 at 10:59 PM venugopal iyer <iyervl@ymail.com> wrote: > > Hi, Dumitru: > > On Thursday, May 16, 2019, 2:13:41 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: > > > On Wed, May 15, 2019 at 10:30 PM venugopal iyer <iyervl@ymail.com> wrote: > > > > Thanks for fixing this! a comment below: > > > > On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: > > > > > > Add tunnel-id creation and parsing functions in encaps.h. > > > > Reported-at: https://bugzilla.redhat.com/1708131 > > Reported-by: Haidong Li <haili@redhat.com> > > Fixes: b520ca7 ("Support for multiple VTEP in OVN") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > ovn/controller/bfd.c | 31 ++++++++------- > > ovn/controller/encaps.c | 87 ++++++++++++++++++++++++++++++++++++++++- > > ovn/controller/encaps.h | 6 +++ > > ovn/controller/ovn-controller.h | 7 ---- > > ovn/controller/physical.c | 51 +++++++++--------------- > > ovn/controller/pinctrl.c | 4 +- > > 6 files changed, 130 insertions(+), 56 deletions(-) > > > > diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c > > index d016e27..b6aef04 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 dcf7810..d467540 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,83 @@ 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) > > +{ > > + const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM); > > + > > + if (!match) { > > + return false; > > + } > > + > > + if (chassis_id) { > > + size_t chassis_id_len = (match - tunnel_id); > > + > > + *chassis_id = xmemdup0(tunnel_id, chassis_id_len); > > + } > > + > > + /* Consume the tunnel-id delimiter. */ > > + match++; > > + > > + if (encap_ip) { > > + /* > > + * If the value has morphed into something other than > > + * <chassis-id><delim><encap-ip>, fail and free already allocated > > + * memory (i.e., chassis_id). > > + */ > > + if (*match == 0) { > > > > If the OVN_..._DELIM happens to be the last char in the tunnel_id (i.e. if somehow it > > is corrupted), is this safe given we have gone beyond it? > > Hi Venu, > > Thanks for taking the time to review this change. > > Just to make sure I understood your comment properly, do you mean the > case when the tunnel_id string gets somehow corrupted and there's no > null terminator on it anymore? > In that case I don't see how we could potentially protect ourselves. > We could validate that the memory after OVN_MVTEP_CHASSISID_DELIM > looks like the string representation of an IP address but there's no > guarantee that we're not accessing memory we don't own triggering > undefined behavior. > Otherwise if the string is valid (i.e., null-terminated) the match++ > above would have consumed the delimiter and in the worst case match > now points to the address of the null string terminator so it's safe > to do the *match == 0 comparison. > > <vi>The latter is what i wanted to confirm; the former, as you mention, we probably > <vi> can't do much. > > Please let me know if you had other checks in mind. > > <vi> No, I am fine with change. > > thanks! > > -venu Hi Venu, Could you please formally Ack the change then? Thanks, Dumitru > > > Thanks, > Dumitru > > > > > rest looks good. > > > > thanks, > > > > -venu > > > > > > + if (chassis_id) { > > + free(*chassis_id); > > + } > > + return false; > > + } > > + *encap_ip = xstrdup(match); > > + } > > + > > + return true; > > +} > > + > > +/* > > + * Returns true if a given 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) > > +{ > > + if (strstr(tunnel_id, chassis_id) != tunnel_id) { > > + return false; > > + } > > + > > + size_t chassis_id_len = strlen(chassis_id); > > + > > + if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) { > > + return false; > > + } > > + > > + if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) { > > + return false; > > + } > > + > > + return true; > > +} > > + > > 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 +178,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 7386404..3e3f731 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" > > @@ -77,38 +78,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; > > } > > > > @@ -998,8 +988,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; > > } > > > > @@ -1055,16 +1046,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); > > @@ -1085,7 +1070,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, > > @@ -1194,7 +1180,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 8ae1f9b..9d1b031 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" > > @@ -2722,7 +2723,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 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wednesday, May 22, 2019, 1:05:30 AM PDT, Dumitru Ceara <dceara@redhat.com> wrote: On Thu, May 16, 2019 at 10:59 PM venugopal iyer <iyervl@ymail.com> wrote: > > Hi, Dumitru: > > On Thursday, May 16, 2019, 2:13:41 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: > > > On Wed, May 15, 2019 at 10:30 PM venugopal iyer <iyervl@ymail.com> wrote: > > > > Thanks for fixing this! a comment below: > > > > On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara <dceara@redhat.com> wrote: > > > > > > Add tunnel-id creation and parsing functions in encaps.h. > > > > Reported-at: https://bugzilla.redhat.com/1708131 > > Reported-by: Haidong Li <haili@redhat.com> > > Fixes: b520ca7 ("Support for multiple VTEP in OVN") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > ovn/controller/bfd.c | 31 ++++++++------- > > ovn/controller/encaps.c | 87 ++++++++++++++++++++++++++++++++++++++++- > > ovn/controller/encaps.h | 6 +++ > > ovn/controller/ovn-controller.h | 7 ---- > > ovn/controller/physical.c | 51 +++++++++--------------- > > ovn/controller/pinctrl.c | 4 +- > > 6 files changed, 130 insertions(+), 56 deletions(-) > > > > diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c > > index d016e27..b6aef04 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 dcf7810..d467540 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,83 @@ 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) > > +{ > > + const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM); > > + > > + if (!match) { > > + return false; > > + } > > + > > + if (chassis_id) { > > + size_t chassis_id_len = (match - tunnel_id); > > + > > + *chassis_id = xmemdup0(tunnel_id, chassis_id_len); > > + } > > + > > + /* Consume the tunnel-id delimiter. */ > > + match++; > > + > > + if (encap_ip) { > > + /* > > + * If the value has morphed into something other than > > + * <chassis-id><delim><encap-ip>, fail and free already allocated > > + * memory (i.e., chassis_id). > > + */ > > + if (*match == 0) { > > > > If the OVN_..._DELIM happens to be the last char in the tunnel_id (i.e. if somehow it > > is corrupted), is this safe given we have gone beyond it? > > Hi Venu, > > Thanks for taking the time to review this change. > > Just to make sure I understood your comment properly, do you mean the > case when the tunnel_id string gets somehow corrupted and there's no > null terminator on it anymore? > In that case I don't see how we could potentially protect ourselves. > We could validate that the memory after OVN_MVTEP_CHASSISID_DELIM > looks like the string representation of an IP address but there's no > guarantee that we're not accessing memory we don't own triggering > undefined behavior. > Otherwise if the string is valid (i.e., null-terminated) the match++ > above would have consumed the delimiter and in the worst case match > now points to the address of the null string terminator so it's safe > to do the *match == 0 comparison. > > <vi>The latter is what i wanted to confirm; the former, as you mention, we probably > <vi> can't do much. > > Please let me know if you had other checks in mind. > > <vi> No, I am fine with change. > > thanks! > > -venu Hi Venu, Could you please formally Ack the change then? Hi, Dumitru: <vi> Changes look good to me. <vi> Acked-by: Venu Iyer <iyervl@ymail.com> thanks, -venu Thanks, Dumitru > > > Thanks, > Dumitru > > > > > rest looks good. > > > > thanks, > > > > -venu > > > > > > + if (chassis_id) { > > + free(*chassis_id); > > + } > > + return false; > > + } > > + *encap_ip = xstrdup(match); > > + } > > + > > + return true; > > +} > > + > > +/* > > + * Returns true if a given 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) > > +{ > > + if (strstr(tunnel_id, chassis_id) != tunnel_id) { > > + return false; > > + } > > + > > + size_t chassis_id_len = strlen(chassis_id); > > + > > + if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) { > > + return false; > > + } > > + > > + if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) { > > + return false; > > + } > > + > > + return true; > > +} > > + > > 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 +178,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 7386404..3e3f731 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" > > @@ -77,38 +78,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; > > } > > > > @@ -998,8 +988,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; > > } > > > > @@ -1055,16 +1046,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); > > @@ -1085,7 +1070,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, > > @@ -1194,7 +1180,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 8ae1f9b..9d1b031 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" > > @@ -2722,7 +2723,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 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, May 13, 2019 at 01:30:59PM +0200, Dumitru Ceara wrote: > Add tunnel-id creation and parsing functions in encaps.h. > > Reported-at: https://bugzilla.redhat.com/1708131 > Reported-by: Haidong Li <haili@redhat.com> > Fixes: b520ca7 ("Support for multiple VTEP in OVN") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Please add a description of the problem that this fixes to the commit message. I know that the bug report probably describes it, but it's best if the commit log makes sense by itself. Thanks, Ben.
On Thu, May 23, 2019 at 11:56 PM Ben Pfaff <blp@ovn.org> wrote: > > On Mon, May 13, 2019 at 01:30:59PM +0200, Dumitru Ceara wrote: > > Add tunnel-id creation and parsing functions in encaps.h. > > > > Reported-at: https://bugzilla.redhat.com/1708131 > > Reported-by: Haidong Li <haili@redhat.com> > > Fixes: b520ca7 ("Support for multiple VTEP in OVN") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Please add a description of the problem that this fixes to the commit > message. I know that the bug report probably describes it, but it's > best if the commit log makes sense by itself. > Sorry, my bad. I sent a v2 patch now with an updated commit message that supplies all the info. Thanks, Dumitru > Thanks, > > Ben.
diff --git a/ovn/controller/bfd.c b/ovn/controller/bfd.c index d016e27..b6aef04 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 dcf7810..d467540 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,83 @@ 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) +{ + const char *match = strchr(tunnel_id, OVN_MVTEP_CHASSISID_DELIM); + + if (!match) { + return false; + } + + if (chassis_id) { + size_t chassis_id_len = (match - tunnel_id); + + *chassis_id = xmemdup0(tunnel_id, chassis_id_len); + } + + /* Consume the tunnel-id delimiter. */ + match++; + + if (encap_ip) { + /* + * If the value has morphed into something other than + * <chassis-id><delim><encap-ip>, fail and free already allocated + * memory (i.e., chassis_id). + */ + if (*match == 0) { + if (chassis_id) { + free(*chassis_id); + } + return false; + } + *encap_ip = xstrdup(match); + } + + return true; +} + +/* + * Returns true if a given 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) +{ + if (strstr(tunnel_id, chassis_id) != tunnel_id) { + return false; + } + + size_t chassis_id_len = strlen(chassis_id); + + if (tunnel_id[chassis_id_len] != OVN_MVTEP_CHASSISID_DELIM) { + return false; + } + + if (encap_ip && strcmp(&tunnel_id[chassis_id_len + 1], encap_ip)) { + return false; + } + + return true; +} + 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 +178,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 7386404..3e3f731 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" @@ -77,38 +78,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; } @@ -998,8 +988,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; } @@ -1055,16 +1046,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); @@ -1085,7 +1070,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, @@ -1194,7 +1180,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 8ae1f9b..9d1b031 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" @@ -2722,7 +2723,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,
Add tunnel-id creation and parsing functions in encaps.h. Reported-at: https://bugzilla.redhat.com/1708131 Reported-by: Haidong Li <haili@redhat.com> Fixes: b520ca7 ("Support for multiple VTEP in OVN") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- ovn/controller/bfd.c | 31 ++++++++------- ovn/controller/encaps.c | 87 ++++++++++++++++++++++++++++++++++++++++- ovn/controller/encaps.h | 6 +++ ovn/controller/ovn-controller.h | 7 ---- ovn/controller/physical.c | 51 +++++++++--------------- ovn/controller/pinctrl.c | 4 +- 6 files changed, 130 insertions(+), 56 deletions(-)