* [PATCH] Reapply "virtio-net: Copy received header to buffer"
@ 2025-04-23 6:19 Akihiko Odaki
2025-04-23 6:31 ` Michael S. Tsirkin
0 siblings, 1 reply; 2+ messages in thread
From: Akihiko Odaki @ 2025-04-23 6:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael S. Tsirkin, Jason Wang, Antoine Damhet, Akihiko Odaki
This reverts commit e28fbd1c525db21f0502b85517f49504c9f9dcd8.
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.
Reapply the mentioned commit with a change to copy the entire packet
instead of only copying the headers to acheive the original goal without
breaking checksums.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Supersedes: <20250405-mtu-v1-1-08c5910fa6fd@daynix.com>
("[PATCH] virtio-net: Copy all for dhclient workaround")
This reapplies commit 7987d2be5a8b ("virtio-net: Copy all for dhclient
workaround"), which was reverted by commit e28fbd1c525d ("Revert
"virtio-net: Copy received header to buffer""), with a fix in the
superseded patch. It also renames identifiers according to the
discussion with Antoine Damhet.
---
hw/net/virtio-net.c | 91 ++++++++++++++++++++++++++++-------------------------
1 file changed, 48 insertions(+), 43 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index bd37651dabb0..f1688e0b2536 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 PacketPrefix {
+ struct virtio_net_hdr_v1_hash virtio_net;
+ uint8_t payload[1500];
+} PacketPrefix;
+
/* 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,42 +1706,46 @@ 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,
- uint8_t *buf, size_t size)
+static void work_around_broken_dhclient(struct PacketPrefix *prefix,
+ size_t *prefix_len, const uint8_t *buf,
+ size_t buf_size, size_t *buf_offset)
{
size_t csum_size = ETH_HLEN + sizeof(struct ip_header) +
sizeof(struct udp_header);
+ uint8_t *payload = (uint8_t *)prefix + *prefix_len;
+
+ buf += *buf_offset;
+ buf_size -= *buf_offset;
- if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
- (size >= csum_size && size < 1500) && /* normal sized MTU */
+ if ((prefix->virtio_net.hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
+ (buf_size >= csum_size && buf_size < sizeof(prefix->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 */
- net_checksum_calculate(buf, size, CSUM_UDP);
- hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+ memcpy(payload, buf, buf_size);
+ net_checksum_calculate(payload, buf_size, CSUM_UDP);
+ prefix->virtio_net.hdr.flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+ *prefix_len += buf_size;
+ *buf_offset += buf_size;
}
}
-static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
- const void *buf, size_t size)
+static size_t receive_prefix(VirtIONet *n, PacketPrefix *prefix,
+ const void *buf, size_t buf_size,
+ size_t *buf_offset)
{
- if (n->has_vnet_hdr) {
- /* FIXME this cast is evil */
- void *wbuf = (void *)buf;
- work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
- size - n->host_hdr_len);
+ size_t prefix_len = n->guest_hdr_len;
- if (n->needs_vnet_hdr_swap) {
- virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
- }
- iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
- } else {
- struct virtio_net_hdr hdr = {
- .flags = 0,
- .gso_type = VIRTIO_NET_HDR_GSO_NONE
- };
- iov_from_buf(iov, iov_cnt, 0, &hdr, sizeof hdr);
+ memcpy(prefix, buf, sizeof(struct virtio_net_hdr));
+
+ *buf_offset = n->host_hdr_len;
+ work_around_broken_dhclient(prefix, &prefix_len, buf, buf_size, buf_offset);
+
+ if (n->needs_vnet_hdr_swap) {
+ virtio_net_hdr_swap(VIRTIO_DEVICE(n), (struct virtio_net_hdr *)prefix);
}
+
+ return prefix_len;
}
static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
@@ -1913,15 +1922,15 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
size_t lens[VIRTQUEUE_MAX_SIZE];
struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE];
- struct virtio_net_hdr_v1_hash extra_hdr;
+ PacketPrefix prefix;
unsigned mhdr_cnt = 0;
size_t offset, i, guest_offset, j;
ssize_t err;
- memset(&extra_hdr, 0, sizeof(extra_hdr));
+ memset(&prefix.virtio_net, 0, sizeof(prefix.virtio_net));
if (n->rss_data.enabled && n->rss_data.enabled_software_rss) {
- int index = virtio_net_process_rss(nc, buf, size, &extra_hdr);
+ int index = virtio_net_process_rss(nc, buf, size, &prefix.virtio_net);
if (index >= 0) {
nc = qemu_get_subqueue(n->nic, index % n->curr_queue_pairs);
}
@@ -1986,23 +1995,19 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
if (n->mergeable_rx_bufs) {
mhdr_cnt = iov_copy(mhdr_sg, ARRAY_SIZE(mhdr_sg),
sg, elem->in_num,
- offsetof(typeof(extra_hdr), hdr.num_buffers),
- sizeof(extra_hdr.hdr.num_buffers));
+ offsetof(typeof(prefix),
+ virtio_net.hdr.num_buffers),
+ sizeof(prefix.virtio_net.hdr.num_buffers));
} else {
- extra_hdr.hdr.num_buffers = cpu_to_le16(1);
+ prefix.virtio_net.hdr.num_buffers = cpu_to_le16(1);
}
- receive_header(n, sg, elem->in_num, buf, size);
- if (n->rss_data.populate_hash) {
- offset = offsetof(typeof(extra_hdr), hash_value);
- iov_from_buf(sg, elem->in_num, offset,
- (char *)&extra_hdr + offset,
- sizeof(extra_hdr.hash_value) +
- sizeof(extra_hdr.hash_report));
- }
- offset = n->host_hdr_len;
- total += n->guest_hdr_len;
- guest_offset = n->guest_hdr_len;
+ guest_offset = n->has_vnet_hdr ?
+ receive_prefix(n, &prefix, buf, size, &offset) :
+ n->guest_hdr_len;
+
+ iov_from_buf(sg, elem->in_num, 0, &prefix, guest_offset);
+ total += guest_offset;
} else {
guest_offset = 0;
}
@@ -2028,11 +2033,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
}
if (mhdr_cnt) {
- virtio_stw_p(vdev, &extra_hdr.hdr.num_buffers, i);
+ virtio_stw_p(vdev, &prefix.virtio_net.hdr.num_buffers, i);
iov_from_buf(mhdr_sg, mhdr_cnt,
0,
- &extra_hdr.hdr.num_buffers,
- sizeof extra_hdr.hdr.num_buffers);
+ &prefix.virtio_net.hdr.num_buffers,
+ sizeof prefix.virtio_net.hdr.num_buffers);
}
for (j = 0; j < i; j++) {
---
base-commit: 1da8f3a3c53b604edfe0d55e475102640490549e
change-id: 20250423-reapply-63176514d76d
Best regards,
--
Akihiko Odaki <akihiko.odaki@daynix.com>
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] Reapply "virtio-net: Copy received header to buffer"
2025-04-23 6:19 [PATCH] Reapply "virtio-net: Copy received header to buffer" Akihiko Odaki
@ 2025-04-23 6:31 ` Michael S. Tsirkin
0 siblings, 0 replies; 2+ messages in thread
From: Michael S. Tsirkin @ 2025-04-23 6:31 UTC (permalink / raw)
To: Akihiko Odaki; +Cc: qemu-devel, Jason Wang, Antoine Damhet
On Wed, Apr 23, 2025 at 03:19:30PM +0900, Akihiko Odaki wrote:
> This reverts commit e28fbd1c525db21f0502b85517f49504c9f9dcd8.
>
> 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.
>
> Reapply the mentioned commit with a change to copy the entire packet
> instead of only copying the headers to acheive the original goal without
achieve
> breaking checksums.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
thanks for the patch!
yet the commit log needs improvement:
I don't think this "reverts commit
e28fbd1c525db21f0502b85517f49504c9f9dcd8" is correct.
and now it's all of the packet not the header.
So just call it:
virtio-net: copy received packet to buffer
and introduce the issue and the fixup, not the archeology.
something like:
commit e28fbd1c525db reintroduced an issue that we attempted
to fix with commit 7987d2be5a8b:
....
fix by ....
HTH
> ---
> Supersedes: <20250405-mtu-v1-1-08c5910fa6fd@daynix.com>
> ("[PATCH] virtio-net: Copy all for dhclient workaround")
>
> This reapplies commit 7987d2be5a8b ("virtio-net: Copy all for dhclient
> workaround"), which was reverted by commit e28fbd1c525d ("Revert
> "virtio-net: Copy received header to buffer""), with a fix in the
> superseded patch. It also renames identifiers according to the
> discussion with Antoine Damhet.
> ---
> hw/net/virtio-net.c | 91 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 48 insertions(+), 43 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index bd37651dabb0..f1688e0b2536 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 PacketPrefix {
> + struct virtio_net_hdr_v1_hash virtio_net;
> + uint8_t payload[1500];
> +} PacketPrefix;
> +
> /* 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,42 +1706,46 @@ 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,
> - uint8_t *buf, size_t size)
> +static void work_around_broken_dhclient(struct PacketPrefix *prefix,
> + size_t *prefix_len, const uint8_t *buf,
> + size_t buf_size, size_t *buf_offset)
> {
> size_t csum_size = ETH_HLEN + sizeof(struct ip_header) +
> sizeof(struct udp_header);
> + uint8_t *payload = (uint8_t *)prefix + *prefix_len;
> +
> + buf += *buf_offset;
> + buf_size -= *buf_offset;
>
> - if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
> - (size >= csum_size && size < 1500) && /* normal sized MTU */
> + if ((prefix->virtio_net.hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
> + (buf_size >= csum_size && buf_size < sizeof(prefix->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 */
> - net_checksum_calculate(buf, size, CSUM_UDP);
> - hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> + memcpy(payload, buf, buf_size);
> + net_checksum_calculate(payload, buf_size, CSUM_UDP);
> + prefix->virtio_net.hdr.flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> + *prefix_len += buf_size;
> + *buf_offset += buf_size;
> }
> }
>
> -static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
> - const void *buf, size_t size)
> +static size_t receive_prefix(VirtIONet *n, PacketPrefix *prefix,
> + const void *buf, size_t buf_size,
> + size_t *buf_offset)
> {
> - if (n->has_vnet_hdr) {
> - /* FIXME this cast is evil */
> - void *wbuf = (void *)buf;
> - work_around_broken_dhclient(wbuf, wbuf + n->host_hdr_len,
> - size - n->host_hdr_len);
> + size_t prefix_len = n->guest_hdr_len;
>
> - if (n->needs_vnet_hdr_swap) {
> - virtio_net_hdr_swap(VIRTIO_DEVICE(n), wbuf);
> - }
> - iov_from_buf(iov, iov_cnt, 0, buf, sizeof(struct virtio_net_hdr));
> - } else {
> - struct virtio_net_hdr hdr = {
> - .flags = 0,
> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> - };
> - iov_from_buf(iov, iov_cnt, 0, &hdr, sizeof hdr);
> + memcpy(prefix, buf, sizeof(struct virtio_net_hdr));
> +
> + *buf_offset = n->host_hdr_len;
> + work_around_broken_dhclient(prefix, &prefix_len, buf, buf_size, buf_offset);
> +
> + if (n->needs_vnet_hdr_swap) {
> + virtio_net_hdr_swap(VIRTIO_DEVICE(n), (struct virtio_net_hdr *)prefix);
> }
> +
> + return prefix_len;
> }
>
> static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> @@ -1913,15 +1922,15 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
> size_t lens[VIRTQUEUE_MAX_SIZE];
> struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE];
> - struct virtio_net_hdr_v1_hash extra_hdr;
> + PacketPrefix prefix;
> unsigned mhdr_cnt = 0;
> size_t offset, i, guest_offset, j;
> ssize_t err;
>
> - memset(&extra_hdr, 0, sizeof(extra_hdr));
> + memset(&prefix.virtio_net, 0, sizeof(prefix.virtio_net));
>
> if (n->rss_data.enabled && n->rss_data.enabled_software_rss) {
> - int index = virtio_net_process_rss(nc, buf, size, &extra_hdr);
> + int index = virtio_net_process_rss(nc, buf, size, &prefix.virtio_net);
> if (index >= 0) {
> nc = qemu_get_subqueue(n->nic, index % n->curr_queue_pairs);
> }
> @@ -1986,23 +1995,19 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> if (n->mergeable_rx_bufs) {
> mhdr_cnt = iov_copy(mhdr_sg, ARRAY_SIZE(mhdr_sg),
> sg, elem->in_num,
> - offsetof(typeof(extra_hdr), hdr.num_buffers),
> - sizeof(extra_hdr.hdr.num_buffers));
> + offsetof(typeof(prefix),
> + virtio_net.hdr.num_buffers),
> + sizeof(prefix.virtio_net.hdr.num_buffers));
> } else {
> - extra_hdr.hdr.num_buffers = cpu_to_le16(1);
> + prefix.virtio_net.hdr.num_buffers = cpu_to_le16(1);
> }
>
> - receive_header(n, sg, elem->in_num, buf, size);
> - if (n->rss_data.populate_hash) {
> - offset = offsetof(typeof(extra_hdr), hash_value);
> - iov_from_buf(sg, elem->in_num, offset,
> - (char *)&extra_hdr + offset,
> - sizeof(extra_hdr.hash_value) +
> - sizeof(extra_hdr.hash_report));
> - }
> - offset = n->host_hdr_len;
> - total += n->guest_hdr_len;
> - guest_offset = n->guest_hdr_len;
> + guest_offset = n->has_vnet_hdr ?
> + receive_prefix(n, &prefix, buf, size, &offset) :
> + n->guest_hdr_len;
> +
> + iov_from_buf(sg, elem->in_num, 0, &prefix, guest_offset);
> + total += guest_offset;
> } else {
> guest_offset = 0;
> }
> @@ -2028,11 +2033,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> }
>
> if (mhdr_cnt) {
> - virtio_stw_p(vdev, &extra_hdr.hdr.num_buffers, i);
> + virtio_stw_p(vdev, &prefix.virtio_net.hdr.num_buffers, i);
> iov_from_buf(mhdr_sg, mhdr_cnt,
> 0,
> - &extra_hdr.hdr.num_buffers,
> - sizeof extra_hdr.hdr.num_buffers);
> + &prefix.virtio_net.hdr.num_buffers,
> + sizeof prefix.virtio_net.hdr.num_buffers);
> }
>
> for (j = 0; j < i; j++) {
>
> ---
> base-commit: 1da8f3a3c53b604edfe0d55e475102640490549e
> change-id: 20250423-reapply-63176514d76d
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-04-23 6:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-23 6:19 [PATCH] Reapply "virtio-net: Copy received header to buffer" Akihiko Odaki
2025-04-23 6:31 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).