| Message ID | 20250408145345.142947-1-adamhet@scaleway.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Revert "virtio-net: Copy received header to buffer" | expand |
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! > > 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 > >
On Thu, Apr 10, 2025 at 3:00 PM Stefan Hajnoczi <stefanha@gmail.com> 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. Hi Jason, I realized Michael may not be online due to public holidays. Could you review this patch? virtio-net is so widely-used and security-critical that a review from a virtio-net expert would be appreciated before I apply any code changes for 10.0. Thank you, Stefan > > Thanks! > > > > > 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 > > > >
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 > > > >
On Mon, Apr 14, 2025 at 6:48 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > 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. Thank you, enjoy the holidays! I will merge this patch for 10.0. Stefan > > > > > > > > > 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 > > > > > > >
On Tue, Apr 08, 2025 at 04:53:33PM +0200, Antoine Damhet 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") Applied for QEMU 10.0, thanks! Stefan
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++) {
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") hw/net/virtio-net.c | 87 +++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 47 deletions(-)