On 15.09.2017 10:59, David Gibson wrote: > On Fri, Sep 15, 2017 at 07:52:49AM +0200, Greg Kurz wrote: >> Dang! The mail relay at OVH has blacklisted Paul's address :-\ >> >> <pau...@samba.org>: 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 <da...@gibson.dropbear.id.au> 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: sta...@vger.kernel.org # v4.7+ >>>> Signed-off-by: Greg Kurz <gr...@kaod.org> >>> >>> 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 <da...@gibson.dropbear.id.au>
LGTM, too: Reviewed-by: Thomas Huth <th...@redhat.com>
signature.asc
Description: OpenPGP digital signature
