On 8/26/26 05:57, Niklas Cassel wrote:
> All VIRTIO_BLK_T_OUT requests issued to sequential zones and all
> VIRTIO_BLK_T_ZONE_APPEND requests must have an offset and a data size
> that are multiples of the write granularity reported by the device
> (virtio 1.4, 5.2.6.1), and a violation is reported as
> VIRTIO_BLK_S_ZONE_UNALIGNED_WP (virtio 1.4, 5.2.6).
>
> Neither request type was fully checked. Zone appends validated only the
> offset, while writes were not checked at all.
>
> Check the size of the appended data, and both the offset and the size of
> a write, against blkconf_zone_write_granularity(), so that every request the
> device accepts is one that the guest driver was told is valid. Writes to
> conventional zones keep no alignment constraint beyond the logical block
> size. The write path performs the check after virtio_blk_sect_range_ok()
> so that the zone index derived from the guest supplied sector is known to
> be in range.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> hw/block/virtio-blk.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f8cda1baa7..7977f4abe5 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -522,7 +522,7 @@ static bool check_zoned_request(VirtIOBlock *s, int64_t
> offset, int64_t len,
> if (append) {
> uint32_t wg_mask = blkconf_zone_write_granularity(&s->conf.conf) - 1;
>
> - if (offset & wg_mask) {
> + if (offset & wg_mask || len & wg_mask) {
> *status = VIRTIO_BLK_S_ZONE_UNALIGNED_WP;
> return false;
> }
> @@ -911,6 +911,29 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> return 0;
> }
>
> + if (is_write) {
> + BlockDriverState *bs = blk_bs(s->blk);
> + int64_t offset = req->sector_num << BDRV_SECTOR_BITS;
> + uint32_t wg_mask =
> + blkconf_zone_write_granularity(&s->conf.conf) - 1;
Getting this without having first checked that this is a write to a zoned disk
is odd...
> +
> + /*
> + * Both the offset and the size of a write to a sequential zone
> + * must be a multiple of the write granularity reported by the
> + * device. Conventional zones are not constrained. The zone index
> + * is derived from a guest supplied sector, so this must come
> after
> + * virtio_blk_sect_range_ok() has bounded it.
> + */
> + if (bs->bl.zoned != BLK_Z_NONE &&
"!=" ??? Why ? The write granularity alignment applies only to zoned disks, no?
So why is this not "bs->bl.zoned == BLK_Z_NONE" ?
> + (offset & wg_mask || req->qiov.size & wg_mask) &&
> + !BDRV_ZT_IS_CONV(bs->wps->wp[offset / bs->bl.zone_size])) {
This condition is a little complex. A little inline helper
"virtio_blk_zoned_write_is_aligned()" or something like that would be nice.
> + virtio_blk_req_complete(req, VIRTIO_BLK_S_ZONE_UNALIGNED_WP);
> + block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
> + g_free(req);
> + return 0;
> + }
> + }
> +
> block_acct_start(blk_get_stats(s->blk), &req->acct, req->qiov.size,
> is_write ? BLOCK_ACCT_WRITE : BLOCK_ACCT_READ);
>
--
Damien Le Moal
Western Digital Research
