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

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

[Andre Przywara]

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

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

[Christoffer Dall]

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 <christoffer.dall@linaro.org>

Thanks!
-Christoffer

> ---
>  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
> 

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

[Christoffer Dall]

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>
> ---
>  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?

Thanks,
-Christoffer

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

[André Przywara]

On 16/08/16 18:30, Christoffer Dall wrote:
> 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>
>> ---
>>  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.

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

[Christoffer Dall]

On Wed, Aug 17, 2016 at 12:45:17AM +0100, Andr? Przywara wrote:
> On 16/08/16 18:30, Christoffer Dall wrote:
> > 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>
> >> ---
> >>  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