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

[Sam Li]

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