On Thu, 25 Jul 2024 at 13:05, Salil Mehta <salil.mehta@xxxxxxxxxx> wrote:
>
> HI Peter,
>
> > From: Peter Maydell <peter.maydell@xxxxxxxxxx>
> > Sent: Thursday, July 25, 2024 11:36 AM
> > To: Michael S. Tsirkin <mst@xxxxxxxxxx>
> >
> > On Tue, 23 Jul 2024 at 11:58, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> > >
> > > From: Salil Mehta <salil.mehta@xxxxxxxxxx>
> > >
> > > 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
