| Message ID | 20260902194423.759355-12-cassel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix the zone write granularity and the zone append limit | expand |
On Wed, Sep 02, 2026 at 09:44:22PM +0200, Niklas Cassel wrote: > raw_co_prw() replaces the offset of a zone append with the write pointer > of the addressed zone, which assumes that the stored value names a > position inside that zone. It does not in two cases. > > A full zone has its write pointer recorded at the end of the zone, since > get_zones_wp() stores start + len for BLK_ZONE_COND_FULL. That is the > first sector of the following zone, so the append is submitted there. The > kernel accepts it whenever that zone is empty, because it is a legal > write at its write pointer, and the completion path advances the wrong > zone because it recomputes the zone index from the replaced offset. The > data is written to a zone that was never addressed and success is > returned: > > zone 2 finished, then a 4 KiB append to zone 2: > After zap done, the append sector is 0x180000 <- zone 3 > zone 2: wptr 0x180000, zcond:14 (full) > zone 3: wptr 0x180008 <- advanced > > A conventional zone has no write pointer at all, and its array entry > carries only the type marker in the top bit, so the offset becomes > negative and the write fails with EINVAL. That is harmless but it reports > nothing about the actual mistake. > > Reject both while the write pointer lock is held, since the state has to > be read and acted on atomically. check_zoned_request() in virtio-blk > refuses an append to a conventional zone, so that case needs a caller > that goes to the driver directly, but nothing there examines whether a > zone is full, so a guest can reach the misdirected write. > > Fixes: 4751d09adcc3 ("block: introduce zone append write for zoned devices") > Signed-off-by: Niklas Cassel <cassel@kernel.org> Could you please review this patch? I did not pick up your tag on v1 because I added a helper as you requested. > --- > block/file-posix.c | 27 +++++++++++++++++++++++++-- > block/io.c | 9 +++++++++ > include/block/block-io.h | 6 ++++++ > 3 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 85d735c079..57ba8dde64 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -2565,8 +2565,31 @@ raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, uint64_t bytes, > bs->bl.zoned != BLK_Z_NONE) { > qemu_co_mutex_lock(&bs->wps->colock); > if (type & QEMU_AIO_ZONE_APPEND) { > - int index = offset / bs->bl.zone_size; > - offset = bs->wps->wp[index]; > + uint32_t index = offset / bs->bl.zone_size; > + uint64_t wp = bs->wps->wp[index]; > + > + /* > + * The write pointer of the addressed zone becomes the offset of > + * the write, so it has to name a position inside that zone. It > + * does not for a conventional zone, which has no write pointer and > + * stores a type marker in the top bit instead, and it does not for > + * a full zone, whose write pointer is reported at the zone end. > + * Either would send the data to a zone that was never addressed. > + */ > + if (BDRV_ZT_IS_CONV(wp)) { > + error_report("zone append at offset 0x%" PRIx64 " addresses a " > + "conventional zone", offset); > + qemu_co_mutex_unlock(&bs->wps->colock); > + return -EINVAL; > + } > + if (bdrv_zone_is_full(bs, index)) { > + error_report("zone append at offset 0x%" PRIx64 " addresses a " > + "full zone", offset); > + qemu_co_mutex_unlock(&bs->wps->colock); > + return -ENOSPC; > + } > + > + offset = wp; > } > } > #endif > diff --git a/block/io.c b/block/io.c > index 705dc73d76..3574e15a55 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -3371,6 +3371,15 @@ out: > return co.ret; > } > > +bool bdrv_zone_is_full(BlockDriverState *bs, uint32_t index) > +{ > + uint64_t zone_end = MIN((uint64_t)(index + 1) * bs->bl.zone_size, > + (uint64_t)bs->total_sectors << BDRV_SECTOR_BITS); > + IO_CODE(); > + > + return bs->wps->wp[index] >= zone_end; > +} > + > void *qemu_blockalign(BlockDriverState *bs, size_t size) > { > IO_CODE(); > diff --git a/include/block/block-io.h b/include/block/block-io.h > index d34d846bb2..2a9505312a 100644 > --- a/include/block/block-io.h > +++ b/include/block/block-io.h > @@ -126,6 +126,12 @@ int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_append(BlockDriverState *bs, > int64_t *offset, > QEMUIOVector *qiov, > BdrvRequestFlags flags); > +/* > + * True when the write pointer of a zone has reached the end of the writable > + * part of that zone, so that nothing more can be written to it until it is > + * reset. The write pointer lock must be held when called. > + */ > +bool bdrv_zone_is_full(BlockDriverState *bs, uint32_t index); > > bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs); > > -- > 2.55.0 >
diff --git a/block/file-posix.c b/block/file-posix.c index 85d735c079..57ba8dde64 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2565,8 +2565,31 @@ raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, uint64_t bytes, bs->bl.zoned != BLK_Z_NONE) { qemu_co_mutex_lock(&bs->wps->colock); if (type & QEMU_AIO_ZONE_APPEND) { - int index = offset / bs->bl.zone_size; - offset = bs->wps->wp[index]; + uint32_t index = offset / bs->bl.zone_size; + uint64_t wp = bs->wps->wp[index]; + + /* + * The write pointer of the addressed zone becomes the offset of + * the write, so it has to name a position inside that zone. It + * does not for a conventional zone, which has no write pointer and + * stores a type marker in the top bit instead, and it does not for + * a full zone, whose write pointer is reported at the zone end. + * Either would send the data to a zone that was never addressed. + */ + if (BDRV_ZT_IS_CONV(wp)) { + error_report("zone append at offset 0x%" PRIx64 " addresses a " + "conventional zone", offset); + qemu_co_mutex_unlock(&bs->wps->colock); + return -EINVAL; + } + if (bdrv_zone_is_full(bs, index)) { + error_report("zone append at offset 0x%" PRIx64 " addresses a " + "full zone", offset); + qemu_co_mutex_unlock(&bs->wps->colock); + return -ENOSPC; + } + + offset = wp; } } #endif diff --git a/block/io.c b/block/io.c index 705dc73d76..3574e15a55 100644 --- a/block/io.c +++ b/block/io.c @@ -3371,6 +3371,15 @@ out: return co.ret; } +bool bdrv_zone_is_full(BlockDriverState *bs, uint32_t index) +{ + uint64_t zone_end = MIN((uint64_t)(index + 1) * bs->bl.zone_size, + (uint64_t)bs->total_sectors << BDRV_SECTOR_BITS); + IO_CODE(); + + return bs->wps->wp[index] >= zone_end; +} + void *qemu_blockalign(BlockDriverState *bs, size_t size) { IO_CODE(); diff --git a/include/block/block-io.h b/include/block/block-io.h index d34d846bb2..2a9505312a 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -126,6 +126,12 @@ int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_append(BlockDriverState *bs, int64_t *offset, QEMUIOVector *qiov, BdrvRequestFlags flags); +/* + * True when the write pointer of a zone has reached the end of the writable + * part of that zone, so that nothing more can be written to it until it is + * reset. The write pointer lock must be held when called. + */ +bool bdrv_zone_is_full(BlockDriverState *bs, uint32_t index); bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
raw_co_prw() replaces the offset of a zone append with the write pointer of the addressed zone, which assumes that the stored value names a position inside that zone. It does not in two cases. A full zone has its write pointer recorded at the end of the zone, since get_zones_wp() stores start + len for BLK_ZONE_COND_FULL. That is the first sector of the following zone, so the append is submitted there. The kernel accepts it whenever that zone is empty, because it is a legal write at its write pointer, and the completion path advances the wrong zone because it recomputes the zone index from the replaced offset. The data is written to a zone that was never addressed and success is returned: zone 2 finished, then a 4 KiB append to zone 2: After zap done, the append sector is 0x180000 <- zone 3 zone 2: wptr 0x180000, zcond:14 (full) zone 3: wptr 0x180008 <- advanced A conventional zone has no write pointer at all, and its array entry carries only the type marker in the top bit, so the offset becomes negative and the write fails with EINVAL. That is harmless but it reports nothing about the actual mistake. Reject both while the write pointer lock is held, since the state has to be read and acted on atomically. check_zoned_request() in virtio-blk refuses an append to a conventional zone, so that case needs a caller that goes to the driver directly, but nothing there examines whether a zone is full, so a guest can reach the misdirected write. Fixes: 4751d09adcc3 ("block: introduce zone append write for zoned devices") Signed-off-by: Niklas Cassel <cassel@kernel.org> --- block/file-posix.c | 27 +++++++++++++++++++++++++-- block/io.c | 9 +++++++++ include/block/block-io.h | 6 ++++++ 3 files changed, 40 insertions(+), 2 deletions(-)