On Thu, Apr 10, 2025 at 03:00:40PM -0400, Stefan Hajnoczi wrote:
> On Tue, Apr 8, 2025 at 10:55 AM Antoine Damhet <adam...@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-sta...@nongnu.org
> > Signed-off-by: Antoine Damhet <adam...@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 <m...@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
> >
> >
