| Message ID | 1365498993-9688-1-git-send-email-asias@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(-)
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) > >
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(-)
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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!
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) > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-)