[PATCH 0/7] virtio-net fixes

[PATCH 0/7] virtio-net fixes

[Akihiko Odaki]

Most of this series are fixes for software RSS and hash reporting, which
should have no production user.

However there is one exception; patch "virtio-net: Fix size check in
dhclient workaround" fixes an out-of-bound access that can be triggered
for anyone who don't use vhost. It has Cc: qemu-stable@nongnu.org and
can be applied independently.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (7):
      net: checksum: Convert data to void *
      virtio-net: Fix size check in dhclient workaround
      virtio-net: Do not check for the queue before RSS
      virtio-net: Fix hash reporting when the queue changes
      virtio-net: Initialize hash reporting values
      virtio-net: Copy received header to buffer
      virtio-net: Fix num_buffers for version 1

 include/net/checksum.h |   2 +-
 hw/net/virtio-net.c    | 109 ++++++++++++++++++++++++++++---------------------
 net/checksum.c         |   4 +-
 3 files changed, 65 insertions(+), 50 deletions(-)
---
base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
change-id: 20240907-queue-f425937a730f

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 1/7] net: checksum: Convert data to void *

[Akihiko Odaki]

Convert the data parameter of net_checksum_calculate() to void * to
save unnecessary casts for callers.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/net/checksum.h | 2 +-
 net/checksum.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/checksum.h b/include/net/checksum.h
index 7dec37e56c78..188e4cca0b7f 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -30,7 +30,7 @@ uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq);
 uint16_t net_checksum_finish(uint32_t sum);
 uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
                              uint8_t *addrs, uint8_t *buf);
-void net_checksum_calculate(uint8_t *data, int length, int csum_flag);
+void net_checksum_calculate(void *data, int length, int csum_flag);
 
 static inline uint32_t
 net_checksum_add(int len, uint8_t *buf)
diff --git a/net/checksum.c b/net/checksum.c
index 1a957e4c0b10..537457d89d07 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -57,7 +57,7 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
     return net_checksum_finish(sum);
 }
 
-void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
+void net_checksum_calculate(void *data, int length, int csum_flag)
 {
     int mac_hdr_len, ip_len;
     struct ip_header *ip;
@@ -101,7 +101,7 @@ void net_checksum_calculate(uint8_t *data, int length, int csum_flag)
         return;
     }
 
-    ip = (struct ip_header *)(data + mac_hdr_len);
+    ip = (struct ip_header *)((uint8_t *)data + mac_hdr_len);
 
     if (IP_HEADER_VERSION(ip) != IP_HEADER_VERSION_4) {
         return; /* not IPv4 */

-- 
2.46.0

[PATCH 2/7] virtio-net: Fix size check in dhclient workaround

[Akihiko Odaki]

work_around_broken_dhclient() accesses IP and UDP headers to detect
relevant packets and to calculate checksums, but it didn't check if
the packet has size sufficient to accommodate them, causing out-of-bound
access hazards. Fix this by correcting the size requirement.

Fixes: 1d41b0c1ec66 ("Work around dhclient brokenness")
Cc: qemu-stable@nongnu.org
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/virtio-net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 8f3097270869..755530c035e4 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1687,8 +1687,11 @@ static void virtio_net_hdr_swap(VirtIODevice *vdev, struct virtio_net_hdr *hdr)
 static void work_around_broken_dhclient(struct virtio_net_hdr *hdr,
                                         uint8_t *buf, size_t size)
 {
+    size_t csum_size = ETH_HLEN + sizeof(struct ip_header) +
+                       sizeof(struct udp_header);
+
     if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && /* missing csum */
-        (size > 27 && 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 */

-- 
2.46.0

[PATCH 3/7] virtio-net: Do not check for the queue before RSS

[Akihiko Odaki]

virtio_net_can_receive() checks if the queue is ready, but RSS will
change the queue to use so, strictly speaking, we may still be able to
receive the packet even if the queue initially provided is not ready.
Perform RSS before virtio_net_can_receive() to cover such a case.

Fixes: 4474e37a5b3a ("virtio-net: implement RX RSS processing")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/virtio-net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 755530c035e4..3ee1ebd88daa 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1901,10 +1901,6 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
     size_t offset, i, guest_offset, j;
     ssize_t err;
 
-    if (!virtio_net_can_receive(nc)) {
-        return -1;
-    }
-
     if (!no_rss && n->rss_data.enabled && n->rss_data.enabled_software_rss) {
         int index = virtio_net_process_rss(nc, buf, size, &extra_hdr);
         if (index >= 0) {
@@ -1913,6 +1909,10 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
         }
     }
 
+    if (!virtio_net_can_receive(nc)) {
+        return -1;
+    }
+
     /* hdr_len refers to the header we supply to the guest */
     if (!virtio_net_has_buffers(q, size + n->guest_hdr_len - n->host_hdr_len)) {
         return 0;

-- 
2.46.0

Re: [PATCH 3/7] virtio-net: Do not check for the queue before RSS

[Jason Wang]

On Sun, Sep 15, 2024 at 9:07 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> virtio_net_can_receive() checks if the queue is ready, but RSS will
> change the queue to use so, strictly speaking, we may still be able to
> receive the packet even if the queue initially provided is not ready.
> Perform RSS before virtio_net_can_receive() to cover such a case.
>
> Fixes: 4474e37a5b3a ("virtio-net: implement RX RSS processing")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/virtio-net.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>

The patch does not apply cleanly:

Applying: virtio-net: Fix hash reporting when the queue changes
error: patch failed: hw/net/virtio-net.c:1901
error: hw/net/virtio-net.c: patch does not apply
Patch failed at 0004 virtio-net: Fix hash reporting when the queue changes
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Thansk

[PATCH 4/7] virtio-net: Fix hash reporting when the queue changes

[Akihiko Odaki]

virtio_net_process_rss() fills the values used for hash reporting, but
the values used to be thrown away with a recursive function call if
the queue changes after RSS. Avoid the function call to keep the values.

Fixes: a4c960eedcd2 ("virtio-net: Do not write hashes to peer buffer")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/virtio-net.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ee1ebd88daa..3753c6aaca83 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1888,10 +1888,10 @@ static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf,
 }
 
 static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
-                                      size_t size, bool no_rss)
+                                      size_t size)
 {
     VirtIONet *n = qemu_get_nic_opaque(nc);
-    VirtIONetQueue *q = virtio_net_get_subqueue(nc);
+    VirtIONetQueue *q;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
     VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
     size_t lens[VIRTQUEUE_MAX_SIZE];
@@ -1901,11 +1901,10 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
     size_t offset, i, guest_offset, j;
     ssize_t err;
 
-    if (!no_rss && n->rss_data.enabled && n->rss_data.enabled_software_rss) {
+    if (n->rss_data.enabled && n->rss_data.enabled_software_rss) {
         int index = virtio_net_process_rss(nc, buf, size, &extra_hdr);
         if (index >= 0) {
-            NetClientState *nc2 = qemu_get_subqueue(n->nic, index);
-            return virtio_net_receive_rcu(nc2, buf, size, true);
+            nc = qemu_get_subqueue(n->nic, index);
         }
     }
 
@@ -1913,6 +1912,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
         return -1;
     }
 
+    q = virtio_net_get_subqueue(nc);
+
     /* hdr_len refers to the header we supply to the guest */
     if (!virtio_net_has_buffers(q, size + n->guest_hdr_len - n->host_hdr_len)) {
         return 0;
@@ -2038,7 +2039,7 @@ static ssize_t virtio_net_do_receive(NetClientState *nc, const uint8_t *buf,
 {
     RCU_READ_LOCK_GUARD();
 
-    return virtio_net_receive_rcu(nc, buf, size, false);
+    return virtio_net_receive_rcu(nc, buf, size);
 }
 
 static void virtio_net_rsc_extract_unit4(VirtioNetRscChain *chain,

-- 
2.46.0

[PATCH 5/7] virtio-net: Initialize hash reporting values

[Akihiko Odaki]

The specification says hash_report should be set to
VIRTIO_NET_HASH_REPORT_NONE if VIRTIO_NET_F_HASH_REPORT is negotiated
but not configured with VIRTIO_NET_CTRL_MQ_RSS_CONFIG. However,
virtio_net_receive_rcu() instead wrote out the content of the extra_hdr
variable, which is not uninitialized in such a case.

Fix this by zeroing the extra_hdr.

Fixes: e22f0603fb2f ("virtio-net: reference implementation of hash report")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/virtio-net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3753c6aaca83..3fc1d10cb9e0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1901,6 +1901,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
     size_t offset, i, guest_offset, j;
     ssize_t err;
 
+    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, &extra_hdr);
         if (index >= 0) {

-- 
2.46.0

[PATCH 6/7] virtio-net: Copy received header to buffer

[Akihiko Odaki]

receive_header() used to cast the const qualifier of the pointer to the
received packet away to modify the header. Avoid this by copying the
received header to buffer.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/virtio-net.c | 85 +++++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 39 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3fc1d10cb9e0..ca4e22344f78 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1685,41 +1685,44 @@ 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,
-                                        uint8_t *buf, size_t size)
+                                        size_t *hdr_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);
 
+    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 */
+        (buf_size >= csum_size && buf_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 */
-        net_checksum_calculate(buf, size, CSUM_UDP);
+        memcpy((uint8_t *)hdr + *hdr_len, buf, csum_size);
+        net_checksum_calculate((uint8_t *)hdr + *hdr_len, csum_size, CSUM_UDP);
         hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
+        *hdr_len += csum_size;
+        *buf_offset += csum_size;
     }
 }
 
-static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
-                           const void *buf, size_t size)
+static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
+                             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 hdr_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(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->needs_vnet_hdr_swap) {
+        virtio_net_hdr_swap(VIRTIO_DEVICE(n), hdr);
     }
+
+    return hdr_len;
 }
 
 static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
@@ -1887,6 +1890,13 @@ 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)
 {
@@ -1896,15 +1906,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;
+    Header hdr;
     unsigned mhdr_cnt = 0;
     size_t offset, i, guest_offset, j;
     ssize_t err;
 
-    memset(&extra_hdr, 0, sizeof(extra_hdr));
+    memset(&hdr.virtio_net, 0, sizeof(hdr.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, &hdr.virtio_net);
         if (index >= 0) {
             nc = qemu_get_subqueue(n->nic, index);
         }
@@ -1969,21 +1979,18 @@ 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(hdr),
+                                             virtio_net.hdr.num_buffers),
+                                    sizeof(hdr.virtio_net.hdr.num_buffers));
             }
 
-            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_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;
         } else {
             guest_offset = 0;
         }
@@ -2009,11 +2016,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, &hdr.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);
+                     &hdr.virtio_net.hdr.num_buffers,
+                     sizeof hdr.virtio_net.hdr.num_buffers);
     }
 
     for (j = 0; j < i; j++) {

-- 
2.46.0

Re: [PATCH 6/7] virtio-net: Copy received header to buffer

[Jason Wang]

On Sun, Sep 15, 2024 at 9:07 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> receive_header() used to cast the const qualifier of the pointer to the
> received packet away to modify the header. Avoid this by copying the
> received header to buffer.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/virtio-net.c | 85 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 46 insertions(+), 39 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 3fc1d10cb9e0..ca4e22344f78 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1685,41 +1685,44 @@ 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,
> -                                        uint8_t *buf, size_t size)
> +                                        size_t *hdr_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);
>
> +    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 */
> +        (buf_size >= csum_size && buf_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 */
> -        net_checksum_calculate(buf, size, CSUM_UDP);
> +        memcpy((uint8_t *)hdr + *hdr_len, buf, csum_size);
> +        net_checksum_calculate((uint8_t *)hdr + *hdr_len, csum_size, CSUM_UDP);
>          hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +        *hdr_len += csum_size;
> +        *buf_offset += csum_size;
>      }
>  }
>
> -static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
> -                           const void *buf, size_t size)
> +static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
> +                             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 hdr_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(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->needs_vnet_hdr_swap) {
> +        virtio_net_hdr_swap(VIRTIO_DEVICE(n), hdr);
>      }
> +
> +    return hdr_len;
>  }
>
>  static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> @@ -1887,6 +1890,13 @@ 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;

I don't see too much value in having this structure especially
considering eth/ip/udp is not even used.

Any reason we can simply use an array as a buffer in virtio_net_receive_rcu()?

Thanks

Re: [PATCH 6/7] virtio-net: Copy received header to buffer

[Jason Wang]

On Mon, Oct 21, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sun, Sep 15, 2024 at 9:07 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > receive_header() used to cast the const qualifier of the pointer to the
> > received packet away to modify the header. Avoid this by copying the
> > received header to buffer.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >  hw/net/virtio-net.c | 85 +++++++++++++++++++++++++++++------------------------
> >  1 file changed, 46 insertions(+), 39 deletions(-)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 3fc1d10cb9e0..ca4e22344f78 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1685,41 +1685,44 @@ 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,
> > -                                        uint8_t *buf, size_t size)
> > +                                        size_t *hdr_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);
> >
> > +    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 */
> > +        (buf_size >= csum_size && buf_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 */
> > -        net_checksum_calculate(buf, size, CSUM_UDP);
> > +        memcpy((uint8_t *)hdr + *hdr_len, buf, csum_size);
> > +        net_checksum_calculate((uint8_t *)hdr + *hdr_len, csum_size, CSUM_UDP);
> >          hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
> > +        *hdr_len += csum_size;
> > +        *buf_offset += csum_size;
> >      }
> >  }
> >
> > -static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
> > -                           const void *buf, size_t size)
> > +static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
> > +                             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 hdr_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(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->needs_vnet_hdr_swap) {
> > +        virtio_net_hdr_swap(VIRTIO_DEVICE(n), hdr);
> >      }
> > +
> > +    return hdr_len;
> >  }
> >
> >  static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> > @@ -1887,6 +1890,13 @@ 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;
>
> I don't see too much value in having this structure especially
> considering eth/ip/udp is not even used.
>
> Any reason we can simply use an array as a buffer in virtio_net_receive_rcu()?

Btw, since the code was for ancient guests and userspace, I wonder how
it is tested.

Thanks

>
> Thanks

Re: [PATCH 6/7] virtio-net: Copy received header to buffer

[Akihiko Odaki]

On 2024/10/21 17:22, Jason Wang wrote:
> On Mon, Oct 21, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On Sun, Sep 15, 2024 at 9:07 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> receive_header() used to cast the const qualifier of the pointer to the
>>> received packet away to modify the header. Avoid this by copying the
>>> received header to buffer.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   hw/net/virtio-net.c | 85 +++++++++++++++++++++++++++++------------------------
>>>   1 file changed, 46 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 3fc1d10cb9e0..ca4e22344f78 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -1685,41 +1685,44 @@ 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,
>>> -                                        uint8_t *buf, size_t size)
>>> +                                        size_t *hdr_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);
>>>
>>> +    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 */
>>> +        (buf_size >= csum_size && buf_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 */
>>> -        net_checksum_calculate(buf, size, CSUM_UDP);
>>> +        memcpy((uint8_t *)hdr + *hdr_len, buf, csum_size);
>>> +        net_checksum_calculate((uint8_t *)hdr + *hdr_len, csum_size, CSUM_UDP);
>>>           hdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM;
>>> +        *hdr_len += csum_size;
>>> +        *buf_offset += csum_size;
>>>       }
>>>   }
>>>
>>> -static void receive_header(VirtIONet *n, const struct iovec *iov, int iov_cnt,
>>> -                           const void *buf, size_t size)
>>> +static size_t receive_header(VirtIONet *n, struct virtio_net_hdr *hdr,
>>> +                             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 hdr_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(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->needs_vnet_hdr_swap) {
>>> +        virtio_net_hdr_swap(VIRTIO_DEVICE(n), hdr);
>>>       }
>>> +
>>> +    return hdr_len;
>>>   }
>>>
>>>   static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>>> @@ -1887,6 +1890,13 @@ 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;
>>
>> I don't see too much value in having this structure especially
>> considering eth/ip/udp is not even used.
>>
>> Any reason we can simply use an array as a buffer in virtio_net_receive_rcu()?

We still need to access virtio_net and it needs to be properly aligned. 
We can still make the latter part an array, but it does not look nicer:

typedef struct Header {
     struct virtio_net_hdr_v1_hash virtio_net;
     uint8_t eth[sizeof(struct eth_header) + sizeof(struct ip_header) + 
sizeof(struct udp_header)];
} Header;

> 
> Btw, since the code was for ancient guests and userspace, I wonder how
> it is tested.

I didn't test with dhclient, but I tested with UDP packets synthesized 
with iperf.

Regards,
Akihiko Odaki

[PATCH 7/7] virtio-net: Fix num_buffers for version 1

[Akihiko Odaki]

The specification says the device MUST set num_buffers to 1 if
VIRTIO_NET_F_MRG_RXBUF has not been negotiated.

Fixes: df91055db5c9 ("virtio-net: enable virtio 1.0")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 hw/net/virtio-net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index ca4e22344f78..b4a3fb575c7c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1982,6 +1982,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
                                     offsetof(typeof(hdr),
                                              virtio_net.hdr.num_buffers),
                                     sizeof(hdr.virtio_net.hdr.num_buffers));
+            } else {
+                hdr.virtio_net.hdr.num_buffers = cpu_to_le16(1);
             }
 
             guest_offset = n->has_vnet_hdr ?

-- 
2.46.0

Re: [PATCH 0/7] virtio-net fixes

[Michael Tokarev]

On 15.09.2024 04:06, Akihiko Odaki wrote:
> Most of this series are fixes for software RSS and hash reporting, which
> should have no production user.
> 
> However there is one exception; patch "virtio-net: Fix size check in
> dhclient workaround" fixes an out-of-bound access that can be triggered
> for anyone who don't use vhost. It has Cc: qemu-stable@nongnu.org and
> can be applied independently.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Akihiko Odaki (7):
>        net: checksum: Convert data to void *
>        virtio-net: Fix size check in dhclient workaround
>        virtio-net: Do not check for the queue before RSS
>        virtio-net: Fix hash reporting when the queue changes
>        virtio-net: Initialize hash reporting values
>        virtio-net: Copy received header to buffer
>        virtio-net: Fix num_buffers for version 1
> 
>   include/net/checksum.h |   2 +-
>   hw/net/virtio-net.c    | 109 ++++++++++++++++++++++++++++---------------------
>   net/checksum.c         |   4 +-
>   3 files changed, 65 insertions(+), 50 deletions(-)

Hi!

Has this patchset been forgotten, or does it wait for some
R-b's ?

Thanks,

/mjt

Re: [PATCH 0/7] virtio-net fixes

[Jason Wang]

Hi Michael:

On Wed, Oct 16, 2024 at 1:58 AM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> On 15.09.2024 04:06, Akihiko Odaki wrote:
> > Most of this series are fixes for software RSS and hash reporting, which
> > should have no production user.
> >
> > However there is one exception; patch "virtio-net: Fix size check in
> > dhclient workaround" fixes an out-of-bound access that can be triggered
> > for anyone who don't use vhost. It has Cc: qemu-stable@nongnu.org and
> > can be applied independently.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> > Akihiko Odaki (7):
> >        net: checksum: Convert data to void *
> >        virtio-net: Fix size check in dhclient workaround
> >        virtio-net: Do not check for the queue before RSS
> >        virtio-net: Fix hash reporting when the queue changes
> >        virtio-net: Initialize hash reporting values
> >        virtio-net: Copy received header to buffer
> >        virtio-net: Fix num_buffers for version 1
> >
> >   include/net/checksum.h |   2 +-
> >   hw/net/virtio-net.c    | 109 ++++++++++++++++++++++++++++---------------------
> >   net/checksum.c         |   4 +-
> >   3 files changed, 65 insertions(+), 50 deletions(-)
>
> Hi!
>
> Has this patchset been forgotten, or does it wait for some
> R-b's ?

Thanks for the reminder.

I would go through this series.

Thanks

>
> Thanks,
>
> /mjt
>