[ovs-dev] [PATCH] ovn-controller: Fix parsing of OVN tunnel IDs

Dumitru Ceara dceara at redhat.com
Wed May 22 08:05:15 UTC 2019


On Thu, May 16, 2019 at 10:59 PM venugopal iyer <iyervl at ymail.com> wrote:
>
> Hi, Dumitru:
>
> On Thursday, May 16, 2019, 2:13:41 AM CDT, Dumitru Ceara <dceara at redhat.com> wrote:
>
>
> 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.
>
> <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 at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list