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

[Paolo Bonzini <pbonzini@redhat.com>]

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