On 1/25/24 22:46, Mike Pattrick wrote:
> Previously the BFD packet creation code did not appropriately set
> offsets or flags. This contributed to issues involving encapsulation and
> the TSO code.
>
> Signed-off-by: Mike Pattrick <[email protected]>
> Fixes: ccc096898c46 ("bfd: Implement Bidirectional Forwarding Detection.")
> Signed-off-by: Mike Pattrick <[email protected]>
One sign-off too many. :)
> ---
> lib/bfd.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 9698576d0..af9998507 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -586,7 +586,6 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
> {
> long long int min_tx, min_rx;
> struct udp_header *udp;
> - struct eth_header *eth;
> struct ip_header *ip;
> struct msg *msg;
>
> @@ -605,15 +604,13 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
> * set. */
> ovs_assert(!(bfd->flags & FLAG_POLL) || !(bfd->flags & FLAG_FINAL));
>
> - dp_packet_reserve(p, 2); /* Properly align after the ethernet header. */
> - eth = dp_packet_put_uninit(p, sizeof *eth);
> - eth->eth_src = eth_addr_is_zero(bfd->local_eth_src)
> - ? eth_src : bfd->local_eth_src;
> - eth->eth_dst = eth_addr_is_zero(bfd->local_eth_dst)
> - ? eth_addr_bfd : bfd->local_eth_dst;
> - eth->eth_type = htons(ETH_TYPE_IP);
> + ip = eth_compose(p,
> + eth_addr_is_zero(bfd->local_eth_dst) ?
> + eth_addr_bfd : bfd->local_eth_dst,
> + eth_addr_is_zero(bfd->local_eth_src) ?
> + eth_src : bfd->local_eth_src,
coding-style.rst:
"""
Break long lines before the ternary operators ``?`` and ``:``, rather than
after them
"""
> + ETH_TYPE_IP, sizeof *ip + sizeof *udp + sizeof *msg);
>
> - ip = dp_packet_put_zeros(p, sizeof *ip);
> ip->ip_ihl_ver = IP_IHL_VER(5, 4);
> ip->ip_tot_len = htons(sizeof *ip + sizeof *udp + sizeof *msg);
> ip->ip_ttl = MAXTTL;
> @@ -621,15 +618,17 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
> ip->ip_proto = IPPROTO_UDP;
> put_16aligned_be32(&ip->ip_src, bfd->ip_src);
> put_16aligned_be32(&ip->ip_dst, bfd->ip_dst);
> - /* Checksum has already been zeroed by put_zeros call. */
> - ip->ip_csum = csum(ip, sizeof *ip);
> + dp_packet_hwol_set_tx_ipv4(p);
> + dp_packet_hwol_set_tx_ip_csum(p);
I don't think we can do that. This code is datapath-agnostic. And we
will not re-calculate the checksum while sending this packet to the kernel
datapath for actions execution.
> + dp_packet_set_l4(p, ip + 1);
>
> - udp = dp_packet_put_zeros(p, sizeof *udp);
> + udp = dp_packet_l4(p);
> udp->udp_src = htons(bfd->udp_src);
> udp->udp_dst = htons(BFD_DEST_PORT);
> udp->udp_len = htons(sizeof *udp + sizeof *msg);
> + /* Checksum already zero from eth_compose. */
>
> - msg = dp_packet_put_uninit(p, sizeof *msg);
> + msg = (struct msg *)(udp + 1);
> msg->vers_diag = (BFD_VERSION << 5) | bfd->diag;
> msg->flags = (bfd->state & STATE_MASK) | bfd->flags;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
