[ovs-dev] [PATCH 2/3] ovn: Encode dhcpv6 PACKET_IN userdata as big endian.

Numan Siddique nusiddiq at redhat.com
Fri Dec 9 13:44:57 UTC 2016


On Fri, Dec 9, 2016 at 8:20 AM, Daniele Di Proietto <diproiettod at vmware.com>
wrote:

> The packet in userdata generated by ovn-controller when translating the
> put_dhcpv6_opt action includes 16-bit integers.
>
> Currently these 16-bit integers are encoded with native endianness.
> This is ok becase the only consumer of that userdata is ovn-controller
> itself, but it means that the OpenFlow action we're generating might
> not really be the same on different hosts.
>
> I think it's better to encode the userdata as big-endian, like the rest
> of OpenFlow messages.
>
> This fixes a test failure on big-endian platforms, because the generated
> OpenFlow bytes were different than expected (the expectation was
> generated on a little endian machine).
>
> Now 'struct dhcp_opt6_header' is identical to 'struct
> dhcpv6_opt_header', but I chose to keep them separate, because they
> have different purposes.  I also renamed the members to avoid confusion.
>
> I haven't tested this on a real setup.
>
> CC: Numan Siddique <nusiddiq at redhat.com>
> Reported-at: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=840770
> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
> ---
>


​Thanks. I tested it and it works as expected.
Acked-by: Numan Siddique <nusiddiq at redhat.com>


​


>  ovn/controller/pinctrl.c | 28 +++++++++++++---------------
>  ovn/lib/actions.c        | 19 ++++++++++++-------
>  ovn/lib/ovn-dhcp.h       |  5 +++--
>  tests/ovn.at             |  8 ++++----
>  4 files changed, 32 insertions(+), 28 deletions(-)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index db9e441..849fbce 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -385,13 +385,13 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata,
>              return false;
>          }
>
> -        uint8_t *userdata_opt_data = ofpbuf_try_pull(userdata,
> -                                                     userdata_opt->len);
> +        size_t size = ntohs(userdata_opt->size);
> +        uint8_t *userdata_opt_data = ofpbuf_try_pull(userdata, size);
>          if (!userdata_opt_data) {
>              return false;
>          }
>
> -        switch (userdata_opt->code) {
> +        switch (ntohs(userdata_opt->opt_code)) {
>          case DHCPV6_OPT_SERVER_ID_CODE:
>          {
>              /* The Server Identifier option carries a DUID
> @@ -405,7 +405,7 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata,
>                  out_dhcpv6_opts, sizeof *opt_server_id);
>
>              opt_server_id->opt.code = htons(DHCPV6_OPT_SERVER_ID_CODE);
> -            opt_server_id->opt.len = htons(userdata_opt->len + 4);
> +            opt_server_id->opt.len = htons(size + 4);
>              opt_server_id->duid_type = htons(DHCPV6_DUID_LL);
>              opt_server_id->hw_type = htons(DHCPV6_HW_TYPE_ETH);
>              memcpy(&opt_server_id->mac, userdata_opt_data,
> @@ -415,7 +415,7 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata,
>
>          case DHCPV6_OPT_IA_ADDR_CODE:
>          {
> -            if (userdata_opt->len != sizeof(struct in6_addr)) {
> +            if (size != sizeof(struct in6_addr)) {
>                  return false;
>              }
>
> @@ -444,9 +444,8 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata,
>              struct dhcpv6_opt_ia_addr *opt_ia_addr = ofpbuf_put_zeros(
>                  out_dhcpv6_opts, sizeof *opt_ia_addr);
>              opt_ia_addr->opt.code = htons(DHCPV6_OPT_IA_ADDR_CODE);
> -            opt_ia_addr->opt.len = htons(userdata_opt->len + 8);
> -            memcpy(opt_ia_addr->ipv6.s6_addr, userdata_opt_data,
> -                   userdata_opt->len);
> +            opt_ia_addr->opt.len = htons(size + 8);
> +            memcpy(opt_ia_addr->ipv6.s6_addr, userdata_opt_data, size);
>              opt_ia_addr->t1 = OVS_BE32_MAX;
>              opt_ia_addr->t2 = OVS_BE32_MAX;
>              break;
> @@ -457,8 +456,8 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata,
>              struct dhcpv6_opt_header *opt_dns = ofpbuf_put_zeros(
>                  out_dhcpv6_opts, sizeof *opt_dns);
>              opt_dns->code = htons(DHCPV6_OPT_DNS_SERVER_CODE);
> -            opt_dns->len = htons(userdata_opt->len);
> -            ofpbuf_put(out_dhcpv6_opts, userdata_opt_data,
> userdata_opt->len);
> +            opt_dns->len = htons(size);
> +            ofpbuf_put(out_dhcpv6_opts, userdata_opt_data, size);
>              break;
>          }
>
> @@ -467,11 +466,10 @@ compose_out_dhcpv6_opts(struct ofpbuf *userdata,
>              struct dhcpv6_opt_header *opt_dsl = ofpbuf_put_zeros(
>                  out_dhcpv6_opts, sizeof *opt_dsl);
>              opt_dsl->code = htons(DHCPV6_OPT_DOMAIN_SEARCH_CODE);
> -            opt_dsl->len = htons(userdata_opt->len + 2);
> -            uint8_t *data = ofpbuf_put_zeros(out_dhcpv6_opts,
> -                                              userdata_opt->len + 2);
> -            *data = userdata_opt->len;
> -            memcpy(data + 1, userdata_opt_data, userdata_opt->len);
> +            opt_dsl->len = htons(size + 2);
> +            uint8_t *data = ofpbuf_put_zeros(out_dhcpv6_opts, size + 2);
> +            *data = size;
> +            memcpy(data + 1, userdata_opt_data, size);
>              break;
>          }
>
> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> index df526c0..fa8f175 100644
> --- a/ovn/lib/actions.c
> +++ b/ovn/lib/actions.c
> @@ -1527,21 +1527,26 @@ encode_put_dhcpv6_option(const struct
> ovnact_dhcp_option *o,
>                           struct ofpbuf *ofpacts)
>  {
>      struct dhcp_opt6_header *opt = ofpbuf_put_uninit(ofpacts, sizeof
> *opt);
> -    opt->code = o->option->code;
> -
>      const union expr_constant *c = o->value.values;
>      size_t n_values = o->value.n_values;
> +    size_t size;
> +
> +    opt->opt_code = htons(o->option->code);
> +
>      if (!strcmp(o->option->type, "ipv6")) {
> -        opt->len = n_values * sizeof(struct in6_addr);
> +        size = n_values * sizeof(struct in6_addr);
> +        opt->size = htons(size);
>          for (size_t i = 0; i < n_values; i++) {
>              ofpbuf_put(ofpacts, &c[i].value.ipv6, sizeof(struct
> in6_addr));
>          }
>      } else if (!strcmp(o->option->type, "mac")) {
> -        opt->len = sizeof(struct eth_addr);
> -        ofpbuf_put(ofpacts, &c->value.mac, opt->len);
> +        size = sizeof(struct eth_addr);
> +        opt->size = htons(size);
> +        ofpbuf_put(ofpacts, &c->value.mac, size);
>      } else if (!strcmp(o->option->type, "str")) {
> -        opt->len = strlen(c->string);
> -        ofpbuf_put(ofpacts, c->string, opt->len);
> +        size = strlen(c->string);
> +        opt->size = htons(size);
> +        ofpbuf_put(ofpacts, c->string, size);
>      }
>  }
>
> diff --git a/ovn/lib/ovn-dhcp.h b/ovn/lib/ovn-dhcp.h
> index ecfcc03..d5561ed 100644
> --- a/ovn/lib/ovn-dhcp.h
> +++ b/ovn/lib/ovn-dhcp.h
> @@ -109,9 +109,10 @@ dhcp_opts_destroy(struct hmap *dhcp_opts)
>      hmap_destroy(dhcp_opts);
>  }
>
> +/* Used in the OpenFlow PACKET_IN userdata */
>  struct dhcp_opt6_header {
> -    uint16_t code;
> -    uint16_t len;
> +    ovs_be16 opt_code;
> +    ovs_be16 size;
>  };
>
>  /* Supported DHCPv6 Message Types */
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 38fd1e5..cb21210 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -950,17 +950,17 @@ put_nd(inport, nd.target, nd.sll);
>
>  # put_dhcpv6_opts
>  reg1[0] = put_dhcpv6_opts(ia_addr = ae70::4, server_id =
> 00:00:00:00:10:02);
> -    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.05.00.10.00.ae.70.00.
> 00.00.00.00.00.00.00.00.00.00.00.00.04.02.00.06.00.00.00.00.
> 00.10.02,pause)
> +    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.00.05.00.10.ae.70.00.
> 00.00.00.00.00.00.00.00.00.00.00.00.04.00.02.00.06.00.00.00.
> 00.10.02,pause)
>  reg1[0] = put_dhcpv6_opts();
>      encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40,pause)
>  reg1[0] = put_dhcpv6_opts(dns_server={ae70::1,ae70::2});
>      formats as reg1[0] = put_dhcpv6_opts(dns_server = {ae70::1, ae70::2});
> -    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.17.00.20.00.ae.70.00.
> 00.00.00.00.00.00.00.00.00.00.00.00.01.ae.70.00.00.00.00.00.
> 00.00.00.00.00.00.00.00.02,pause)
> +    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.00.17.00.20.ae.70.00.
> 00.00.00.00.00.00.00.00.00.00.00.00.01.ae.70.00.00.00.00.00.
> 00.00.00.00.00.00.00.00.02,pause)
>  reg1[0] = put_dhcpv6_opts(server_id=12:34:56:78:9a:bc,
> dns_server={ae70::1,ae89::2});
>      formats as reg1[0] = put_dhcpv6_opts(server_id = 12:34:56:78:9a:bc,
> dns_server = {ae70::1, ae89::2});
> -    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.02.00.06.00.12.34.56.
> 78.9a.bc.17.00.20.00.ae.70.00.00.00.00.00.00.00.00.00.00.00.
> 00.00.01.ae.89.00.00.00.00.00.00.00.00.00.00.00.00.00.02,pause)
> +    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.00.02.00.06.12.34.56.
> 78.9a.bc.00.17.00.20.ae.70.00.00.00.00.00.00.00.00.00.00.00.
> 00.00.01.ae.89.00.00.00.00.00.00.00.00.00.00.00.00.00.02,pause)
>  reg1[0] = put_dhcpv6_opts(domain_search = "ovn.org");
> -    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.18.00.07.00.6f.76.6e.
> 2e.6f.72.67,pause)
> +    encodes as controller(userdata=00.00.00.
> 05.00.00.00.00.00.01.de.10.00.00.00.40.00.18.00.07.6f.76.6e.
> 2e.6f.72.67,pause)
>  reg1[0] = put_dhcpv6_opts(x = 1.2.3.4);
>      Syntax error at `x' expecting DHCPv6 option name.
>  reg1[0] = put_dhcpv6_opts(ia_addr=ae70::4, "hi");
> --
> 2.10.2
>
>


More information about the dev mailing list