Re: [PATCH v2] block/file-posix: fix wps checking in raw_co_prw

[Damien Le Moal]

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
Western Digital Research