[ovs-dev] flow: Fix using pointer to member of packed struct icmp6_hdr.
diff mbox series

Message ID 20191001170400.21661-1-i.maximets@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] flow: Fix using pointer to member of packed struct icmp6_hdr.
Related show

Commit Message

Ilya Maximets Oct. 1, 2019, 5:04 p.m. UTC
OVS has no structure definition for ICMPv6 header with additional
data. More precisely, it has, but this structure named as
'icmp6_error_header' and only suitable to store error related
extended information.  'flow_compose_l4' stores additional
information in reserved bits by using system defined structure
'icmp6_hdr', which is marked as 'packed' and this leads to
build failure with gcc >= 9:

  lib/flow.c:3041:34: error:
    taking address of packed member of 'struct icmp6_hdr' may result
    in an unaligned pointer value [-Werror=address-of-packed-member]

        uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fix that by renaming 'icmp6_error_header' to 'icmp6_data_header'
and allowing it to store not only errors, but any type of additional
information by analogue with 'struct icmp6_hdr'.
All the usages of 'struct icmp6_hdr' replaced with this new structure.
Removed redundant conversions between network and host representations.
Now fields are always in be.

This also, probably, makes flow_compose_l4 more robust by avoiding
possible unaligned accesses to 32 bit value.

Fixes: 9b2b84973db7 ("Support for match & set ICMPv6 reserved and options type fields")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/conntrack.c |  2 +-
 lib/flow.c      | 77 +++++++++++++++++++++++++------------------------
 lib/packets.h   | 12 +++++---
 3 files changed, 48 insertions(+), 43 deletions(-)

Comments

William Tu Oct. 8, 2019, 4:55 p.m. UTC | #1
On Tue, Oct 01, 2019 at 08:04:00PM +0300, Ilya Maximets wrote:
> OVS has no structure definition for ICMPv6 header with additional
> data. More precisely, it has, but this structure named as
> 'icmp6_error_header' and only suitable to store error related
> extended information.  'flow_compose_l4' stores additional
> information in reserved bits by using system defined structure
> 'icmp6_hdr', which is marked as 'packed' and this leads to
> build failure with gcc >= 9:
> 
>   lib/flow.c:3041:34: error:
>     taking address of packed member of 'struct icmp6_hdr' may result
>     in an unaligned pointer value [-Werror=address-of-packed-member]
> 
>         uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
>                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Fix that by renaming 'icmp6_error_header' to 'icmp6_data_header'
> and allowing it to store not only errors, but any type of additional
> information by analogue with 'struct icmp6_hdr'.
> All the usages of 'struct icmp6_hdr' replaced with this new structure.
> Removed redundant conversions between network and host representations.
> Now fields are always in be.
> 
> This also, probably, makes flow_compose_l4 more robust by avoiding
> possible unaligned accesses to 32 bit value.
> 
> Fixes: 9b2b84973db7 ("Support for match & set ICMPv6 reserved and options type fields")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Looks good to me! Thanks.
Tested on travis and cirrus.

Acked-by: William Tu <u9012063@gmail.com>

> ---
>  lib/conntrack.c |  2 +-
>  lib/flow.c      | 77 +++++++++++++++++++++++++------------------------
>  lib/packets.h   | 12 +++++---
>  3 files changed, 48 insertions(+), 43 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 98c61940b..df7b9fa7a 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -719,7 +719,7 @@ reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
>          icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
>      } else {
>          struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
> -        struct icmp6_error_header *icmp6 = dp_packet_l4(pkt);
> +        struct icmp6_data_header *icmp6 = dp_packet_l4(pkt);
>          struct ovs_16aligned_ip6_hdr *inner_l3_6 =
>              (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1);
>          /* This call is already verified to succeed during the code path from
> diff --git a/lib/flow.c b/lib/flow.c
> index 773f2f7b2..317e3712c 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -399,14 +399,14 @@ parse_ethertype(const void **datap, size_t *sizep)
>   * and 'arp_buf[]' are filled in.  If the packet is not an ND packet, 'false'
>   * is returned and no values are filled in on '*nd_target' or 'arp_buf[]'. */
>  static inline bool
> -parse_icmpv6(const void **datap, size_t *sizep, const struct icmp6_hdr *icmp,
> -             uint32_t *rso_flags, const struct in6_addr **nd_target,
> +parse_icmpv6(const void **datap, size_t *sizep,
> +             const struct icmp6_data_header *icmp6,
> +             ovs_be32 *rso_flags, const struct in6_addr **nd_target,
>               struct eth_addr arp_buf[2], uint8_t *opt_type)
>  {
> -    const uint32_t *reserved;
> -    if (icmp->icmp6_code != 0 ||
> -        (icmp->icmp6_type != ND_NEIGHBOR_SOLICIT &&
> -         icmp->icmp6_type != ND_NEIGHBOR_ADVERT)) {
> +    if (icmp6->icmp6_base.icmp6_code != 0 ||
> +        (icmp6->icmp6_base.icmp6_type != ND_NEIGHBOR_SOLICIT &&
> +         icmp6->icmp6_base.icmp6_type != ND_NEIGHBOR_ADVERT)) {
>          return false;
>      }
>  
> @@ -414,12 +414,7 @@ parse_icmpv6(const void **datap, size_t *sizep, const struct icmp6_hdr *icmp,
>      arp_buf[1] = eth_addr_zero;
>      *opt_type = 0;
>  
> -    reserved = data_try_pull(datap, sizep, sizeof(uint32_t));
> -    if (OVS_UNLIKELY(!reserved)) {
> -        /* Invalid ND packet. */
> -        return false;
> -    }
> -    *rso_flags = *reserved;
> +    *rso_flags = get_16aligned_be32(icmp6->icmp6_data.be32);
>  
>      *nd_target = data_try_pull(datap, sizep, sizeof **nd_target);
>      if (OVS_UNLIKELY(!*nd_target)) {
> @@ -1024,17 +1019,18 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
>                  miniflow_pad_to_64(mf, igmp_group_ip4);
>              }
>          } else if (OVS_LIKELY(nw_proto == IPPROTO_ICMPV6)) {
> -            if (OVS_LIKELY(size >= sizeof(struct icmp6_hdr))) {
> +            if (OVS_LIKELY(size >= sizeof(struct icmp6_data_header))) {
>                  const struct in6_addr *nd_target;
>                  struct eth_addr arp_buf[2];
>                  /* This will populate whether we received Option 1
>                   * or Option 2. */
>                  uint8_t opt_type;
>                  /* This holds the ND Reserved field. */
> -                uint32_t rso_flags;
> -                const struct icmp6_hdr *icmp = data_pull(&data,
> -                                               &size,ICMP6_HEADER_LEN);
> -                if (parse_icmpv6(&data, &size, icmp,
> +                ovs_be32 rso_flags;
> +                const struct icmp6_data_header *icmp6;
> +
> +                icmp6 = data_pull(&data, &size, sizeof *icmp6);
> +                if (parse_icmpv6(&data, &size, icmp6,
>                                   &rso_flags, &nd_target, arp_buf, &opt_type)) {
>                      if (nd_target) {
>                          miniflow_push_words(mf, nd_target, nd_target,
> @@ -1053,16 +1049,20 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
>                           * This will zero out the tcp_flags & pad3 field. */
>                          miniflow_pad_to_64(mf, arp_tha);
>                      }
> -                    miniflow_push_be16(mf, tp_src, htons(icmp->icmp6_type));
> -                    miniflow_push_be16(mf, tp_dst, htons(icmp->icmp6_code));
> +                    miniflow_push_be16(mf, tp_src,
> +                                       htons(icmp6->icmp6_base.icmp6_type));
> +                    miniflow_push_be16(mf, tp_dst,
> +                                       htons(icmp6->icmp6_base.icmp6_code));
>                      miniflow_pad_to_64(mf, tp_dst);
>                      /* Fill ND reserved field. */
> -                    miniflow_push_be32(mf, igmp_group_ip4, htonl(rso_flags));
> +                    miniflow_push_be32(mf, igmp_group_ip4, rso_flags);
>                      miniflow_pad_to_64(mf, igmp_group_ip4);
>                  } else {
>                      /* ICMPv6 but not ND. */
> -                    miniflow_push_be16(mf, tp_src, htons(icmp->icmp6_type));
> -                    miniflow_push_be16(mf, tp_dst, htons(icmp->icmp6_code));
> +                    miniflow_push_be16(mf, tp_src,
> +                                       htons(icmp6->icmp6_base.icmp6_type));
> +                    miniflow_push_be16(mf, tp_dst,
> +                                       htons(icmp6->icmp6_base.icmp6_code));
>                      miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>                      miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>                  }
> @@ -3035,15 +3035,16 @@ flow_compose_l4(struct dp_packet *p, const struct flow *flow,
>              igmp->igmp_code = ntohs(flow->tp_dst);
>              put_16aligned_be32(&igmp->group, flow->igmp_group_ip4);
>          } else if (flow->nw_proto == IPPROTO_ICMPV6) {
> -            struct icmp6_hdr *icmp = dp_packet_put_zeros(p, sizeof *icmp);
> -            icmp->icmp6_type = ntohs(flow->tp_src);
> -            icmp->icmp6_code = ntohs(flow->tp_dst);
> -            uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
> -            *reserved = ntohl(flow->igmp_group_ip4);
> -
> -            if (icmp->icmp6_code == 0 &&
> -                (icmp->icmp6_type == ND_NEIGHBOR_SOLICIT ||
> -                 icmp->icmp6_type == ND_NEIGHBOR_ADVERT)) {
> +            struct icmp6_data_header *icmp6;
> +
> +            icmp6 = dp_packet_put_zeros(p, sizeof *icmp6);
> +            icmp6->icmp6_base.icmp6_type = ntohs(flow->tp_src);
> +            icmp6->icmp6_base.icmp6_code = ntohs(flow->tp_dst);
> +            put_16aligned_be32(icmp6->icmp6_data.be32, flow->igmp_group_ip4);
> +
> +            if (icmp6->icmp6_base.icmp6_code == 0 &&
> +                (icmp6->icmp6_base.icmp6_type == ND_NEIGHBOR_SOLICIT ||
> +                 icmp6->icmp6_base.icmp6_type == ND_NEIGHBOR_ADVERT)) {
>                  struct in6_addr *nd_target;
>                  struct ovs_nd_lla_opt *lla_opt;
>  
> @@ -3062,9 +3063,9 @@ flow_compose_l4(struct dp_packet *p, const struct flow *flow,
>                      lla_opt->type = ND_OPT_TARGET_LINKADDR;
>                      lla_opt->mac = flow->arp_tha;
>                  }
> -            } else if (icmp->icmp6_code == 0 &&
> -                       (icmp->icmp6_type == ICMP6_ECHO_REQUEST ||
> -                        icmp->icmp6_type == ICMP6_ECHO_REPLY)) {
> +            } else if (icmp6->icmp6_base.icmp6_code == 0 &&
> +                       (icmp6->icmp6_base.icmp6_type == ICMP6_ECHO_REQUEST ||
> +                        icmp6->icmp6_base.icmp6_type == ICMP6_ECHO_REPLY)) {
>                  flow_compose_l7(p, l7, l7_len);
>              } else {
>                  /* XXX Add inner IP packet for e.g. destination unreachable? */
> @@ -3109,11 +3110,11 @@ flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow,
>              igmp->igmp_csum = 0;
>              igmp->igmp_csum = csum(igmp, l4_len);
>          } else if (flow->nw_proto == IPPROTO_ICMPV6) {
> -            struct icmp6_hdr *icmp = dp_packet_l4(p);
> +            struct icmp6_data_header *icmp6 = dp_packet_l4(p);
>  
> -            icmp->icmp6_cksum = 0;
> -            icmp->icmp6_cksum = (OVS_FORCE uint16_t)
> -                csum_finish(csum_continue(pseudo_hdr_csum, icmp, l4_len));
> +            icmp6->icmp6_base.icmp6_cksum = 0;
> +            icmp6->icmp6_base.icmp6_cksum =
> +                csum_finish(csum_continue(pseudo_hdr_csum, icmp6, l4_len));
>          }
>      }
>  }
> diff --git a/lib/packets.h b/lib/packets.h
> index c78defb89..d19f6e3ca 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -996,12 +996,16 @@ struct icmp6_header {
>  };
>  BUILD_ASSERT_DECL(ICMP6_HEADER_LEN == sizeof(struct icmp6_header));
>  
> -#define ICMP6_ERROR_HEADER_LEN 8
> -struct icmp6_error_header {
> +#define ICMP6_DATA_HEADER_LEN 8
> +struct icmp6_data_header {
>      struct icmp6_header icmp6_base;
> -    ovs_be32 icmp6_error_ext;
> +    union {
> +        ovs_16aligned_be32 be32[1];
> +        ovs_be16           be16[2];
> +        uint8_t            u8[4];
> +    } icmp6_data;
>  };
> -BUILD_ASSERT_DECL(ICMP6_ERROR_HEADER_LEN == sizeof(struct icmp6_error_header));
> +BUILD_ASSERT_DECL(ICMP6_DATA_HEADER_LEN == sizeof(struct icmp6_data_header));
>  
>  uint32_t packet_csum_pseudoheader6(const struct ovs_16aligned_ip6_hdr *);
>  ovs_be16 packet_csum_upperlayer6(const struct ovs_16aligned_ip6_hdr *,
> -- 
> 2.17.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Ilya Maximets Oct. 9, 2019, 4:54 p.m. UTC | #2
On 08.10.2019 18:55, William Tu wrote:
> On Tue, Oct 01, 2019 at 08:04:00PM +0300, Ilya Maximets wrote:
>> OVS has no structure definition for ICMPv6 header with additional
>> data. More precisely, it has, but this structure named as
>> 'icmp6_error_header' and only suitable to store error related
>> extended information.  'flow_compose_l4' stores additional
>> information in reserved bits by using system defined structure
>> 'icmp6_hdr', which is marked as 'packed' and this leads to
>> build failure with gcc >= 9:
>>
>>    lib/flow.c:3041:34: error:
>>      taking address of packed member of 'struct icmp6_hdr' may result
>>      in an unaligned pointer value [-Werror=address-of-packed-member]
>>
>>          uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
>>                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Fix that by renaming 'icmp6_error_header' to 'icmp6_data_header'
>> and allowing it to store not only errors, but any type of additional
>> information by analogue with 'struct icmp6_hdr'.
>> All the usages of 'struct icmp6_hdr' replaced with this new structure.
>> Removed redundant conversions between network and host representations.
>> Now fields are always in be.
>>
>> This also, probably, makes flow_compose_l4 more robust by avoiding
>> possible unaligned accesses to 32 bit value.
>>
>> Fixes: 9b2b84973db7 ("Support for match & set ICMPv6 reserved and options type fields")
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Looks good to me! Thanks.
> Tested on travis and cirrus.
> 
> Acked-by: William Tu <u9012063@gmail.com>

Thanks, William!

Ben, could you, please, take a look at this too?

I just want to be sure that this change is aligned with
expectations from the network header structures in OVS.

Best regards, Ilya Maximets.
Ben Pfaff Oct. 9, 2019, 6:18 p.m. UTC | #3
On Wed, Oct 09, 2019 at 06:54:29PM +0200, Ilya Maximets wrote:
> On 08.10.2019 18:55, William Tu wrote:
> > On Tue, Oct 01, 2019 at 08:04:00PM +0300, Ilya Maximets wrote:
> > > OVS has no structure definition for ICMPv6 header with additional
> > > data. More precisely, it has, but this structure named as
> > > 'icmp6_error_header' and only suitable to store error related
> > > extended information.  'flow_compose_l4' stores additional
> > > information in reserved bits by using system defined structure
> > > 'icmp6_hdr', which is marked as 'packed' and this leads to
> > > build failure with gcc >= 9:
> > > 
> > >    lib/flow.c:3041:34: error:
> > >      taking address of packed member of 'struct icmp6_hdr' may result
> > >      in an unaligned pointer value [-Werror=address-of-packed-member]
> > > 
> > >          uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
> > >                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > Fix that by renaming 'icmp6_error_header' to 'icmp6_data_header'
> > > and allowing it to store not only errors, but any type of additional
> > > information by analogue with 'struct icmp6_hdr'.
> > > All the usages of 'struct icmp6_hdr' replaced with this new structure.
> > > Removed redundant conversions between network and host representations.
> > > Now fields are always in be.
> > > 
> > > This also, probably, makes flow_compose_l4 more robust by avoiding
> > > possible unaligned accesses to 32 bit value.
> > > 
> > > Fixes: 9b2b84973db7 ("Support for match & set ICMPv6 reserved and options type fields")
> > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> > 
> > Looks good to me! Thanks.
> > Tested on travis and cirrus.
> > 
> > Acked-by: William Tu <u9012063@gmail.com>
> 
> Thanks, William!
> 
> Ben, could you, please, take a look at this too?
> 
> I just want to be sure that this change is aligned with
> expectations from the network header structures in OVS.

It seems like a positive change.  We used to have periodic problems with
misaligned accesses on RISC architectures and moving to the 16aligned_*
types, while annoying in some ways, has avoided those.  This change is
helpful because it updates a struct that somehow had been missed
previously.

Acked-by: Ben Pfaff <blp@ovn.org>
Ilya Maximets Oct. 10, 2019, 9:41 a.m. UTC | #4
On 09.10.2019 20:18, Ben Pfaff wrote:
> On Wed, Oct 09, 2019 at 06:54:29PM +0200, Ilya Maximets wrote:
>> On 08.10.2019 18:55, William Tu wrote:
>>> On Tue, Oct 01, 2019 at 08:04:00PM +0300, Ilya Maximets wrote:
>>>> OVS has no structure definition for ICMPv6 header with additional
>>>> data. More precisely, it has, but this structure named as
>>>> 'icmp6_error_header' and only suitable to store error related
>>>> extended information.  'flow_compose_l4' stores additional
>>>> information in reserved bits by using system defined structure
>>>> 'icmp6_hdr', which is marked as 'packed' and this leads to
>>>> build failure with gcc >= 9:
>>>>
>>>>     lib/flow.c:3041:34: error:
>>>>       taking address of packed member of 'struct icmp6_hdr' may result
>>>>       in an unaligned pointer value [-Werror=address-of-packed-member]
>>>>
>>>>           uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
>>>>                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> Fix that by renaming 'icmp6_error_header' to 'icmp6_data_header'
>>>> and allowing it to store not only errors, but any type of additional
>>>> information by analogue with 'struct icmp6_hdr'.
>>>> All the usages of 'struct icmp6_hdr' replaced with this new structure.
>>>> Removed redundant conversions between network and host representations.
>>>> Now fields are always in be.
>>>>
>>>> This also, probably, makes flow_compose_l4 more robust by avoiding
>>>> possible unaligned accesses to 32 bit value.
>>>>
>>>> Fixes: 9b2b84973db7 ("Support for match & set ICMPv6 reserved and options type fields")
>>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>>
>>> Looks good to me! Thanks.
>>> Tested on travis and cirrus.
>>>
>>> Acked-by: William Tu <u9012063@gmail.com>
>>
>> Thanks, William!
>>
>> Ben, could you, please, take a look at this too?
>>
>> I just want to be sure that this change is aligned with
>> expectations from the network header structures in OVS.
> 
> It seems like a positive change.  We used to have periodic problems with
> misaligned accesses on RISC architectures and moving to the 16aligned_*
> types, while annoying in some ways, has avoided those.  This change is
> helpful because it updates a struct that somehow had been missed
> previously.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>
> 

Thanks! I applied this to master and backported to 2.12 with
a slight addition for OVN code.
I'll send a patch for OVN repo with these changes.

Best regards, Ilya Maximets.

Patch
diff mbox series

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 98c61940b..df7b9fa7a 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -719,7 +719,7 @@  reverse_nat_packet(struct dp_packet *pkt, const struct conn *conn)
         icmp->icmp_csum = csum(icmp, tail - (char *) icmp - pad);
     } else {
         struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt);
-        struct icmp6_error_header *icmp6 = dp_packet_l4(pkt);
+        struct icmp6_data_header *icmp6 = dp_packet_l4(pkt);
         struct ovs_16aligned_ip6_hdr *inner_l3_6 =
             (struct ovs_16aligned_ip6_hdr *) (icmp6 + 1);
         /* This call is already verified to succeed during the code path from
diff --git a/lib/flow.c b/lib/flow.c
index 773f2f7b2..317e3712c 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -399,14 +399,14 @@  parse_ethertype(const void **datap, size_t *sizep)
  * and 'arp_buf[]' are filled in.  If the packet is not an ND packet, 'false'
  * is returned and no values are filled in on '*nd_target' or 'arp_buf[]'. */
 static inline bool
-parse_icmpv6(const void **datap, size_t *sizep, const struct icmp6_hdr *icmp,
-             uint32_t *rso_flags, const struct in6_addr **nd_target,
+parse_icmpv6(const void **datap, size_t *sizep,
+             const struct icmp6_data_header *icmp6,
+             ovs_be32 *rso_flags, const struct in6_addr **nd_target,
              struct eth_addr arp_buf[2], uint8_t *opt_type)
 {
-    const uint32_t *reserved;
-    if (icmp->icmp6_code != 0 ||
-        (icmp->icmp6_type != ND_NEIGHBOR_SOLICIT &&
-         icmp->icmp6_type != ND_NEIGHBOR_ADVERT)) {
+    if (icmp6->icmp6_base.icmp6_code != 0 ||
+        (icmp6->icmp6_base.icmp6_type != ND_NEIGHBOR_SOLICIT &&
+         icmp6->icmp6_base.icmp6_type != ND_NEIGHBOR_ADVERT)) {
         return false;
     }
 
@@ -414,12 +414,7 @@  parse_icmpv6(const void **datap, size_t *sizep, const struct icmp6_hdr *icmp,
     arp_buf[1] = eth_addr_zero;
     *opt_type = 0;
 
-    reserved = data_try_pull(datap, sizep, sizeof(uint32_t));
-    if (OVS_UNLIKELY(!reserved)) {
-        /* Invalid ND packet. */
-        return false;
-    }
-    *rso_flags = *reserved;
+    *rso_flags = get_16aligned_be32(icmp6->icmp6_data.be32);
 
     *nd_target = data_try_pull(datap, sizep, sizeof **nd_target);
     if (OVS_UNLIKELY(!*nd_target)) {
@@ -1024,17 +1019,18 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
                 miniflow_pad_to_64(mf, igmp_group_ip4);
             }
         } else if (OVS_LIKELY(nw_proto == IPPROTO_ICMPV6)) {
-            if (OVS_LIKELY(size >= sizeof(struct icmp6_hdr))) {
+            if (OVS_LIKELY(size >= sizeof(struct icmp6_data_header))) {
                 const struct in6_addr *nd_target;
                 struct eth_addr arp_buf[2];
                 /* This will populate whether we received Option 1
                  * or Option 2. */
                 uint8_t opt_type;
                 /* This holds the ND Reserved field. */
-                uint32_t rso_flags;
-                const struct icmp6_hdr *icmp = data_pull(&data,
-                                               &size,ICMP6_HEADER_LEN);
-                if (parse_icmpv6(&data, &size, icmp,
+                ovs_be32 rso_flags;
+                const struct icmp6_data_header *icmp6;
+
+                icmp6 = data_pull(&data, &size, sizeof *icmp6);
+                if (parse_icmpv6(&data, &size, icmp6,
                                  &rso_flags, &nd_target, arp_buf, &opt_type)) {
                     if (nd_target) {
                         miniflow_push_words(mf, nd_target, nd_target,
@@ -1053,16 +1049,20 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
                          * This will zero out the tcp_flags & pad3 field. */
                         miniflow_pad_to_64(mf, arp_tha);
                     }
-                    miniflow_push_be16(mf, tp_src, htons(icmp->icmp6_type));
-                    miniflow_push_be16(mf, tp_dst, htons(icmp->icmp6_code));
+                    miniflow_push_be16(mf, tp_src,
+                                       htons(icmp6->icmp6_base.icmp6_type));
+                    miniflow_push_be16(mf, tp_dst,
+                                       htons(icmp6->icmp6_base.icmp6_code));
                     miniflow_pad_to_64(mf, tp_dst);
                     /* Fill ND reserved field. */
-                    miniflow_push_be32(mf, igmp_group_ip4, htonl(rso_flags));
+                    miniflow_push_be32(mf, igmp_group_ip4, rso_flags);
                     miniflow_pad_to_64(mf, igmp_group_ip4);
                 } else {
                     /* ICMPv6 but not ND. */
-                    miniflow_push_be16(mf, tp_src, htons(icmp->icmp6_type));
-                    miniflow_push_be16(mf, tp_dst, htons(icmp->icmp6_code));
+                    miniflow_push_be16(mf, tp_src,
+                                       htons(icmp6->icmp6_base.icmp6_type));
+                    miniflow_push_be16(mf, tp_dst,
+                                       htons(icmp6->icmp6_base.icmp6_code));
                     miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
                     miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
                 }
@@ -3035,15 +3035,16 @@  flow_compose_l4(struct dp_packet *p, const struct flow *flow,
             igmp->igmp_code = ntohs(flow->tp_dst);
             put_16aligned_be32(&igmp->group, flow->igmp_group_ip4);
         } else if (flow->nw_proto == IPPROTO_ICMPV6) {
-            struct icmp6_hdr *icmp = dp_packet_put_zeros(p, sizeof *icmp);
-            icmp->icmp6_type = ntohs(flow->tp_src);
-            icmp->icmp6_code = ntohs(flow->tp_dst);
-            uint32_t *reserved = &icmp->icmp6_dataun.icmp6_un_data32[0];
-            *reserved = ntohl(flow->igmp_group_ip4);
-
-            if (icmp->icmp6_code == 0 &&
-                (icmp->icmp6_type == ND_NEIGHBOR_SOLICIT ||
-                 icmp->icmp6_type == ND_NEIGHBOR_ADVERT)) {
+            struct icmp6_data_header *icmp6;
+
+            icmp6 = dp_packet_put_zeros(p, sizeof *icmp6);
+            icmp6->icmp6_base.icmp6_type = ntohs(flow->tp_src);
+            icmp6->icmp6_base.icmp6_code = ntohs(flow->tp_dst);
+            put_16aligned_be32(icmp6->icmp6_data.be32, flow->igmp_group_ip4);
+
+            if (icmp6->icmp6_base.icmp6_code == 0 &&
+                (icmp6->icmp6_base.icmp6_type == ND_NEIGHBOR_SOLICIT ||
+                 icmp6->icmp6_base.icmp6_type == ND_NEIGHBOR_ADVERT)) {
                 struct in6_addr *nd_target;
                 struct ovs_nd_lla_opt *lla_opt;
 
@@ -3062,9 +3063,9 @@  flow_compose_l4(struct dp_packet *p, const struct flow *flow,
                     lla_opt->type = ND_OPT_TARGET_LINKADDR;
                     lla_opt->mac = flow->arp_tha;
                 }
-            } else if (icmp->icmp6_code == 0 &&
-                       (icmp->icmp6_type == ICMP6_ECHO_REQUEST ||
-                        icmp->icmp6_type == ICMP6_ECHO_REPLY)) {
+            } else if (icmp6->icmp6_base.icmp6_code == 0 &&
+                       (icmp6->icmp6_base.icmp6_type == ICMP6_ECHO_REQUEST ||
+                        icmp6->icmp6_base.icmp6_type == ICMP6_ECHO_REPLY)) {
                 flow_compose_l7(p, l7, l7_len);
             } else {
                 /* XXX Add inner IP packet for e.g. destination unreachable? */
@@ -3109,11 +3110,11 @@  flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow,
             igmp->igmp_csum = 0;
             igmp->igmp_csum = csum(igmp, l4_len);
         } else if (flow->nw_proto == IPPROTO_ICMPV6) {
-            struct icmp6_hdr *icmp = dp_packet_l4(p);
+            struct icmp6_data_header *icmp6 = dp_packet_l4(p);
 
-            icmp->icmp6_cksum = 0;
-            icmp->icmp6_cksum = (OVS_FORCE uint16_t)
-                csum_finish(csum_continue(pseudo_hdr_csum, icmp, l4_len));
+            icmp6->icmp6_base.icmp6_cksum = 0;
+            icmp6->icmp6_base.icmp6_cksum =
+                csum_finish(csum_continue(pseudo_hdr_csum, icmp6, l4_len));
         }
     }
 }
diff --git a/lib/packets.h b/lib/packets.h
index c78defb89..d19f6e3ca 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -996,12 +996,16 @@  struct icmp6_header {
 };
 BUILD_ASSERT_DECL(ICMP6_HEADER_LEN == sizeof(struct icmp6_header));
 
-#define ICMP6_ERROR_HEADER_LEN 8
-struct icmp6_error_header {
+#define ICMP6_DATA_HEADER_LEN 8
+struct icmp6_data_header {
     struct icmp6_header icmp6_base;
-    ovs_be32 icmp6_error_ext;
+    union {
+        ovs_16aligned_be32 be32[1];
+        ovs_be16           be16[2];
+        uint8_t            u8[4];
+    } icmp6_data;
 };
-BUILD_ASSERT_DECL(ICMP6_ERROR_HEADER_LEN == sizeof(struct icmp6_error_header));
+BUILD_ASSERT_DECL(ICMP6_DATA_HEADER_LEN == sizeof(struct icmp6_data_header));
 
 uint32_t packet_csum_pseudoheader6(const struct ovs_16aligned_ip6_hdr *);
 ovs_be16 packet_csum_upperlayer6(const struct ovs_16aligned_ip6_hdr *,