| Message ID | 20221102133304.325392-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] pinctrl: Use 16-aligned 32-bit fields in bfd_msg. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Nov 2, 2022 at 2:33 PM Dumitru Ceara <dceara@redhat.com> wrote: > This avoids misaligned accesses as flagged by UBSan when running CoPP > system tests: > > controller/pinctrl.c:7129:28: runtime error: member access within > misaligned address 0x61b0000627be for type 'const struct bfd_msg', which > requires 4 byte alignment > 0x61b0000627be: note: pointer points here > 00 20 d3 d4 20 60 05 18 a8 99 e7 7b 92 1d 36 73 00 0f 42 40 00 0f 42 > 40 00 00 00 00 00 00 00 03 > ^ > #0 0x621b8f in pinctrl_check_bfd_msg > /root/ovn/controller/pinctrl.c:7129:28 > #1 0x621b8f in pinctrl_handle_bfd_msg > /root/ovn/controller/pinctrl.c:7183:10 > #2 0x621b8f in process_packet_in > /root/ovn/controller/pinctrl.c:3320:9 > #3 0x621b8f in pinctrl_recv /root/ovn/controller/pinctrl.c:3386:9 > #4 0x621b8f in pinctrl_handler /root/ovn/controller/pinctrl.c:3481:17 > #5 0xa2d89a in ovsthread_wrapper > /root/ovn/ovs/lib/ovs-thread.c:422:12 > #6 0x7fcb598081ce in start_thread (/lib64/libpthread.so.0+0x81ce) > #7 0x7fcb58439dd2 in clone (/lib64/libc.so.6+0x39dd2) > > Fixes: 117203584d98 ("controller: introduce BFD tx path in > ovn-controller.") > Reported-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/pinctrl.c | 26 ++++++++++++++------------ > lib/ovn-l7.h | 10 +++++----- > 2 files changed, 19 insertions(+), 17 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 8859cb080d..9913d5ae12 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6890,13 +6890,14 @@ pinctrl_find_bfd_monitor_entry_by_port(char *ip, > uint16_t port) > } > > static struct bfd_entry * > -pinctrl_find_bfd_monitor_entry_by_disc(char *ip, ovs_be32 disc) > +pinctrl_find_bfd_monitor_entry_by_disc(const char *ip, > + const ovs_16aligned_be32 *disc) > { > struct bfd_entry *ret = NULL, *entry; > > HMAP_FOR_EACH_WITH_HASH (entry, node, hash_string(ip, 0), > &bfd_monitor_map) { > - if (entry->local_disc == disc) { > + if (entry->local_disc == get_16aligned_be32(disc)) { > ret = entry; > break; > } > @@ -6955,11 +6956,11 @@ bfd_monitor_put_bfd_msg(struct bfd_entry *entry, > struct dp_packet *packet, > msg->length = BFD_PACKET_LEN; > msg->flags = final ? BFD_FLAG_FINAL : 0; > msg->flags |= entry->state << 6; > - msg->my_disc = entry->local_disc; > - msg->your_disc = entry->remote_disc; > + put_16aligned_be32(&msg->my_disc, entry->local_disc); > + put_16aligned_be32(&msg->your_disc, entry->remote_disc); > /* min_tx and min_rx are in us - RFC 5880 page 9 */ > - msg->min_tx = htonl(entry->local_min_tx * 1000); > - msg->min_rx = htonl(entry->local_min_rx * 1000); > + put_16aligned_be32(&msg->min_tx, htonl(entry->local_min_tx * 1000)); > + put_16aligned_be32(&msg->min_rx, htonl(entry->local_min_rx * 1000)); > > if (!IN6_IS_ADDR_V4MAPPED(&entry->ip_src)) { > /* IPv6 needs UDP checksum calculated */ > @@ -7154,7 +7155,7 @@ pinctrl_check_bfd_msg(const struct flow *ip_flow, > struct dp_packet *pkt_in) > return false; > } > > - if (!msg->my_disc) { > + if (!get_16aligned_be32(&msg->my_disc)) { > VLOG_DBG_RL(&rl, "BFD action: my_disc not set"); > return false; > } > @@ -7165,7 +7166,8 @@ pinctrl_check_bfd_msg(const struct flow *ip_flow, > struct dp_packet *pkt_in) > } > > enum bfd_state peer_state = msg->flags >> 6; > - if (peer_state >= BFD_STATE_INIT && !msg->your_disc) { > + if (peer_state >= BFD_STATE_INIT > + && !get_16aligned_be32(&msg->your_disc)) { > VLOG_DBG_RL(&rl, > "BFD action: remote state is %s and your_disc is not > set", > bfd_get_status(peer_state)); > @@ -7193,7 +7195,7 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const > struct flow *ip_flow, > > const struct bfd_msg *msg = dp_packet_get_udp_payload(pkt_in); > struct bfd_entry *entry = > - pinctrl_find_bfd_monitor_entry_by_disc(ip_src, msg->your_disc); > + pinctrl_find_bfd_monitor_entry_by_disc(ip_src, &msg->your_disc); > > if (!entry) { > free(ip_src); > @@ -7201,9 +7203,9 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const > struct flow *ip_flow, > } > > bool change_state = false; > - entry->remote_disc = msg->my_disc; > - uint32_t remote_min_tx = ntohl(msg->min_tx) / 1000; > - entry->remote_min_rx = ntohl(msg->min_rx) / 1000; > + entry->remote_disc = get_16aligned_be32(&msg->my_disc); > + uint32_t remote_min_tx = ntohl(get_16aligned_be32(&msg->min_tx)) / > 1000; > + entry->remote_min_rx = ntohl(get_16aligned_be32(&msg->min_rx)) / 1000; > entry->detection_timeout = msg->mult * MAX(remote_min_tx, > entry->local_min_rx); > > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h > index b03b945d89..2b20bc380a 100644 > --- a/lib/ovn-l7.h > +++ b/lib/ovn-l7.h > @@ -37,11 +37,11 @@ struct bfd_msg { > uint8_t flags; > uint8_t mult; > uint8_t length; > - ovs_be32 my_disc; > - ovs_be32 your_disc; > - ovs_be32 min_tx; > - ovs_be32 min_rx; > - ovs_be32 min_rx_echo; > + ovs_16aligned_be32 my_disc; > + ovs_16aligned_be32 your_disc; > + ovs_16aligned_be32 min_tx; > + ovs_16aligned_be32 min_rx; > + ovs_16aligned_be32 min_rx_echo; > }; > BUILD_ASSERT_DECL(BFD_PACKET_LEN == sizeof(struct bfd_msg)); > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > It would be nice to make the ovs bfd struct public so it can be reused also in ovn. However, that takes some time, this is good, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 11/15/22 07:52, Ales Musil wrote: > On Wed, Nov 2, 2022 at 2:33 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> This avoids misaligned accesses as flagged by UBSan when running CoPP >> system tests: >> >> controller/pinctrl.c:7129:28: runtime error: member access within >> misaligned address 0x61b0000627be for type 'const struct bfd_msg', which >> requires 4 byte alignment >> 0x61b0000627be: note: pointer points here >> 00 20 d3 d4 20 60 05 18 a8 99 e7 7b 92 1d 36 73 00 0f 42 40 00 0f 42 >> 40 00 00 00 00 00 00 00 03 >> ^ >> #0 0x621b8f in pinctrl_check_bfd_msg >> /root/ovn/controller/pinctrl.c:7129:28 >> #1 0x621b8f in pinctrl_handle_bfd_msg >> /root/ovn/controller/pinctrl.c:7183:10 >> #2 0x621b8f in process_packet_in >> /root/ovn/controller/pinctrl.c:3320:9 >> #3 0x621b8f in pinctrl_recv /root/ovn/controller/pinctrl.c:3386:9 >> #4 0x621b8f in pinctrl_handler /root/ovn/controller/pinctrl.c:3481:17 >> #5 0xa2d89a in ovsthread_wrapper >> /root/ovn/ovs/lib/ovs-thread.c:422:12 >> #6 0x7fcb598081ce in start_thread (/lib64/libpthread.so.0+0x81ce) >> #7 0x7fcb58439dd2 in clone (/lib64/libc.so.6+0x39dd2) >> >> Fixes: 117203584d98 ("controller: introduce BFD tx path in >> ovn-controller.") >> Reported-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> controller/pinctrl.c | 26 ++++++++++++++------------ >> lib/ovn-l7.h | 10 +++++----- >> 2 files changed, 19 insertions(+), 17 deletions(-) >> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index 8859cb080d..9913d5ae12 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -6890,13 +6890,14 @@ pinctrl_find_bfd_monitor_entry_by_port(char *ip, >> uint16_t port) >> } >> >> static struct bfd_entry * >> -pinctrl_find_bfd_monitor_entry_by_disc(char *ip, ovs_be32 disc) >> +pinctrl_find_bfd_monitor_entry_by_disc(const char *ip, >> + const ovs_16aligned_be32 *disc) >> { >> struct bfd_entry *ret = NULL, *entry; >> >> HMAP_FOR_EACH_WITH_HASH (entry, node, hash_string(ip, 0), >> &bfd_monitor_map) { >> - if (entry->local_disc == disc) { >> + if (entry->local_disc == get_16aligned_be32(disc)) { >> ret = entry; >> break; >> } >> @@ -6955,11 +6956,11 @@ bfd_monitor_put_bfd_msg(struct bfd_entry *entry, >> struct dp_packet *packet, >> msg->length = BFD_PACKET_LEN; >> msg->flags = final ? BFD_FLAG_FINAL : 0; >> msg->flags |= entry->state << 6; >> - msg->my_disc = entry->local_disc; >> - msg->your_disc = entry->remote_disc; >> + put_16aligned_be32(&msg->my_disc, entry->local_disc); >> + put_16aligned_be32(&msg->your_disc, entry->remote_disc); >> /* min_tx and min_rx are in us - RFC 5880 page 9 */ >> - msg->min_tx = htonl(entry->local_min_tx * 1000); >> - msg->min_rx = htonl(entry->local_min_rx * 1000); >> + put_16aligned_be32(&msg->min_tx, htonl(entry->local_min_tx * 1000)); >> + put_16aligned_be32(&msg->min_rx, htonl(entry->local_min_rx * 1000)); >> >> if (!IN6_IS_ADDR_V4MAPPED(&entry->ip_src)) { >> /* IPv6 needs UDP checksum calculated */ >> @@ -7154,7 +7155,7 @@ pinctrl_check_bfd_msg(const struct flow *ip_flow, >> struct dp_packet *pkt_in) >> return false; >> } >> >> - if (!msg->my_disc) { >> + if (!get_16aligned_be32(&msg->my_disc)) { >> VLOG_DBG_RL(&rl, "BFD action: my_disc not set"); >> return false; >> } >> @@ -7165,7 +7166,8 @@ pinctrl_check_bfd_msg(const struct flow *ip_flow, >> struct dp_packet *pkt_in) >> } >> >> enum bfd_state peer_state = msg->flags >> 6; >> - if (peer_state >= BFD_STATE_INIT && !msg->your_disc) { >> + if (peer_state >= BFD_STATE_INIT >> + && !get_16aligned_be32(&msg->your_disc)) { >> VLOG_DBG_RL(&rl, >> "BFD action: remote state is %s and your_disc is not >> set", >> bfd_get_status(peer_state)); >> @@ -7193,7 +7195,7 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const >> struct flow *ip_flow, >> >> const struct bfd_msg *msg = dp_packet_get_udp_payload(pkt_in); >> struct bfd_entry *entry = >> - pinctrl_find_bfd_monitor_entry_by_disc(ip_src, msg->your_disc); >> + pinctrl_find_bfd_monitor_entry_by_disc(ip_src, &msg->your_disc); >> >> if (!entry) { >> free(ip_src); >> @@ -7201,9 +7203,9 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const >> struct flow *ip_flow, >> } >> >> bool change_state = false; >> - entry->remote_disc = msg->my_disc; >> - uint32_t remote_min_tx = ntohl(msg->min_tx) / 1000; >> - entry->remote_min_rx = ntohl(msg->min_rx) / 1000; >> + entry->remote_disc = get_16aligned_be32(&msg->my_disc); >> + uint32_t remote_min_tx = ntohl(get_16aligned_be32(&msg->min_tx)) / >> 1000; >> + entry->remote_min_rx = ntohl(get_16aligned_be32(&msg->min_rx)) / 1000; >> entry->detection_timeout = msg->mult * MAX(remote_min_tx, >> entry->local_min_rx); >> >> diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h >> index b03b945d89..2b20bc380a 100644 >> --- a/lib/ovn-l7.h >> +++ b/lib/ovn-l7.h >> @@ -37,11 +37,11 @@ struct bfd_msg { >> uint8_t flags; >> uint8_t mult; >> uint8_t length; >> - ovs_be32 my_disc; >> - ovs_be32 your_disc; >> - ovs_be32 min_tx; >> - ovs_be32 min_rx; >> - ovs_be32 min_rx_echo; >> + ovs_16aligned_be32 my_disc; >> + ovs_16aligned_be32 your_disc; >> + ovs_16aligned_be32 min_tx; >> + ovs_16aligned_be32 min_rx; >> + ovs_16aligned_be32 min_rx_echo; >> }; >> BUILD_ASSERT_DECL(BFD_PACKET_LEN == sizeof(struct bfd_msg)); >> >> -- >> 2.31.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > It would be nice to make the ovs bfd struct public so it can be reused also > in ovn. You're right, we should spend some time to do that. Until then.. > However, that takes some time, this is good, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> > Thanks! I pushed this to the main branch.
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 8859cb080d..9913d5ae12 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -6890,13 +6890,14 @@ pinctrl_find_bfd_monitor_entry_by_port(char *ip, uint16_t port) } static struct bfd_entry * -pinctrl_find_bfd_monitor_entry_by_disc(char *ip, ovs_be32 disc) +pinctrl_find_bfd_monitor_entry_by_disc(const char *ip, + const ovs_16aligned_be32 *disc) { struct bfd_entry *ret = NULL, *entry; HMAP_FOR_EACH_WITH_HASH (entry, node, hash_string(ip, 0), &bfd_monitor_map) { - if (entry->local_disc == disc) { + if (entry->local_disc == get_16aligned_be32(disc)) { ret = entry; break; } @@ -6955,11 +6956,11 @@ bfd_monitor_put_bfd_msg(struct bfd_entry *entry, struct dp_packet *packet, msg->length = BFD_PACKET_LEN; msg->flags = final ? BFD_FLAG_FINAL : 0; msg->flags |= entry->state << 6; - msg->my_disc = entry->local_disc; - msg->your_disc = entry->remote_disc; + put_16aligned_be32(&msg->my_disc, entry->local_disc); + put_16aligned_be32(&msg->your_disc, entry->remote_disc); /* min_tx and min_rx are in us - RFC 5880 page 9 */ - msg->min_tx = htonl(entry->local_min_tx * 1000); - msg->min_rx = htonl(entry->local_min_rx * 1000); + put_16aligned_be32(&msg->min_tx, htonl(entry->local_min_tx * 1000)); + put_16aligned_be32(&msg->min_rx, htonl(entry->local_min_rx * 1000)); if (!IN6_IS_ADDR_V4MAPPED(&entry->ip_src)) { /* IPv6 needs UDP checksum calculated */ @@ -7154,7 +7155,7 @@ pinctrl_check_bfd_msg(const struct flow *ip_flow, struct dp_packet *pkt_in) return false; } - if (!msg->my_disc) { + if (!get_16aligned_be32(&msg->my_disc)) { VLOG_DBG_RL(&rl, "BFD action: my_disc not set"); return false; } @@ -7165,7 +7166,8 @@ pinctrl_check_bfd_msg(const struct flow *ip_flow, struct dp_packet *pkt_in) } enum bfd_state peer_state = msg->flags >> 6; - if (peer_state >= BFD_STATE_INIT && !msg->your_disc) { + if (peer_state >= BFD_STATE_INIT + && !get_16aligned_be32(&msg->your_disc)) { VLOG_DBG_RL(&rl, "BFD action: remote state is %s and your_disc is not set", bfd_get_status(peer_state)); @@ -7193,7 +7195,7 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const struct flow *ip_flow, const struct bfd_msg *msg = dp_packet_get_udp_payload(pkt_in); struct bfd_entry *entry = - pinctrl_find_bfd_monitor_entry_by_disc(ip_src, msg->your_disc); + pinctrl_find_bfd_monitor_entry_by_disc(ip_src, &msg->your_disc); if (!entry) { free(ip_src); @@ -7201,9 +7203,9 @@ pinctrl_handle_bfd_msg(struct rconn *swconn, const struct flow *ip_flow, } bool change_state = false; - entry->remote_disc = msg->my_disc; - uint32_t remote_min_tx = ntohl(msg->min_tx) / 1000; - entry->remote_min_rx = ntohl(msg->min_rx) / 1000; + entry->remote_disc = get_16aligned_be32(&msg->my_disc); + uint32_t remote_min_tx = ntohl(get_16aligned_be32(&msg->min_tx)) / 1000; + entry->remote_min_rx = ntohl(get_16aligned_be32(&msg->min_rx)) / 1000; entry->detection_timeout = msg->mult * MAX(remote_min_tx, entry->local_min_rx); diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h index b03b945d89..2b20bc380a 100644 --- a/lib/ovn-l7.h +++ b/lib/ovn-l7.h @@ -37,11 +37,11 @@ struct bfd_msg { uint8_t flags; uint8_t mult; uint8_t length; - ovs_be32 my_disc; - ovs_be32 your_disc; - ovs_be32 min_tx; - ovs_be32 min_rx; - ovs_be32 min_rx_echo; + ovs_16aligned_be32 my_disc; + ovs_16aligned_be32 your_disc; + ovs_16aligned_be32 min_tx; + ovs_16aligned_be32 min_rx; + ovs_16aligned_be32 min_rx_echo; }; BUILD_ASSERT_DECL(BFD_PACKET_LEN == sizeof(struct bfd_msg));
This avoids misaligned accesses as flagged by UBSan when running CoPP system tests: controller/pinctrl.c:7129:28: runtime error: member access within misaligned address 0x61b0000627be for type 'const struct bfd_msg', which requires 4 byte alignment 0x61b0000627be: note: pointer points here 00 20 d3 d4 20 60 05 18 a8 99 e7 7b 92 1d 36 73 00 0f 42 40 00 0f 42 40 00 00 00 00 00 00 00 03 ^ #0 0x621b8f in pinctrl_check_bfd_msg /root/ovn/controller/pinctrl.c:7129:28 #1 0x621b8f in pinctrl_handle_bfd_msg /root/ovn/controller/pinctrl.c:7183:10 #2 0x621b8f in process_packet_in /root/ovn/controller/pinctrl.c:3320:9 #3 0x621b8f in pinctrl_recv /root/ovn/controller/pinctrl.c:3386:9 #4 0x621b8f in pinctrl_handler /root/ovn/controller/pinctrl.c:3481:17 #5 0xa2d89a in ovsthread_wrapper /root/ovn/ovs/lib/ovs-thread.c:422:12 #6 0x7fcb598081ce in start_thread (/lib64/libpthread.so.0+0x81ce) #7 0x7fcb58439dd2 in clone (/lib64/libc.so.6+0x39dd2) Fixes: 117203584d98 ("controller: introduce BFD tx path in ovn-controller.") Reported-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/pinctrl.c | 26 ++++++++++++++------------ lib/ovn-l7.h | 10 +++++----- 2 files changed, 19 insertions(+), 17 deletions(-)