On Fri, Sep 15, 2017 at 07:52:49AM +0200, Greg Kurz wrote: > Dang! The mail relay at OVH has blacklisted Paul's address :-\ > > <paulus@xxxxxxxxx>: host smtp.samba.org[144.76.82.148] said: 550-blacklisted at > zen.spamhaus.org 550 https://www.spamhaus.org/sbl/query/SBL370982 (in reply > to RCPT TO command) > > Cc'ing Paul at ozlabs.org > > On Fri, 15 Sep 2017 10:48:39 +1000 > David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > On Thu, Sep 14, 2017 at 11:56:25PM +0200, Greg Kurz wrote: > > > The following program causes a kernel oops: > > > > > > #include <sys/types.h> > > > #include <sys/stat.h> > > > #include <fcntl.h> > > > #include <sys/ioctl.h> > > > #include <linux/kvm.h> > > > > > > main() > > > { > > > int fd = open("/dev/kvm", O_RDWR); > > > ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_PPC_HTM); > > > } > > > > > > 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. > > > > > > Let's use the hv_enabled fallback variable, like everywhere > > > else in this function. > > > > > > Fixes: 23528bb21ee2 ("KVM: PPC: Introduce KVM_CAP_PPC_HTM") > > > Cc: stable@xxxxxxxxxxxxxxx # v4.7+ > > > Signed-off-by: Greg Kurz <groug@xxxxxxxx> > > > > 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. > > > > Hmmm... this is what we get with this patch applied: > > open("/dev/kvm", O_RDWR) = 3 > ioctl(3, KVM_CHECK_EXTENSION, 0x84) = 1 <== if HV is present > ioctl(3, KVM_CREATE_VM, 0x1) = 4 <== HV > ioctl(4, KVM_CHECK_EXTENSION, 0x84) = 1 > ioctl(3, KVM_CREATE_VM, 0x2) = 5 <== PR > ioctl(5, KVM_CHECK_EXTENSION, 0x84) = 0 > > The hv_enabled variable is set as follows: > > /* Assume we're using HV mode when the HV module is loaded */ > int hv_enabled = 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 = is_kvmppc_hv_enabled(kvm); > } > > so we're good. :) Oh, sorry, missed that bit. In that case. Reviewed-by: David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> > 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 this > function. > > > > --- > > > arch/powerpc/kvm/powerpc.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > 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, long ext) > > > break; > > > #endif > > > case KVM_CAP_PPC_HTM: > > > - r = cpu_has_feature(CPU_FTR_TM_COMP) && > > > - is_kvmppc_hv_enabled(kvm); > > > + r = cpu_has_feature(CPU_FTR_TM_COMP) && hv_enabled; > > > break; > > > default: > > > r = 0; > > > > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature