Re: [PULL v2 37/61] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Peter Maydell]

On Tue, 23 Jul 2024 at 11:58, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Salil Mehta <salil.mehta@huawei.com>
>
> KVM vCPU creation is done once during the vCPU realization when Qemu vCPU 
> thread
> is spawned. This is common to all the architectures as of now.
>
> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
> support vCPU removal. Therefore, its representative KVM vCPU object/context in
> Qemu is parked.
>
> Refactor architecture common logic so that some APIs could be reused by vCPU
> Hotplug code of some architectures likes ARM, Loongson etc. Update new/old 
> APIs
> with trace events. New APIs qemu_{create,park,unpark}_vcpu() can be externally
> called. No functional change is intended here.

Hi; Coverity points out an issue with this code (CID 1558552):

> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
> +{
> +    struct KVMParkedVcpu *cpu;
> +    int kvm_fd = -ENOENT;
> +
> +    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> +        if (cpu->vcpu_id == vcpu_id) {
> +            QLIST_REMOVE(cpu, node);
> +            kvm_fd = cpu->kvm_fd;
> +            g_free(cpu);
> +        }
> +    }

If you are going to remove an entry from a list as you
iterate over it, you can't use QLIST_FOREACH(), because
QLIST_FOREACH will look at the next pointer of the
iteration variable at the end of the loop when it
wants to advance to the next node. In this case we've
already freed 'cpu', so it would be reading freed memory.

Should we break out of the loop when we find the entry?

If we do need to continue iteration after removing the
list node, you need to use QLIST_FOREACH_SAFE() to do
the list iteration.

> -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> -{
> -    struct KVMParkedVcpu *cpu;
> -
> -    QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> -        if (cpu->vcpu_id == vcpu_id) {
> -            int kvm_fd;
> -
> -            QLIST_REMOVE(cpu, node);
> -            kvm_fd = cpu->kvm_fd;
> -            g_free(cpu);
> -            return kvm_fd;

In this old piece of code we were OK using QLIST_FOREACH
because we returned immediately we took the node off
the list and didn't continue the iteration.

> -        }
> -    }
> -
> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> -}

thanks
-- PMM