Re: [PATCH] Reapply "virtio-net: Copy received header to buffer"

["Michael S. Tsirkin" <mst@redhat.com>]

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>