[PATCH] KVM: arm64: ITS: avoid re-mapping LPIs

When a guest wants to map a device-ID/event-ID combination that is
already mapped, we may end up in a situation where an LPI is never
"put", thus never being freed.
Since the GICv3 spec says that mapping an already mapped LPI is
UNPREDICTABLE, lets just bail out early in this situation to avoid
any potential leaks.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9533080..4660a7d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c

@@ -731,7 +731,7 @@ 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, *new_itte = NULL;
+	struct its_itte *itte;
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;

@@ -749,6 +749,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
 		return E_ITS_MAPTI_PHYSICALID_OOR;
 
+	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
+	if (find_itte(its, device_id, event_id))
+		return 0;
+
 	collection = find_collection(its, coll_id);
 	if (!collection) {
 		int ret = vgic_its_alloc_collection(its, &collection, coll_id);

@@ -757,20 +761,16 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 		new_coll = collection;
 	}
 
-	itte = find_itte(its, device_id, event_id);
+	itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
 	if (!itte) {
-		itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
-		if (!itte) {
-			if (new_coll)
-				vgic_its_free_collection(its, coll_id);
-			return -ENOMEM;
-		}
-
-		new_itte = itte;
-		itte->event_id	= event_id;
-		list_add_tail(&itte->itte_list, &device->itt_head);
+		if (new_coll)
+			vgic_its_free_collection(its, coll_id);
+		return -ENOMEM;
 	}
 
+	itte->event_id	= event_id;
+	list_add_tail(&itte->itte_list, &device->itt_head);
+
 	itte->collection = collection;
 	itte->lpi = lpi_nr;
 

@@ -778,8 +778,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	if (IS_ERR(irq)) {
 		if (new_coll)
 			vgic_its_free_collection(its, coll_id);
-		if (new_itte)
-			its_free_itte(kvm, new_itte);
+		its_free_itte(kvm, itte);
 		return PTR_ERR(irq);
 	}
 	itte->irq = irq;

-- 
2.9.0

On Tue, Aug 16, 2016 at 05:51:06PM +0100, Andre Przywara wrote:

When a guest wants to map a device-ID/event-ID combination that is
already mapped, we may end up in a situation where an LPI is never
"put", thus never being freed.
Since the GICv3 spec says that mapping an already mapped LPI is
UNPREDICTABLE, lets just bail out early in this situation to avoid
any potential leaks.

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

Reviewed-by: Christoffer Dall <redacted>

Thanks!
-Christoffer

On Tue, Aug 16, 2016 at 05:51:06PM +0100, Andre Przywara wrote:

By the way, this made me think how these errors are handled, and unless
I'm mistaken, the return value from vgic_its_handle_command() is simply
discarded, so even when we return things like -ENOMEM, this is just
ignored?  Is this really the intention?

Thanks,
-Christoffer

On 16/08/16 18:30, Christoffer Dall wrote:

On Tue, Aug 16, 2016 at 05:51:06PM +0100, Andre Przywara wrote:

quoted

When a guest wants to map a device-ID/event-ID combination that is
already mapped, we may end up in a situation where an LPI is never
"put", thus never being freed.
Since the GICv3 spec says that mapping an already mapped LPI is
UNPREDICTABLE, lets just bail out early in this situation to avoid
any potential leaks.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9533080..4660a7d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c

@@ -731,7 +731,7 @@ 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, *new_itte = NULL;
+	struct its_itte *itte;
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;

@@ -749,6 +749,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
 		return E_ITS_MAPTI_PHYSICALID_OOR;
 
+	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
+	if (find_itte(its, device_id, event_id))
+		return 0;
+

By the way, this made me think how these errors are handled, and unless
I'm mistaken, the return value from vgic_its_handle_command() is simply
discarded, so even when we return things like -ENOMEM, this is just
ignored?  Is this really the intention?

Yes, at least at the moment. The spec does not specify how ITS errors
should be communicated (IMPLEMENTATION DEFINED), only that an error
condition itself can be signaled via an SError - for which atm we lack
any code to inject, if I am not mistaken.
Still I wanted to assign those error codes: IMHO it improves readability
and simplifies any later extension in that respect.

For the Linux errors (like -ENOMEM): Due to the asynchronous nature of
the ITS command handling and also the guest triggering the commands,
there is really no better way to report those OoM conditions, for
instance, so I treated them the same as "proper" ITS errors.

Hope that helps.
Cheers,
Andre.

On Wed, Aug 17, 2016 at 12:45:17AM +0100, Andr? Przywara wrote:

On 16/08/16 18:30, Christoffer Dall wrote:

quoted

On Tue, Aug 16, 2016 at 05:51:06PM +0100, Andre Przywara wrote:

quoted

When a guest wants to map a device-ID/event-ID combination that is
already mapped, we may end up in a situation where an LPI is never
"put", thus never being freed.
Since the GICv3 spec says that mapping an already mapped LPI is
UNPREDICTABLE, lets just bail out early in this situation to avoid
any potential leaks.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9533080..4660a7d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c

@@ -731,7 +731,7 @@ 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, *new_itte = NULL;
+	struct its_itte *itte;
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;

@@ -749,6 +749,10 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	    lpi_nr >= max_lpis_propbaser(kvm->arch.vgic.propbaser))
 		return E_ITS_MAPTI_PHYSICALID_OOR;
 
+	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
+	if (find_itte(its, device_id, event_id))
+		return 0;
+

By the way, this made me think how these errors are handled, and unless
I'm mistaken, the return value from vgic_its_handle_command() is simply
discarded, so even when we return things like -ENOMEM, this is just
ignored?  Is this really the intention?

Yes, at least at the moment. The spec does not specify how ITS errors
should be communicated (IMPLEMENTATION DEFINED), only that an error
condition itself can be signaled via an SError - for which atm we lack
any code to inject, if I am not mistaken.
Still I wanted to assign those error codes: IMHO it improves readability
and simplifies any later extension in that respect.

It's fine to return error codes, but at the very least we should have a
comment saying "We throw away all errors because we cannot handle them
and this is always fine to do, because of X".

For the Linux errors (like -ENOMEM): Due to the asynchronous nature of
the ITS command handling and also the guest triggering the commands,
there is really no better way to report those OoM conditions, for
instance, so I treated them the same as "proper" ITS errors.

I feel like a -ENOMEM should be reported back to userspace so we can
give up on our giant resource hogging VM instead of just grinding on.

Isn't this all done as part of a MMIO write, so you can return the error
from that thing?

If you were running this in a separate thread, it would be an entirely
more difficult matter.

Thanks,
-Christoffer