diff mbox

[ovs-dev,IPv6,v2,02/10] packets: Cleanup ND compose functions.

Message ID 1469773580-33112-2-git-send-email-jpettit@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit July 29, 2016, 6:26 a.m. UTC 
Rename "compose_nd" and "compose_na" to "compose_nd_ns" and
"compose_nd_na", respecively, to be clearer about their functionality.
This will also make it more consistent when we add Neighbor Discover
Router Solicitation/Advertisement compose functions.

Also change the source and destination IPv6 addresses to take
"struct in6_addr" arguments, which are more common in the code base.

Signed-off-by: Justin Pettit <jpettit@ovn.org>

---
v1->v2: Renamed functions to be consistent when adding more ND messages.
        Fix alignment issues introduced with taking "struct in6_addr".

 lib/packets.c                | 44 +++++++++++++++++++++++++-------------------
 lib/packets.h                | 14 ++++++++------
 ofproto/ofproto-dpif-xlate.c |  2 +-
 ovn/controller/pinctrl.c     | 11 +++--------
 4 files changed, 37 insertions(+), 34 deletions(-)

Comments

Ben Pfaff July 29, 2016, 4:49 p.m. UTC | #1 
On Thu, Jul 28, 2016 at 11:26:12PM -0700, Justin Pettit wrote:
> Rename "compose_nd" and "compose_na" to "compose_nd_ns" and
> "compose_nd_na", respecively, to be clearer about their functionality.
> This will also make it more consistent when we add Neighbor Discover
> Router Solicitation/Advertisement compose functions.
> 
> Also change the source and destination IPv6 addresses to take
> "struct in6_addr" arguments, which are more common in the code base.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> ---
> v1->v2: Renamed functions to be consistent when adding more ND messages.
>         Fix alignment issues introduced with taking "struct in6_addr".

I don't think that the ALIGNED_CASTs in compose_nd_ns() and
compose_nd_na() are necessarily safe, on a system that lacks s6_addr32.

Other than that,
Acked-by: Ben Pfaff <blp@ovn.org>

Thanks,

Ben.
diff mbox

Patch

diff --git a/lib/packets.c b/lib/packets.c
index 1bf887e..b2316e7 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -1319,29 +1319,36 @@  compose_arp__(struct dp_packet *b)
     dp_packet_set_l3(b, arp);
 }
 
-/* This function expect packet with ethernet header with correct
+/* This function expects packet with ethernet header with correct
  * l3 pointer set. */
 static void *
-compose_ipv6(struct dp_packet *packet, uint8_t proto, const ovs_be32 src[4],
-             const ovs_be32 dst[4], uint8_t key_tc, ovs_be32 key_fl,
-             uint8_t key_hl, int size)
+compose_ipv6(struct dp_packet *packet, uint8_t proto,
+             const struct in6_addr *src, const struct in6_addr *dst,
+             uint8_t key_tc, ovs_be32 key_fl, uint8_t key_hl, int size)
 {
     struct ip6_hdr *nh;
     void *data;
 
+    /* Copy 'src' and 'dst' to temporary buffers to prevent misaligned
+     * accesses. */
+    ovs_be32 sbuf[4], dbuf[4];
+    memcpy(sbuf, src, sizeof sbuf);
+    memcpy(dbuf, dst, sizeof dbuf);
+
     nh = dp_packet_l3(packet);
     nh->ip6_vfc = 0x60;
     nh->ip6_nxt = proto;
     nh->ip6_plen = htons(size);
     data = dp_packet_put_zeros(packet, size);
     dp_packet_set_l4(packet, data);
-    packet_set_ipv6(packet, src, dst, key_tc, key_fl, key_hl);
+    packet_set_ipv6(packet, sbuf, dbuf, key_tc, key_fl, key_hl);
     return data;
 }
 
+/* Compose an IPv6 Neighbor Discovery Neighbor Solicitation message. */
 void
-compose_nd(struct dp_packet *b, const struct eth_addr eth_src,
-           struct in6_addr *ipv6_src, struct in6_addr *ipv6_dst)
+compose_nd_ns(struct dp_packet *b, const struct eth_addr eth_src,
+              const struct in6_addr *ipv6_src, const struct in6_addr *ipv6_dst)
 {
     struct in6_addr sn_addr;
     struct eth_addr eth_dst;
@@ -1353,11 +1360,8 @@  compose_nd(struct dp_packet *b, const struct eth_addr eth_src,
     ipv6_multicast_to_ethernet(&eth_dst, &sn_addr);
 
     eth_compose(b, eth_dst, eth_src, ETH_TYPE_IPV6, IPV6_HEADER_LEN);
-    ns = compose_ipv6(b, IPPROTO_ICMPV6,
-                      ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr),
-                      ALIGNED_CAST(ovs_be32 *, sn_addr.s6_addr),
-                      0, 0, 255,
-                      ND_MSG_LEN + ND_OPT_LEN);
+    ns = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, &sn_addr,
+                      0, 0, 255, ND_MSG_LEN + ND_OPT_LEN);
 
     ns->icmph.icmp6_type = ND_NEIGHBOR_SOLICIT;
     ns->icmph.icmp6_code = 0;
@@ -1375,19 +1379,20 @@  compose_nd(struct dp_packet *b, const struct eth_addr eth_src,
                                                       ND_MSG_LEN + ND_OPT_LEN));
 }
 
+/* Compose an IPv6 Neighbor Discovery Neighbor Advertisement message. */
 void
-compose_na(struct dp_packet *b,
-           const struct eth_addr eth_src, const struct eth_addr eth_dst,
-           const ovs_be32 ipv6_src[4], const ovs_be32 ipv6_dst[4],
-           ovs_be32 rso_flags)
+compose_nd_na(struct dp_packet *b,
+              const struct eth_addr eth_src, const struct eth_addr eth_dst,
+              const struct in6_addr *ipv6_src, const struct in6_addr *ipv6_dst,
+              ovs_be32 rso_flags)
 {
     struct ovs_nd_msg *na;
     struct ovs_nd_opt *nd_opt;
     uint32_t icmp_csum;
 
     eth_compose(b, eth_dst, eth_src, ETH_TYPE_IPV6, IPV6_HEADER_LEN);
-    na = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, ipv6_dst, 0, 0, 255,
-                      ND_MSG_LEN + ND_OPT_LEN);
+    na = compose_ipv6(b, IPPROTO_ICMPV6, ipv6_src, ipv6_dst,
+                      0, 0, 255, ND_MSG_LEN + ND_OPT_LEN);
 
     na->icmph.icmp6_type = ND_NEIGHBOR_ADVERT;
     na->icmph.icmp6_code = 0;
@@ -1397,7 +1402,8 @@  compose_na(struct dp_packet *b,
     nd_opt->nd_opt_type = ND_OPT_TARGET_LINKADDR;
     nd_opt->nd_opt_len = 1;
 
-    packet_set_nd(b, ipv6_src, eth_addr_zero, eth_src);
+    packet_set_nd(b, ALIGNED_CAST(ovs_be32 *, ipv6_src->s6_addr),
+                  eth_addr_zero, eth_src);
     na->icmph.icmp6_cksum = 0;
     icmp_csum = packet_csum_pseudoheader6(dp_packet_l3(b));
     na->icmph.icmp6_cksum = csum_finish(csum_continue(icmp_csum, na,
diff --git a/lib/packets.h b/lib/packets.h
index 6ab235a..9b98b29 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -1099,12 +1099,14 @@  void compose_arp(struct dp_packet *, uint16_t arp_op,
                  const struct eth_addr arp_sha,
                  const struct eth_addr arp_tha, bool broadcast,
                  ovs_be32 arp_spa, ovs_be32 arp_tpa);
-void compose_nd(struct dp_packet *, const struct eth_addr eth_src,
-                struct in6_addr *, struct in6_addr *);
-void compose_na(struct dp_packet *,
-                const struct eth_addr eth_src, const struct eth_addr eth_dst,
-                const ovs_be32 ipv6_src[4], const ovs_be32 ipv6_dst[4],
-                ovs_be32 rso_flags);
+void compose_nd_ns(struct dp_packet *, const struct eth_addr eth_src,
+                   const struct in6_addr *ipv6_src,
+                   const struct in6_addr *ipv6_dst);
+void compose_nd_na(struct dp_packet *, const struct eth_addr eth_src,
+                   const struct eth_addr eth_dst,
+                   const struct in6_addr *ipv6_src,
+                   const struct in6_addr *ipv6_dst,
+                   ovs_be32 rso_flags);
 uint32_t packet_csum_pseudoheader(const struct ip_header *);
 void IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6);
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 160da5b..0b2ad29 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2875,7 +2875,7 @@  tnl_send_nd_request(struct xlate_ctx *ctx, const struct xport *out_dev,
     struct dp_packet packet;
 
     dp_packet_init(&packet, 0);
-    compose_nd(&packet, eth_src, ipv6_src, ipv6_dst);
+    compose_nd_ns(&packet, eth_src, ipv6_src, ipv6_dst);
     compose_table_xlate(ctx, out_dev, &packet);
     dp_packet_uninit(&packet);
 }
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 416dad6..06103dd 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -966,18 +966,13 @@  pinctrl_handle_na(const struct flow *ip_flow,
     struct dp_packet packet;
     dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
 
-    ovs_be32 ipv6_src[4], ipv6_dst[4];
-    memcpy(ipv6_dst, &ip_flow->ipv6_src, sizeof ipv6_src);
-    memcpy(ipv6_src, &ip_flow->nd_target, sizeof ipv6_dst);
-
     /* xxx These flags are not exactly correct.  Look at section 7.2.4
      * xxx of RFC 4861.  For example, we need to set ND_RSO_ROUTER for
      * xxx router's interfaces and ND_RSO_SOLICITED only if it was
      * xxx requested. */
-    compose_na(&packet,
-               ip_flow->dl_dst, ip_flow->dl_src,
-               ipv6_src, ipv6_dst,
-               htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
+    compose_nd_na(&packet, ip_flow->dl_dst, ip_flow->dl_src,
+                  &ip_flow->nd_target, &ip_flow->ipv6_src,
+                  htonl(ND_RSO_SOLICITED | ND_RSO_OVERRIDE));
 
     /* Reload previous packet metadata. */
     uint64_t ofpacts_stub[4096 / 8];