On Sat, Apr 05, 2025 at 05:04:28PM +0900, Akihiko Odaki wrote:
The goal of commit 7987d2be5a8b ("virtio-net: Copy received header to
buffer") was to remove the need to patch the (const) input buffer with a
recomputed UDP checksum by copying headers to a RW region and inject the
checksum there. The patch computed the checksum only from the header
fields (missing the rest of the payload) producing an invalid one
and making guests fail to acquire a DHCP lease.
Fix the issue by copying the entire packet instead of only copying the
headers.
Fixes: 7987d2be5a8b ("virtio-net: Copy received header to buffer")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2727
Cc: qemu-sta...@nongnu.org
Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
Tested-By: Antoine Damhet <adam...@scaleway.com>
---
This patch aims to resolves the issue the following one also does:
https://lore.kernel.org/qemu-devel/20250404151835.328368-1-adam...@scaleway.com
The difference from the mentioned patch is that this patch also
preserves that the original intent of regressing change, which is to
remove the need to patch the (const) input buffer with a recomputed UDP
checksum.
To Antoine Damhet:
I confirmed that DHCP is currently not working and this patch fixes the
issue, but I would appreciate if you also confirm the fix as I already
have done testing badly for the regressing patch.
Thanks for the swift response, ideally I'd like a non-regression test in
the testsuite but a quick test showed me that I couldn't easily
reproduce with user networking so unless someone has a great idea it
would be a pain.
---
hw/net/virtio-net.c | 35 ++++++++++++++++-------------------
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de87cfadffe1..a920358a89c5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1687,6 +1687,11 @@ static void virtio_net_hdr_swap(VirtIODevice *vdev,
struct virtio_net_hdr *hdr)
virtio_tswap16s(vdev, &hdr->csum_offset);
}
+typedef struct Header {
+ struct virtio_net_hdr_v1_hash virtio_net;
+ uint8_t payload[1500];
+} Header;
+
/* dhclient uses AF_PACKET but doesn't pass auxdata to the kernel so
* it never finds out that the packets don't have valid checksums. This
* causes dhclient to get upset. Fedora's carried a patch for ages to
@@ -1701,7 +1706,7 @@ static void virtio_net_hdr_swap(VirtIODevice *vdev,
struct virtio_net_hdr *hdr)
* we should provide a mechanism to disable it to avoid polluting the host
* cache.
*/
-static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
+static void work_around_broken_dhclient(struct Header *hdr,
size_t *hdr_len, const uint8_t *buf,
size_t buf_size, size_t *buf_offset)
{
@@ -1711,20 +1716,20 @@ static void work_around_broken_dhclient(struct
virtio_net_hdr *hdr,
buf += *buf_offset;
buf_size -= *buf_offset;
- if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
- (buf_size >= csum_size && buf_size < 1500) && /* normal sized MTU */
+ if ((hdr->virtio_net.hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /*
missing csum */
+ (buf_size >= csum_size && buf_size < sizeof(hdr->payload)) && /*
normal sized MTU */
(buf[12] == 0x08 && buf[13] == 0x00) && /* ethertype == IPv4 */
(buf[23] == 17) && /* ip.protocol == UDP */
(buf[34] == 0 && buf[35] == 67)) { /* udp.srcport == bootps */
- memcpy((uint8_t *)hdr + *hdr_len, buf, csum_size);
- net_checksum_calculate((uint8_t *)hdr + *hdr_len, csum_size, CSUM_UDP);
- hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
- *hdr_len += csum_size;
- *buf_offset += csum_size;
+ memcpy((uint8_t *)hdr + *hdr_len, buf, buf_size);
+ net_checksum_calculate((uint8_t *)hdr + *hdr_len, buf_size, CSUM_UDP);
+ hdr->virtio_net.hdr.flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+ *hdr_len += buf_size;
+ *buf_offset += buf_size;
}
}
-static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
+static size_t receive_header(VirtIONet *n, Header *hdr,
const void *buf, size_t buf_size,
size_t *buf_offset)
`receive_header` can now "receive" the whole packet that's kinda
misleading. I though another approach would be to only do the
detection/flag patching from receive_header and recompute the checksum
directly in the final `iov`, this would also eliminate the extra payload
copy.
