Hello,
On Wed, Mar 5, 2025 at 4:01 PM Mike Pattrick <m...@redhat.com> wrote:
> > @@ -7104,25 +7120,60 @@ netdev_linux_parse_vnet_hdr(struct dp_packet *b)
> > }
> >
> > if (vnet->flags == VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > - uint16_t l4proto = 0;
> > -
> > - if (netdev_linux_parse_l2(b, &l4proto)) {
> > + uint16_t csum_offset = (OVS_FORCE uint16_t) vnet->csum_offset;
> > + uint16_t csum_start = (OVS_FORCE uint16_t) vnet->csum_start;
> > + bool l4_csum_partial = false;
> > + uint16_t l4proto;
> > + uint16_t l2_len;
> > + uint16_t l3_len;
> > +
> > + if (netdev_linux_parse_packet(b, &l2_len, &l3_len, &l4proto)) {
> > return EINVAL;
> > }
> >
> > - if (l4proto == IPPROTO_UDP) {
> > - dp_packet_hwol_set_csum_udp(b);
> > + if (csum_start && csum_offset && csum_start == l2_len + l3_len) {
>
> 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.
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
