KVM: PPC: Take the kvm->srcu lock in kvmppc_h_logical_ci_load/store()
diff mbox

Message ID 1442559448-1947-1-git-send-email-thuth@redhat.com
State Accepted
Headers show

Commit Message

Thomas Huth Sept. 18, 2015, 6:57 a.m. UTC
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(+)

Comments

David Gibson Sept. 21, 2015, 1:37 a.m. UTC | #1
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;
>
Paolo Bonzini Sept. 21, 2015, 5:50 a.m. UTC | #2
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
Thomas Huth Sept. 21, 2015, 6:50 a.m. UTC | #3
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
Paul Mackerras Sept. 21, 2015, 7:59 a.m. UTC | #4
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
Paolo Bonzini Sept. 21, 2015, 9:30 a.m. UTC | #5
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

Patch
diff mbox

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;