virtio-net: correct header length math

We were requesting too much when checking buffer
length: size already includes host header length.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

hw/virtio-net.c +1 -1

index 06ba481..2646c87 100644
@@ -527,17 +527,18 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    size_t hdr_len, offset, i;
+    size_t guest_hdr_len, offset, i, host_hdr_len;

Quoted 4 lines · Michael S. Tsirkin

We were requesting too much when checking buffer
length: size already includes host header length.

Signed-off-by Michael S. Tsirkin <[email protected]>

All changes here are ok, but we are still missing more checks.

Quoted 14 lines

---
 hw/virtio-net.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

hw/virtio-net.c +1 -1

index 06ba481..2646c87 100644
@@ -527,17 +527,18 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    size_t hdr_len, offset, i;
+    size_t guest_hdr_len, offset, i, host_hdr_len;

hw/virtio-net.c static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ · line 539

Quoted 10 lines

hw/virtio-net.c +1 -1

 
     if (!virtio_net_can_receive(&n->nic->nc))
         return -1;
 
     /* hdr_len refers to the header we supply to the guest */
-    hdr_len = n->mergeable_rx_bufs ?
+    guest_hdr_len = n->mergeable_rx_bufs ?
         sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
 
 

nitpit: empty extra line

Quoted 5 lines

hw/virtio-net.c +2 -1

-    if (!virtio_net_has_buffers(n, size + hdr_len))
+    host_hdr_len = n->has_vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
+    if (!virtio_net_has_buffers(n, size + guest_hdr_len - host_hdr_len))
         return 0;
 

hw/virtio-net.c static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ · line 544

Quoted 1 line

hw/virtio-net.c +0 -0

     if (!receive_filter(n, buf, size))

size is not used by receive_filter. We are assuming that size is at least 16 + sizeof(struct virtio_net_hdr).

    while (offset < size) {

we are still testing offset with size, but we read headers from there also :(

From where? As far as I can tell we always read size bytes from buf. No?

Later, Juan.

We were requesting too much when checking buffer
length: size already includes host header length.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

Quoted 7 lines · Michael S. Tsirkin

We were requesting too much when checking buffer
length: size already includes host header length.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 hw/virtio-net.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

Couple of minor comments. But I'll still provide the

ACK

hw/virtio-net.c static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ · line 563

Quoted 11 lines

hw/virtio-net.c +4 -3

@@ -556,9 +557,10 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
             virtqueue_pop(n->rx_vq, &elem) == 0) {
             if (i == 0)
                 return -1;
-            fprintf(stderr, "virtio-net truncating packet: "
-                    "offset %zd, size %zd, hdr_len %zd\n",
-                    offset, size, hdr_len);
+            fprintf(stderr, "virtio-net truncating packet: i %d mergeable %d "
+                    "offset %zd, size %zd, guest hdr %zd, host hdr %zd\n",
+		    i, n->mergeable_rx_bufs,
+                    offset, size, guest_hdr_len, host_hdr_len);

Again, tab usage and 'guest hdr len' and 'host hdr len' make better sense than 'guest hdr' and 'host hdr'

Review static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ · line 564

Quoted 1 line

(quoted diff) +0 -0

             exit(1);

Can we drop the packet(s) instead of exiting?

Amit