From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from 1.mo2.mail-out.ovh.net ([46.105.63.121]:49710 "EHLO 1.mo2.mail-out.ovh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751116AbdIOGBJ (ORCPT ); Fri, 15 Sep 2017 02:01:09 -0400 Received: from player770.ha.ovh.net (b6.ovh.net [213.186.33.56]) by mo2.mail-out.ovh.net (Postfix) with ESMTP id EDDA8ABDE8 for ; Fri, 15 Sep 2017 07:52:58 +0200 (CEST) Date: Fri, 15 Sep 2017 07:52:49 +0200 From: Greg Kurz To: David Gibson Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Paul Mackerras , Sam Bobroff , Paolo Bonzini , linuxppc-dev@lists.ozlabs.org, stable@vger.kernel.org Subject: Re: [PATCH] KVM: PPC: fix oops when checking KVM_CAP_PPC_HTM Message-ID: <20170915075249.0e9879a4@bahia.lan> In-Reply-To: <20170915004839.GA5250@umbus.fritz.box> References: <150542618501.6859.11512107352972110416.stgit@bahia.lan> <20170915004839.GA5250@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/2v=IlCQP/zUiszbsYapkfca"; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --Sig_/2v=IlCQP/zUiszbsYapkfca Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Dang! The mail relay at OVH has blacklisted Paul's address :-\ : host smtp.samba.org[144.76.82.148] said: 550-blackliste= d at zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in r= eply to RCPT TO command) Cc'ing Paul at ozlabs.org On Fri, 15 Sep 2017 10:48:39 +1000 David Gibson wrote: > On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote: > > The following program causes a kernel oops: > >=20 > > #include > > #include > > #include > > #include > > #include > >=20 > > main() > > { > > int fd =3D open("/dev/kvm", O_RDWR); > > ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM); > > } > >=20 > > This happens because when using the global KVM fd with > > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets > > called with a NULL kvm argument, which gets dereferenced > > in is_kvmppc_hv_enabled(). Spotted while reading the code. > >=20 > > Let's use the hv_enabled fallback variable, like everywhere > > else in this function. > >=20 > > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM") > > Cc: stable@vger.kernel.org # v4.7+ > > Signed-off-by: Greg Kurz =20 >=20 > I don't think this is right. I'm pretty sure you want to fall back to > hv_enabled *only when* kvm is NULL. Otherwise if you have a PR guest > on an HV capable machine, this will give the wrong answer, when called > for that specific VM. >=20 Hmmm... this is what we get with this patch applied: open("/dev/kvm", O_RDWR) =3D 3 ioctl(3, KVM_CHECK_EXTENSION, 0x84) =3D 1 <=3D=3D if HV is present ioctl(3, KVM_CREATE_VM, 0x1) =3D 4 <=3D=3D HV ioctl(4, KVM_CHECK_EXTENSION, 0x84) =3D 1 ioctl(3, KVM_CREATE_VM, 0x2) =3D 5 <=3D=3D PR ioctl(5, KVM_CHECK_EXTENSION, 0x84) =3D 0 The hv_enabled variable is set as follows: /* Assume we're using HV mode when the HV module is loaded */ int hv_enabled =3D kvmppc_hv_ops ? 1 : 0; if (kvm) { /* * Hooray - we know which VM type we're running on. Depend on * that rather than the guess above. */ hv_enabled =3D is_kvmppc_hv_enabled(kvm); } so we're good. :) The last sentence in the commit message is maybe^wprobably not comprehensive enough... What about the following ? The hv_enabled variable is initialized to 1 if HV is loaded or 0 otherwise. In the case KVM_CHECK_EXTENSION is used with a VM fd, hv_enabled is updated to is_kvmppc_hv_enabled(kvm). Let's use it here, like everywhere else in th= is function. > > --- > > arch/powerpc/kvm/powerpc.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > >=20 > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > > index 3480faaf1ef8..ee279c7f4802 100644 > > --- a/arch/powerpc/kvm/powerpc.c > > +++ b/arch/powerpc/kvm/powerpc.c > > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, l= ong ext) > > break; > > #endif > > case KVM_CAP_PPC_HTM: > > - r =3D cpu_has_feature(CPU_FTR_TM_COMP) && > > - is_kvmppc_hv_enabled(kvm); > > + r =3D cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled; > > break; > > default: > > r =3D 0; > > =20 >=20 --Sig_/2v=IlCQP/zUiszbsYapkfca Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQQr1DtEU17Ap5iU26IC/DrrAQHbwgUCWbtqsQAKCRAC/DrrAQHb wkw/AJ4hcObSU0V8DToF5lB4G8R7/oXVvQCginbzy+RwhmgfKqEf0nsX5zvJoR8= =vUvB -----END PGP SIGNATURE----- --Sig_/2v=IlCQP/zUiszbsYapkfca-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Kurz Subject: Re: [PATCH] KVM: PPC: fix oops when checking KVM_CAP_PPC_HTM Date: Fri, 15 Sep 2017 07:52:49 +0200 Message-ID: <20170915075249.0e9879a4@bahia.lan> References: <150542618501.6859.11512107352972110416.stgit@bahia.lan> <20170915004839.GA5250@umbus.fritz.box> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/2v=IlCQP/zUiszbsYapkfca"; protocol="application/pgp-signature" Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, stable@vger.kernel.org, Paolo Bonzini , linuxppc-dev@lists.ozlabs.org, Sam Bobroff To: David Gibson Return-path: In-Reply-To: <20170915004839.GA5250@umbus.fritz.box> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org --Sig_/2v=IlCQP/zUiszbsYapkfca Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Dang! The mail relay at OVH has blacklisted Paul's address :-\ : host smtp.samba.org[144.76.82.148] said: 550-blackliste= d at zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in r= eply to RCPT TO command) Cc'ing Paul at ozlabs.org On Fri, 15 Sep 2017 10:48:39 +1000 David Gibson wrote: > On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote: > > The following program causes a kernel oops: > >=20 > > #include > > #include > > #include > > #include > > #include > >=20 > > main() > > { > > int fd =3D open("/dev/kvm", O_RDWR); > > ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM); > > } > >=20 > > This happens because when using the global KVM fd with > > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets > > called with a NULL kvm argument, which gets dereferenced > > in is_kvmppc_hv_enabled(). Spotted while reading the code. > >=20 > > Let's use the hv_enabled fallback variable, like everywhere > > else in this function. > >=20 > > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM") > > Cc: stable@vger.kernel.org # v4.7+ > > Signed-off-by: Greg Kurz =20 >=20 > I don't think this is right. I'm pretty sure you want to fall back to > hv_enabled *only when* kvm is NULL. Otherwise if you have a PR guest > on an HV capable machine, this will give the wrong answer, when called > for that specific VM. >=20 Hmmm... this is what we get with this patch applied: open("/dev/kvm", O_RDWR) =3D 3 ioctl(3, KVM_CHECK_EXTENSION, 0x84) =3D 1 <=3D=3D if HV is present ioctl(3, KVM_CREATE_VM, 0x1) =3D 4 <=3D=3D HV ioctl(4, KVM_CHECK_EXTENSION, 0x84) =3D 1 ioctl(3, KVM_CREATE_VM, 0x2) =3D 5 <=3D=3D PR ioctl(5, KVM_CHECK_EXTENSION, 0x84) =3D 0 The hv_enabled variable is set as follows: /* Assume we're using HV mode when the HV module is loaded */ int hv_enabled =3D kvmppc_hv_ops ? 1 : 0; if (kvm) { /* * Hooray - we know which VM type we're running on. Depend on * that rather than the guess above. */ hv_enabled =3D is_kvmppc_hv_enabled(kvm); } so we're good. :) The last sentence in the commit message is maybe^wprobably not comprehensive enough... What about the following ? The hv_enabled variable is initialized to 1 if HV is loaded or 0 otherwise. In the case KVM_CHECK_EXTENSION is used with a VM fd, hv_enabled is updated to is_kvmppc_hv_enabled(kvm). Let's use it here, like everywhere else in th= is function. > > --- > > arch/powerpc/kvm/powerpc.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > >=20 > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > > index 3480faaf1ef8..ee279c7f4802 100644 > > --- a/arch/powerpc/kvm/powerpc.c > > +++ b/arch/powerpc/kvm/powerpc.c > > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, l= ong ext) > > break; > > #endif > > case KVM_CAP_PPC_HTM: > > - r =3D cpu_has_feature(CPU_FTR_TM_COMP) && > > - is_kvmppc_hv_enabled(kvm); > > + r =3D cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled; > > break; > > default: > > r =3D 0; > > =20 >=20 --Sig_/2v=IlCQP/zUiszbsYapkfca Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQQr1DtEU17Ap5iU26IC/DrrAQHbwgUCWbtqsQAKCRAC/DrrAQHb wkw/AJ4hcObSU0V8DToF5lB4G8R7/oXVvQCginbzy+RwhmgfKqEf0nsX5zvJoR8= =vUvB -----END PGP SIGNATURE----- --Sig_/2v=IlCQP/zUiszbsYapkfca-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Kurz Date: Fri, 15 Sep 2017 05:52:49 +0000 Subject: Re: [PATCH] KVM: PPC: fix oops when checking KVM_CAP_PPC_HTM Message-Id: <20170915075249.0e9879a4@bahia.lan> MIME-Version: 1 Content-Type: multipart/mixed; boundary="Sig_/2v=IlCQP/zUiszbsYapkfca" List-Id: References: <150542618501.6859.11512107352972110416.stgit@bahia.lan> <20170915004839.GA5250@umbus.fritz.box> In-Reply-To: <20170915004839.GA5250@umbus.fritz.box> To: David Gibson Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Paul Mackerras , Sam Bobroff , Paolo Bonzini , linuxppc-dev@lists.ozlabs.org, stable@vger.kernel.org --Sig_/2v=IlCQP/zUiszbsYapkfca Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Dang! The mail relay at OVH has blacklisted Paul's address :-\ : host smtp.samba.org[144.76.82.148] said: 550-blackliste= d at zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in r= eply to RCPT TO command) Cc'ing Paul at ozlabs.org On Fri, 15 Sep 2017 10:48:39 +1000 David Gibson wrote: > On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote: > > The following program causes a kernel oops: > >=20 > > #include > > #include > > #include > > #include > > #include > >=20 > > main() > > { > > int fd =3D open("/dev/kvm", O_RDWR); > > ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM); > > } > >=20 > > This happens because when using the global KVM fd with > > KVM_CHECK_EXTENSION, kvm_vm_ioctl_check_extension() gets > > called with a NULL kvm argument, which gets dereferenced > > in is_kvmppc_hv_enabled(). Spotted while reading the code. > >=20 > > Let's use the hv_enabled fallback variable, like everywhere > > else in this function. > >=20 > > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM") > > Cc: stable@vger.kernel.org # v4.7+ > > Signed-off-by: Greg Kurz =20 >=20 > I don't think this is right. I'm pretty sure you want to fall back to > hv_enabled *only when* kvm is NULL. Otherwise if you have a PR guest > on an HV capable machine, this will give the wrong answer, when called > for that specific VM. >=20 Hmmm... this is what we get with this patch applied: open("/dev/kvm", O_RDWR) =3D 3 ioctl(3, KVM_CHECK_EXTENSION, 0x84) =3D 1 <=3D=3D if HV is present ioctl(3, KVM_CREATE_VM, 0x1) =3D 4 <=3D=3D HV ioctl(4, KVM_CHECK_EXTENSION, 0x84) =3D 1 ioctl(3, KVM_CREATE_VM, 0x2) =3D 5 <=3D=3D PR ioctl(5, KVM_CHECK_EXTENSION, 0x84) =3D 0 The hv_enabled variable is set as follows: /* Assume we're using HV mode when the HV module is loaded */ int hv_enabled =3D kvmppc_hv_ops ? 1 : 0; if (kvm) { /* * Hooray - we know which VM type we're running on. Depend on * that rather than the guess above. */ hv_enabled =3D is_kvmppc_hv_enabled(kvm); } so we're good. :) The last sentence in the commit message is maybe^wprobably not comprehensive enough... What about the following ? The hv_enabled variable is initialized to 1 if HV is loaded or 0 otherwise. In the case KVM_CHECK_EXTENSION is used with a VM fd, hv_enabled is updated to is_kvmppc_hv_enabled(kvm). Let's use it here, like everywhere else in th= is function. > > --- > > arch/powerpc/kvm/powerpc.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > >=20 > > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > > index 3480faaf1ef8..ee279c7f4802 100644 > > --- a/arch/powerpc/kvm/powerpc.c > > +++ b/arch/powerpc/kvm/powerpc.c > > @@ -644,8 +644,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, l= ong ext) > > break; > > #endif > > case KVM_CAP_PPC_HTM: > > - r =3D cpu_has_feature(CPU_FTR_TM_COMP) && > > - is_kvmppc_hv_enabled(kvm); > > + r =3D cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled; > > break; > > default: > > r =3D 0; > > =20 >=20 --Sig_/2v=IlCQP/zUiszbsYapkfca Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQQr1DtEU17Ap5iU26IC/DrrAQHbwgUCWbtqsQAKCRAC/DrrAQHb wkw/AJ4hcObSU0V8DToF5lB4G8R7/oXVvQCginbzy+RwhmgfKqEf0nsX5zvJoR8= =vUvB -----END PGP SIGNATURE----- --Sig_/2v=IlCQP/zUiszbsYapkfca--