On 8/28/26 15:07, Damien Le Moal wrote:
> 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" ?
Doh! My bad. I was reading "NONE" as "ZONE"... 90 degrees off on the first
letter rotation :)
>
>> + (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
