[PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Zhenzhong Duan]

After CPR transfer, source QEMU close kvm fd and free kvm_state,
"query-balloon" will check kvm_state->sync_mmu and trigger NULL
pointer reference.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 accel/kvm/kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 9060599cd7..a3e2d11763 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3479,7 +3479,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
 
 bool kvm_has_sync_mmu(void)
 {
-    return kvm_state->sync_mmu;
+    return kvm_state && kvm_state->sync_mmu;
 }
 
 int kvm_has_vcpu_events(void)
-- 
2.47.1

Re: [PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Markus Armbruster]

Zhenzhong Duan <zhenzhong.duan@intel.com> writes:

> After CPR transfer, source QEMU close kvm fd and free kvm_state,
> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
> pointer reference.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  accel/kvm/kvm-all.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 9060599cd7..a3e2d11763 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3479,7 +3479,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
>  
>  bool kvm_has_sync_mmu(void)
>  {
> -    return kvm_state->sync_mmu;
> +    return kvm_state && kvm_state->sync_mmu;
>  }
>  
>  int kvm_has_vcpu_events(void)

This dereference could signify there's a general assumption *kvm_state
is valid, i.e. there might be more dereferences hiding in the code.

Have you checked?

Is freeing @kvm_state after CPR transfer useful?

Re: [PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Steven Sistare]

On 9/26/2025 12:48 AM, Markus Armbruster wrote:
> Zhenzhong Duan <zhenzhong.duan@intel.com> writes:
> 
>> After CPR transfer, source QEMU close kvm fd and free kvm_state,
>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>> pointer reference.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   accel/kvm/kvm-all.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 9060599cd7..a3e2d11763 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3479,7 +3479,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
>>   
>>   bool kvm_has_sync_mmu(void)
>>   {
>> -    return kvm_state->sync_mmu;
>> +    return kvm_state && kvm_state->sync_mmu;
>>   }
>>   
>>   int kvm_has_vcpu_events(void)
> 
> This dereference could signify there's a general assumption *kvm_state
> is valid, i.e. there might be more dereferences hiding in the code.
> 
> Have you checked?
> 
> Is freeing @kvm_state after CPR transfer useful?

There are other references to kvm_state which are checked by earlier cpr patches.
The risk/consequence of missing something is low, because this only occurs on source
QEMU after the target is live.

The KVM instance is closed to release KVM page table references and speed execv
during cpr-exec mode.

- Steve

RE: [PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Markus Armbruster <armbru@redhat.com>
>Subject: Re: [PATCH 5/5] accel/kvm: Fix SIGSEGV when execute
>"query-balloon" after CPR transfer
>
>Zhenzhong Duan <zhenzhong.duan@intel.com> writes:
>
>> After CPR transfer, source QEMU close kvm fd and free kvm_state,
>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>> pointer reference.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  accel/kvm/kvm-all.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index 9060599cd7..a3e2d11763 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -3479,7 +3479,7 @@ int kvm_device_access(int fd, int group, uint64_t
>attr,
>>
>>  bool kvm_has_sync_mmu(void)
>>  {
>> -    return kvm_state->sync_mmu;
>> +    return kvm_state && kvm_state->sync_mmu;
>>  }
>>
>>  int kvm_has_vcpu_events(void)
>
>This dereference could signify there's a general assumption *kvm_state
>is valid, i.e. there might be more dereferences hiding in the code.

Yes, agree with your concern, let me pursues other way.
>
>Have you checked?
>
>Is freeing @kvm_state after CPR transfer useful?

kvm_state was NULLed, let me try to keep it for query just like in kernel
task struct is retained when child process exit.

I'd like to add your Suggested-by on this if you don't object.

Thanks
Zhenzhong

Re: [PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Markus Armbruster]

"Duan, Zhenzhong" <zhenzhong.duan@intel.com> writes:

>>-----Original Message-----
>>From: Markus Armbruster <armbru@redhat.com>

[...]

>>Is freeing @kvm_state after CPR transfer useful?
>
> kvm_state was NULLed, let me try to keep it for query just like in kernel
> task struct is retained when child process exit.
>
> I'd like to add your Suggested-by on this if you don't object.

I don't :)

Re: [PATCH 5/5] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Steven Sistare]

On 9/25/2025 10:25 PM, Zhenzhong Duan wrote:
> After CPR transfer, source QEMU close kvm fd and free kvm_state,
> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
> pointer reference.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Steve Sistare <steven.sistare@oracle.com>

> ---
>   accel/kvm/kvm-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 9060599cd7..a3e2d11763 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3479,7 +3479,7 @@ int kvm_device_access(int fd, int group, uint64_t attr,
>   
>   bool kvm_has_sync_mmu(void)
>   {
> -    return kvm_state->sync_mmu;
> +    return kvm_state && kvm_state->sync_mmu;
>   }
>   
>   int kvm_has_vcpu_events(void)