Re: [Qemu-devel] [PATCH] vhost-user-blk: enable discard/write zeroes features

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]

On Mon, Jan 14, 2019 at 03:35:17PM +0800, Changpeng Liu wrote:
> Linux commit 1f23816b8 "virtio_blk: add discard and write zeroes support"
> added the support in the Guest kernel, while here enable the feature bits
> support with vhost-user-blk driver.  Also enable the test example utility
> with DISCARD command support.

The commit message mentions write zeroes but this patch only covers
discard.  Will you send a separate patch for write zeros?

> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>

CCed Stefano, who is working on hw/block/virtio-blk.c emulation support.

> @@ -157,6 +161,29 @@ vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt)
>      return rc;
>  }
>  
> +static int
> +vub_discard(VubReq *req, struct iovec *iov, uint32_t iovcnt)
> +{
> +    if (iovcnt != 1) {

This is a virtio specification violation.  The iovec layout is
intentionally not part of the specification, leaving the guest driver
free to choose its preferred layout.

The device backend must accept any layout, including splitting a struct
across iovecs or even many small iovecs of just 1 byte!

> +        fprintf(stderr, "Invalid Discard IOV count\n");
> +        return -1;
> +    }
> +
> +    #if defined(__linux__) && defined(BLKDISCARD)
> +    VubDev *vdev_blk = req->vdev_blk;
> +    struct virtio_blk_discard_write_zeroes *desc =
> +                       (struct virtio_blk_discard_write_zeroes *)iov->iov_base;

Missing input size check.  Even if this example isn't used in
production, it's important to validate inputs since other people will
implement their vhost-user backend based on this example.

Please check that sizeof(*desc) bytes are really available before
accessing it.

> +    case VIRTIO_BLK_T_DISCARD: {
> +        int rc;
> +        rc = vub_discard(req, &elem->out_sg[1], out_num);
> +        if (rc == 0) {
>              req->in->status = VIRTIO_BLK_S_OK;
> -            req->size = elem->in_sg[0].iov_len;
> -            vub_req_complete(req);
> -            break;
> -        }
> -        default: {
> +        } else {
>              req->in->status = VIRTIO_BLK_S_UNSUPP;

Is there no IOERR case?  BLKDISCARD can probably fail due to an I/O
error and that shouldn't be reported as UNSUPP.

> @@ -454,7 +492,7 @@ vub_get_blocksize(int fd)
>  
>  #if defined(__linux__) && defined(BLKSSZGET)
>      if (ioctl(fd, BLKSSZGET, &blocksize) == 0) {
> -        return blocklen;
> +        return blocksize;
>      }
>  #endif

Unrelated bug fix?  Please submit a separate patch.

Do you know why the patchew, Travis, etc continuous integration systems
didn't detect the compile error?  Please ensure that
contrib/vhost-user-blk/ is covered by CI.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]