[1/3] KVM: PPC: Book3S HV: Fix race in reading change bit when removing HPTE

Message ID 1429853947-30681-2-git-send-email-paulus@samba.org
State New, archived
Headers show

Commit Message

Paul Mackerras April 24, 2015, 5:39 a.m.
The reference (R) and change (C) bits in a HPT entry can be set by
hardware at any time up until the HPTE is invalidated and the TLB
invalidation sequence has completed.  This means that when removing
a HPTE, we need to read the HPTE after the invalidation sequence has
completed in order to obtain reliable values of R and C.  The code
in kvmppc_do_h_remove() used to do this.  However, commit 6f22bd3265fb
("KVM: PPC: Book3S HV: Make HTAB code LE host aware") removed the
read after invalidation as a side effect of other changes.  This
restores the read of the HPTE after invalidation.

The user-visible effect of this bug would be that when migrating a
guest, there is a small probability that a page modified by the guest
and then unmapped by the guest might not get re-transmitted and thus
the destination might end up with a stale copy of the page.

Fixes: 6f22bd3265fb
Cc: stable@vger.kernel.org # v3.17+
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kvm/book3s_hv_rm_mmu.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Aneesh Kumar K.V April 28, 2015, 5:06 a.m. | #1
Paul Mackerras <paulus@samba.org> writes:

> The reference (R) and change (C) bits in a HPT entry can be set by
> hardware at any time up until the HPTE is invalidated and the TLB
> invalidation sequence has completed.  This means that when removing
> a HPTE, we need to read the HPTE after the invalidation sequence has
> completed in order to obtain reliable values of R and C.  The code
> in kvmppc_do_h_remove() used to do this.  However, commit 6f22bd3265fb
> ("KVM: PPC: Book3S HV: Make HTAB code LE host aware") removed the
> read after invalidation as a side effect of other changes.  This
> restores the read of the HPTE after invalidation.
>
> The user-visible effect of this bug would be that when migrating a
> guest, there is a small probability that a page modified by the guest
> and then unmapped by the guest might not get re-transmitted and thus
> the destination might end up with a stale copy of the page.
>
> Fixes: 6f22bd3265fb
> Cc: stable@vger.kernel.org # v3.17+
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
>  arch/powerpc/kvm/book3s_hv_rm_mmu.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index f6bf0b1..5c1737f 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -413,14 +413,12 @@ long kvmppc_do_h_remove(struct kvm *kvm, unsigned long flags,
>  	rev = real_vmalloc_addr(&kvm->arch.revmap[pte_index]);
>  	v = pte & ~HPTE_V_HVLOCK;
>  	if (v & HPTE_V_VALID) {
> -		u64 pte1;
> -
> -		pte1 = be64_to_cpu(hpte[1]);
>  		hpte[0] &= ~cpu_to_be64(HPTE_V_VALID);
> -		rb = compute_tlbie_rb(v, pte1, pte_index);
> +		rb = compute_tlbie_rb(v, be64_to_cpu(hpte[1]), pte_index);
>  		do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags), true);
>  		/* Read PTE low word after tlbie to get final R/C values */
> -		remove_revmap_chain(kvm, pte_index, rev, v, pte1);
> +		remove_revmap_chain(kvm, pte_index, rev, v,
> +				    be64_to_cpu(hpte[1]));
>  	}

May be add the above commit message as a code comment ?

>  	r = rev->guest_rpte & ~HPTE_GR_RESERVED;
>  	note_hpte_modification(kvm, rev);
> -- 
> 2.1.4
>
> --
> 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

--
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
Paul Mackerras April 28, 2015, 9:10 a.m. | #2
On Tue, Apr 28, 2015 at 10:36:52AM +0530, Aneesh Kumar K.V wrote:
> Paul Mackerras <paulus@samba.org> writes:
> 
> > The reference (R) and change (C) bits in a HPT entry can be set by
> > hardware at any time up until the HPTE is invalidated and the TLB
> > invalidation sequence has completed.  This means that when removing
> > a HPTE, we need to read the HPTE after the invalidation sequence has
> > completed in order to obtain reliable values of R and C.  The code
> > in kvmppc_do_h_remove() used to do this.  However, commit 6f22bd3265fb
> > ("KVM: PPC: Book3S HV: Make HTAB code LE host aware") removed the
> > read after invalidation as a side effect of other changes.  This
> > restores the read of the HPTE after invalidation.
> >
> > The user-visible effect of this bug would be that when migrating a
> > guest, there is a small probability that a page modified by the guest
> > and then unmapped by the guest might not get re-transmitted and thus
> > the destination might end up with a stale copy of the page.
> >
> > Fixes: 6f22bd3265fb
> > Cc: stable@vger.kernel.org # v3.17+
> > Signed-off-by: Paul Mackerras <paulus@samba.org>
> > ---
> >  arch/powerpc/kvm/book3s_hv_rm_mmu.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index f6bf0b1..5c1737f 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -413,14 +413,12 @@ long kvmppc_do_h_remove(struct kvm *kvm, unsigned long flags,
> >  	rev = real_vmalloc_addr(&kvm->arch.revmap[pte_index]);
> >  	v = pte & ~HPTE_V_HVLOCK;
> >  	if (v & HPTE_V_VALID) {
> > -		u64 pte1;
> > -
> > -		pte1 = be64_to_cpu(hpte[1]);
> >  		hpte[0] &= ~cpu_to_be64(HPTE_V_VALID);
> > -		rb = compute_tlbie_rb(v, pte1, pte_index);
> > +		rb = compute_tlbie_rb(v, be64_to_cpu(hpte[1]), pte_index);
> >  		do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags), true);
> >  		/* Read PTE low word after tlbie to get final R/C values */
> > -		remove_revmap_chain(kvm, pte_index, rev, v, pte1);
> > +		remove_revmap_chain(kvm, pte_index, rev, v,
> > +				    be64_to_cpu(hpte[1]));
> >  	}
> 
> May be add the above commit message as a code comment ?

Well, that's what "/* Read PTE low word after tlbie to get final R/C
values */" was trying to be, originally, but maybe it would be helpful
to expand on it.

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

Patch

diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index f6bf0b1..5c1737f 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -413,14 +413,12 @@  long kvmppc_do_h_remove(struct kvm *kvm, unsigned long flags,
 	rev = real_vmalloc_addr(&kvm->arch.revmap[pte_index]);
 	v = pte & ~HPTE_V_HVLOCK;
 	if (v & HPTE_V_VALID) {
-		u64 pte1;
-
-		pte1 = be64_to_cpu(hpte[1]);
 		hpte[0] &= ~cpu_to_be64(HPTE_V_VALID);
-		rb = compute_tlbie_rb(v, pte1, pte_index);
+		rb = compute_tlbie_rb(v, be64_to_cpu(hpte[1]), pte_index);
 		do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags), true);
 		/* Read PTE low word after tlbie to get final R/C values */
-		remove_revmap_chain(kvm, pte_index, rev, v, pte1);
+		remove_revmap_chain(kvm, pte_index, rev, v,
+				    be64_to_cpu(hpte[1]));
 	}
 	r = rev->guest_rpte & ~HPTE_GR_RESERVED;
 	note_hpte_modification(kvm, rev);