[PATCH] dma-helpers: Fix iovec alignment

[PATCH] dma-helpers: Fix iovec alignment

[Stefan Fritsch]

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(-)

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);
     }
 
     dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,
-- 
2.43.0

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

[Philippe Mathieu-Daudé]

On 12/4/24 10:06, 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")

Cc'ing developers mentioned in this commit.

> Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
> ---
>   system/dma-helpers.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> 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);
>       }
>   
>       dbs->acb = dbs->io_func(dbs->offset, &dbs->iov,

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

[Eric Blake]

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).

> 
> 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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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>
> 
> 

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

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]

On Tue, Apr 16, 2024 at 7:54 AM Stefan Fritsch <sf@sfritsch.de> wrote:

> 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?
>

Whew, it's been such a long time since I've looked at this, I barely
remember. I think my email now knows more about the problem than *I*
currently do. I'm glad I wrote it ...

I do not currently know what happens if you fix the first issue but not the
other four; I don't know if it gets passably "less broken" or if it causes
other bigger issues... and I don't have the bandwidth to test that out
currently, my apologies.

If there's a renewed interest in fixing this, I can try to start ramping
back up on it, but I have a few other projects to finish first, but
possibly I could discuss this at KVM Forum to help delineate the work into
manageable chunks.

~js

[-- Attachment #2: Type: text/html, Size: 3321 bytes --]

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

[Stefan Fritsch]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Mon, 12 Aug 2024, John Snow wrote:

> On Tue, Apr 16, 2024 at 7:54 AM Stefan Fritsch <sf@sfritsch.de> wrote:
> 
> > 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?
> >
> 
> Whew, it's been such a long time since I've looked at this, I barely
> remember. I think my email now knows more about the problem than *I*
> currently do. I'm glad I wrote it ...
> 
> I do not currently know what happens if you fix the first issue but not the
> other four; I don't know if it gets passably "less broken" or if it causes
> other bigger issues... and I don't have the bandwidth to test that out
> currently, my apologies.
> 
> If there's a renewed interest in fixing this, I can try to start ramping
> back up on it, but I have a few other projects to finish first, but
> possibly I could discuss this at KVM Forum to help delineate the work into
> manageable chunks.

We came across this while chasing another bug, which turned out to be in 
other unrelated code. I also don't know if fixing this first of several 
issues improves the situation for people running an affected 
configuration. We only use qemu configurations where the broken code path 
is not hit. So, I am not sure this can be called renewed interest in 
fixing the issue. But if this is committed, I think there should be at 
least a comment pointing to the other issues.

Cheers,
Stefan

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

[Michael Tokarev]

12.04.2024 18:25, 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.

Hi!

Has this change (proposed for 9.0) been forgotten or is it not needed
anymore?

Thanks,

/mjt

>> 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).
> 
>>
>> 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>
> 

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

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

[Michael Tokarev]

11.08.2024 20:47, Michael Tokarev пишет:
> 12.04.2024 18:25, 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.
> 
> Hi!
> 
> Has this change (proposed for 9.0) been forgotten or is it not needed
> anymore?

Ping #2?

Thanks,

/mjt

>>> 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).
>>
>>>
>>> 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>
>>
> 


-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

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

[Peter Xu]

On Sat, Dec 21, 2024 at 12:07:05AM +0300, Michael Tokarev wrote:
> 11.08.2024 20:47, Michael Tokarev пишет:
> > 12.04.2024 18:25, 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.
> > 
> > Hi!
> > 
> > Has this change (proposed for 9.0) been forgotten or is it not needed
> > anymore?
> 
> Ping #2?

Hello, Michael,

Looks like we didn't reach a consensus on merging it?  See:

https://lore.kernel.org/qemu-devel/CAFn=p-Y1sAXfzx6cWEgeYyMyG3yn1vEpDmV2K8qWtLx4otm4tw@mail.gmail.com/

Thanks,

-- 
Peter Xu