| Message ID | 20160816165106.25057-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)