On 11/15/22 07:52, Ales Musil wrote: > On Wed, Nov 2, 2022 at 2:33 PM Dumitru Ceara <[email protected]> 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 <[email protected]> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> 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 >> [email protected] >> 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 <[email protected]> > Thanks! I pushed this to the main branch. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
