Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Michael Ellerman <mpe@ellerman.id.au>
To:     Kautuk Consul <kconsul@linux.vnet.ibm.com>
Cc:     Nicholas Piggin <npiggin@gmail.com>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Fabiano Rosas <farosas@linux.ibm.com>,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arch/powerpc/kvm: kvmppc_core_vcpu_create_hv: check for
 kzalloc failure
In-Reply-To: <ZCLHFw1U4Mq/QK2A@li-a450e7cc-27df-11b2-a85c-b5a9ac31e8ef.ibm.com>
References: <20230323074718.2810914-1-kconsul@linux.vnet.ibm.com>
 <87pm8tcir3.fsf@mpe.ellerman.id.au>
 <ZCK96ohvWRY12zZ3@li-a450e7cc-27df-11b2-a85c-b5a9ac31e8ef.ibm.com>
 <ZCLHFw1U4Mq/QK2A@li-a450e7cc-27df-11b2-a85c-b5a9ac31e8ef.ibm.com>
Date:   Tue, 28 Mar 2023 23:02:09 +1100
Message-ID: <87fs9pcce6.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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 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.

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