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 <stefa...@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:
