* Srivatsa Vaddagiri <quic_svaddagi@xxxxxxxxxxx> [2023-02-20 14:45:55]:
> * Elliot Berman <quic_eberman@xxxxxxxxxxx> [2023-02-14 13:24:26]:
>
> > static void gh_vm_free(struct work_struct *work)
> > {
> > struct gh_vm *ghvm = container_of(work, struct gh_vm, free_work);
> > struct gh_vm_mem *mapping, *tmp;
> > int ret;
> >
> > - mutex_lock(&ghvm->mm_lock);
> > - list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, list) {
> > - gh_vm_mem_reclaim(ghvm, mapping);
> > - kfree(mapping);
> > + switch (ghvm->vm_status) {
> > +unknown_state:
> > + case GH_RM_VM_STATUS_RUNNING:
> > + gh_vm_stop(ghvm);
> > + fallthrough;
> > + case GH_RM_VM_STATUS_INIT_FAILED:
> > + case GH_RM_VM_STATUS_LOAD:
> > + case GH_RM_VM_STATUS_LOAD_FAILED:
> > + mutex_lock(&ghvm->mm_lock);
> > + list_for_each_entry_safe(mapping, tmp, &ghvm->memory_mappings, list) {
> > + gh_vm_mem_reclaim(ghvm, mapping);
> > + kfree(mapping);
> > + }
> > + mutex_unlock(&ghvm->mm_lock);
> > + fallthrough;
> > + case GH_RM_VM_STATUS_NO_STATE:
> > + ret = gh_rm_dealloc_vmid(ghvm->rm, ghvm->vmid);
> > + if (ret)
> > + pr_warn("Failed to deallocate vmid: %d\n", ret);
> > +
> > + gh_rm_notifier_unregister(ghvm->rm, &ghvm->nb);
> > + put_gh_rm(ghvm->rm);
> > + kfree(ghvm);
> > + break;
> > + default:
> > + pr_err("VM is unknown state: %d, assuming it's running.\n", ghvm->vm_status);
> > + goto unknown_state;
>
> 'goto unknown_state' here leads to a infinite loop AFAICS. For example consider
> the case where VM_START failed (due to mem_lend operation) causing VM state to
> be GH_RM_VM_STATUS_RESET. A subsequent close(vmfd) can leads to that forever
> loop.
Hmm ..that's not a good example perhaps (VM state is set to
GH_RM_VM_STATUS_INIT_FAILED in failed case). Nevertheless I think we should
avoid the goto in case of unknown state.
- vatsa
