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)
> 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.
While with this change, won't an invalid L4 packet encapsulated in a
valid udp header will be treated as valid L4?
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.
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
