* [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
@ 2015-09-18 6:57 Thomas Huth
2015-09-21 1:37 ` David Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2015-09-18 6:57 UTC (permalink / raw)
To: kvm-ppc, Paul Mackerras, Alexander Graf; +Cc: kvm, David Gibson
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(+)
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;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
2015-09-18 6:57 [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store() Thomas Huth
@ 2015-09-21 1:37 ` David Gibson
2015-09-21 5:50 ` Paolo Bonzini
2015-09-21 6:50 ` Thomas Huth
0 siblings, 2 replies; 6+ messages in thread
From: David Gibson @ 2015-09-21 1:37 UTC (permalink / raw)
To: Thomas Huth; +Cc: kvm-ppc, Paul Mackerras, Alexander Graf, kvm
[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]
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;
>
--
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 #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
2015-09-21 1:37 ` David Gibson
@ 2015-09-21 5:50 ` Paolo Bonzini
2015-09-21 7:59 ` Paul Mackerras
2015-09-21 6:50 ` Thomas Huth
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2015-09-21 5:50 UTC (permalink / raw)
To: David Gibson, Thomas Huth; +Cc: kvm-ppc, Paul Mackerras, Alexander Graf, kvm
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
2015-09-21 5:50 ` Paolo Bonzini
@ 2015-09-21 7:59 ` Paul Mackerras
2015-09-21 9:30 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Paul Mackerras @ 2015-09-21 7:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: David Gibson, Thomas Huth, kvm-ppc, Alexander Graf, kvm
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
2015-09-21 7:59 ` Paul Mackerras
@ 2015-09-21 9:30 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2015-09-21 9:30 UTC (permalink / raw)
To: Paul Mackerras; +Cc: David Gibson, Thomas Huth, kvm-ppc, Alexander Graf, kvm
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
2015-09-21 1:37 ` David Gibson
2015-09-21 5:50 ` Paolo Bonzini
@ 2015-09-21 6:50 ` Thomas Huth
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2015-09-21 6:50 UTC (permalink / raw)
To: David Gibson; +Cc: kvm-ppc, Paul Mackerras, Alexander Graf, kvm
[-- Attachment #1: Type: text/plain, Size: 1203 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-21 9:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-18 6:57 [PATCH] KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store() Thomas Huth
2015-09-21 1:37 ` David Gibson
2015-09-21 5:50 ` Paolo Bonzini
2015-09-21 7:59 ` Paul Mackerras
2015-09-21 9:30 ` Paolo Bonzini
2015-09-21 6:50 ` Thomas Huth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).