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

Reply via email to