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

[Peter Maydell]

On Thu, 25 Jul 2024 at 13:05, Salil Mehta <salil.mehta@huawei.com> wrote:
>
> HI Peter,
>
> >  From: Peter Maydell <peter.maydell@linaro.org>
> >  Sent: Thursday, July 25, 2024 11:36 AM
> >  To: Michael S. Tsirkin <mst@redhat.com>
> >
> >  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?
>
>
> Thanks for identifying this. Yes, a  break is missing. Should I send a fix 
> for this
> now or you can incorporate it?

The code is already in upstream git, so please send a patch
to fix the bug.

thanks
-- PMM