| Message ID | 20230607185741.4238-1-faithilikerun@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] block/file-posix: fix wps checking in raw_co_prw | expand |
On 6/8/23 03:57, Sam Li wrote: > If the write operation fails and the wps is NULL, then accessing it will > lead to data corruption. > > Solving the issue by adding a nullptr checking in get_zones_wp() where > the wps is used. > > This issue is found by Peter Maydell using the Coverity Tool (CID > 1512459). > > Signed-off-by: Sam Li <faithilikerun@gmail.com> > --- > block/file-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index ac1ed54811..4a6c71c7f5 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -2523,7 +2523,7 @@ out: > } > } > } else { > - if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { > + if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) { > update_zones_wp(bs, s->fd, 0, 1); Nit: this could be: } else if (wps && type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { However, both if & else side do something only if the above condition is true and we only need to that for a zoned drive. So the entire code block could really be simplified to be a lot more readable. Something like this (totally untested, not even compiled): #if defined(CONFIG_BLKZONED) if (bs->bl.zone_size && (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) { BlockZoneWps *wps = bs->wps; uint64_t *wp; if (!wps) { return ret; } if (ret) { /* write error: update the wp from the underlying device */ update_zones_wp(bs, s->fd, 0, 1); goto unlock; } wp = &wps->wp[offset / bs->bl.zone_size]; if (BDRV_ZT_IS_CONV(*wp)) { /* Conventional zones do not have a write pointer */ goto unlock; } /* Return the written position for zone append */ if (type & QEMU_AIO_ZONE_APPEND) { *s->offset = *wp; trace_zbd_zone_append_complete(bs, *s->offset >> BDRV_SECTOR_BITS); } /* Advance the wp if needed */ if (offset + bytes > *wp) { *wp = offset + bytes; } unlock: qemu_co_mutex_unlock(&wps->colock); } #endif And making this entire block a helper function (e.g. advance_zone_wp()) would further clean the code. But that should be done in another patch. Care to send one ?
Damien Le Moal <dlemoal@kernel.org> 于2023年6月8日周四 09:29写道： > > On 6/8/23 03:57, Sam Li wrote: > > If the write operation fails and the wps is NULL, then accessing it will > > lead to data corruption. > > > > Solving the issue by adding a nullptr checking in get_zones_wp() where > > the wps is used. > > > > This issue is found by Peter Maydell using the Coverity Tool (CID > > 1512459). > > > > Signed-off-by: Sam Li <faithilikerun@gmail.com> > > --- > > block/file-posix.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index ac1ed54811..4a6c71c7f5 100644 > > --- a/block/file-posix.c > > +++ b/block/file-posix.c > > @@ -2523,7 +2523,7 @@ out: > > } > > } > > } else { > > - if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { > > + if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) { > > update_zones_wp(bs, s->fd, 0, 1); > > Nit: this could be: > > } else if (wps && type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { > > However, both if & else side do something only if the above condition is true > and we only need to that for a zoned drive. So the entire code block could > really be simplified to be a lot more readable. Something like this (totally > untested, not even compiled): > > #if defined(CONFIG_BLKZONED) > if (bs->bl.zone_size && (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) { > BlockZoneWps *wps = bs->wps; > uint64_t *wp; > > if (!wps) { > return ret; > } > > if (ret) { > /* write error: update the wp from the underlying device */ > update_zones_wp(bs, s->fd, 0, 1); > goto unlock; > } > > wp = &wps->wp[offset / bs->bl.zone_size]; > if (BDRV_ZT_IS_CONV(*wp)) { > /* Conventional zones do not have a write pointer */ > goto unlock; > } > > /* Return the written position for zone append */ > if (type & QEMU_AIO_ZONE_APPEND) { > *s->offset = *wp; > trace_zbd_zone_append_complete(bs, > *s->offset >> BDRV_SECTOR_BITS); > } > > /* Advance the wp if needed */ > if (offset + bytes > *wp) { > *wp = offset + bytes; > } > > unlock: > qemu_co_mutex_unlock(&wps->colock); > } > #endif > > And making this entire block a helper function (e.g. advance_zone_wp()) would > further clean the code. But that should be done in another patch. Care to send one ? Sure. If replacing the current code block by saying advance_zone_wp(), I guess this patch won't be necessary. So I will send another patch (advance_zone_wp()...) after testing. Sam
diff --git a/block/file-posix.c b/block/file-posix.c index ac1ed54811..4a6c71c7f5 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2523,7 +2523,7 @@ out: } } } else { - if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { + if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) { update_zones_wp(bs, s->fd, 0, 1); } }
If the write operation fails and the wps is NULL, then accessing it will lead to data corruption. Solving the issue by adding a nullptr checking in get_zones_wp() where the wps is used. This issue is found by Peter Maydell using the Coverity Tool (CID 1512459). Signed-off-by: Sam Li <faithilikerun@gmail.com> --- block/file-posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)