[PATCH] KVM: Fix dirty ring mmap incorrect size due to renaming accident

[PATCH] KVM: Fix dirty ring mmap incorrect size due to renaming accident

[Peter Xu]

Found this when I wanted to try the per-vcpu dirty rate series out, then I
found that it's not really working and it can quickly hang death a guest.  I
found strange errors (e.g. guest crash after migration) happens even without
the per-vcpu dirty rate series.

When merging dirty ring, probably no one notice that the trivial renaming diff
[1] missed two existing references of kvm_dirty_ring_sizes; they do matter
since otherwise we'll mmap() a shorter range of memory after the renaming.

I think it didn't SIGBUS for me easily simply because some other stuff within
qemu mmap()ed right after the dirty rings (e.g. when testing 4096 slots, it
aligned with one small page on x86), so when we access the rings we've been
reading/writting to random memory elsewhere of qemu.

Fix the two sizes when map/unmap the shared dirty gfn memory.

[1] https://lore.kernel.org/qemu-devel/dac5f0c6-1bca-3daf-e5d2-6451dbbaca93@redhat.com/

Cc: Hyman Huang <huangy81@chinatelecom.cn>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 accel/kvm/kvm-all.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c7ec5388500..e5b10dd129c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -411,7 +411,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
     }
 
     if (cpu->kvm_dirty_gfns) {
-        ret = munmap(cpu->kvm_dirty_gfns, s->kvm_dirty_ring_size);
+        ret = munmap(cpu->kvm_dirty_gfns, s->kvm_dirty_ring_bytes);
         if (ret < 0) {
             goto err;
         }
@@ -495,7 +495,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
 
     if (s->kvm_dirty_ring_size) {
         /* Use MAP_SHARED to share pages with the kernel */
-        cpu->kvm_dirty_gfns = mmap(NULL, s->kvm_dirty_ring_size,
+        cpu->kvm_dirty_gfns = mmap(NULL, s->kvm_dirty_ring_bytes,
                                    PROT_READ | PROT_WRITE, MAP_SHARED,
                                    cpu->kvm_fd,
                                    PAGE_SIZE * KVM_DIRTY_LOG_PAGE_OFFSET);
-- 
2.31.1

Re: [PATCH] KVM: Fix dirty ring mmap incorrect size due to renaming accident

[Paolo Bonzini]

On 09/06/21 03:43, Peter Xu wrote:
> Found this when I wanted to try the per-vcpu dirty rate series out, then I
> found that it's not really working and it can quickly hang death a guest.  I
> found strange errors (e.g. guest crash after migration) happens even without
> the per-vcpu dirty rate series.
> 
> When merging dirty ring, probably no one notice that the trivial renaming diff
> [1] missed two existing references of kvm_dirty_ring_sizes; they do matter
> since otherwise we'll mmap() a shorter range of memory after the renaming.
> 
> I think it didn't SIGBUS for me easily simply because some other stuff within
> qemu mmap()ed right after the dirty rings (e.g. when testing 4096 slots, it
> aligned with one small page on x86), so when we access the rings we've been
> reading/writting to random memory elsewhere of qemu.
> 
> Fix the two sizes when map/unmap the shared dirty gfn memory.
> 
> [1] https://lore.kernel.org/qemu-devel/dac5f0c6-1bca-3daf-e5d2-6451dbbaca93@redhat.com/
> 
> Cc: Hyman Huang <huangy81@chinatelecom.cn>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   accel/kvm/kvm-all.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c7ec5388500..e5b10dd129c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -411,7 +411,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>       }
>   
>       if (cpu->kvm_dirty_gfns) {
> -        ret = munmap(cpu->kvm_dirty_gfns, s->kvm_dirty_ring_size);
> +        ret = munmap(cpu->kvm_dirty_gfns, s->kvm_dirty_ring_bytes);
>           if (ret < 0) {
>               goto err;
>           }
> @@ -495,7 +495,7 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>   
>       if (s->kvm_dirty_ring_size) {
>           /* Use MAP_SHARED to share pages with the kernel */
> -        cpu->kvm_dirty_gfns = mmap(NULL, s->kvm_dirty_ring_size,
> +        cpu->kvm_dirty_gfns = mmap(NULL, s->kvm_dirty_ring_bytes,
>                                      PROT_READ | PROT_WRITE, MAP_SHARED,
>                                      cpu->kvm_fd,
>                                      PAGE_SIZE * KVM_DIRTY_LOG_PAGE_OFFSET);
> 

Oops, sorry.  I queued it.

Paolo