On Mon, Apr 14, 2025 at 6:48 PM Michael S. Tsirkin <m...@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 <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.
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
> > >
> > >
>
