[PATCH v2 0/3] KVM: arm64: vgic-its-fixes

[PATCH v2 0/3] KVM: arm64: vgic-its-fixes

[Christoffer Dall]

Here are three small fixes resulting from looking at the pull request of
kvmarm/next into kvm/next.

They basically deal with corner cases in the reference count handling and
atomicity issues.

See the individual patches for changelogs.

Christoffer Dall (3):
  KVM: arm64: vgic-its: Handle errors from vgic_add_lpi
  KVM: arm64: vgic-its: Plug race in vgic_put_irq
  KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

 virt/kvm/arm/vgic/vgic-its.c     | 21 +++++++++++++++++----
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 24 ++++++++++++++----------
 virt/kvm/arm/vgic/vgic.c         | 10 +++++-----
 3 files changed, 36 insertions(+), 19 deletions(-)

-- 
2.9.0

[PATCH v2 1/3] KVM: arm64: vgic-its: Handle errors from vgic_add_lpi

[Christoffer Dall]

During low memory conditions, we could be dereferencing a NULL pointer
when vgic_add_lpi fails to allocate memory.

Consider for example this call sequence:

  vgic_its_cmd_handle_mapi
      itte->irq = vgic_add_lpi(kvm, lpi_nr);
          update_lpi_config(kvm, itte->irq, NULL);
              ret = kvm_read_guest(kvm, propbase + irq->intid
	                                             ^^^^
						     kaboom?

Instead, return an error pointer from vgic_add_lpi and check the return
value from its single caller.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---

Notes:
    Changes since v1:
     - Stylistic changes
     - Use its_free_itte to free new_itte on error path

 virt/kvm/arm/vgic/vgic-its.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07411cf..4e16880e 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -51,7 +51,7 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid)
 
 	irq = kzalloc(sizeof(struct vgic_irq), GFP_KERNEL);
 	if (!irq)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(&irq->lpi_list);
 	INIT_LIST_HEAD(&irq->ap_list);
@@ -502,7 +502,8 @@ static void its_free_itte(struct kvm *kvm, struct its_itte *itte)
 	list_del(&itte->itte_list);
 
 	/* This put matches the get in vgic_add_lpi. */
-	vgic_put_irq(kvm, itte->irq);
+	if (itte->irq)
+		vgic_put_irq(kvm, itte->irq);
 
 	kfree(itte);
 }
@@ -693,10 +694,11 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	u32 event_id = its_cmd_get_id(its_cmd);
 	u32 coll_id = its_cmd_get_collection(its_cmd);
-	struct its_itte *itte;
+	struct its_itte *itte, *new_itte = NULL;
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;
+	struct vgic_irq *irq;
 
 	device = find_its_device(its, device_id);
 	if (!device)
@@ -727,13 +729,24 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 			return -ENOMEM;
 		}
 
+		new_itte = itte;
 		itte->event_id	= event_id;
 		list_add_tail(&itte->itte_list, &device->itt_head);
 	}
 
 	itte->collection = collection;
 	itte->lpi = lpi_nr;
-	itte->irq = vgic_add_lpi(kvm, lpi_nr);
+
+	irq = vgic_add_lpi(kvm, lpi_nr);
+	if (IS_ERR(irq)) {
+		if (new_coll)
+			vgic_its_free_collection(its, coll_id);
+		if (new_itte)
+			its_free_itte(kvm, new_itte);
+		return PTR_ERR(irq);
+	}
+	itte->irq = irq;
+
 	update_affinity_itte(kvm, itte);
 
 	/*
-- 
2.9.0

[PATCH v2 2/3] KVM: arm64: vgic-its: Plug race in vgic_put_irq

[Christoffer Dall]

Right now the following sequence of events can happen:

  1. Thread X calls vgic_put_irq
  2. Thread Y calls vgic_add_lpi
  3. Thread Y gets lpi_list_lock
  4. Thread X drops the ref count to 0 and blocks on lpi_list_lock
  5. Thread Y finds the irq via the lpi_list_lock, raises the ref
     count to 1, and release the lpi_list_lock.
  6. Thread X proceeds and frees the irq.

Avoid this by holding the spinlock around the kref_put.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---

Notes:
    Changes since v1:
     - Stylistic change: Don't use else branch

 virt/kvm/arm/vgic/vgic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index e7aeac7..e83b7fe 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -117,17 +117,17 @@ static void vgic_irq_release(struct kref *ref)
 
 void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
 {
-	struct vgic_dist *dist;
+	struct vgic_dist *dist = &kvm->arch.vgic;
 
 	if (irq->intid < VGIC_MIN_LPI)
 		return;
 
-	if (!kref_put(&irq->refcount, vgic_irq_release))
+	spin_lock(&dist->lpi_list_lock);
+	if (!kref_put(&irq->refcount, vgic_irq_release)) {
+		spin_unlock(&dist->lpi_list_lock);
 		return;
+	};
 
-	dist = &kvm->arch.vgic;
-
-	spin_lock(&dist->lpi_list_lock);
 	list_del(&irq->lpi_list);
 	dist->lpi_list_count--;
 	spin_unlock(&dist->lpi_list_lock);
-- 
2.9.0

[PATCH v2 2/3] KVM: arm64: vgic-its: Plug race in vgic_put_irq

[Andre Przywara]

Hi,

On 09/08/16 12:16, Christoffer Dall wrote:
> Right now the following sequence of events can happen:
> 
>   1. Thread X calls vgic_put_irq
>   2. Thread Y calls vgic_add_lpi
>   3. Thread Y gets lpi_list_lock
>   4. Thread X drops the ref count to 0 and blocks on lpi_list_lock
>   5. Thread Y finds the irq via the lpi_list_lock, raises the ref
>      count to 1, and release the lpi_list_lock.
>   6. Thread X proceeds and frees the irq.
> 
> Avoid this by holding the spinlock around the kref_put.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks!
Andre

> ---
> 
> Notes:
>     Changes since v1:
>      - Stylistic change: Don't use else branch
> 
>  virt/kvm/arm/vgic/vgic.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index e7aeac7..e83b7fe 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -117,17 +117,17 @@ static void vgic_irq_release(struct kref *ref)
>  
>  void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
>  {
> -	struct vgic_dist *dist;
> +	struct vgic_dist *dist = &kvm->arch.vgic;
>  
>  	if (irq->intid < VGIC_MIN_LPI)
>  		return;
>  
> -	if (!kref_put(&irq->refcount, vgic_irq_release))
> +	spin_lock(&dist->lpi_list_lock);
> +	if (!kref_put(&irq->refcount, vgic_irq_release)) {
> +		spin_unlock(&dist->lpi_list_lock);
>  		return;
> +	};
>  
> -	dist = &kvm->arch.vgic;
> -
> -	spin_lock(&dist->lpi_list_lock);
>  	list_del(&irq->lpi_list);
>  	dist->lpi_list_count--;
>  	spin_unlock(&dist->lpi_list_lock);
> 

[PATCH v2 3/3] KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

[Christoffer Dall]

There are two problems with the current implementation of the MMIO
handlers for the propbaser and pendbaser:

First, the write to the value itself is not guaranteed to be an atomic
64-bit write so two concurrent writes to the structure field could be
intermixed.

Second, because we do a read-modify-update operation without any
synchronization, if we have two 32-bit accesses to separate parts of the
register, we can loose one of them.

By using the atomic cmpxchg64 we should cover both issues above.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---

Notes:
    Changes since v1:
     - Use atomic cmpxchg64 instead of taking a lock

 virt/kvm/arm/vgic/vgic-mmio-v3.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ff668e0..a50d5ba 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -306,16 +306,18 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
 {
 	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
-	u64 propbaser = dist->propbaser;
+	u64 old_propbaser, propbaser;
 
 	/* Storing a value with LPIs already enabled is undefined */
 	if (vgic_cpu->lpis_enabled)
 		return;
 
-	propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
-	propbaser = vgic_sanitise_propbaser(propbaser);
-
-	dist->propbaser = propbaser;
+	do {
+		old_propbaser = dist->propbaser;
+		propbaser = old_propbaser;
+		propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
+		propbaser = vgic_sanitise_propbaser(propbaser);
+	} while (cmpxchg64(&dist->propbaser, old_propbaser, propbaser));
 }
 
 static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu,
@@ -331,16 +333,18 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
 				     unsigned long val)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
-	u64 pendbaser = vgic_cpu->pendbaser;
+	u64 old_pendbaser, pendbaser;
 
 	/* Storing a value with LPIs already enabled is undefined */
 	if (vgic_cpu->lpis_enabled)
 		return;
 
-	pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
-	pendbaser = vgic_sanitise_pendbaser(pendbaser);
-
-	vgic_cpu->pendbaser = pendbaser;
+	do {
+		old_pendbaser = vgic_cpu->pendbaser;
+		pendbaser = old_pendbaser;
+		pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
+		pendbaser = vgic_sanitise_pendbaser(pendbaser);
+	} while (cmpxchg64(&vgic_cpu->pendbaser, old_pendbaser, pendbaser));
 }
 
 /*
-- 
2.9.0

[PATCH v2 3/3] KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

[Andre Przywara]

Hi,

On 09/08/16 12:16, Christoffer Dall wrote:
> There are two problems with the current implementation of the MMIO
> handlers for the propbaser and pendbaser:
> 
> First, the write to the value itself is not guaranteed to be an atomic
> 64-bit write so two concurrent writes to the structure field could be
> intermixed.
> 
> Second, because we do a read-modify-update operation without any
> synchronization, if we have two 32-bit accesses to separate parts of the
> register, we can loose one of them.
> 
> By using the atomic cmpxchg64 we should cover both issues above.

This is really a neat solution. The actual implementation of cmpxchg64
is really hard to chase in the code, but given that the prototype is
(ptr, old, new) and it complies with the usual compare-exchange
semantics this looks good to me.

> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
> 
> Notes:
>     Changes since v1:
>      - Use atomic cmpxchg64 instead of taking a lock
> 
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ff668e0..a50d5ba 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -306,16 +306,18 @@ static void vgic_mmio_write_propbase(struct kvm_vcpu *vcpu,
>  {
>  	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	u64 propbaser = dist->propbaser;
> +	u64 old_propbaser, propbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
>  	if (vgic_cpu->lpis_enabled)
>  		return;
>  
> -	propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
> -	propbaser = vgic_sanitise_propbaser(propbaser);
> -
> -	dist->propbaser = propbaser;
> +	do {
> +		old_propbaser = dist->propbaser;
> +		propbaser = old_propbaser;
> +		propbaser = update_64bit_reg(propbaser, addr & 4, len, val);
> +		propbaser = vgic_sanitise_propbaser(propbaser);
> +	} while (cmpxchg64(&dist->propbaser, old_propbaser, propbaser));
>  }
>  
>  static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu,
> @@ -331,16 +333,18 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
>  				     unsigned long val)
>  {
>  	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	u64 pendbaser = vgic_cpu->pendbaser;
> +	u64 old_pendbaser, pendbaser;
>  
>  	/* Storing a value with LPIs already enabled is undefined */
>  	if (vgic_cpu->lpis_enabled)
>  		return;
>  
> -	pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
> -	pendbaser = vgic_sanitise_pendbaser(pendbaser);
> -
> -	vgic_cpu->pendbaser = pendbaser;
> +	do {
> +		old_pendbaser = vgic_cpu->pendbaser;
> +		pendbaser = old_pendbaser;
> +		pendbaser = update_64bit_reg(pendbaser, addr & 4, len, val);
> +		pendbaser = vgic_sanitise_pendbaser(pendbaser);
> +	} while (cmpxchg64(&vgic_cpu->pendbaser, old_pendbaser, pendbaser));
>  }
>  
>  /*
>