Virtualization - Jun 2016 - [PATCH] vhost/scsi: fix reuse of &vq->iov[out] in response

The address of the iovec &vq->iov[out] is not guaranteed to contain the
scsi
command's response iovec throughout the lifetime of the command.  Rather, it
is more likely to contain an iovec from an immediately following command
after looping back around to vhost_get_vq_desc().  Pass along the iovec
entirely instead.

Fixes: 79c14141a487 ("vhost/scsi: Convert completion path to use
copy_to_iter")
Signed-off-by: Benjamin Coddington <bcodding at redhat.com>
---
 drivers/vhost/scsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 0e6fd55..c93e125 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -88,7 +88,7 @@ struct vhost_scsi_cmd {
 	struct scatterlist *tvc_prot_sgl;
 	struct page **tvc_upages;
 	/* Pointer to response header iovec */
-	struct iovec *tvc_resp_iov;
+	struct iovec tvc_resp_iov;
 	/* Pointer to vhost_scsi for our device */
 	struct vhost_scsi *tvc_vhost;
 	/* Pointer to vhost_virtqueue for the cmd */
@@ -557,7 +557,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work
*work)
 		memcpy(v_rsp.sense, cmd->tvc_sense_buf,
 		       se_cmd->scsi_sense_length);
 
-		iov_iter_init(&iov_iter, READ, cmd->tvc_resp_iov,
+		iov_iter_init(&iov_iter, READ, &cmd->tvc_resp_iov,
 			      cmd->tvc_in_iovs, sizeof(v_rsp));
 		ret = copy_to_iter(&v_rsp, sizeof(v_rsp), &iov_iter);
 		if (likely(ret == sizeof(v_rsp))) {
@@ -1054,7 +1054,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct
vhost_virtqueue *vq)
 		}
 		cmd->tvc_vhost = vs;
 		cmd->tvc_vq = vq;
-		cmd->tvc_resp_iov = &vq->iov[out];
+		cmd->tvc_resp_iov = vq->iov[out];
 		cmd->tvc_in_iovs = in;
 
 		pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
-- 
2.5.5

On Mon, Jun 06, 2016 at 06:01:48PM -0400, Benjamin Coddington
wrote:
> The address of the iovec &vq->iov[out] is not guaranteed to contain
the scsi
> command's response iovec throughout the lifetime of the command. 
Rather, it
> is more likely to contain an iovec from an immediately following command
> after looping back around to vhost_get_vq_desc().  Pass along the iovec
> entirely instead.
> 
> Fixes: 79c14141a487 ("vhost/scsi: Convert completion path to use
copy_to_iter")
> Signed-off-by: Benjamin Coddington <bcodding at redhat.com>
Thanks, and sorry about the delayed response.
Originally I thought there's an issue here in that in theory, the response
could be split across multiple iov entries.

However, I now noticed that this is already unsupported:

                if (unlikely(vq->iov[out].iov_len < rsp_size)) {
                        vq_err(vq, "Expecting at least
virtio_scsi_cmd_resp"
                                " size, got %zu bytes\n",
vq->iov[out].iov_len);
                        break;
                }

So I will queue this, but I think that we need to look at removing the
assumption here longer term.
> ---
>  drivers/vhost/scsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 0e6fd55..c93e125 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -88,7 +88,7 @@ struct vhost_scsi_cmd {
>  	struct scatterlist *tvc_prot_sgl;
>  	struct page **tvc_upages;
>  	/* Pointer to response header iovec */
> -	struct iovec *tvc_resp_iov;
> +	struct iovec tvc_resp_iov;
>  	/* Pointer to vhost_scsi for our device */
>  	struct vhost_scsi *tvc_vhost;
>  	/* Pointer to vhost_virtqueue for the cmd */
> @@ -557,7 +557,7 @@ static void vhost_scsi_complete_cmd_work(struct
vhost_work *work)
>  		memcpy(v_rsp.sense, cmd->tvc_sense_buf,
>  		       se_cmd->scsi_sense_length);
>  
> -		iov_iter_init(&iov_iter, READ, cmd->tvc_resp_iov,
> +		iov_iter_init(&iov_iter, READ, &cmd->tvc_resp_iov,
>  			      cmd->tvc_in_iovs, sizeof(v_rsp));
>  		ret = copy_to_iter(&v_rsp, sizeof(v_rsp), &iov_iter);
>  		if (likely(ret == sizeof(v_rsp))) {
> @@ -1054,7 +1054,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct
vhost_virtqueue *vq)
>  		}
>  		cmd->tvc_vhost = vs;
>  		cmd->tvc_vq = vq;
> -		cmd->tvc_resp_iov = &vq->iov[out];
> +		cmd->tvc_resp_iov = vq->iov[out];
>  		cmd->tvc_in_iovs = in;
>  
>  		pr_debug("vhost_scsi got command opcode: %#02x, lun: %d\n",
> -- 
> 2.5.5