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. | expand |
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
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.
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>
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.
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 *,
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(-)