On Wed, Mar 12, 2025 at 4:57 PM David Marchand <[email protected]> wrote: > > Hello David, > > > > Still going through the patches, but one of the concerns I have is > > I've previously encountered issues where the csum_start/offset > > delivered via AF_PACKET was incorrect in the case of vlan+vxlan > > packets. This is one of the reasons why OVS currently doesn't validate > > these values. > > Can you provide details? > I would rather fix this kernel bug (or find a dedicated workaround). > How can I reproduce this issue? > > Or maybe it is related to what I see below: > > > > > I think it's reasonable to assume dp_packet_ol_set_l4_csum_partial if > > these values are set. > > Unconditionnally marking as L4 partial does not work with encapsulated > traffic, since inner checksum may be partial (this can be seen with > the unit test datapath - ping over vxlan tunnel, for example). > > With TSO enabled, an encapsulated packet sent by the kernel has both a > (overall valid, once inner is fixed) outer L4 checksum and an invalid > inner L4 checksum. > csum_start/csum_offset point at the inner part. > > In practice, this current patch of mine fixes the situation (note: I > had to remove check on packet_type and l3_ofs in ovs_dump_packet for > below debug): > > 7122 if (vnet->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) { > (gdb) ovs_dump_packets b -n -vvv > 16:08:25.461061 IP (tos 0x0, ttl 64, id 52391, offset 0, flags [none], > proto UDP (17), length 110) > 172.31.1.1.52791 > 172.31.1.100.vxlan: [bad udp cksum 0x2966 -> > 0xcf9e!] VXLAN, flags [I] (0x08), vni 0 > IP (tos 0x0, ttl 64, id 1573, offset 0, flags [DF], proto TCP (6), length 60) > 10.1.1.1.47450 > 10.1.1.100.search-agent: Flags [S], cksum 0x1695 > (incorrect -> 0xbccd), seq 4096662719, win 64860, options [mss > 1410,sackOK,TS val 119316842 ecr 0,nop,wscale 7], length 0 > (gdb) n 7 > 7164 *csum_l4 = csum(l4, dp_packet_size(b) - csum_start); > (gdb) n > 7166 if (l4proto == IPPROTO_TCP) { > (gdb) ovs_dump_packets b -n -vvv > 16:08:35.561216 IP (tos 0x0, ttl 64, id 52391, offset 0, flags [none], > proto UDP (17), length 110) > 172.31.1.1.52791 > 172.31.1.100.vxlan: [udp sum ok] VXLAN, flags > [I] (0x08), vni 0 > IP (tos 0x0, ttl 64, id 1573, offset 0, flags [DF], proto TCP (6), length 60) > 10.1.1.1.47450 > 10.1.1.100.search-agent: Flags [S], cksum 0xbccd > (correct), seq 4096662719, win 64860, options [mss 1410,sackOK,TS val > 119316842 ecr 0,nop,wscale 7], length 0 > > > > > > > + if (csum_offset == offsetof(struct tcp_header, tcp_csum) > > > + && l4proto == IPPROTO_TCP) { > > > + dp_packet_hwol_set_csum_tcp(b); > > > + l4_csum_partial = true; > > > + } else if (csum_offset == offsetof(struct udp_header, > > > udp_csum) > > > + && l4proto == IPPROTO_UDP) { > > > + dp_packet_hwol_set_csum_udp(b); > > > + l4_csum_partial = true; > > > + } else if (csum_offset == offsetof(struct sctp_header, > > > sctp_csum) > > > + && l4proto == IPPROTO_SCTP) { > > > + dp_packet_hwol_set_csum_sctp(b); > > > > I was thinking about the possibility of having a flag to indicate > > something like dp_packet_hwol_set_csum_any(), it would be the union of > > tcp, udp, and sctp. Then possibly on egress or miniflow_extract, we > > could check for that value and populate the correct flag. > > I don't see the need for a _any() helper. > Setting the Tx flags is indeed done along OVS protocol validation in > miniflow_extract(). > > I suspect parse_tcp_flags() (for the simple matching optimisation) is > not doing a good job as it sets no l4 tx flags, this may be what was > missing initially.
I posted a v4 based on this last comment. Let's continue in the new thread. -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
