Re: [PATCH for-5.2 08/10] block/export: port virtio-blk discard/write zeroes input validation

[Max Reitz]

On 11.11.20 13:43, Stefan Hajnoczi wrote:
> Validate discard/write zeroes the same way we do for virtio-blk. Some of
> these checks are mandated by the VIRTIO specification, others are
> internal to QEMU.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/export/vhost-user-blk-server.c | 115 +++++++++++++++++++++------
>   1 file changed, 92 insertions(+), 23 deletions(-)
>
> diff --git a/block/export/vhost-user-blk-server.c 
> b/block/export/vhost-user-blk-server.c
> index 62672d1cb9..3295d3c951 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c

[...]

> @@ -58,30 +60,101 @@ static void vu_blk_req_complete(VuBlkReq *req)
>       free(req);
>   }
>   
> +static bool vu_blk_sect_range_ok(VuBlkExport *vexp, uint64_t sector,
> +                                 size_t size)
> +{
> +    uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
> +    uint64_t total_sectors;
> +
> +    if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {

I wonder why you pass a byte length, then shift it down to sectors, when 
it’s kind of unsafe on the caller’s side to even calculate that byte 
length (because the caller has to verify that shifting the sector count 
up to be a byte length is safe).

Wouldn’t it be simpler to pass nb_sectors (perhaps even as uint64_t, 
because why not) and then compare that against BDRV_REQUEST_MAX_BYTES here?

(With how the code is written, it also has the problem of rounding down 
@size.  Which isn’t a problem in practice because the caller effectively 
guarantees that @size is aligned to sectors, but it still means that the 
code looks a bit strange.  As in, “Why is it safe to round down?  Ah, 
because the caller always produces an aligned value.  But why does the 
caller even pass a byte count, then?  Especially given that the offset 
is passed as a sector index, too.”)

> +        return false;
> +    }
> +    if ((sector << BDRV_SECTOR_BITS) % vexp->blk_size) {

This made me wonder why the discard/write-zeroes sector granularity 
would be BDRV_SECTOR_BITS and not blk_size, which is used for IN/OUT 
(read/write) requests.

I still don’t know, but judging from the only reference I could find 
quickly (contrib/vhost-user-blk/vhost-user-blk.c), it seems correct.

OTOH, I wonder whether BDRV_SECTOR_BITS/BDRV_SECTOR_SIZE are the correct 
constants.  Isn’t it just 9/512 as per some specification, i.e., 
shouldn’t it be independent of qemu’s block layer’s sector size?

> +        return false;
> +    }
> +    blk_get_geometry(vexp->export.blk, &total_sectors);
> +    if (sector > total_sectors || nb_sectors > total_sectors - sector) {
> +        return false;
> +    }
> +    return true;
> +}
> +

[...]

> @@ -170,19 +243,13 @@ static void coroutine_fn vu_blk_virtio_process_req(void 
> *opaque)
>       }
>       case VIRTIO_BLK_T_DISCARD:
>       case VIRTIO_BLK_T_WRITE_ZEROES: {
> -        int rc;
> -
>           if (!vexp->writable) {
>               req->in->status = VIRTIO_BLK_S_IOERR;
>               break;
>           }
>   
> -        rc = vu_blk_discard_write_zeroes(blk, &elem->out_sg[1], out_num, type);
> -        if (rc == 0) {
> -            req->in->status = VIRTIO_BLK_S_OK;
> -        } else {
> -            req->in->status = VIRTIO_BLK_S_IOERR;
> -        }
> +        req->in->status = vu_blk_discard_write_zeroes(vexp, elem->out_sg,
> +                                                      out_num, type);

elem->out_sg is different from &elem->out_sg[1], but from what I can 
tell vu_blk_discard_write_zeroes() doesn’t really change in how it 
treats @iov.

I’m confused.  Is that a bug fix?  (If so, it isn’t mentioned in the 
commit message)

Apart from this, the patch looks good to me (although there are the two 
things mentioned above that I find a bit strange, but definitely not wrong).

Max

>           break;
>       }
>       default: