Re: [PATCH] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

Re: [PATCH] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Michael S. Tsirkin]

On Tue, Apr 09, 2013 at 05:16:33PM +0800, Asias He wrote:
> If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
> we will leak the tv_vmd. Free tv_vmd on fail path.
> 
> Signed-off-by: Asias He <asias@redhat.com>

Another one for 3.9 I think.

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/vhost/tcm_vhost.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index 3351ed3..1f9116c 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -860,7 +860,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
>  				" bytes, out: %d, in: %d\n",
>  				vq->iov[out].iov_len, out, in);
> -			break;
> +			goto err;
>  		}
>  
>  		tv_cmd->tvc_resp = vq->iov[out].iov_base;
> @@ -882,7 +882,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
>  				scsi_command_size(tv_cmd->tvc_cdb),
>  				TCM_VHOST_MAX_CDB_SIZE);
> -			break; /* TODO */
> +			goto err;
>  		}
>  		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
>  
> @@ -895,7 +895,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  					data_direction == DMA_TO_DEVICE);
>  			if (unlikely(ret)) {
>  				vq_err(vq, "Failed to map iov to sgl\n");
> -				break; /* TODO */
> +				goto err;
>  			}
>  		}
>  
> @@ -916,6 +916,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  	}
>  
>  	mutex_unlock(&vq->mutex);
> +	return;
> +
> +err:
> +	vhost_scsi_free_cmd(tv_cmd);
> +	mutex_unlock(&vq->mutex);
>  }
>  
>  static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
> -- 
> 1.8.1.4

[PATCH] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Asias He]

If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
we will leak the tv_vmd. Free tv_vmd on fail path.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 3351ed3..1f9116c 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -860,7 +860,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
 				" bytes, out: %d, in: %d\n",
 				vq->iov[out].iov_len, out, in);
-			break;
+			goto err;
 		}
 
 		tv_cmd->tvc_resp = vq->iov[out].iov_base;
@@ -882,7 +882,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 				scsi_command_size(tv_cmd->tvc_cdb),
 				TCM_VHOST_MAX_CDB_SIZE);
-			break; /* TODO */
+			goto err;
 		}
 		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
 
@@ -895,7 +895,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 					data_direction == DMA_TO_DEVICE);
 			if (unlikely(ret)) {
 				vq_err(vq, "Failed to map iov to sgl\n");
-				break; /* TODO */
+				goto err;
 			}
 		}
 
@@ -916,6 +916,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	}
 
 	mutex_unlock(&vq->mutex);
+	return;
+
+err:
+	vhost_scsi_free_cmd(tv_cmd);
+	mutex_unlock(&vq->mutex);
 }
 
 static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
-- 
1.8.1.4

Re: [PATCH] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Nicholas A. Bellinger]

On Tue, 2013-04-09 at 17:16 +0800, Asias He wrote:
> If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
> we will leak the tv_vmd. Free tv_vmd on fail path.
> 
> Signed-off-by: Asias He <asias@redhat.com>
> ---
>  drivers/vhost/tcm_vhost.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index 3351ed3..1f9116c 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -860,7 +860,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
>  				" bytes, out: %d, in: %d\n",
>  				vq->iov[out].iov_len, out, in);
> -			break;
> +			goto err;
>  		}
>  
>  		tv_cmd->tvc_resp = vq->iov[out].iov_base;
> @@ -882,7 +882,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
>  				scsi_command_size(tv_cmd->tvc_cdb),
>  				TCM_VHOST_MAX_CDB_SIZE);
> -			break; /* TODO */
> +			goto err;
>  		}
>  		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
>  
> @@ -895,7 +895,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  					data_direction == DMA_TO_DEVICE);
>  			if (unlikely(ret)) {
>  				vq_err(vq, "Failed to map iov to sgl\n");
> -				break; /* TODO */
> +				goto err;
>  			}
>  		}
>  

Mmmm, I think these cases also require a VIRTIO_SCSI_S_BAD_TARGET +
__copy_to_user + vhost_add_used_and_signal similar to how !tv_tpg is
handled..  Otherwise virtio-scsi will end up in scsi timeout -> abort,
no..?

Ditto for the vhost_scsi_allocate_cmd failure case..

vhost-net uses vhost_discard_vq_desc for some failure cases,  is that
needed here for the failure cases before __copy_from_user is called..?

> @@ -916,6 +916,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
>  	}
>  
>  	mutex_unlock(&vq->mutex);
> +	return;
> +
> +err:
> +	vhost_scsi_free_cmd(tv_cmd);
> +	mutex_unlock(&vq->mutex);
>  }
>  
>  static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)

[PATCH 0/3] tcm_vhost fix cmd leak and bad target

[Asias He]

Asias He (3):
  tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq
  tcm_vhost: Add vhost_scsi_send_bad_target() helper
  tcm_vhost: Send bad target to guest when cmd fails

 drivers/vhost/tcm_vhost.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

-- 
1.8.1.4

[PATCH 1/3] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Asias He]

If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
we will leak the tv_vmd. Free tv_vmd on fail path.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 10f2d30..e8d1a1f 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -715,7 +715,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 				scsi_command_size(tv_cmd->tvc_cdb),
 				TCM_VHOST_MAX_CDB_SIZE);
-			break; /* TODO */
+			goto err;
 		}
 		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
 
@@ -728,7 +728,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 					data_direction == DMA_TO_DEVICE);
 			if (unlikely(ret)) {
 				vq_err(vq, "Failed to map iov to sgl\n");
-				break; /* TODO */
+				goto err;
 			}
 		}
 
@@ -749,6 +749,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	}
 
 	mutex_unlock(&vq->mutex);
+	return;
+
+err:
+	vhost_scsi_free_cmd(tv_cmd);
+	mutex_unlock(&vq->mutex);
 }
 
 static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
-- 
1.8.1.4

[PATCH 2/3] tcm_vhost: Add vhost_scsi_send_bad_target() helper

[Asias He]

Share the send bad target code with other use cases.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index e8d1a1f..1c719ed 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -569,6 +569,23 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 	}
 }
 
+static void vhost_scsi_send_bad_target(struct vhost_scsi *vs,
+	struct vhost_virtqueue *vq, int head, unsigned out)
+{
+	struct virtio_scsi_cmd_resp __user *resp;
+	struct virtio_scsi_cmd_resp rsp;
+	int ret;
+
+	memset(&rsp, 0, sizeof(rsp));
+	rsp.response = VIRTIO_SCSI_S_BAD_TARGET;
+	resp = vq->iov[out].iov_base;
+	ret = __copy_to_user(resp, &rsp, sizeof(rsp));
+	if (!ret)
+		vhost_add_used_and_signal(&vs->dev, vq, head, 0);
+	else
+		pr_err("Faulted on virtio_scsi_cmd_resp\n");
+}
+
 static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	struct vhost_virtqueue *vq)
 {
@@ -664,19 +681,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 
 		/* Target does not exist, fail the request */
 		if (unlikely(!tv_tpg)) {
-			struct virtio_scsi_cmd_resp __user *resp;
-			struct virtio_scsi_cmd_resp rsp;
-
-			memset(&rsp, 0, sizeof(rsp));
-			rsp.response = VIRTIO_SCSI_S_BAD_TARGET;
-			resp = vq->iov[out].iov_base;
-			ret = __copy_to_user(resp, &rsp, sizeof(rsp));
-			if (!ret)
-				vhost_add_used_and_signal(&vs->dev,
-							  vq, head, 0);
-			else
-				pr_err("Faulted on virtio_scsi_cmd_resp\n");
-
+			vhost_scsi_send_bad_target(vs, vq, out, head);
 			continue;
 		}
 
-- 
1.8.1.4

[PATCH 3/3] tcm_vhost: Send bad target to guest when cmd fails

[Asias He]

Send bad target to guest in case:
1) we can not allocate the cmd
2) fail to submit the cmd

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 1c719ed..4dc6f2d 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -694,7 +694,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 		if (IS_ERR(tv_cmd)) {
 			vq_err(vq, "vhost_scsi_allocate_cmd failed %ld\n",
 					PTR_ERR(tv_cmd));
-			break;
+			goto err_cmd;
 		}
 		pr_debug("Allocated tv_cmd: %p exp_data_len: %d, data_direction"
 			": %d\n", tv_cmd, exp_data_len, data_direction);
@@ -720,7 +720,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 				scsi_command_size(tv_cmd->tvc_cdb),
 				TCM_VHOST_MAX_CDB_SIZE);
-			goto err;
+			goto err_free;
 		}
 		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
 
@@ -733,7 +733,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 					data_direction == DMA_TO_DEVICE);
 			if (unlikely(ret)) {
 				vq_err(vq, "Failed to map iov to sgl\n");
-				goto err;
+				goto err_free;
 			}
 		}
 
@@ -756,8 +756,10 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	mutex_unlock(&vq->mutex);
 	return;
 
-err:
+err_free:
 	vhost_scsi_free_cmd(tv_cmd);
+err_cmd:
+	vhost_scsi_send_bad_target(vs, vq, out, head);
 	mutex_unlock(&vq->mutex);
 }
 
-- 
1.8.1.4

Re: [PATCH] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Asias He]

On Tue, Apr 09, 2013 at 08:46:42AM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2013-04-09 at 17:16 +0800, Asias He wrote:
> > If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
> > we will leak the tv_vmd. Free tv_vmd on fail path.
> > 
> > Signed-off-by: Asias He <asias@redhat.com>
> > ---
> >  drivers/vhost/tcm_vhost.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > index 3351ed3..1f9116c 100644
> > --- a/drivers/vhost/tcm_vhost.c
> > +++ b/drivers/vhost/tcm_vhost.c
> > @@ -860,7 +860,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
> >  				" bytes, out: %d, in: %d\n",
> >  				vq->iov[out].iov_len, out, in);
> > -			break;
> > +			goto err;
> >  		}
> >  
> >  		tv_cmd->tvc_resp = vq->iov[out].iov_base;
> > @@ -882,7 +882,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
> >  				scsi_command_size(tv_cmd->tvc_cdb),
> >  				TCM_VHOST_MAX_CDB_SIZE);
> > -			break; /* TODO */
> > +			goto err;
> >  		}
> >  		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
> >  
> > @@ -895,7 +895,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  					data_direction == DMA_TO_DEVICE);
> >  			if (unlikely(ret)) {
> >  				vq_err(vq, "Failed to map iov to sgl\n");
> > -				break; /* TODO */
> > +				goto err;
> >  			}
> >  		}
> >  
> 
> Mmmm, I think these cases also require a VIRTIO_SCSI_S_BAD_TARGET +
> __copy_to_user + vhost_add_used_and_signal similar to how !tv_tpg is
> handled..  Otherwise virtio-scsi will end up in scsi timeout -> abort,
> no..?
> 
> Ditto for the vhost_scsi_allocate_cmd failure case..

Sent out new patches.

> vhost-net uses vhost_discard_vq_desc for some failure cases,  is that
> needed here for the failure cases before __copy_from_user is called..?

I don't think it is useful. vhost_discard_vq_desc reverse the effect of
vhost_get_vq_desc. If we put it back in the queue and next time we will
still fail.

> > @@ -916,6 +916,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  	}
> >  
> >  	mutex_unlock(&vq->mutex);
> > +	return;
> > +
> > +err:
> > +	vhost_scsi_free_cmd(tv_cmd);
> > +	mutex_unlock(&vq->mutex);
> >  }
> >  
> >  static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
> 
> 

-- 
Asias

Re: [PATCH 0/3] tcm_vhost fix cmd leak and bad target

[Asias He]

On Wed, Apr 10, 2013 at 11:23:08AM +0800, Asias He wrote:
> Asias He (3):
>   tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq
>   tcm_vhost: Add vhost_scsi_send_bad_target() helper
>   tcm_vhost: Send bad target to guest when cmd fails
> 
>  drivers/vhost/tcm_vhost.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)

Forgot to send this out. This series is on top of this one. 

From a0b5d53d057a34330c811f5cd4264182f392b374 Mon Sep 17 00:00:00 2001
From: Asias He <asias@redhat.com>
Date: Wed, 10 Apr 2013 10:39:43 +0800
Subject: [PATCH] tcm_vhost: Remove double check of response

We did the length of response check twice.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index c127731..28c112f 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -705,15 +705,6 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 
 		tv_cmd->tvc_vhost = vs;
 		tv_cmd->tvc_vq = vq;
-
-		if (unlikely(vq->iov[out].iov_len !=
-				sizeof(struct virtio_scsi_cmd_resp))) {
-			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
-				" bytes, out: %d, in: %d\n",
-				vq->iov[out].iov_len, out, in);
-			break;
-		}
-
 		tv_cmd->tvc_resp = vq->iov[out].iov_base;
 
 		/*
-- 
1.8.1.4


-- 
Asias

[PATCH v2 0/4] tcm_vhost fix cmd leak and send bad target

[Asias He]

v2:
- Fix the order of out and head parameter.

Asias He (4):
  tcm_vhost: Remove double check of response
  tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq
  tcm_vhost: Add vhost_scsi_send_bad_target() helper
  tcm_vhost: Send bad target to guest when cmd fails

 drivers/vhost/tcm_vhost.c | 53 +++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/4] tcm_vhost: Remove double check of response

[Asias He]

We did the length of response check twice.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index c127731..28c112f 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -705,15 +705,6 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 
 		tv_cmd->tvc_vhost = vs;
 		tv_cmd->tvc_vq = vq;
-
-		if (unlikely(vq->iov[out].iov_len !=
-				sizeof(struct virtio_scsi_cmd_resp))) {
-			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
-				" bytes, out: %d, in: %d\n",
-				vq->iov[out].iov_len, out, in);
-			break;
-		}
-
 		tv_cmd->tvc_resp = vq->iov[out].iov_base;
 
 		/*
-- 
1.8.1.4

[PATCH v2 2/4] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Asias He]

If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
we will leak the tv_vmd. Free tv_vmd on fail path.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 28c112f..210d59e 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -724,7 +724,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 				scsi_command_size(tv_cmd->tvc_cdb),
 				TCM_VHOST_MAX_CDB_SIZE);
-			break; /* TODO */
+			goto err;
 		}
 		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
 
@@ -737,7 +737,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 					data_direction == DMA_TO_DEVICE);
 			if (unlikely(ret)) {
 				vq_err(vq, "Failed to map iov to sgl\n");
-				break; /* TODO */
+				goto err;
 			}
 		}
 
@@ -758,6 +758,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	}
 
 	mutex_unlock(&vq->mutex);
+	return;
+
+err:
+	vhost_scsi_free_cmd(tv_cmd);
+	mutex_unlock(&vq->mutex);
 }
 
 static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
-- 
1.8.1.4

[PATCH v2 3/4] tcm_vhost: Add vhost_scsi_send_bad_target() helper

[Asias He]

Share the send bad target code with other use cases.

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 210d59e..1bb0fb4 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -578,6 +578,23 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 	}
 }
 
+static void vhost_scsi_send_bad_target(struct vhost_scsi *vs,
+	struct vhost_virtqueue *vq, int head, unsigned out)
+{
+	struct virtio_scsi_cmd_resp __user *resp;
+	struct virtio_scsi_cmd_resp rsp;
+	int ret;
+
+	memset(&rsp, 0, sizeof(rsp));
+	rsp.response = VIRTIO_SCSI_S_BAD_TARGET;
+	resp = vq->iov[out].iov_base;
+	ret = __copy_to_user(resp, &rsp, sizeof(rsp));
+	if (!ret)
+		vhost_add_used_and_signal(&vs->dev, vq, head, 0);
+	else
+		pr_err("Faulted on virtio_scsi_cmd_resp\n");
+}
+
 static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	struct vhost_virtqueue *vq)
 {
@@ -673,19 +690,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 
 		/* Target does not exist, fail the request */
 		if (unlikely(!tv_tpg)) {
-			struct virtio_scsi_cmd_resp __user *resp;
-			struct virtio_scsi_cmd_resp rsp;
-
-			memset(&rsp, 0, sizeof(rsp));
-			rsp.response = VIRTIO_SCSI_S_BAD_TARGET;
-			resp = vq->iov[out].iov_base;
-			ret = __copy_to_user(resp, &rsp, sizeof(rsp));
-			if (!ret)
-				vhost_add_used_and_signal(&vs->dev,
-							  vq, head, 0);
-			else
-				pr_err("Faulted on virtio_scsi_cmd_resp\n");
-
+			vhost_scsi_send_bad_target(vs, vq, head, out);
 			continue;
 		}
 
-- 
1.8.1.4

[PATCH v2 4/4] tcm_vhost: Send bad target to guest when cmd fails

[Asias He]

Send bad target to guest in case:
1) we can not allocate the cmd
2) fail to submit the cmd

Signed-off-by: Asias He <asias@redhat.com>
---
 drivers/vhost/tcm_vhost.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 1bb0fb4..957a0b9 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -703,7 +703,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 		if (IS_ERR(tv_cmd)) {
 			vq_err(vq, "vhost_scsi_allocate_cmd failed %ld\n",
 					PTR_ERR(tv_cmd));
-			break;
+			goto err_cmd;
 		}
 		pr_debug("Allocated tv_cmd: %p exp_data_len: %d, data_direction"
 			": %d\n", tv_cmd, exp_data_len, data_direction);
@@ -729,7 +729,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
 				scsi_command_size(tv_cmd->tvc_cdb),
 				TCM_VHOST_MAX_CDB_SIZE);
-			goto err;
+			goto err_free;
 		}
 		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
 
@@ -742,7 +742,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 					data_direction == DMA_TO_DEVICE);
 			if (unlikely(ret)) {
 				vq_err(vq, "Failed to map iov to sgl\n");
-				goto err;
+				goto err_free;
 			}
 		}
 
@@ -765,8 +765,10 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
 	mutex_unlock(&vq->mutex);
 	return;
 
-err:
+err_free:
 	vhost_scsi_free_cmd(tv_cmd);
+err_cmd:
+	vhost_scsi_send_bad_target(vs, vq, head, out);
 	mutex_unlock(&vq->mutex);
 }
 
-- 
1.8.1.4

Re: [PATCH v2 0/4] tcm_vhost fix cmd leak and send bad target

[Nicholas A. Bellinger]

On Wed, 2013-04-10 at 15:06 +0800, Asias He wrote:
> v2:
> - Fix the order of out and head parameter.
> 
> Asias He (4):
>   tcm_vhost: Remove double check of response
>   tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq
>   tcm_vhost: Add vhost_scsi_send_bad_target() helper
>   tcm_vhost: Send bad target to guest when cmd fails
> 
>  drivers/vhost/tcm_vhost.c | 53 +++++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)
> 

Looks good.  MST, care to ACK for 3.9..?

Thanks Asias!

--nab

Re: [PATCH v2 0/4] tcm_vhost fix cmd leak and send bad target

[Michael S. Tsirkin]

On Wed, Apr 10, 2013 at 02:19:02PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2013-04-10 at 15:06 +0800, Asias He wrote:
> > v2:
> > - Fix the order of out and head parameter.
> > 
> > Asias He (4):
> >   tcm_vhost: Remove double check of response
> >   tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq
> >   tcm_vhost: Add vhost_scsi_send_bad_target() helper
> >   tcm_vhost: Send bad target to guest when cmd fails
> > 
> >  drivers/vhost/tcm_vhost.c | 53 +++++++++++++++++++++++++----------------------
> >  1 file changed, 28 insertions(+), 25 deletions(-)
> > 
> 
> Looks good.  MST, care to ACK for 3.9..?
> 
> Thanks Asias!
> 
> --nab

Sounds like a reasonable thing to apply for 3.9.

Acked-by: Michael S. Tsirkin <mst@redhat.com>


-- 
MST

Re: [PATCH v2 0/4] tcm_vhost fix cmd leak and send bad target

[Asias He]

On Thu, Apr 11, 2013 at 10:22:33AM +0300, Michael S. Tsirkin wrote:
> On Wed, Apr 10, 2013 at 02:19:02PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2013-04-10 at 15:06 +0800, Asias He wrote:
> > > v2:
> > > - Fix the order of out and head parameter.
> > > 
> > > Asias He (4):
> > >   tcm_vhost: Remove double check of response
> > >   tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq
> > >   tcm_vhost: Add vhost_scsi_send_bad_target() helper
> > >   tcm_vhost: Send bad target to guest when cmd fails
> > > 
> > >  drivers/vhost/tcm_vhost.c | 53 +++++++++++++++++++++++++----------------------
> > >  1 file changed, 28 insertions(+), 25 deletions(-)
> > > 
> > 
> > Looks good.  MST, care to ACK for 3.9..?
> > 
> > Thanks Asias!
> > 
> > --nab
> 
> Sounds like a reasonable thing to apply for 3.9.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks!

-- 
Asias

Re: [PATCH] tcm_vhost: Fix tv_cmd leak in vhost_scsi_handle_vq

[Michael S. Tsirkin]

On Tue, Apr 09, 2013 at 08:46:42AM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2013-04-09 at 17:16 +0800, Asias He wrote:
> > If we fail to submit the allocated tv_vmd to tcm_vhost_submission_work,
> > we will leak the tv_vmd. Free tv_vmd on fail path.
> > 
> > Signed-off-by: Asias He <asias@redhat.com>
> > ---
> >  drivers/vhost/tcm_vhost.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> > index 3351ed3..1f9116c 100644
> > --- a/drivers/vhost/tcm_vhost.c
> > +++ b/drivers/vhost/tcm_vhost.c
> > @@ -860,7 +860,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  			vq_err(vq, "Expecting virtio_scsi_cmd_resp, got %zu"
> >  				" bytes, out: %d, in: %d\n",
> >  				vq->iov[out].iov_len, out, in);
> > -			break;
> > +			goto err;
> >  		}
> >  
> >  		tv_cmd->tvc_resp = vq->iov[out].iov_base;
> > @@ -882,7 +882,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  				" exceeds SCSI_MAX_VARLEN_CDB_SIZE: %d\n",
> >  				scsi_command_size(tv_cmd->tvc_cdb),
> >  				TCM_VHOST_MAX_CDB_SIZE);
> > -			break; /* TODO */
> > +			goto err;
> >  		}
> >  		tv_cmd->tvc_lun = ((v_req.lun[2] << 8) | v_req.lun[3]) & 0x3FFF;
> >  
> > @@ -895,7 +895,7 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  					data_direction == DMA_TO_DEVICE);
> >  			if (unlikely(ret)) {
> >  				vq_err(vq, "Failed to map iov to sgl\n");
> > -				break; /* TODO */
> > +				goto err;
> >  			}
> >  		}
> >  
> 
> Mmmm, I think these cases also require a VIRTIO_SCSI_S_BAD_TARGET +
> __copy_to_user + vhost_add_used_and_signal similar to how !tv_tpg is
> handled..  Otherwise virtio-scsi will end up in scsi timeout -> abort,
> no..?
> 
> Ditto for the vhost_scsi_allocate_cmd failure case..
> 
> vhost-net uses vhost_discard_vq_desc for some failure cases,  is that
> needed here for the failure cases before __copy_from_user is called..?

If you call vq_err, then you generally want to call
vhost_discard_vq_desc. This signals userspace that
it should stop vhost and dump the ring for debugging
(or in theory, recover and let vhost continue),
so you don't want to consume the problematic request.

This is the right thing to do for fatal things, e.g. ring errors,
but probably not scsi errors which need to be propagated to guest.

> > @@ -916,6 +916,11 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs,
> >  	}
> >  
> >  	mutex_unlock(&vq->mutex);
> > +	return;
> > +
> > +err:
> > +	vhost_scsi_free_cmd(tv_cmd);
> > +	mutex_unlock(&vq->mutex);
> >  }
> >  
> >  static void vhost_scsi_ctl_handle_kick(struct vhost_work *work)
>