On Thu, Oct 24, 2024 at 05:36:10PM +0100, Daniel P. Berrangé wrote: > On Mon, Oct 21, 2024 at 12:14:38AM -0400, Laine Stump wrote: > > After some discussion with Phil Sutter and Eric Garver (nftables > > people), they suggested that, while nftables doesn't have an action > > that will *compute* the checksum of a packet, it *does* have an action > > that will set the checksum to 0, and that maybe we should try > > that. Then Phil tried it himself by manually adding such a rule to a > > running system, and verified that it did fix the issue at least for > > FreeBSD guests. > > > > So over the weekend I came up with a patch to add a checksum 0 rule to > > the rules setup for each virtual network. This is that patch. > > > > I have so far verified that this patch enables FreeBSD to receive the > > DHCP response and get an IP address, and that it hasn't *broken* this > > functionality for a random old Fedora image I had (Fedora 27!?!?! I > > really need to update my test images!!). Before pushing it I would > > like to verify that zeroing the checksum of DHCP response packets > > doesn't break any other guest, so I would appreciate the help of > > anyone who could build and install libvirt with this patch and let me > > know of both successes and failures of any guest to acquire an IP > > address with DHCP. Once I've received enough positive reports (and 0 > > negative reports!) then we can think about pushing this patch (and > > also backporting it downstream to Fedora 40) > > On the one hand it is good that you test this and found it to > to work. > > What concerns me is a lack of understanding of /why/ it works. > > AFAICT there is nothing in the TCP RFC documenting all-zeros > as a special case for indicating absent checksums. Doh, we're dealing with UDP for DHCP, not TCP /facepalm The UDP RFC 768 says [quote] If the computed checksum is zero, it is transmitted as all ones (the equivalent in one's complement arithmetic). An all zero transmitted checksum value means that the transmitter generated no checksum (for debugging or for higher level protocols that don't care). [/quote] > I'd really like to know /why/ it works, so we can be confident > we're relying on intentional behaviour, as opposed to a happy > accident. I've finally found the dhclient code that makes this work https://github.com/isc-projects/dhcp/blob/master/common/packet.c#L365 /* UDP check sum may be optional (udp.uh_sum == 0) or not ready if checksum * offloading is in use */ udp_packets_seen++; if (udp.uh_sum && csum_ready) { /* Check the UDP header checksum - since the received packet header * contains the UDP checksum calculated by the transmitter, calculating * it now should come out to zero. */ .... IOW, if 'uh_sum' is all zeroes, then it skips checksum validation, which is exactly what we want. > Functionally your patch does what it claims to do, so codewise > I'm happy to say Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>, > but I'd rather not merge it without a deeper understandnig. We can go ahead and merge this, as its matching specified UDP behaviour. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
