| Message ID | 20100624155407.GA11165@redhat.com |
|---|---|
| State | New |
| Headers | show |
On (Thu) Jun 24 2010 [18:54:07], Michael S. Tsirkin wrote: > We were requesting too much when checking buffer > length: size already includes host header length. > > Further, we should not exit if we get a packet that > is too long, since this might not be under control > of the guest. Just drop the packet. control of the host? > @@ -579,19 +581,32 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ > mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base; > > offset += receive_header(n, sg, elem.in_num, > - buf + offset, size - offset, hdr_len); > - total += hdr_len; > + buf + offset, size - offset, guest_hdr_len); > + total += guest_hdr_len; > } > > /* copy in packet. ugh */ > len = iov_from_buf(sg, elem.in_num, > buf + offset, size - offset); > total += len; > + offset += len; > + /* If buffers can't be merged, at this point we > + * must have consumed the complete packet. > + * Otherwise, drop it. */ > + if (!n->mergeable_rx_bufs && offset < size) { > +#if 0 > + fprintf(stderr, "virtio-net truncated non-mergeable packet: " > + > + "i %zd mergeable %d offset %zd, size %zd, " > + "guest hdr len %zd, host hdr len %zd\n", > + i, n->mergeable_rx_bufs, > + offset, size, guest_hdr_len, host_hdr_len); > +#endif > + return size; > + } Before returning, won't you have to finish off the virtqueue operations -- fill, flush, kick, etc.? Amit
On Fri, Jun 25, 2010 at 12:47:03PM +0530, Amit Shah wrote: > On (Thu) Jun 24 2010 [18:54:07], Michael S. Tsirkin wrote: > > We were requesting too much when checking buffer > > length: size already includes host header length. > > > > Further, we should not exit if we get a packet that > > is too long, since this might not be under control > > of the guest. Just drop the packet. > > control of the host? Well, host too I guess. What I was trying to say, it might not be the fault of the guest that it got a packet that is too long. > > @@ -579,19 +581,32 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ > > mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base; > > > > offset += receive_header(n, sg, elem.in_num, > > - buf + offset, size - offset, hdr_len); > > - total += hdr_len; > > + buf + offset, size - offset, guest_hdr_len); > > + total += guest_hdr_len; > > } > > > > /* copy in packet. ugh */ > > len = iov_from_buf(sg, elem.in_num, > > buf + offset, size - offset); > > total += len; > > + offset += len; > > + /* If buffers can't be merged, at this point we > > + * must have consumed the complete packet. > > + * Otherwise, drop it. */ > > + if (!n->mergeable_rx_bufs && offset < size) { > > +#if 0 > > + fprintf(stderr, "virtio-net truncated non-mergeable packet: " > > + > > + "i %zd mergeable %d offset %zd, size %zd, " > > + "guest hdr len %zd, host hdr len %zd\n", > > + i, n->mergeable_rx_bufs, > > + offset, size, guest_hdr_len, host_hdr_len); > > +#endif > > + return size; > > + } > > Before returning, won't you have to finish off the virtqueue operations > -- fill, flush, kick, etc.? > > Amit No, this would consume a buffer. We just want to drop the packet without side-effects. But we do need to decrement the inuse counter. I'll respin the patch.
diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 06ba481..1018c32 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -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; 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); - 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; if (!receive_filter(n, buf, size)) @@ -552,13 +553,14 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ total = 0; - if ((i != 0 && !n->mergeable_rx_bufs) || - virtqueue_pop(n->rx_vq, &elem) == 0) { + if (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 unexpected empty queue: " + "i %zd mergeable %d offset %zd, size %zd, " + "guest hdr len %zd, host hdr len %zd guest features 0x%x\n", + i, n->mergeable_rx_bufs, offset, size, + guest_hdr_len, host_hdr_len, n->vdev.guest_features); exit(1); } @@ -567,7 +569,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ exit(1); } - if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != hdr_len) { + if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) { fprintf(stderr, "virtio-net header not in first element\n"); exit(1); } @@ -579,19 +581,32 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base; offset += receive_header(n, sg, elem.in_num, - buf + offset, size - offset, hdr_len); - total += hdr_len; + buf + offset, size - offset, guest_hdr_len); + total += guest_hdr_len; } /* copy in packet. ugh */ len = iov_from_buf(sg, elem.in_num, buf + offset, size - offset); total += len; + offset += len; + /* If buffers can't be merged, at this point we + * must have consumed the complete packet. + * Otherwise, drop it. */ + if (!n->mergeable_rx_bufs && offset < size) { +#if 0 + fprintf(stderr, "virtio-net truncated non-mergeable packet: " + + "i %zd mergeable %d offset %zd, size %zd, " + "guest hdr len %zd, host hdr len %zd\n", + i, n->mergeable_rx_bufs, + offset, size, guest_hdr_len, host_hdr_len); +#endif + return size; + } /* signal other side */ virtqueue_fill(n->rx_vq, &elem, total, i++); - - offset += len; } if (mhdr)
We were requesting too much when checking buffer length: size already includes host header length. Further, we should not exit if we get a packet that is too long, since this might not be under control of the guest. Just drop the packet. Red Hat bz 591494 Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Changes from v2: fixed format warning. Changes from v1: drop packet instead of exit. hw/virtio-net.c | 41 ++++++++++++++++++++++++++++------------- 1 files changed, 28 insertions(+), 13 deletions(-)