On Thu, Oct 24, 2024 at 06:42:55PM +0100, Daniel P. Berrangé wrote: > 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. For another example, OpenBSD does NOT use ISC dhclient, and also explicitly skips treating an all-zeros checksum as an error https://github.com/openbsd/src/blob/master/sbin/dhcpleased/engine.c#L855 if (usum != 0 && usum != sum) { log_warnx("%s: bad UDP checksum", __func__); return; } > > > 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 :| > 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 :|
