On Tue, Feb 25, 2025 at 1:08 PM David Marchand <david.march...@redhat.com> wrote: > > Hello Mike, > > On Tue, Feb 25, 2025 at 7:11 AM Mike Pattrick <m...@redhat.com> wrote: > > > > Previously pop_header would strip offload flags include RX state(good, > > partial, bad). This assumes that the offload status of the inner layer > > does not need to be retained past outer layer removal. However other > > locations in the packet processing pipeline will take the lack of RX > > offload flags to indicate that the packet has a correct checksum, > > leading to incorrectly processed packets. > > > > Convert conn_key_extract() to use the RX state instead of TX offload > > protocol flags. Now conn_key_extract() will properly not calculate > > checksums when a checksum offloaded packet is decapsulated before ct(). > > > > Update Netdev-linux to set the proper RX state instead of relaying on TX > > flags. > > > > Fixes: 3337e6d91c5b ("userspace: Enable L4 checksum offloading by default.") > > Fixes: 1a2bb11817a4 ("netdev-dpdk: Enable Rx checksum offloading feature on > > DPDK physical ports.") > > Signed-off-by: Mike Pattrick <m...@redhat.com> > > --- > > lib/conntrack.c | 2 +- > > lib/dp-packet.h | 8 ++++++++ > > lib/netdev-linux.c | 2 ++ > > lib/netdev.c | 4 ++-- > > 4 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index f4b150bee..6977fd4ef 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -2237,7 +2237,7 @@ conn_key_extract(struct conntrack *ct, struct > > dp_packet *pkt, ovs_be16 dl_type, > > if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), > > &ctx->icmp_related, l3, > > !dp_packet_l4_checksum_good(pkt) && > > - !dp_packet_hwol_tx_l4_checksum(pkt), > > + !dp_packet_ol_l4_csum_partial(pkt), > > (Note: those non consistent prefixes are a pain when reading the code, > I mean dp_packet_l4_checksum_ and dp_packet_ol_l4_csum_, but also ip > checksum and tunnel..) > > For readability, I would introduce a dp_packet_l4_checksum_unknown() helper: > > static inline bool OVS_WARN_UNUSED_RESULT > dp_packet_l4_checksum_unknown(const struct dp_packet *p) > { > return !(*dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RX_L4_CKSUM_MASK); > } > > And we can skip csum check for UDP tunneled (inside OVS) packets, as > their current UDP header is partially filled. > So I would rather see a: > > if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), > &ctx->icmp_related, l3, > - !dp_packet_l4_checksum_good(pkt) && > - !dp_packet_hwol_tx_l4_checksum(pkt), > + !dp_packet_is_udp_tunnel(pkt) > + && dp_packet_l4_checksum_unknown(pkt) >
On the surface level I agree, though during the first iteration of the TSO patch set, refactor patches were rejected. I could add this as a 3rd patch. > > > NULL)) { > > ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); > > return true; > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h > > index 0f487a428..182cb4429 100644 > > --- a/lib/dp-packet.h > > +++ b/lib/dp-packet.h > > @@ -1070,6 +1070,14 @@ dp_packet_rss_valid(const struct dp_packet *p) > > return *dp_packet_ol_flags_ptr(p) & DP_PACKET_OL_RSS_HASH; > > } > > > > +static inline void > > +dp_packet_reset_tx_offload(struct dp_packet *p) > > +{ > > + *dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_SUPPORTED_MASK | > > + DP_PACKET_OL_RX_L4_CKSUM_MASK | > > + DP_PACKET_OL_RX_IP_CKSUM_MASK; > > +} > > + > > static inline void > > dp_packet_reset_offload(struct dp_packet *p) > > { > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > > index 19bf62ece..5b4950b14 100644 > > --- a/lib/netdev-linux.c > > +++ b/lib/netdev-linux.c > > @@ -7120,9 +7120,11 @@ netdev_linux_parse_vnet_hdr(struct dp_packet *b) > > * are parsed during miniflow extraction. */ > > b->csum_start = (OVS_FORCE uint16_t) vnet->csum_start; > > b->csum_offset = (OVS_FORCE uint16_t) vnet->csum_offset; > > + dp_packet_ol_set_l4_csum_partial(b); > > } else { > > b->csum_start = 0; > > b->csum_offset = 0; > > + dp_packet_ol_set_l4_csum_good(b); > > } > > > > int ret = 0; > > diff --git a/lib/netdev.c b/lib/netdev.c > > index 9dd94ebdd..c5babb5d2 100644 > > --- a/lib/netdev.c > > +++ b/lib/netdev.c > > @@ -961,10 +961,10 @@ netdev_pop_header(struct netdev *netdev, struct > > dp_packet_batch *batch) > > DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) { > > packet = netdev->netdev_class->pop_header(packet); > > if (packet) { > > - /* Reset the offload flags if present, to avoid wrong > > + /* Reset the TX offload flags if present, to avoid wrong > > * interpretation in the further packet processing when > > * recirculated.*/ > > - dp_packet_reset_offload(packet); > > + dp_packet_reset_tx_offload(packet); > > I am not following here. > > With the current code, when popping a header (of a tunnel packet > received from a port) and resetting those Rx flags, in case of > conntrack, the packet will be L4 "unknown" and the csum will be > checked for the inner part. Currently the checksum will almost always be checked. This sort of works because packets from external won't have a partial checksum. Encapsulated checksum offloaded virtio packets don't work due to FDP-1147, and netdev-linux packets are saved by csum_start / csum_offset, which had the byproduct of also tagging the packet as partial after the flags are reset. > While with this change, won't an invalid L4 packet encapsulated in a > valid udp header will be treated as valid L4? This is a good point. How do you feel about carrying over partial flag, but not other flags? > > I also wonder: is it possible that an L4 invalid (non tunneled) packet > enters OVS, then gets encapsulated (here, L4 inner csum status gets > lost), then after some packet processing gets decapsulated again (and > then fixed on the egress path)? > > I also have some doubt about receiving a eth / ip / udp / vxlan / eth > / arp packet. > On popping the header, we would keep the Rx L4 info for the UDP > header, on a arp packet. But aren't the RX flags only used in conjunction with TX flags, conn key, etc? > > > Maybe this could work, if avoiding the Rx flags reset for internally > encapsulated traffic? > But otherwise I think this reset is necessary. > > > > pkt_metadata_init_conn(&packet->md); > > dp_packet_batch_refill(batch, packet, i); > > } > > -- > > 2.48.1 > > > > > -- > David Marchand > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev