[ovs-dev] [PATCH] ovn-controller: Fix parsing of OVN tunnel IDs
Dumitru Ceara
dceara at redhat.com
Thu May 16 07:12:56 UTC 2019
On Wed, May 15, 2019 at 10:30 PM venugopal iyer <iyervl at ymail.com> wrote:
>
> Thanks for fixing this! a comment below:
>
> On Monday, May 13, 2019, 6:31:28 AM CDT, Dumitru Ceara <dceara at 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 at redhat.com>
> Fixes: b520ca7 ("Support for multiple VTEP in OVN")
> Signed-off-by: Dumitru Ceara <dceara at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list