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

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

On Thu, Apr 10, 2025 at 03:00:40PM -0400, Stefan Hajnoczi wrote:
> On Tue, Apr 8, 2025 at 10:55 AM Antoine Damhet <adamhet@scaleway.com> wrote:
> >
> > This reverts commit 7987d2be5a8bc3a502f89ba8cf3ac3e09f64d1ce.
> >
> > The goal 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.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2727
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Antoine Damhet <adamhet@scaleway.com>
> > ---
> > v2: Rebased on master due to conflict with c17ad4b11bd2 (
> > "virtio-net: Fix num_buffers for version 1")
> 
> Michael: Please review this and send a pull request for 10.0 (-rc4
> will be tagged on Tuesday). There was a conflict so this is not a
> mechanical revert.
> 
> Thanks!


Backlogged because of holidays, sorry.


Revert looks good:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

but this (userspace networking) is mostly Jason's area, not mine.



> >
> >  hw/net/virtio-net.c | 87 +++++++++++++++++++++------------------------
> >  1 file changed, 40 insertions(+), 47 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 340c6b642224..bd37651dabb0 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1702,44 +1702,41 @@ static void virtio_net_hdr_swap(VirtIODevice *vdev, struct virtio_net_hdr *hdr)
> >   * cache.
> >   */
> >  static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
> > -                                        size_t *hdr_len, const uint8_t *buf,
> > -                                        size_t buf_size, size_t *buf_offset)
> > +                                        uint8_t *buf, size_t size)
> >  {
> >      size_t csum_size = ETH_HLEN + sizeof(struct ip_header) +
> >                         sizeof(struct udp_header);
> >
> > -    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 */
> > +        (size >= csum_size && size < 1500) && /* 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);
> > +        net_checksum_calculate(buf, size, CSUM_UDP);
> >          hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > -        *hdr_len += csum_size;
> > -        *buf_offset += csum_size;
> >      }
> >  }
> >
> > -static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
> > -                             const void *buf, size_t buf_size,
> > -                             size_t *buf_offset)
> > +static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
> > +                           const void *buf, size_t size)
> >  {
> > -    size_t hdr_len = n->guest_hdr_len;
> > -
> > -    memcpy(hdr, buf, sizeof(struct virtio_net_hdr));
> > -
> > -    *buf_offset = n->host_hdr_len;
> > -    work_around_broken_dhclient(hdr, &hdr_len, buf, buf_size, 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);
> >
> > -    if (n->needs_vnet_hdr_swap) {
> > -        virtio_net_hdr_swap(VIRTIO_DEVICE(n), hdr);
> > +        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);
> >      }
> > -
> > -    return hdr_len;
> >  }
> >
> >  static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> > @@ -1907,13 +1904,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)
> >  {
> > @@ -1923,15 +1913,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];
> > -    Header hdr;
> > +    struct virtio_net_hdr_v1_hash extra_hdr;
> >      unsigned mhdr_cnt = 0;
> >      size_t offset, i, guest_offset, j;
> >      ssize_t err;
> >
> > -    memset(&hdr.virtio_net, 0, sizeof(hdr.virtio_net));
> > +    memset(&extra_hdr, 0, sizeof(extra_hdr));
> >
> >      if (n->rss_data.enabled && n->rss_data.enabled_software_rss) {
> > -        int index = virtio_net_process_rss(nc, buf, size, &hdr.virtio_net);
> > +        int index = virtio_net_process_rss(nc, buf, size, &extra_hdr);
> >          if (index >= 0) {
> >              nc = qemu_get_subqueue(n->nic, index % n->curr_queue_pairs);
> >          }
> > @@ -1996,20 +1986,23 @@ 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(hdr),
> > -                                             virtio_net.hdr.num_buffers),
> > -                                    sizeof(hdr.virtio_net.hdr.num_buffers));
> > +                                    offsetof(typeof(extra_hdr), hdr.num_buffers),
> > +                                    sizeof(extra_hdr.hdr.num_buffers));
> >              } else {
> > -                hdr.virtio_net.hdr.num_buffers = cpu_to_le16(1);
> > +                extra_hdr.hdr.num_buffers = cpu_to_le16(1);
> >              }
> >
> > -            guest_offset = n->has_vnet_hdr ?
> > -                           receive_header(n, (struct virtio_net_hdr *)&hdr,
> > -                                          buf, size, &offset) :
> > -                           n->guest_hdr_len;
> > -
> > -            iov_from_buf(sg, elem->in_num, 0, &hdr, guest_offset);
> > -            total += guest_offset;
> > +            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;
> >          } else {
> >              guest_offset = 0;
> >          }
> > @@ -2035,11 +2028,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
> >      }
> >
> >      if (mhdr_cnt) {
> > -        virtio_stw_p(vdev, &hdr.virtio_net.hdr.num_buffers, i);
> > +        virtio_stw_p(vdev, &extra_hdr.hdr.num_buffers, i);
> >          iov_from_buf(mhdr_sg, mhdr_cnt,
> >                       0,
> > -                     &hdr.virtio_net.hdr.num_buffers,
> > -                     sizeof hdr.virtio_net.hdr.num_buffers);
> > +                     &extra_hdr.hdr.num_buffers,
> > +                     sizeof extra_hdr.hdr.num_buffers);
> >      }
> >
> >      for (j = 0; j < i; j++) {
> > --
> > 2.49.0
> >
> >