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
