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.
However, that takes some time, this is good, thanks.
Acked-by: Ales Musil <[email protected]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
