From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Ellerman Subject: Re: [PATCH] KVM: x86: Avoid NULL dereference in kvm_apic_accept_pic_intr() Date: Wed, 08 Feb 2012 21:41:28 +1100 Message-ID: <1328697688.3308.4.camel@concordia> References: <1328596327-18662-1-git-send-email-michael@ellerman.id.au> <20120207193839.GA20281@amt.cnet> Reply-To: michael@ellerman.id.au Mime-Version: 1.0 Content-Type: multipart/signed; micalg="pgp-sha256"; protocol="application/pgp-signature"; boundary="=-wP/ibVg5xKQsaHGFFUfW" Cc: kvm@vger.kernel.org, avi@redhat.com To: Marcelo Tosatti Return-path: Received: from ozlabs.org ([203.10.76.45]:54201 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750872Ab2BHKlm (ORCPT ); Wed, 8 Feb 2012 05:41:42 -0500 In-Reply-To: <20120207193839.GA20281@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: --=-wP/ibVg5xKQsaHGFFUfW Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2012-02-07 at 17:38 -0200, Marcelo Tosatti wrote: > On Tue, Feb 07, 2012 at 05:32:07PM +1100, Michael Ellerman wrote: > > A test case which does the following: > >=20 > > ioctl(vmfd, KVM_CREATE_VCPU, 0); > > ioctl(vmfd, KVM_CREATE_IRQCHIP); > > ioctl(cpufd, KVM_RUN); > >=20 > > Can oops in kvm_apic_accept_pic_intr() because vcpu->arch.apic =3D=3D N= ULL. > >=20 > > Because irqchip_in_kernel() is false when we create the vcpu we leave > > vcpu->arch.apic uninitialised (in kvm_arch_vcpu_init()). Then when we r= un, > > irqchip_in_kernel() is true, but we didn't do the correct initialisatio= n. > >=20 > > The root of the problem seems to be that there is an assumption that > > KVM_CREATE_IRQCHIP will be called before any VCPUs are created. The > > documentation says "sets up future vcpus to have a local APIC". > >=20 > > So the simplest fix seems to be to enforce that ordering in the code. >=20 > Ugh. With your patch below there is still the window for a race: >=20 > kvm_arch_vcpu_init can create a vcpu without vcpu->arch.apic,=20 > block on mutex_lock(kvm->lock). Meanwhile a separate thread is on > KVM_CREATE_IRQCHIP holding kvm->lock, finds online_vcpus =3D=3D 0 and=20 > proceeds. Then kvm_arch_vcpu_init finishes. Yeah bugger. I missed that most of the vcpu create is done without the mutex held. > Moving mutex_lock(kvm->lock) to the beginning of > kvm_vm_ioctl_create_vcpu should fix it? Yeah I think so. The slight problem is that arch code in the formerly unlocked vcpu create path might take the mutex, powerpc does, but AFAICS that is the only arch that does. So as long as we remove that it should work. I'm travelling today and tomorrow but I'll get you a new patch as soon as I get a chance. cheers --=-wP/ibVg5xKQsaHGFFUfW Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAABCAAGBQJPMlFYAAoJEFHr6jzI4aWALGgP/0gTAAsRVcB5KeI9bzxOEz5O KTrd6d3oDcwNlq12CQsjoPO56vY7yT/sNI7p7CE+sYckkRgtIyTQqcN75fEFOHrx zhGWwOnjf1NRCfJMllodxPLd4PpyA3hctPq/+xVIZriD4438WwfV0VXHRYM8ZbZ+ 3bbHkMIZM8/km2WQJ41AHBbJkXwvIBN9LdybInOGTnKICFyG4auY5JUwl7fJNZmF lNQ0ugXKp8aeOmesTQix7GBd/vSBklHvWDev0BLf5E8casplVNoCOeaFNoq6rI5f zoqNFlZwgM+FfxhXdE9UHFWIzK/aAl4g1dIRzYQqUTYy07nUaJZMhA1jXqxkoGVt kDz35VgCYOPoe+hbyPc0AEHjKnjbADHM1cPw4hAcJ2r8TvHRZRaQVZ+JkbUvY9tk U14t0Bm0wlDGwrxBK9sc/va95ZEAbDCpB2BQ5H+6M2q74xebTE0clUtU1O84umpN W0jDcZfVZIr4fEvtsBXHxI1ZdIBfs6/2qQgTgmFMkd4VreU+wbfhi9QgoZeOvXbq xSh/06VbHmFdEh8cP5BZGFUEeCgf+qNznuUIVAd6cU1Az2xLrwsiMDEG61rURV12 2D9pIAiYMMkFwbOXsLYq+Lv4LUyOOeaQ5+MSnRFz0VK2DgbIRq69iOIQq3B8gmC2 PVKsCjbfH1UnqOuEitNm =yH8F -----END PGP SIGNATURE----- --=-wP/ibVg5xKQsaHGFFUfW--