There is also a related patch to the FreeBSD Xen netfront driver from Roger that goes with this patch:

https://bugs.freebsd.org/bugzilla/attachment.cgi?id=180335&action=diff&headers=0#b/sys/dev/xen/netfront/netfront.c_sec1

Hiya,

So 0x0000 means "disable UDP checksum". What if the sender sends a checksum of 0xffff ? yes its invalid, but what if they do? where's that handled?

@adrian good question. AFAICT sender sending a checksum of 0xffff would be correctly handled in the FreeBSD kernel UDP stack in udp_input()

Ping?

If I understand this correctly, when a 0xffff checksum comes in via wire and the receiving NIC has no RXCSUM offloading, dhclient with this patch does not check the checksum. Is this a correct behavior? I think the conditional (usum != 0xffff) should be added only when the NIC has IFCAP_RXCSUM and it is enabled. Whether this flag is activated or not can be obtained by SIOSGIFCAP ioctl (ifr->ifr_curcap).

I'm contemplating withdrawing this review as I agree that usum == 0xffff is not a good test for whether the UDP checksum validation was offloaded to the NIC/driver. And @hrs this applies even if we can use SIOSGIFCAP to look for IFCAP_RXCSUM.

I had a different proposed patch to this dhclient issue which @royger was not in favor of, so we pursued this different approach.

Can folks please take a quick look at https://bugs.freebsd.org/bugzilla/attachment.cgi?id=180307&action=diff to see if that is a more acceptable patch?

In D9832#207512, @bhavesh.davda_gmail.com wrote:

Can folks please take a quick look at https://bugs.freebsd.org/bugzilla/attachment.cgi?id=180307&action=diff to see if that is a more acceptable patch?

I don't like that patch because this is not a Xen specific problem (although it's more common on Xen), and as so it shouldn't be solved in a Xen specific way. It's perfectly valid for a nic to output a packet with checksum == 0xffff and the CSUM_DATA_VALID | CSUM_PSEUDO_HDR flags set, according to the mbuf(9) man page.

FWIW, this was fixed not long ago in isc-dhcp with the following commit:

https://source.isc.org/cgi-bin/gitweb.cgi?p=dhcp.git;a=patch;h=7ff6ae5aa85754119319def3c7f225a40da299c4

The mbuf(9) man page specifically says this:

If a particular network interface just indicates success or failure of TCP or UDP checksum validation without returning the exact value of the checksum to the host CPU, its driver can mark CSUM_DATA_VALID and CSUM_PSEUDO_HDR in csum_flags, and set csum_data to 0xFFFF hexadecimal to indicate a valid checksum. It is a peculiarity of the algorithm used that the Internet checksum calculated over any valid packet will be 0xFFFF as long as the original checksum field is included.

@royger 's proposed patch @ https://bugs.freebsd.org/bugzilla/attachment.cgi?id=180335&action=diff&headers=0#b/sys/dev/xen/netfront/netfront.c_sec1 fixes the Xen netfront driver to conform to this recommendation in the mbuf(9) man page.

However, m->m_pkthdr.csum_data == 0xFFFF only helps udp_input() that I previously cited to "validate" that the received UDP checksum is "correct":

if (uh->uh_sum) {
        u_short uh_sum;
        if ((m->m_pkthdr.csum_flags & CSUM_DATA_VALID) &&
            !cscov_partial) {
                if (m->m_pkthdr.csum_flags & CSUM_PSEUDO_HDR)
                        uh_sum = m->m_pkthdr.csum_data; // this will be 0xffff with the Xen netfront driver change
                        ...
                uh_sum ^= 0xffff;  // This will make uh_sum '0'
        }
        if (uh_sum) { // This will be false as a result of the above, so packet won't be dropped. But the UDP checksum in the packet payload won't have been modified at all
                UDPSTAT_INC(udps_badsum);
                m_freem(m);
                return (IPPROTO_DONE);
        }

So in other words, my proposed patch in this review won't work.

In D9832#207524, @bhavesh.davda_gmail.com wrote:

The mbuf(9) man page specifically says this:

If a particular network interface just indicates success or failure of TCP or UDP checksum validation without returning the exact value of the checksum to the host CPU, its driver can mark CSUM_DATA_VALID and CSUM_PSEUDO_HDR in csum_flags, and set csum_data to 0xFFFF hexadecimal to indicate a valid checksum. It is a peculiarity of the algorithm used that the Internet checksum calculated over any valid packet will be 0xFFFF as long as the original checksum field is included.

@royger 's proposed patch @ https://bugs.freebsd.org/bugzilla/attachment.cgi?id=180335&action=diff&headers=0#b/sys/dev/xen/netfront/netfront.c_sec1 fixes the Xen netfront driver to conform to this recommendation in the mbuf(9) man page.

However, m->m_pkthdr.csum_data == 0xFFFF only helps udp_input() that I previously cited to "validate" that the received UDP checksum is "correct":

if (uh->uh_sum) {
        u_short uh_sum;
        if ((m->m_pkthdr.csum_flags & CSUM_DATA_VALID) &&
            !cscov_partial) {
                if (m->m_pkthdr.csum_flags & CSUM_PSEUDO_HDR)
                        uh_sum = m->m_pkthdr.csum_data; // this will be 0xffff with the Xen netfront driver change
                        ...
                uh_sum ^= 0xffff;  // This will make uh_sum '0'
        }
        if (uh_sum) { // This will be false as a result of the above, so packet won't be dropped. But the UDP checksum in the packet payload won't have been modified at all
                UDPSTAT_INC(udps_badsum);
                m_freem(m);
                return (IPPROTO_DONE);
        }

So in other words, my proposed patch in this review won't work.

Can dhclient check for the CSUM_DATA_VALID | CSUM_PSEUDO_HDR flags and just skip checking the checksum?

In D9832#208335, @royger wrote:

Can dhclient check for the CSUM_DATA_VALID | CSUM_PSEUDO_HDR flags and just skip checking the checksum?

I'm sorry @royger I don't quite understand. dhclient is a userspace daemon using the bpf interface to send and receive packets from the FreeBSD kernel. the CSUM_* mbuf(9) flags are only available in the kernel, so not sure how dhclient can check for them over the bpf interface.