netdev - Re: [PATCH RFC 11/13] vhost/scsi: switch to buf APIs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200605083616.GB59410@stefanha-x1.localdomain>
Date:   Fri, 5 Jun 2020 09:36:16 +0100
From:   Stefan Hajnoczi <stefanha@...il.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        Eugenio Pérez <eperezma@...hat.com>,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH RFC 11/13] vhost/scsi: switch to buf APIs

On Tue, Jun 02, 2020 at 09:06:20AM -0400, Michael S. Tsirkin wrote:
> Switch to buf APIs. Doing this exposes a spec violation in vhost scsi:
> all used bufs are marked with length 0.
> Fix that is left for another day.
> 
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> ---
>  drivers/vhost/scsi.c | 73 ++++++++++++++++++++++++++------------------
>  1 file changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index c39952243fd3..c426c4e899c7 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -71,8 +71,8 @@ struct vhost_scsi_inflight {
>  };
>  
>  struct vhost_scsi_cmd {
> -	/* Descriptor from vhost_get_vq_desc() for virt_queue segment */
> -	int tvc_vq_desc;
> +	/* Descriptor from vhost_get_avail_buf() for virt_queue segment */
> +	struct vhost_buf tvc_vq_desc;
>  	/* virtio-scsi initiator task attribute */
>  	int tvc_task_attr;
>  	/* virtio-scsi response incoming iovecs */
> @@ -213,7 +213,7 @@ struct vhost_scsi {
>   * Context for processing request and control queue operations.
>   */
>  struct vhost_scsi_ctx {
> -	int head;
> +	struct vhost_buf buf;
>  	unsigned int out, in;
>  	size_t req_size, rsp_size;
>  	size_t out_size, in_size;
> @@ -443,6 +443,20 @@ static int vhost_scsi_check_stop_free(struct se_cmd *se_cmd)
>  	return target_put_sess_cmd(se_cmd);
>  }
>  
> +/* Signal to guest that request finished with no input buffer. */
> +/* TODO calling this when writing into buffer and most likely a bug */
> +static void vhost_scsi_signal_noinput(struct vhost_dev *vdev,
> +				      struct vhost_virtqueue *vq,
> +				      struct vhost_buf *bufp)
> +{
> +	struct vhost_buf buf = *bufp;
> +
> +	buf.in_len = 0;
> +	vhost_put_used_buf(vq, &buf);

Yes, this behavior differs from the QEMU virtio-scsi device
implementation. I think it's just a quirk that is probably my fault (I
guess I thought the length information is already encoded in the payload
SCSI headers so we have no use for the used descriptor length field).

Whether it's worth changing now or is an interesting question. In theory
it would make vhost-scsi more spec compliant and guest drivers might be
happier (especially drivers for niche OSes that were only tested against
QEMU's virtio-scsi). On the other hand, it's a guest-visible change that
could break similar niche drivers that assume length is always 0.

I'd leave it as-is unless people hit issues that justify the risk of
changing it.

Reviewed-by: Stefan Hajnoczi <stefanha@...hat.com>

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)