[PATCH] virtio-net: Copy all for dhclient workaround

[PATCH] virtio-net: Copy all for dhclient workaround

[Akihiko Odaki]

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>

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Jason Wang]

On Sat, Apr 5, 2025 at 4:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> 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-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.

Considering we are about to release, I'd prefer to do the reverting
first and refine on the next version.

Thanks

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Antoine Damhet]

[-- Attachment #1: Type: text/plain, Size: 6526 bytes --]

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-stable@nongnu.org
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Tested-By: Antoine Damhet <adamhet@scaleway.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.

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.

Cheers,

-- 
Antoine 'xdbob' Damhet
Engineer @scaleway


>  {
> @@ -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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Akihiko Odaki]

On 2025/04/07 17:29, Antoine Damhet wrote:
> 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-stable@nongnu.org
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Tested-By: Antoine Damhet <adamhet@scaleway.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.
> 
> 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.

It is possible to avoid copying but I chose not to do that because this 
is not a hot path and the code complexity required for that does not 
look worthwhile for me.

But I agree that the names of receive_header() and Header structure are 
misleading. The reasoning I used to convince myself is that the "Header" 
is at the head of the packet at least. I'd like to hear if you have an 
idea of better naming; otherwise I would rather leave it as is.

Regards,
Akihiko Odaki

> 
> Cheers,
> 

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Antoine Damhet]

[-- Attachment #1: Type: text/plain, Size: 6361 bytes --]

On Fri, Apr 11, 2025 at 05:01:01PM +0900, Akihiko Odaki wrote:
> On 2025/04/07 17:29, Antoine Damhet wrote:
> > 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-stable@nongnu.org
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > 
> > Tested-By: Antoine Damhet <adamhet@scaleway.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.
> > 
> > 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.
> 
> It is possible to avoid copying but I chose not to do that because this is
> not a hot path and the code complexity required for that does not look
> worthwhile for me.

Understood and OK.

> 
> But I agree that the names of receive_header() and Header structure are
> misleading. The reasoning I used to convince myself is that the "Header" is
> at the head of the packet at least. I'd like to hear if you have an idea of
> better naming; otherwise I would rather leave it as is.

Maybe we can sidestep this entirely, do we need to do the workaround
_inside_ `receive_header` ? WDYT of the following pseudocode:

```
guest_offset = receive_header(&header);
iov_from_buf(&header);
work_around_broken_dhclient(&header, &payload);
iov_from_buf(&payload);
```

If not maybe something along the line of "PacketPrefix" or
"PacketStart".

Regards,

-- 
Antoine 'xdbob' Damhet
Engineer @scaleway



> 
> Regards,
> Akihiko Odaki
> 
> > 
> > Cheers,
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Akihiko Odaki]

On 2025/04/11 22:20, Antoine Damhet wrote:
> On Fri, Apr 11, 2025 at 05:01:01PM +0900, Akihiko Odaki wrote:
>> On 2025/04/07 17:29, Antoine Damhet wrote:
>>> 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-stable@nongnu.org
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>
>>> Tested-By: Antoine Damhet <adamhet@scaleway.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.
>>>
>>> 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.
>>
>> It is possible to avoid copying but I chose not to do that because this is
>> not a hot path and the code complexity required for that does not look
>> worthwhile for me.
> 
> Understood and OK.
> 
>>
>> But I agree that the names of receive_header() and Header structure are
>> misleading. The reasoning I used to convince myself is that the "Header" is
>> at the head of the packet at least. I'd like to hear if you have an idea of
>> better naming; otherwise I would rather leave it as is.
> 
> Maybe we can sidestep this entirely, do we need to do the workaround
> _inside_ `receive_header` ? WDYT of the following pseudocode:
> 
> ```
> guest_offset = receive_header(&header);
> iov_from_buf(&header);
> work_around_broken_dhclient(&header, &payload);
> iov_from_buf(&payload);
> ```

net_checksum_calculate() currently needs a contiguous buffer so it needs 
to be changed and it also requires one additional iov_from_buf() call. 
It's a bit too complicated to workaround the naming problem I think.

> 
> If not maybe something along the line of "PacketPrefix" or
> "PacketStart".

Now I'm inclined for "PacketPrefix". In a normal context, "prefix" and 
"start" are no different from "header", but in the networking context, 
"header" is frequently used to describe the metadata and implies it 
doesn't contain data. Usually I don't like to choose wordings according 
to such an implied nuance, but avoiding the word "header" here has a 
practical value.

I'll probably choose "prefix" instead of "start" since it sounds more 
specific than "start".

Regards,
Akihiko Odaki

> 
> Regards,
> 

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Michael Tokarev]

On 05.04.2025 11:04, 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-stable@nongnu.org
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Ping?  Is this change still needed?

Thanks,

/mjt

Re: [PATCH] virtio-net: Copy all for dhclient workaround

[Jason Wang]

On Mon, May 12, 2025 at 6:11 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> On 05.04.2025 11:04, 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-stable@nongnu.org
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> Ping?  Is this change still needed?

Yes, Michael asked for more details for the problem solved here. So
I'd expect there would be a respin here.

https://patchew.org/QEMU/20250424-reapply-v2-1-d0ba763ac782@daynix.com/

Thanks

>
> Thanks,
>
> /mjt
>