[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));
>  }
>  
>  /*
> 

[PATCH 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.

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     | 18 ++++++++++++++----
 virt/kvm/arm/vgic/vgic-mmio-v3.c | 10 ++++++++--
 virt/kvm/arm/vgic/vgic.c         | 20 ++++++++++----------
 3 files changed, 32 insertions(+), 16 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>
---
Changes since v1:
 - Don't errornously get an extra kref refernce for the struct vgic_irq
 - Don't rework the entire error handling of the function, but follow
   what Marc suggested he prefers based on his fixup patch.
 virt/kvm/arm/vgic/vgic-its.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07411cf..424f7a5 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);
@@ -693,10 +693,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)
@@ -720,7 +721,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 
 	itte = find_itte(its, device_id, event_id);
 	if (!itte) {
-		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
+		new_itte = itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
 		if (!itte) {
 			if (new_coll)
 				vgic_its_free_collection(its, coll_id);
@@ -733,7 +734,16 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 
 	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);
+		kfree(new_itte);
+		return PTR_ERR(irq);
+	}
+	itte->irq = irq;
+
 	update_affinity_itte(kvm, itte);
 
 	/*
-- 
2.9.0

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

[Andre Przywara]

Hi,

On 03/08/16 17:13, Christoffer Dall wrote:
> 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);

Ouch! Thanks for catching this unhandled error return!

>           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>
> ---
> Changes since v1:
>  - Don't errornously get an extra kref refernce for the struct vgic_irq
>  - Don't rework the entire error handling of the function, but follow
>    what Marc suggested he prefers based on his fixup patch.
>  virt/kvm/arm/vgic/vgic-its.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 07411cf..424f7a5 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);
> @@ -693,10 +693,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)
> @@ -720,7 +721,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  
>  	itte = find_itte(its, device_id, event_id);
>  	if (!itte) {
> -		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
> +		new_itte = itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);

Nit: Aren't double assignments frowned upon in the kernel?

>  		if (!itte) {
>  			if (new_coll)
>  				vgic_its_free_collection(its, coll_id);
> @@ -733,7 +734,16 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  
>  	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);
> +		kfree(new_itte);

But at this point we already have added that ITTE to the
device->itt_head, haven't we?
Since we hold the its_lock, would a simple:

		if (new_itte) {
			list_del(&itte->itte_list);
			kfree(new_itte);
		}

suffice to fix this?

> +		return PTR_ERR(irq);
> +	}
> +	itte->irq = irq;
> +
>  	update_affinity_itte(kvm, itte);
>  
>  	/*
> 

Cheers,
Andre.

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

[Christoffer Dall]

On Mon, Aug 08, 2016 at 12:00:50PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 03/08/16 17:13, Christoffer Dall wrote:
> > 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);
> 
> Ouch! Thanks for catching this unhandled error return!
> 
> >           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>
> > ---
> > Changes since v1:
> >  - Don't errornously get an extra kref refernce for the struct vgic_irq
> >  - Don't rework the entire error handling of the function, but follow
> >    what Marc suggested he prefers based on his fixup patch.
> >  virt/kvm/arm/vgic/vgic-its.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> > index 07411cf..424f7a5 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);
> > @@ -693,10 +693,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)
> > @@ -720,7 +721,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  
> >  	itte = find_itte(its, device_id, event_id);
> >  	if (!itte) {
> > -		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
> > +		new_itte = itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
> 
> Nit: Aren't double assignments frowned upon in the kernel?
> 

Seems like it is accoding to CodingStyle, although it can be found
numerous places in the code base.  But you're right, let's follow the
official style.

> >  		if (!itte) {
> >  			if (new_coll)
> >  				vgic_its_free_collection(its, coll_id);
> > @@ -733,7 +734,16 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  
> >  	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);
> > +		kfree(new_itte);
> 
> But at this point we already have added that ITTE to the
> device->itt_head, haven't we?
> Since we hold the its_lock, would a simple:
> 
> 		if (new_itte) {
> 			list_del(&itte->itte_list);
> 			kfree(new_itte);
> 		}
> 
> suffice to fix this?
> 

hmm, it would be good to call its_free_itte for this.  But then that
would put a reference on an IRQ, which wouldn't necessarily have been
taken.  That could be reworked by changing its_free_itte like this:

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 424f7a5..6342c92 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -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 (iite->irq)
+		vgic_put_irq(kvm, itte->irq);
 
 	kfree(itte);
 }

But this makes me wonder how we're really dealing with reference counts
in the case where you find an itte and don't need to allocate one.
Would this BUG_ON ever fire?:

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 424f7a5..a33fbf1 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -730,6 +730,8 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 
 		itte->event_id	= event_id;
 		list_add_tail(&itte->itte_list, &device->itt_head);
+	} else {
+		BUG_ON(itte->irq);
 	}
 
 	itte->collection = collection;

If not, I don't understand how you can just assign the irq field on
the itte without putting whatever IRQ there may already be held with a
reference there.

Can you explain me the flow of how an itte is allocated, but not
assigned an IRQ, and then later found in vgic_its_cmd_handle_mapi?

> > +		return PTR_ERR(irq);
> > +	}
> > +	itte->irq = irq;
> > +
> >  	update_affinity_itte(kvm, itte);
> >  
> >  	/*
> > 
> 
Thanks,
-Christoffer