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

["Michael S. Tsirkin" <mst@redhat.com>]

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)
>