Re: [PATCH] dma-helpers: Fix iovec alignment

[Stefan Fritsch]

adding John Snow to CC because he investigated this in 2020.

On Fri, 12 Apr 2024, Eric Blake wrote:

> On Fri, Apr 12, 2024 at 10:06:17AM +0200, Stefan Fritsch wrote:
> > Commit 99868af3d0 changed the hardcoded constant BDRV_SECTOR_SIZE to a
> > dynamic field 'align' but introduced a bug. qemu_iovec_discard_back()
> > is now passed the wanted iov length instead of the actually required
> > amount that should be removed from the end of the iov.
> > 
> > The bug can likely only be hit in uncommon configurations, e.g. with
> > icount enabled or when reading from disk directly to device memory.
> > 
> > Fixes: 99868af3d0a75cf6 ("dma-helpers: explicitly pass alignment into DMA 
> > helpers")
> > Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> > ---
> >  system/dma-helpers.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> Wow, that bug has been latent for a while (2016, v2.8.0).  As such, I
> don't think it's worth holding up 9.0 for, but it is definitely worth
> cc'ing qemu-stable (done now).

we had some more internal discussions and did some more googling, and it 
turns out that this is actually part of a known issue:

https://lists.nongnu.org/archive/html/qemu-block/2020-07/msg01866.html
https://gitlab.com/qemu-project/qemu/-/issues/259

In the above mail to qemu-block, John Snow listed 4 problems in the code 
but my patch only fixes the first one. Considering that the code may also 
write data to the wrong place (problem 2), I wonder if an assert in the 
same place would be better until the underlying issues have been fixed?


> > 
> > diff --git a/system/dma-helpers.c b/system/dma-helpers.c
> > index 9b221cf94e..c9677fd39b 100644
> > --- a/system/dma-helpers.c
> > +++ b/system/dma-helpers.c
> > @@ -174,8 +174,7 @@ static void dma_blk_cb(void *opaque, int ret)
> >      }
> >  
> >      if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) {
> > -        qemu_iovec_discard_back(&dbs->iov,
> > -                                QEMU_ALIGN_DOWN(dbs->iov.size, 
> > dbs->align));
> > +        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size % dbs->align);
> 
> Before the regression, it was:
> 
> -    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
> -        qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & 
> ~BDRV_SECTOR_MASK);
> +    if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) {
> +        qemu_iovec_discard_back(&dbs->iov,
> 
> If dbs->align is always a power of two, we can use '& (dbs->align -
> 1)' to avoid a hardware division, to match the original code; bug
> QEMU_IS_ALIGNED does not require a power of two, so your choice of '%
> dbs->align' seems reasonable.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>