1. Patchew
  2. QEMU
  3. View series

[PATCH] dma-helpers: Fix iovec alignment

Stefan Fritsch posted 1 patch 1 year, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240412080617.1299883-1-sf@sfritsch.de
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
system/dma-helpers.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[PATCH] dma-helpers: Fix iovec alignment
Posted by Stefan Fritsch 1 year, 10 months ago 
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
Posted by Eric Blake 1 year, 10 months ago 
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
Posted by Michael Tokarev 1 year, 6 months ago 
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
Posted by Michael Tokarev 1 year, 1 month ago 
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
Posted by Peter Xu 1 year, 1 month ago 
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
Re: [PATCH] dma-helpers: Fix iovec alignment
Posted by Stefan Fritsch 1 year, 9 months ago 
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
Posted by John Snow 1 year, 6 months ago 
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
Re: [PATCH] dma-helpers: Fix iovec alignment
Posted by Stefan Fritsch 1 year, 5 months ago 
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
Posted by Philippe Mathieu-Daudé 1 year, 10 months ago 
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,

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.