| Message ID | 20240125214657.734291-3-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/5] dp-packet: Reset offload/offsets when clearing a packet. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <mkp@redhat.com> > Fixes: ccc096898c46 ("bfd: Implement Bidirectional Forwarding Detection.") > Signed-off-by: Mike Pattrick <mkp@redhat.com> 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; >
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, + 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); + 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;