RX checksum offloading problem

[Yonghyeon PYUN]

On Fri, May 09, 2014 at 04:22:36PM +0200, Michael Tuexen wrote:
> 
> On 09 May 2014, at 12:46, Michael Tuexen <Michael.Tuexen at lurchi.franken.de> wrote:
> 
> > On 09 May 2014, at 03:35, Yonghyeon PYUN <pyunyh at gmail.com> wrote:
> > 
> >> On Thu, May 08, 2014 at 08:40:22PM +0200, Michael Tuexen wrote:
> >>> On 07 May 2014, at 10:37, Yonghyeon PYUN <pyunyh at gmail.com> wrote:
> >>> 
> >>>> On Wed, May 07, 2014 at 10:07:09AM +0200, Michael Tuexen wrote:
> >>>>> On 07 May 2014, at 09:56, Yonghyeon PYUN <pyunyh at gmail.com> wrote:
> >>>>> 
> >>>>>> On Sat, May 03, 2014 at 11:52:47AM +0200, Michael Tuexen wrote:
> >>>>>>> On 02 May 2014, at 16:02, Bjoern A. Zeeb <bz at FreeBSD.org> wrote:
> >>>>>>> 
> >>>>>>>> 
> >>>>>>>> On 02 May 2014, at 10:22 , Michael Tuexen <Michael.Tuexen at lurchi.franken.de> wrote:
> >>>>>>>> 
> >>>>>>>>> Dear all,
> >>>>>>>>> 
> >>>>>>>>> during testing I found that FreeBSD head (on a raspberry pi) accepts SCTP packet
> >>>>>>>>> with bad checksums. After debugging this I figured out that this is a problem with
> >>>>>>>>> the csum_flags defined in mbuf.h.
> >>>>>>>>> 
> >>>>>>>>> The SCTP code on its input path checks for CSUM_SCTP_VALID, which is defined in mbuf.h:
> >>>>>>>>> #define CSUM_SCTP_VALID         CSUM_L4_VALID
> >>>>>>>>> This makes sense: If CSUM_SCTP_VALID is set in csum_flags, the packet is considered
> >>>>>>>>> to have a correct checksum.
> >>>>>>>>> 
> >>>>>>>>> For UDP and TCP some drivers calculate the UDP/TCP checksum and set CSUM_DATA_VALID in
> >>>>>>>>> csum_flags to indicate that the UDP/TCP should consider csum_data to figure out if
> >>>>>>>>> the packet has a correct checksum. The problem is that CSUM_DATA_VALID is defined as
> >>>>>>>>> #define CSUM_DATA_VALID         CSUM_L4_VALID
> >>>>>>>>> In this case the semantic is not that the packet has a valid checksum, but the csum_data
> >>>>>>>>> field contains information.
> >>>>>>>>> 
> >>>>>>>>> Now the following happens (on the raspberry pi the driver used is
> >>>>>>>>> dev/usb/net/if_smsc.c
> >>>>>>>>> 
> >>>>>>>>> 1. A packet is received and if it is not too short, the checksum computed
> >>>>>>>>> is stored in csum_data and the flag CSUM_DATA_VALID is set. This happens
> >>>>>>>>> for all IP packets, not only for UDP and TCP packets.
> >>>>>>>>> 2. In case of SCTP packets, the SCTP interprets CSUM_DATA_VALID as CSUM_SCTP_VALID
> >>>>>>>>> and accepts the packet. So no SCTP checksum check ever happened.
> >>>>>>>>> 
> >>>>>>>>> Alternatives to fix this:
> >>>>>>>>> 
> >>>>>>>>> 1. Change all drivers to set CSUM_DATA_VALID only in case of UDP or TCP packets, since
> >>>>>>>>> it only makes sense in these cases.
> >>>>>>>> 
> >>>>>>>> Wait, or for SCTP in cad the crc32 (I think it was)  was actually checked but not otherwise.   This is how it should be imho.  It seems like a driver bug.
> >>>>>>> I went through the list of drivers and you are right, it seems to be a bug
> >>>>>>> in if_smsc.c. Most of the other drivers check for UDP/TCP, a small set I can't tell.
> >>>>>>> 
> >>>>>> 
> >>>>>> I'm not sure how the controller computes TCP/UDP checksum values.
> >>>>>> It seems the publicly available data sheet was highly sanitized so
> >>>>>> it was useless to me.  The comment in the driver says that the
> >>>>> Same for me...
> >>>>>> controller computes RX checksum after the IPv4 header to the end of
> >>>>>> ethernet frame. After seeing that comment, three questions popped
> >>>>>> up:
> >>>>>> 
> >>> OK, I did some testing. It looks like the card is just computing the
> >>> checksum over the IP payload taking the correct IP header length into account.
> >>>>>> 1. Is the controller smart enough to skip IP options header in
> >>>>>> TCP/UDP checksum offloading?
> >>> Yes, I can send fragmented and un-fragmented UDP packets with IP options
> >>> and they are handled correctly. Even if the last fragment is too short.
> >> 
> >> I'm assuming you're taking about receiving fragmented UDP packets
> >> with RX checksum offloading, right?
> > Correct.
> >> 
> >>>>>> 2. How controller handles UDP checksum value 0x0000(i.e. sender
> >>>>>> didn't compute UDP checksum)?
> >>> This case isn't handled. However, udp_input() looks first for zero checksums
> >>> and only after that in the csum_flags. So it doesn't result in any problems.
> >>> Would you prefer not to set CSUM_DATA_VALID in this case?
> >> 
> >> At least, it correctly updates UDP stats of netstat(1).
> > Let me double check that...
> I double checked it. The statistic counters are incremented.
> Please note that we had a bug in the sending code of head, which
> made it impossible to send UDP packets with 0 checksum. That is
> fixed in
> http://svnweb.freebsd.org/base?view=revision&revision=265776

Thanks.

> 
> So any preference whether to report CSUM_DATA_VALID if a UDP packet
> with checksum 0 is received or not? I'm pretty open, since it does
> not have any effect right now...
> 

Because upper stack correctly counts for these packets, your change
(report CSUM_DATA_VALID for UDP packet with checksum value 0) looks
fine.  I don't remember how pf/ipf handles that case though but we
can easily fix pf/ipf once we see breakage.

> Best regards
> Michael