| Message ID | 1442559448-1947-1-git-send-email-thuth@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Sep 18, 2015 at 08:57:28AM +0200, Thomas Huth wrote: > Access to the kvm->buses (like with the kvm_io_bus_read() and -write() > functions) has to be protected via the kvm->srcu lock. > The kvmppc_h_logical_ci_load() and -store() functions are missing > this lock so far, so let's add it there, too. > This fixes the problem that the kernel reports "suspicious RCU usage" > when lock debugging is enabled. > > Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1 > Signed-off-by: Thomas Huth <thuth@redhat.com> Nice catch. Looks like I missed this because the places kvm_io_bus_{read,write}() are called on x86 are buried about 5 layers below where the srcu lock is taken :/. Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > arch/powerpc/kvm/book3s.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c > index d75bf32..096e5eb 100644 > --- a/arch/powerpc/kvm/book3s.c > +++ b/arch/powerpc/kvm/book3s.c > @@ -828,12 +828,15 @@ int kvmppc_h_logical_ci_load(struct kvm_vcpu *vcpu) > unsigned long size = kvmppc_get_gpr(vcpu, 4); > unsigned long addr = kvmppc_get_gpr(vcpu, 5); > u64 buf; > + int srcu_idx; > int ret; > > if (!is_power_of_2(size) || (size > sizeof(buf))) > return H_TOO_HARD; > > + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); > ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, &buf); > + srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); > if (ret != 0) > return H_TOO_HARD; > > @@ -868,6 +871,7 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu) > unsigned long addr = kvmppc_get_gpr(vcpu, 5); > unsigned long val = kvmppc_get_gpr(vcpu, 6); > u64 buf; > + int srcu_idx; > int ret; > > switch (size) { > @@ -891,7 +895,9 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu) > return H_TOO_HARD; > } > > + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); > ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, size, &buf); > + srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); > if (ret != 0) > return H_TOO_HARD; >
On 21/09/2015 03:37, David Gibson wrote: > On Fri, Sep 18, 2015 at 08:57:28AM +0200, Thomas Huth wrote: >> Access to the kvm->buses (like with the kvm_io_bus_read() and >> -write() functions) has to be protected via the kvm->srcu lock. >> The kvmppc_h_logical_ci_load() and -store() functions are >> missing this lock so far, so let's add it there, too. This fixes >> the problem that the kernel reports "suspicious RCU usage" when >> lock debugging is enabled. >> >> Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1 Signed-off-by: >> Thomas Huth <thuth@redhat.com> > > Nice catch. Looks like I missed this because the places > kvm_io_bus_{read,write}() are called on x86 are buried about 5 > layers below where the srcu lock is taken :/. > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >> --- arch/powerpc/kvm/book3s.c | 6 ++++++ 1 file changed, 6 >> insertions(+) >> >> diff --git a/arch/powerpc/kvm/book3s.c >> b/arch/powerpc/kvm/book3s.c index d75bf32..096e5eb 100644 --- >> a/arch/powerpc/kvm/book3s.c +++ b/arch/powerpc/kvm/book3s.c @@ >> -828,12 +828,15 @@ int kvmppc_h_logical_ci_load(struct kvm_vcpu >> *vcpu) unsigned long size = kvmppc_get_gpr(vcpu, 4); unsigned >> long addr = kvmppc_get_gpr(vcpu, 5); u64 buf; + int srcu_idx; int >> ret; >> >> if (!is_power_of_2(size) || (size > sizeof(buf))) return >> H_TOO_HARD; >> >> + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); ret = >> kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, &buf); + >> srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); if (ret != 0) >> return H_TOO_HARD; >> >> @@ -868,6 +871,7 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu >> *vcpu) unsigned long addr = kvmppc_get_gpr(vcpu, 5); unsigned >> long val = kvmppc_get_gpr(vcpu, 6); u64 buf; + int srcu_idx; int >> ret; >> >> switch (size) { @@ -891,7 +895,9 @@ int >> kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu) return >> H_TOO_HARD; } >> >> + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); ret = >> kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, size, &buf); + >> srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); if (ret != 0) >> return H_TOO_HARD; >> > Paul, shall I take this directly into my tree for -rc3? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/09/15 03:37, David Gibson wrote: > On Fri, Sep 18, 2015 at 08:57:28AM +0200, Thomas Huth wrote: >> Access to the kvm->buses (like with the kvm_io_bus_read() and -write() >> functions) has to be protected via the kvm->srcu lock. >> The kvmppc_h_logical_ci_load() and -store() functions are missing >> this lock so far, so let's add it there, too. >> This fixes the problem that the kernel reports "suspicious RCU usage" >> when lock debugging is enabled. >> >> Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1 >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > Nice catch. Looks like I missed this because the places > kvm_io_bus_{read,write}() are called on x86 are buried about 5 layers > below where the srcu lock is taken :/. AFAIK the philosophy for taking the srcu lock is completely different between powerpc and x86. On powerpc it is only taken when needed (and released immediately afterwards), while the x86 code tries to hold it the whole time while not being in the guest and not being in userspace. See vcpu_enter_guest() in the x86 code for example, the lock is dropped before entering the guest, and taken again before leaving this function. Thomas
On Mon, Sep 21, 2015 at 07:50:22AM +0200, Paolo Bonzini wrote: > > > On 21/09/2015 03:37, David Gibson wrote: > > On Fri, Sep 18, 2015 at 08:57:28AM +0200, Thomas Huth wrote: > >> Access to the kvm->buses (like with the kvm_io_bus_read() and > >> -write() functions) has to be protected via the kvm->srcu lock. > >> The kvmppc_h_logical_ci_load() and -store() functions are > >> missing this lock so far, so let's add it there, too. This fixes > >> the problem that the kernel reports "suspicious RCU usage" when > >> lock debugging is enabled. > >> > >> Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1 Signed-off-by: > >> Thomas Huth <thuth@redhat.com> > > > > Nice catch. Looks like I missed this because the places > > kvm_io_bus_{read,write}() are called on x86 are buried about 5 > > layers below where the srcu lock is taken :/. > > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> ... > Paul, > > shall I take this directly into my tree for -rc3? > > Paolo I have that and two other fixes in my kvm-ppc-fixes branch on kernel.org. They were in linux-next today. I was going to send you a pull request tomorrow, but if you are about to send stuff off to Linus you could pull now from: git://git.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git kvm-ppc-fixes The three patches in there are: Gautham R. Shenoy (1): KVM: PPC: Book3S HV: Pass the correct trap argument to kvmhv_commence_exit Paul Mackerras (1): KVM: PPC: Book3S HV: Fix handling of interrupted VCPUs Thomas Huth (1): KVM: PPC: Book3S: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store() The one from Gautham is a 1-liner that has been around for months and got missed, and is obviously correct. The one from me fixes a regression that was introduced in 4.3-rc1 by one of my patches, which causes oopses and soft lockups due to a use-after-free bug. Thanks, Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/09/2015 09:59, Paul Mackerras wrote: > I was going to send you a > pull request tomorrow, but if you are about to send stuff off to Linus > you could pull now from: > > git://git.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc.git kvm-ppc-fixes Ok, I'll pull from here tomorrow. The pull request will go to Linus around Thursday. Paolo > The three patches in there are: > > Gautham R. Shenoy (1): > KVM: PPC: Book3S HV: Pass the correct trap argument to kvmhv_commence_exit > > Paul Mackerras (1): > KVM: PPC: Book3S HV: Fix handling of interrupted VCPUs > > Thomas Huth (1): > KVM: PPC: Book3S: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store() > > The one from Gautham is a 1-liner that has been around for months and > got missed, and is obviously correct. The one from me fixes a > regression that was introduced in 4.3-rc1 by one of my patches, which > causes oopses and soft lockups due to a use-after-free bug. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c index d75bf32..096e5eb 100644 --- a/arch/powerpc/kvm/book3s.c +++ b/arch/powerpc/kvm/book3s.c @@ -828,12 +828,15 @@ int kvmppc_h_logical_ci_load(struct kvm_vcpu *vcpu) unsigned long size = kvmppc_get_gpr(vcpu, 4); unsigned long addr = kvmppc_get_gpr(vcpu, 5); u64 buf; + int srcu_idx; int ret; if (!is_power_of_2(size) || (size > sizeof(buf))) return H_TOO_HARD; + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, size, &buf); + srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); if (ret != 0) return H_TOO_HARD; @@ -868,6 +871,7 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu) unsigned long addr = kvmppc_get_gpr(vcpu, 5); unsigned long val = kvmppc_get_gpr(vcpu, 6); u64 buf; + int srcu_idx; int ret; switch (size) { @@ -891,7 +895,9 @@ int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu) return H_TOO_HARD; } + srcu_idx = srcu_read_lock(&vcpu->kvm->srcu); ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, size, &buf); + srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx); if (ret != 0) return H_TOO_HARD;
Access to the kvm->buses (like with the kvm_io_bus_read() and -write() functions) has to be protected via the kvm->srcu lock. The kvmppc_h_logical_ci_load() and -store() functions are missing this lock so far, so let's add it there, too. This fixes the problem that the kernel reports "suspicious RCU usage" when lock debugging is enabled. Fixes: 99342cf8044420eebdf9297ca03a14cb6a7085a1 Signed-off-by: Thomas Huth <thuth@redhat.com> --- arch/powerpc/kvm/book3s.c | 6 ++++++ 1 file changed, 6 insertions(+)