On Fri, Apr 17, 2015 at 03:59:21PM +0800, Fam Zheng wrote:
> When getting invalid data from vring, virtqueue_pop used to print an
> error and exit.
>
> Add an errp parameter so it can return the error to callers.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> hw/9pfs/virtio-9p.c | 2 +-
> hw/block/virtio-blk.c | 2 +-
> hw/char/virtio-serial-bus.c | 10 +++----
> hw/net/virtio-net.c | 6 ++--
> hw/scsi/virtio-scsi.c | 2 +-
> hw/virtio/virtio-balloon.c | 4 +--
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 70
> ++++++++++++++++++++++++++++++++++-----------
> include/hw/virtio/virtio.h | 2 +-
> 9 files changed, 69 insertions(+), 31 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 4964da0..17d0c4a 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -3259,7 +3259,7 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
> ssize_t len;
>
> while ((pdu = alloc_pdu(s)) &&
> - (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
> + (len = virtqueue_pop(vq, &pdu->elem, &error_abort)) != 0) {
> uint8_t *ptr;
> pdu->s = s;
> BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f7d8528..0b66ee1 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -191,7 +191,7 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock
> *s)
> {
> VirtIOBlockReq *req = virtio_blk_alloc_request(s);
>
> - if (!virtqueue_pop(s->vq, &req->elem)) {
> + if (!virtqueue_pop(s->vq, &req->elem, &error_abort)) {
> virtio_blk_free_request(req);
> return NULL;
> }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index a56dafc..76a934b 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -94,7 +94,7 @@ static size_t write_to_port(VirtIOSerialPort *port,
> while (offset < size) {
> size_t len;
>
> - if (!virtqueue_pop(vq, &elem)) {
> + if (!virtqueue_pop(vq, &elem, &error_abort)) {
> break;
> }
>
> @@ -116,7 +116,7 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice
> *vdev)
> if (!virtio_queue_ready(vq)) {
> return;
> }
> - while (virtqueue_pop(vq, &elem)) {
> + while (virtqueue_pop(vq, &elem, &error_abort)) {
> virtqueue_push(vq, &elem, 0);
> }
> virtio_notify(vdev, vq);
> @@ -137,7 +137,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port,
> VirtQueue *vq,
>
> /* Pop an elem only if we haven't left off a previous one mid-way */
> if (!port->elem.out_num) {
> - if (!virtqueue_pop(vq, &port->elem)) {
> + if (!virtqueue_pop(vq, &port->elem, &error_abort)) {
> break;
> }
> port->iov_idx = 0;
> @@ -190,7 +190,7 @@ static size_t send_control_msg(VirtIOSerial *vser, void
> *buf, size_t len)
> if (!virtio_queue_ready(vq)) {
> return 0;
> }
> - if (!virtqueue_pop(vq, &elem)) {
> + if (!virtqueue_pop(vq, &elem, &error_abort)) {
> return 0;
> }
>
> @@ -420,7 +420,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>
> len = 0;
> buf = NULL;
> - while (virtqueue_pop(vq, &elem)) {
> + while (virtqueue_pop(vq, &elem, &error_abort)) {
> size_t cur_len;
>
> cur_len = iov_size(elem.out_sg, elem.out_num);
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 59f76bc..bbcb51f 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -804,7 +804,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev,
> VirtQueue *vq)
> struct iovec *iov, *iov2;
> unsigned int iov_cnt;
>
> - while (virtqueue_pop(vq, &elem)) {
> + while (virtqueue_pop(vq, &elem, &error_abort)) {
> if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
> iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
> error_report("virtio-net ctrl missing headers");
> @@ -1031,7 +1031,7 @@ static ssize_t virtio_net_receive(NetClientState *nc,
> const uint8_t *buf, size_t
>
> total = 0;
>
> - if (virtqueue_pop(q->rx_vq, &elem) == 0) {
> + if (virtqueue_pop(q->rx_vq, &elem, &error_abort) == 0) {
> if (i == 0)
> return -1;
> error_report("virtio-net unexpected empty queue: "
> @@ -1134,7 +1134,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> return num_packets;
> }
>
> - while (virtqueue_pop(q->tx_vq, &elem)) {
> + while (virtqueue_pop(q->tx_vq, &elem, &error_abort)) {
> ssize_t ret, len;
> unsigned int out_num = elem.out_num;
> struct iovec *out_sg = &elem.out_sg[0];
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index c9bea06..40ba03d 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -177,7 +177,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
> static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
> {
> VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
> - if (!virtqueue_pop(vq, &req->elem)) {
> + if (!virtqueue_pop(vq, &req->elem, &error_abort)) {
> virtio_scsi_free_req(req);
> return NULL;
> }
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 95b0643..e26c0a7 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -206,7 +206,7 @@ static void virtio_balloon_handle_output(VirtIODevice
> *vdev, VirtQueue *vq)
> VirtQueueElement elem;
> MemoryRegionSection section;
>
> - while (virtqueue_pop(vq, &elem)) {
> + while (virtqueue_pop(vq, &elem, &error_abort)) {
> size_t offset = 0;
> uint32_t pfn;
>
> @@ -246,7 +246,7 @@ static void virtio_balloon_receive_stats(VirtIODevice
> *vdev, VirtQueue *vq)
> size_t offset = 0;
> qemu_timeval tv;
>
> - if (!virtqueue_pop(vq, elem)) {
> + if (!virtqueue_pop(vq, elem, &error_abort)) {
> goto out;
> }
>
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 9e1bd75..c3cbdc3 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -56,7 +56,7 @@ static void chr_read(void *opaque, const void *buf, size_t
> size)
>
> offset = 0;
> while (offset < size) {
> - if (!virtqueue_pop(vrng->vq, &elem)) {
> + if (!virtqueue_pop(vrng->vq, &elem, &error_abort)) {
> break;
> }
> len = iov_from_buf(elem.in_sg, elem.in_num,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1cd454b..e6f9f6b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -481,29 +481,49 @@ fail:
> error_setg(errp, "virtio: error trying to map MMIO memory");
> }
>
> -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> +static void virtqueue_undo_map_sg(struct iovec *sg, hwaddr *addr,
> + size_t num_sg, int is_write)
> +{
> + int i;
> +
> + for (i = 0; i < num_sg; i++) {
> + cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len,
> + is_write, 0);
> + }
> +}
> +
> +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp)
> {
> unsigned int i, head, max;
> + int ret;
> hwaddr desc_pa = vq->vring.desc;
> VirtIODevice *vdev = vq->vdev;
> + Error *local_err = NULL;
>
> - if (!virtqueue_num_heads(vq, vq->last_avail_idx, &error_abort))
> - return 0;
> + ret = virtqueue_num_heads(vq, vq->last_avail_idx, &local_err);
> + if (ret <= 0) {
> + goto err;
> + }
>
Strange.
Doesn't this make pop print an error if ring is empty?
> /* When we start there are none of either input nor output. */
> elem->out_num = elem->in_num = 0;
>
> max = vq->vring.num;
>
> - i = head = virtqueue_get_head(vq, vq->last_avail_idx++, &error_abort);
> + ret = virtqueue_get_head(vq, vq->last_avail_idx++, &local_err);
> + if (ret < 0) {
> + goto err;
> + }
> + head = i = ret;
> +
> if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> vring_set_avail_event(vq, vq->last_avail_idx);
> }
>
> if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
> if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
> - error_report("Invalid size for indirect buffer table");
> - exit(1);
> + error_setg(errp, "Invalid size for indirect buffer table");
> + return -EINVAL;
Again, you have both error_setg and a return code.
There's no need for this.
Just return ring empty on error.
> }
>
> /* loop over the indirect descriptor table */
> @@ -518,15 +538,17 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>
> if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_WRITE) {
> if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
> - error_report("Too many write descriptors in indirect table");
> - exit(1);
> + error_setg(errp,
> + "Too many write descriptors in indirect table");
> + return -EINVAL;
> }
> elem->in_addr[elem->in_num] = vring_desc_addr(vdev, desc_pa, i);
> sg = &elem->in_sg[elem->in_num++];
> } else {
> if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
> - error_report("Too many read descriptors in indirect table");
> - exit(1);
> + error_setg(errp,
> + "Too many read descriptors in indirect table");
> + return -EINVAL;
> }
> elem->out_addr[elem->out_num] = vring_desc_addr(vdev, desc_pa,
> i);
> sg = &elem->out_sg[elem->out_num++];
> @@ -536,20 +558,31 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>
> /* If we've got too many, that implies a descriptor loop. */
> if ((elem->in_num + elem->out_num) > max) {
> - error_report("Looped descriptor");
> - exit(1);
> + error_setg(errp, "Looped descriptor");
> + return -EINVAL;
> }
> - i = virtqueue_next_desc(vdev, desc_pa, i, max, &error_abort);
> - if (i == max) {
> + ret = virtqueue_next_desc(vdev, desc_pa, i, max, &local_err);
> + if (ret < 0) {
> + goto err;
> + } else if (ret == max) {
> break;
> }
> + i = ret;
> }
>
> /* Now map what we have collected */
> virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1,
> - &error_abort);
> + &local_err);
> + if (local_err) {
> + ret = -EINVAL;
> + goto err;
> + }
> virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0,
> - &error_abort);
> + &local_err);
> + if (local_err) {
> + ret = -EINVAL;
> + goto err_unmap;
> + }
>
> elem->index = head;
>
> @@ -557,6 +590,11 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>
> trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
> return elem->in_num + elem->out_num;
> +err_unmap:
> + virtqueue_undo_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
> +err:
> + error_propagate(errp, local_err);
> + return ret;
> }
>
> /* virtio device */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index a37ee64..c478f48 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -142,7 +142,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement
> *elem,
> void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
> size_t num_sg, int is_write,
> Error **errp);
> -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
> +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp);
> int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
> unsigned int out_bytes);
> void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> --
> 1.9.3
