From 60c1cd75dc8bf9346a68dca403bc85d54937a0e7 Mon Sep 17 00:00:00 2001 From: Dariusz Stojaczyk Date: Mon, 8 Jan 2018 21:35:53 +0100 Subject: [PATCH] vhost_blk: set proper vring_used_elem->len This `len` field describes the amount of bytes *written* by the device. It is used to prevent the driver from reading buffer memory that wasn't really written to - it could contain either leftover values from previous requests or even be completely unitialized. In a couple of invalid-request error conditions we still write the VIRTIO_BLK_S_UNSUPP response while returning used_len == 0, but that's fine. A properly written driver will likely handle both of these cases the same way. Change-Id: I8e9f3a8051561096e2ee755122d534b4ffd94db1 Signed-off-by: Dariusz Stojaczyk Reviewed-on: https://review.gerrithub.io/393963 Reviewed-by: Pawel Kaminski Tested-by: SPDK Automated Test System Reviewed-by: Pawel Wodkowski Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- lib/vhost/vhost_blk.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/lib/vhost/vhost_blk.c b/lib/vhost/vhost_blk.c index 37a111401..8a72db158 100644 --- a/lib/vhost/vhost_blk.c +++ b/lib/vhost/vhost_blk.c @@ -56,7 +56,8 @@ struct spdk_vhost_blk_task { /* If set, the task is currently used for I/O processing. */ bool used; - uint32_t length; + /** Number of bytes that were written. */ + uint32_t used_len; uint16_t iovcnt; struct iovec iovs[SPDK_VHOST_IOVS_MAX]; }; @@ -85,7 +86,8 @@ invalid_blk_request(struct spdk_vhost_blk_task *task, uint8_t status) *task->status = status; } - spdk_vhost_vq_used_ring_enqueue(&task->bvdev->vdev, task->vq, task->req_idx, 0); + spdk_vhost_vq_used_ring_enqueue(&task->bvdev->vdev, task->vq, task->req_idx, + task->used_len); blk_task_finish(task); SPDK_DEBUGLOG(SPDK_LOG_VHOST_BLK_DATA, "Invalid request (status=%" PRIu8")\n", status); } @@ -162,7 +164,7 @@ blk_request_finish(bool success, struct spdk_vhost_blk_task *task) { *task->status = success ? VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR; spdk_vhost_vq_used_ring_enqueue(&task->bvdev->vdev, task->vq, task->req_idx, - task->length); + task->used_len); SPDK_DEBUGLOG(SPDK_LOG_VHOST_BLK, "Finished task (%p) req_idx=%d\n status: %s\n", task, task->req_idx, success ? "OK" : "FAIL"); blk_task_finish(task); @@ -184,9 +186,10 @@ process_blk_request(struct spdk_vhost_blk_task *task, struct spdk_vhost_blk_dev const struct virtio_blk_outhdr *req; struct iovec *iov; uint32_t type; + uint32_t payload_len; int rc; - if (blk_iovs_setup(&bvdev->vdev, vq, task->req_idx, task->iovs, &task->iovcnt, &task->length)) { + if (blk_iovs_setup(&bvdev->vdev, vq, task->req_idx, task->iovs, &task->iovcnt, &payload_len)) { SPDK_DEBUGLOG(SPDK_LOG_VHOST_BLK, "Invalid request (req_idx = %"PRIu16").\n", task->req_idx); /* Only READ and WRITE are supported for now. */ invalid_blk_request(task, VIRTIO_BLK_S_UNSUPP); @@ -214,7 +217,7 @@ process_blk_request(struct spdk_vhost_blk_task *task, struct spdk_vhost_blk_dev } task->status = iov->iov_base; - task->length -= sizeof(*req) + 1; + payload_len -= sizeof(*req) + sizeof(*task->status); task->iovcnt -= 2; type = req->type; @@ -226,7 +229,7 @@ process_blk_request(struct spdk_vhost_blk_task *task, struct spdk_vhost_blk_dev switch (type) { case VIRTIO_BLK_T_IN: case VIRTIO_BLK_T_OUT: - if (spdk_unlikely((task->length & (512 - 1)) != 0)) { + if (spdk_unlikely((payload_len & (512 - 1)) != 0)) { SPDK_ERRLOG("%s - passed IO buffer is not multiple of 512b (req_idx = %"PRIu16").\n", type ? "WRITE" : "READ", task->req_idx); invalid_blk_request(task, VIRTIO_BLK_S_UNSUPP); @@ -234,13 +237,15 @@ process_blk_request(struct spdk_vhost_blk_task *task, struct spdk_vhost_blk_dev } if (type == VIRTIO_BLK_T_IN) { + task->used_len = payload_len + sizeof(*task->status); rc = spdk_bdev_readv(bvdev->bdev_desc, bvdev->bdev_io_channel, &task->iovs[1], task->iovcnt, req->sector * 512, - task->length, blk_request_complete_cb, task); + payload_len, blk_request_complete_cb, task); } else if (!bvdev->readonly) { + task->used_len = sizeof(*task->status); rc = spdk_bdev_writev(bvdev->bdev_desc, bvdev->bdev_io_channel, &task->iovs[1], task->iovcnt, req->sector * 512, - task->length, blk_request_complete_cb, task); + payload_len, blk_request_complete_cb, task); } else { SPDK_DEBUGLOG(SPDK_LOG_VHOST_BLK, "Device is in read-only mode!\n"); rc = -1; @@ -252,12 +257,13 @@ process_blk_request(struct spdk_vhost_blk_task *task, struct spdk_vhost_blk_dev } break; case VIRTIO_BLK_T_GET_ID: - if (!task->iovcnt || !task->length) { + if (!task->iovcnt || !payload_len) { invalid_blk_request(task, VIRTIO_BLK_S_UNSUPP); return -1; } - task->length = spdk_min((size_t)VIRTIO_BLK_ID_BYTES, task->iovs[1].iov_len); - spdk_strcpy_pad(task->iovs[1].iov_base, spdk_bdev_get_product_name(bvdev->bdev), task->length, ' '); + task->used_len = spdk_min((size_t)VIRTIO_BLK_ID_BYTES, task->iovs[1].iov_len); + spdk_strcpy_pad(task->iovs[1].iov_base, spdk_bdev_get_product_name(bvdev->bdev), + task->used_len, ' '); blk_request_finish(true, task); break; default: @@ -306,6 +312,7 @@ process_vq(struct spdk_vhost_blk_dev *bvdev, struct spdk_vhost_virtqueue *vq) task->used = true; task->iovcnt = SPDK_COUNTOF(task->iovs); task->status = NULL; + task->used_len = 0; rc = process_blk_request(task, bvdev, vq); if (rc == 0) {