From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Ellerman <michael@ellerman.id.au>
Subject: Re: [PATCH] KVM: x86: Avoid NULL dereference in
 kvm_apic_accept_pic_intr()
Date: Wed, 08 Feb 2012 23:13:52 +1100
Message-ID: <1328703232.5682.3.camel@concordia>
References: <1328596327-18662-1-git-send-email-michael@ellerman.id.au>
	 <20120207193839.GA20281@amt.cnet> <1328697688.3308.4.camel@concordia>
Reply-To: michael@ellerman.id.au
Mime-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha256";
	protocol="application/pgp-signature"; boundary="=-tgLaCXRo+xj8Hk6NeN4z"
Cc: kvm@vger.kernel.org, avi@redhat.com
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from ozlabs.org ([203.10.76.45]:57160 "EHLO ozlabs.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1756396Ab2BHMOD (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 8 Feb 2012 07:14:03 -0500
In-Reply-To: <1328697688.3308.4.camel@concordia>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


--=-tgLaCXRo+xj8Hk6NeN4z
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, 2012-02-08 at 21:41 +1100, Michael Ellerman wrote:
> 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=
 NULL.
> > >=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=
 run,
> > > irqchip_in_kernel() is true, but we didn't do the correct initialisat=
ion.
> > >=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.
>=20
> Yeah bugger. I missed that most of the vcpu create is done without the
> mutex held.
>=20
> > Moving mutex_lock(kvm->lock) to the beginning of
> > kvm_vm_ioctl_create_vcpu should fix it?

Hmm, maybe not.

How are the locks meant to nest?

If we move the mutex up, we will be calling kvm_arch_vcpu_setup() with
the kvm->lock held, which calls vcpu_load() which takes vcpu->mutex.

So we would be taking the kvm->lock (A) then vcpu->mutex (B).

And I think the following path takes vcpu->mutex (B) then kvm->lock (A).

kvm_vcpu_ioctl
 -> vcpu_load
    -> mutex_lock(&vcpu->mutex);
 -> kvm_arch_vcpu_ioctl()
    KVM_GET_MSRS:
    -> msr_io(vcpu, argp, kvm_get_msr, 1)
      -> __msr_io(..)
        -> srcu_read_lock(&vcpu->kvm->srcu);
        -> kvm_get_msr()
          -> kvm_x86_ops->get_msr()
            -> vmx_get_msr()
               default:
               -> kvm_get_msr_common()
                  case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
                  -> mutex_lock(&vcpu->kvm->lock);


But trawling through that code path was a bit of a mess, so maybe I
missed something somewhere.

cheers




--=-tgLaCXRo+xj8Hk6NeN4z
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)

iQIcBAABCAAGBQJPMmcAAAoJEFHr6jzI4aWA1pkQAKONWFkIZY3c1w8Xh2oj0ZNw
EN+z/TaULY/57AJVgg4+lPxAe/HEL2VV8ti1K4J/46IWIIQAwow1epdk+VSrZJwb
alAWTBjIMv32p6mU6RlUSsBhPluwb7dzvhU3Tb8Ror+BNLlG7Z+pEYB3M71664Zt
a91Lac0/NZw0wwi/p6wNLzZAKsWyQ1/FTdbiOoEFw1gRcAKFhUaH8yFk4DoeOxbD
DDi8zCETPyjAHrZEZX9qbNSQulG3Wb8Avl4Ni6cXCm1UOS6033+24hmZfVOJG144
UN5VdBIlodQsHLXHHjWEPhI0B8TU8lnJaRuj/HV2OostB+erHoyGxYbRA8zjnqgj
LWNKbf9XoksbiYzYEmUcxQIvY35IUw5e7ByQ+rHbeR3jljaLUbkPc8KOe6sAvlEm
vWuSilBwG1l3GX3w1382P77RfKSWLENK3N6KUX9g162fjQdXoY2c1F/31EAUtCnn
An+YwgWTc5kv/k1Od/2SHicafOizUHU9lGnD9BaR0fTzjIqiqk6hb+dJY9bQNFRD
dZo1ibcCRsKKOCaOvXoa1k6oTuMDcZDCmPeBEA2F/ntkhBq9mSGn52WnVrOx3EyD
nEiZ+fq2MIGQUa0FdxJk10ojtJB7TlufSo7MG0QOywHNzSdgkovZe4rXHtbMGJyQ
gNpf0bIKN6Ds/Pte6QhG
=ZmM7
-----END PGP SIGNATURE-----

--=-tgLaCXRo+xj8Hk6NeN4z--