Messages in this thread | | | Date | Tue, 28 Mar 2023 18:04:32 +0530 | From | Kautuk Consul <> | Subject | Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for kzalloc failure |
| |
On 2023-03-28 23:02:09, Michael Ellerman wrote: > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes: > > On 2023-03-28 15:44:02, Kautuk Consul wrote: > >> On 2023-03-28 20:44:48, Michael Ellerman wrote: > >> > Kautuk Consul <kconsul@linux.vnet.ibm.com> writes: > >> > > kvmppc_vcore_create() might not be able to allocate memory through > >> > > kzalloc. In that case the kvm->arch.online_vcores shouldn't be > >> > > incremented. > >> > > >> > I agree that looks wrong. > >> > > >> > Have you tried to test what goes wrong if it fails? It looks like it > >> > will break the LPCR update, which likely will cause the guest to crash > >> > horribly. > > Also, are you referring to the code in kvmppc_update_lpcr()? > > That code will not crash as it checks for the vc before trying to > > dereference it. > > Yeah that's what I was looking at. I didn't mean it would crash, but > that it would bail out early when it sees a NULL vcore, leaving other > vcores with the wrong LPCR value. > > But as you say it doesn't happen because qemu quits on the first ENOMEM. > > And regardless if qemu does something that means the guest is broken > that's just a qemu bug, no big deal as far as the kernel is concerned. But there could be another user-mode application other than qemu that actually tries to create a vcpu after it gets a -ENOMEM for another vcpu. Shouldn't the kernel be independent of qemu? > > > But the following 2 places that utilize the arch.online_vcores will have > > problems in logic if the usermode test-case doesn't pull down the > > kvm context after the -ENOMEM vcpu allocation failure: > > book3s_hv.c:3030: if (!kvm->arch.online_vcores) { > > book3s_hv_rm_mmu.c:44: if (kvm->arch.online_vcores == 1 && local_paca->kvm_hstate.kvm_vcpu) > > OK. Both of those look harmless to the host. Harmless to the host in terms of a crash, not in terms of behavior. For example in the case of kvmhv_set_smt_mode: If we got a kzalloc failure once (and online_vcores was wrongly incremented), then if kvmhv_set_smt_mode() is called after that then it would be not be setting the arch.smt_mode and arch.emul_smt_mode correctly and it would be wrongly returning with -EBUSY instead of 0. Isn't that incorrect with respect to the intent of the code ? I agree that applications like qemu might not do that but don't we need to have some integrity with respect to the intent and value of variable use ? What about good code and logic quality ? > > If we find a case where a misbehaving qemu can crash the host then we > need to be a bit more careful and treat it at least as a > denial-of-service bug. But looks like this is not one of those. > > cheers beers
| |