qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Akihiko Odaki <akihiko.odaki@daynix.com>
To: qemu-devel@nongnu.org, Antoine Damhet <adamhet@scaleway.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	 devel@daynix.com, qemu-stable@nongnu.org,
	 Akihiko Odaki <akihiko.odaki@daynix.com>
Subject: [PATCH] virtio-net: Copy all for dhclient workaround
Date: Sat, 05 Apr 2025 17:04:28 +0900	[thread overview]
Message-ID: <20250405-mtu-v1-1-08c5910fa6fd@daynix.com> (raw)

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-stable@nongnu.org
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
This patch aims to resolves the issue the following one also does:
https://lore.kernel.org/qemu-devel/20250404151835.328368-1-adamhet@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.
---
 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)
 {
@@ -1736,7 +1741,7 @@ static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
     work_around_broken_dhclient(hdr, &hdr_len, buf, buf_size, buf_offset);
 
     if (n->needs_vnet_hdr_swap) {
-        virtio_net_hdr_swap(VIRTIO_DEVICE(n), hdr);
+        virtio_net_hdr_swap(VIRTIO_DEVICE(n), (struct virtio_net_hdr *)hdr);
     }
 
     return hdr_len;
@@ -1907,13 +1912,6 @@ static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf,
     return (index == new_index) ? -1 : new_index;
 }
 
-typedef struct Header {
-    struct virtio_net_hdr_v1_hash virtio_net;
-    struct eth_header eth;
-    struct ip_header ip;
-    struct udp_header udp;
-} Header;
-
 static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
                                       size_t size)
 {
@@ -2002,8 +2000,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
             }
 
             guest_offset = n->has_vnet_hdr ?
-                           receive_header(n, (struct virtio_net_hdr *)&hdr,
-                                          buf, size, &offset) :
+                           receive_header(n, &hdr, buf, size, &offset) :
                            n->guest_hdr_len;
 
             iov_from_buf(sg, elem->in_num, 0, &hdr, guest_offset);

---
base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
change-id: 20250405-mtu-834d4c62c93c

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>



             reply	other threads:[~2025-04-05  8:05 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-05  8:04 Akihiko Odaki [this message]
2025-04-07  2:09 ` [PATCH] virtio-net: Copy all for dhclient workaround Jason Wang
2025-04-07  8:29 ` Antoine Damhet
2025-04-11  8:01   ` Akihiko Odaki
2025-04-11 13:20     ` Antoine Damhet
2025-04-19  6:56       ` Akihiko Odaki
2025-05-12 10:11 ` Michael Tokarev
2025-05-14  2:51   ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250405-mtu-v1-1-08c5910fa6fd@daynix.com \
    --to=akihiko.odaki@daynix.com \
    --cc=adamhet@scaleway.com \
    --cc=devel@daynix.com \
    --cc=jasowang@redhat.com \
    --cc=mst@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-stable@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).