[ovs-dev,branch-3.2,v4] dp-packet: Correct IPv4 checksum calculation.

During the transition towards checksum offloading, the function to
handle software fallback of IPv4 checksums didn't account for the
options field.

Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
Reported-by: Jun Wang <junwang01@cestc.cn>
Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
NB: In 3.2 the early checksum offloading patches caused packets from
the netdev-dummy/receive appctl command to resolve checksums prior to
odp-execute. This fixed in 3.3, but that can't easily be backported
without also backporting unrelated code.

To enable this patch to be backported, I gave one of the packets a
correct checksum and changed the comment so it was correct but also the
same number of lines as in the previous patch.

---
 lib/dp-packet.h      | 11 +++++++++--
 tests/dpif-netdev.at | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

--
2.43.5

On 8/15/24 22:03, Mike Pattrick wrote:
> During the transition towards checksum offloading, the function to
> handle software fallback of IPv4 checksums didn't account for the
> options field.
> 
> Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
> Reported-by: Jun Wang <junwang01@cestc.cn>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---
> NB: In 3.2 the early checksum offloading patches caused packets from
> the netdev-dummy/receive appctl command to resolve checksums prior to
> odp-execute. This fixed in 3.3, but that can't easily be backported
> without also backporting unrelated code.
> 
> To enable this patch to be backported, I gave one of the packets a
> correct checksum and changed the comment so it was correct but also the
> same number of lines as in the previous patch.
> 

Hi, Mike.  Thanks for looking into this, but I'm still a bit confused
about what is going on.  The packets in the test do not trigger the
dp_packet_ip_set_header_csum() call, i.e. these tests do not actually
test the code change in this patch.  However, the previous ${bad_frame}
does trigger the function and the checksum is re-calculated for it.

What's the difference between these two?  Are IP options affecting the
logic somehow?

Best regards, Ilya Maximets.

> ---
>  lib/dp-packet.h      | 11 +++++++++--
>  tests/dpif-netdev.at | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 70ddf8aa4..25a0d31a1 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -1142,10 +1142,17 @@ static inline void
>  dp_packet_ip_set_header_csum(struct dp_packet *p)
>  {
>      struct ip_header *ip = dp_packet_l3(p);
> +    size_t l3_size = dp_packet_l3_size(p);
> +    size_t ip_len;
> 
>      ovs_assert(ip);
> -    ip->ip_csum = 0;
> -    ip->ip_csum = csum(ip, sizeof *ip);
> +
> +    ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
> +
> +    if (OVS_LIKELY(ip_len >= IP_HEADER_LEN && ip_len < l3_size)) {
> +        ip->ip_csum = 0;
> +        ip->ip_csum = csum(ip, ip_len);
> +    }
>  }
> 
>  /* Returns 'true' if the packet 'p' has good integrity and the
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 061e13af8..93226521a 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -807,6 +807,41 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${bad_frame}])
>  AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
>  AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
>  ])
> +
> +dnl Test with IP optional fields in a valid packet.  Note that this packet
> +dnl contains a correct checksum, but the following packet doesn't. This is
> +dnl because the dummy interface resets checksum offload on packet recipt.
> +m4_define([OPT_PKT], m4_join([],
> +dnl eth(dst=aa:aa:aa:aa:aa:aa,src=bb:bb:bb:bb:bb:bb,type=0x0800)
> +[aaaaaaaaaaaabbbbbbbbbbbb0800],
> +dnl ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,len=60,tot_len=68,csum=0x94d6)
> +[4f000044abab0000400194d60a0000010a000002],
> +dnl IPv4 Opt: type 7 (Record Route) len 39 + type 0 (EOL).
> +[07270c010203040a000003000000000000000000],
> +[0000000000000000000000000000000000000000],
> +dnl icmp(type=8,code=0), csum 0x3e2f incorrect, should be 0x412f.
> +[08003e2fb6d00000]))
> +
> +dnl IP header indicates optional fields but doesn't contain any.
> +m4_define([MICROGRAM], m4_join([],
> +dnl eth(dst=aa:aa:aa:aa:aa:aa,src=bb:bb:bb:bb:bb:bb,type=0x0800)
> +[aaaaaaaaaaaabbbbbbbbbbbb0800],
> +dnl ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,len=60,tot_len=68,csum=0xeeee)
> +[4f000044abab00004001eeee0a0000010a000002]))
> +
> +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 OPT_PKT])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 MICROGRAM])
> +AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> +
> +dnl Build the expected modified packets. The first packet has a valid IPv4
> +dnl checksum and modified destination IP address. The second packet isn't
> +dnl expected to change.
> +AT_CHECK([echo "OPT_PKT" | sed -e "s/0a000002/c0a80101/" -e "s/94d6/dd2e/" > expout])
> +AT_CHECK([echo "MICROGRAM" >> expout])
> +AT_CHECK([tail -n 2 p2.pcap.txt], [0], [expout])
> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> 
> --
> 2.43.5
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Fri, Aug 16, 2024 at 10:04 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 8/15/24 22:03, Mike Pattrick wrote:
> > During the transition towards checksum offloading, the function to
> > handle software fallback of IPv4 checksums didn't account for the
> > options field.
> >
> > Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
> > Reported-by: Jun Wang <junwang01@cestc.cn>
> > Signed-off-by: Mike Pattrick <mkp@redhat.com>
> > ---
> > NB: In 3.2 the early checksum offloading patches caused packets from
> > the netdev-dummy/receive appctl command to resolve checksums prior to
> > odp-execute. This fixed in 3.3, but that can't easily be backported
> > without also backporting unrelated code.
> >
> > To enable this patch to be backported, I gave one of the packets a
> > correct checksum and changed the comment so it was correct but also the
> > same number of lines as in the previous patch.
> >
>
> Hi, Mike.  Thanks for looking into this, but I'm still a bit confused
> about what is going on.  The packets in the test do not trigger the
> dp_packet_ip_set_header_csum() call, i.e. these tests do not actually
> test the code change in this patch.  However, the previous ${bad_frame}
> does trigger the function and the checksum is re-calculated for it.
>
> What's the difference between these two?  Are IP options affecting the
> logic somehow?

This is unrelated to the content of the packet, and is instead related
to how dummy/receive packets are handled in offloading scenarios. This
was fixed in: 084c8087292c ("userspace: Support VXLAN and GENEVE
TSO.")



>
> Best regards, Ilya Maximets.
>
> > ---
> >  lib/dp-packet.h      | 11 +++++++++--
> >  tests/dpif-netdev.at | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> > index 70ddf8aa4..25a0d31a1 100644
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -1142,10 +1142,17 @@ static inline void
> >  dp_packet_ip_set_header_csum(struct dp_packet *p)
> >  {
> >      struct ip_header *ip = dp_packet_l3(p);
> > +    size_t l3_size = dp_packet_l3_size(p);
> > +    size_t ip_len;
> >
> >      ovs_assert(ip);
> > -    ip->ip_csum = 0;
> > -    ip->ip_csum = csum(ip, sizeof *ip);
> > +
> > +    ip_len = IP_IHL(ip->ip_ihl_ver) * 4;
> > +
> > +    if (OVS_LIKELY(ip_len >= IP_HEADER_LEN && ip_len < l3_size)) {
> > +        ip->ip_csum = 0;
> > +        ip->ip_csum = csum(ip, ip_len);
> > +    }
> >  }
> >
> >  /* Returns 'true' if the packet 'p' has good integrity and the
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index 061e13af8..93226521a 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -807,6 +807,41 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${bad_frame}])
> >  AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> >  AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> >  ])
> > +
> > +dnl Test with IP optional fields in a valid packet.  Note that this packet
> > +dnl contains a correct checksum, but the following packet doesn't. This is
> > +dnl because the dummy interface resets checksum offload on packet recipt.
> > +m4_define([OPT_PKT], m4_join([],
> > +dnl eth(dst=aa:aa:aa:aa:aa:aa,src=bb:bb:bb:bb:bb:bb,type=0x0800)
> > +[aaaaaaaaaaaabbbbbbbbbbbb0800],
> > +dnl ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,len=60,tot_len=68,csum=0x94d6)
> > +[4f000044abab0000400194d60a0000010a000002],
> > +dnl IPv4 Opt: type 7 (Record Route) len 39 + type 0 (EOL).
> > +[07270c010203040a000003000000000000000000],
> > +[0000000000000000000000000000000000000000],
> > +dnl icmp(type=8,code=0), csum 0x3e2f incorrect, should be 0x412f.
> > +[08003e2fb6d00000]))
> > +
> > +dnl IP header indicates optional fields but doesn't contain any.
> > +m4_define([MICROGRAM], m4_join([],
> > +dnl eth(dst=aa:aa:aa:aa:aa:aa,src=bb:bb:bb:bb:bb:bb,type=0x0800)
> > +[aaaaaaaaaaaabbbbbbbbbbbb0800],
> > +dnl ipv4(dst=10.0.0.2,src=10.0.0.1,proto=1,len=60,tot_len=68,csum=0xeeee)
> > +[4f000044abab00004001eeee0a0000010a000002]))
> > +
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 OPT_PKT])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 MICROGRAM])
> > +AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > +
> > +dnl Build the expected modified packets. The first packet has a valid IPv4
> > +dnl checksum and modified destination IP address. The second packet isn't
> > +dnl expected to change.
> > +AT_CHECK([echo "OPT_PKT" | sed -e "s/0a000002/c0a80101/" -e "s/94d6/dd2e/" > expout])
> > +AT_CHECK([echo "MICROGRAM" >> expout])
> > +AT_CHECK([tail -n 2 p2.pcap.txt], [0], [expout])
> > +
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> >
> > --
> > 2.43.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>

On 8/19/24 09:23, Mike Pattrick wrote:
> On Fri, Aug 16, 2024 at 10:04 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>>
>> On 8/15/24 22:03, Mike Pattrick wrote:
>>> During the transition towards checksum offloading, the function to
>>> handle software fallback of IPv4 checksums didn't account for the
>>> options field.
>>>
>>> Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
>>> Reported-by: Jun Wang <junwang01@cestc.cn>
>>> Signed-off-by: Mike Pattrick <mkp@redhat.com>
>>> ---
>>> NB: In 3.2 the early checksum offloading patches caused packets from
>>> the netdev-dummy/receive appctl command to resolve checksums prior to
>>> odp-execute. This fixed in 3.3, but that can't easily be backported
>>> without also backporting unrelated code.
>>>
>>> To enable this patch to be backported, I gave one of the packets a
>>> correct checksum and changed the comment so it was correct but also the
>>> same number of lines as in the previous patch.
>>>
>>
>> Hi, Mike.  Thanks for looking into this, but I'm still a bit confused
>> about what is going on.  The packets in the test do not trigger the
>> dp_packet_ip_set_header_csum() call, i.e. these tests do not actually
>> test the code change in this patch.  However, the previous ${bad_frame}
>> does trigger the function and the checksum is re-calculated for it.
>>
>> What's the difference between these two?  Are IP options affecting the
>> logic somehow?
> 
> This is unrelated to the content of the packet, and is instead related
> to how dummy/receive packets are handled in offloading scenarios. This
> was fixed in: 084c8087292c ("userspace: Support VXLAN and GENEVE
> TSO.")

So, I traced the packets through the code.  And it is, in fact, related to
the content of the packet.  All the previous packets in the test have the
same signature - IPv4 / TCP with 192.168.123.1 destination.  So, after the
first good frame we have a datapath flow installed that looks like this:

  in_port(1),eth_type(0x0800),ipv4(dst=192.168.123.1,proto=6,frag=no),
  actions:set(ipv4(dst=192.168.1.1)),2

We have a match on destination IP, because our OF rule is changing it.
All the incoming packets have the same offlad flags set.  They are all
marked for Tx IPv4 checksum offload and all marked as having good csum.

However, the newly added packets in this patch are ICMP packets and have
a different destination address, so they are going through upcall.
And on 3.2 branch all the packets sent through upcall have their checksums
explicitly fixed up beforehand:

  dp_netdev_upcall() -> dp_packet_ol_send_prepare(packet_, 0);

Since we have csum_good flag set, the preparation removes the Tx offload
flag.  Later, the IP address modification no longer sees the Tx offload flag
and modifies checksum incrementally.

If the packet will not go through upcall, then the Tx offload flag will be
preserved up until dp_packet_ip_set_header_csum() and the checksum will be
fully re-calculated triggering the issue fixed in this patch.

So, we either need to send the same TCP packet, but with IP options
to not trigger an upcall, or we could just send the same packet twice
and only check the second pass that will go through the datapath without
triggering an upcall.

Best regards, Ilya Maximets.

On Tue, Aug 20, 2024 at 6:09 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 8/19/24 09:23, Mike Pattrick wrote:
> > On Fri, Aug 16, 2024 at 10:04 AM Ilya Maximets <i.maximets@ovn.org> wrote:
> >>
> >> On 8/15/24 22:03, Mike Pattrick wrote:
> >>> During the transition towards checksum offloading, the function to
> >>> handle software fallback of IPv4 checksums didn't account for the
> >>> options field.
> >>>
> >>> Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
> >>> Reported-by: Jun Wang <junwang01@cestc.cn>
> >>> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> >>> ---
> >>> NB: In 3.2 the early checksum offloading patches caused packets from
> >>> the netdev-dummy/receive appctl command to resolve checksums prior to
> >>> odp-execute. This fixed in 3.3, but that can't easily be backported
> >>> without also backporting unrelated code.
> >>>
> >>> To enable this patch to be backported, I gave one of the packets a
> >>> correct checksum and changed the comment so it was correct but also the
> >>> same number of lines as in the previous patch.
> >>>
> >>
> >> Hi, Mike.  Thanks for looking into this, but I'm still a bit confused
> >> about what is going on.  The packets in the test do not trigger the
> >> dp_packet_ip_set_header_csum() call, i.e. these tests do not actually
> >> test the code change in this patch.  However, the previous ${bad_frame}
> >> does trigger the function and the checksum is re-calculated for it.
> >>
> >> What's the difference between these two?  Are IP options affecting the
> >> logic somehow?
> >
> > This is unrelated to the content of the packet, and is instead related
> > to how dummy/receive packets are handled in offloading scenarios. This
> > was fixed in: 084c8087292c ("userspace: Support VXLAN and GENEVE
> > TSO.")
>
> So, I traced the packets through the code.  And it is, in fact, related to
> the content of the packet.  All the previous packets in the test have the
> same signature - IPv4 / TCP with 192.168.123.1 destination.  So, after the
> first good frame we have a datapath flow installed that looks like this:
>
>   in_port(1),eth_type(0x0800),ipv4(dst=192.168.123.1,proto=6,frag=no),
>   actions:set(ipv4(dst=192.168.1.1)),2
>
> We have a match on destination IP, because our OF rule is changing it.
> All the incoming packets have the same offlad flags set.  They are all
> marked for Tx IPv4 checksum offload and all marked as having good csum.
>
> However, the newly added packets in this patch are ICMP packets and have
> a different destination address, so they are going through upcall.
> And on 3.2 branch all the packets sent through upcall have their checksums
> explicitly fixed up beforehand:
>
>   dp_netdev_upcall() -> dp_packet_ol_send_prepare(packet_, 0);
>
> Since we have csum_good flag set, the preparation removes the Tx offload
> flag.  Later, the IP address modification no longer sees the Tx offload flag
> and modifies checksum incrementally.
>
> If the packet will not go through upcall, then the Tx offload flag will be
> preserved up until dp_packet_ip_set_header_csum() and the checksum will be
> fully re-calculated triggering the issue fixed in this patch.
>
> So, we either need to send the same TCP packet, but with IP options
> to not trigger an upcall, or we could just send the same packet twice
> and only check the second pass that will go through the datapath without
> triggering an upcall.

I said that the behaviour wasn't content dependent because the call to
send_prepare happened in the upcall - regardless of packet content. If
you'll accept the solution, I can resubmit with sending the options
packet twice, which also resolves the issue.


Cheers,
M

>
> Best regards, Ilya Maximets.
>

On 8/22/24 16:30, Mike Pattrick wrote:
> On Tue, Aug 20, 2024 at 6:09 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>>
>> On 8/19/24 09:23, Mike Pattrick wrote:
>>> On Fri, Aug 16, 2024 at 10:04 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>>>>
>>>> On 8/15/24 22:03, Mike Pattrick wrote:
>>>>> During the transition towards checksum offloading, the function to
>>>>> handle software fallback of IPv4 checksums didn't account for the
>>>>> options field.
>>>>>
>>>>> Fixes: 5d11c47d3ebe ("userspace: Enable IP checksum offloading by default.")
>>>>> Reported-by: Jun Wang <junwang01@cestc.cn>
>>>>> Signed-off-by: Mike Pattrick <mkp@redhat.com>
>>>>> ---
>>>>> NB: In 3.2 the early checksum offloading patches caused packets from
>>>>> the netdev-dummy/receive appctl command to resolve checksums prior to
>>>>> odp-execute. This fixed in 3.3, but that can't easily be backported
>>>>> without also backporting unrelated code.
>>>>>
>>>>> To enable this patch to be backported, I gave one of the packets a
>>>>> correct checksum and changed the comment so it was correct but also the
>>>>> same number of lines as in the previous patch.
>>>>>
>>>>
>>>> Hi, Mike.  Thanks for looking into this, but I'm still a bit confused
>>>> about what is going on.  The packets in the test do not trigger the
>>>> dp_packet_ip_set_header_csum() call, i.e. these tests do not actually
>>>> test the code change in this patch.  However, the previous ${bad_frame}
>>>> does trigger the function and the checksum is re-calculated for it.
>>>>
>>>> What's the difference between these two?  Are IP options affecting the
>>>> logic somehow?
>>>
>>> This is unrelated to the content of the packet, and is instead related
>>> to how dummy/receive packets are handled in offloading scenarios. This
>>> was fixed in: 084c8087292c ("userspace: Support VXLAN and GENEVE
>>> TSO.")
>>
>> So, I traced the packets through the code.  And it is, in fact, related to
>> the content of the packet.  All the previous packets in the test have the
>> same signature - IPv4 / TCP with 192.168.123.1 destination.  So, after the
>> first good frame we have a datapath flow installed that looks like this:
>>
>>   in_port(1),eth_type(0x0800),ipv4(dst=192.168.123.1,proto=6,frag=no),
>>   actions:set(ipv4(dst=192.168.1.1)),2
>>
>> We have a match on destination IP, because our OF rule is changing it.
>> All the incoming packets have the same offlad flags set.  They are all
>> marked for Tx IPv4 checksum offload and all marked as having good csum.
>>
>> However, the newly added packets in this patch are ICMP packets and have
>> a different destination address, so they are going through upcall.
>> And on 3.2 branch all the packets sent through upcall have their checksums
>> explicitly fixed up beforehand:
>>
>>   dp_netdev_upcall() -> dp_packet_ol_send_prepare(packet_, 0);
>>
>> Since we have csum_good flag set, the preparation removes the Tx offload
>> flag.  Later, the IP address modification no longer sees the Tx offload flag
>> and modifies checksum incrementally.
>>
>> If the packet will not go through upcall, then the Tx offload flag will be
>> preserved up until dp_packet_ip_set_header_csum() and the checksum will be
>> fully re-calculated triggering the issue fixed in this patch.
>>
>> So, we either need to send the same TCP packet, but with IP options
>> to not trigger an upcall, or we could just send the same packet twice
>> and only check the second pass that will go through the datapath without
>> triggering an upcall.
> 
> I said that the behaviour wasn't content dependent because the call to
> send_prepare happened in the upcall - regardless of packet content.

My point was that the fact the we're having an upcall depends on the
packet content.  If we use a TCP packet with 192.168.123.1 destination,
we'll not have an upcall.

> If you'll accept the solution, I can resubmit with sending the options
> packet twice, which also resolves the issue.

Double sending seems reasonable.

As a separate change we may also want to enhance the test on the main
branch by sending two packets as well (and checking both of them), so
we can cover both the upcall and non-upcall path.

Best regards, Ilya Maximets.