qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb()
@ 2024-07-31 17:00 Peter Maydell
  2024-07-31 21:07 ` Philippe Mathieu-Daudé
  2025-02-17  7:26 ` Philippe Mathieu-Daudé
  0 siblings, 2 replies; 4+ messages in thread
From: Peter Maydell @ 2024-07-31 17:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum

In amdvi_update_iotlb() we will only put a new entry in the hash
table if to_cache.perm is not IOMMU_NONE.  However we allocate the
memory for the new AMDVIIOTLBEntry and for the hash table key
regardless.  This means that in the IOMMU_NONE case we will leak the
memory we alloacted.

Move the allocations into the if() to the point where we know we're
going to add the item to the hash table.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2452
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Tested with 'make check' and 'make check-avocado' only, but the
bug and fix seem straightforward...
---
 hw/i386/amd_iommu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 6d4fde72f9b..87643d28917 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -357,12 +357,12 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
                                uint64_t gpa, IOMMUTLBEntry to_cache,
                                uint16_t domid)
 {
-    AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
-    uint64_t *key = g_new(uint64_t, 1);
-    uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
-
     /* don't cache erroneous translations */
     if (to_cache.perm != IOMMU_NONE) {
+        AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
+        uint64_t *key = g_new(uint64_t, 1);
+        uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
+
         trace_amdvi_cache_update(domid, PCI_BUS_NUM(devid), PCI_SLOT(devid),
                 PCI_FUNC(devid), gpa, to_cache.translated_addr);
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb()
  2024-07-31 17:00 [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb() Peter Maydell
@ 2024-07-31 21:07 ` Philippe Mathieu-Daudé
  2025-02-17  7:26 ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-07-31 21:07 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum

On 31/7/24 19:00, Peter Maydell wrote:
> In amdvi_update_iotlb() we will only put a new entry in the hash
> table if to_cache.perm is not IOMMU_NONE.  However we allocate the
> memory for the new AMDVIIOTLBEntry and for the hash table key
> regardless.  This means that in the IOMMU_NONE case we will leak the
> memory we alloacted.
> 
> Move the allocations into the if() to the point where we know we're
> going to add the item to the hash table.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2452
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Tested with 'make check' and 'make check-avocado' only, but the
> bug and fix seem straightforward...
> ---
>   hw/i386/amd_iommu.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb()
  2024-07-31 17:00 [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb() Peter Maydell
  2024-07-31 21:07 ` Philippe Mathieu-Daudé
@ 2025-02-17  7:26 ` Philippe Mathieu-Daudé
  2025-02-20 15:55   ` Alejandro Jimenez
  1 sibling, 1 reply; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-17  7:26 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum

On 31/7/24 19:00, Peter Maydell wrote:
> In amdvi_update_iotlb() we will only put a new entry in the hash
> table if to_cache.perm is not IOMMU_NONE.  However we allocate the
> memory for the new AMDVIIOTLBEntry and for the hash table key
> regardless.  This means that in the IOMMU_NONE case we will leak the
> memory we alloacted.
> 
> Move the allocations into the if() to the point where we know we're
> going to add the item to the hash table.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2452
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Tested with 'make check' and 'make check-avocado' only, but the
> bug and fix seem straightforward...

Oh now I remembered:
https://lore.kernel.org/qemu-devel/CACGkMEtjmpX8G9HYZ0r3n5ErhAENKhQ81f4ocfCYrh=XoF=5hw@mail.gmail.com/

> ---
>   hw/i386/amd_iommu.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 6d4fde72f9b..87643d28917 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -357,12 +357,12 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
>                                  uint64_t gpa, IOMMUTLBEntry to_cache,
>                                  uint16_t domid)
>   {
> -    AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
> -    uint64_t *key = g_new(uint64_t, 1);
> -    uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
> -
>       /* don't cache erroneous translations */
>       if (to_cache.perm != IOMMU_NONE) {
> +        AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
> +        uint64_t *key = g_new(uint64_t, 1);
> +        uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
> +
>           trace_amdvi_cache_update(domid, PCI_BUS_NUM(devid), PCI_SLOT(devid),
>                   PCI_FUNC(devid), gpa, to_cache.translated_addr);
>   



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb()
  2025-02-17  7:26 ` Philippe Mathieu-Daudé
@ 2025-02-20 15:55   ` Alejandro Jimenez
  0 siblings, 0 replies; 4+ messages in thread
From: Alejandro Jimenez @ 2025-02-20 15:55 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Peter Maydell, qemu-devel
  Cc: Michael S. Tsirkin, Marcel Apfelbaum, Suravee Suthikulpanit

+ Suravee

On 2/17/25 02:26, Philippe Mathieu-Daudé wrote:
> On 31/7/24 19:00, Peter Maydell wrote:
>> In amdvi_update_iotlb() we will only put a new entry in the hash
>> table if to_cache.perm is not IOMMU_NONE.  However we allocate the
>> memory for the new AMDVIIOTLBEntry and for the hash table key
>> regardless.  This means that in the IOMMU_NONE case we will leak the
>> memory we alloacted.
>>
>> Move the allocations into the if() to the point where we know we're
>> going to add the item to the hash table.
>>
>> Cc: qemu-stable@nongnu.org
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2452
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Tested with 'make check' and 'make check-avocado' only, but the
>> bug and fix seem straightforward...
> 
> Oh now I remembered:
> https://lore.kernel.org/qemu-devel/CACGkMEtjmpX8G9HYZ0r3n5ErhAENKhQ81f4ocfCYrh=XoF=5hw@mail.gmail.com/

Re: AMD vIOMMU status, this recent series:

https://lore.kernel.org/all/20240927172913.121477-1-santosh.shukla@amd.com/

added support for using it with VFIO and the guest in passthrough mode.

I'm currently working on adding support for DMA remapping, and have a basic prototype that boots a guest with a VFIO assigned device and iommu.passthrough=0, which exercises the change in this patch.

I am using the fixes and optimizations that have gone into the VTD emulation code as a template to bring the AMDVi DMA remap up to a functional state. I'll send an RFC soon once I have fixed a remaining issue with the prototype.

Thank you,
Alejandro

> 
>> ---
>>   hw/i386/amd_iommu.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 6d4fde72f9b..87643d28917 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -357,12 +357,12 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
>>                                  uint64_t gpa, IOMMUTLBEntry to_cache,
>>                                  uint16_t domid)
>>   {
>> -    AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
>> -    uint64_t *key = g_new(uint64_t, 1);
>> -    uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
>> -
>>       /* don't cache erroneous translations */
>>       if (to_cache.perm != IOMMU_NONE) {
>> +        AMDVIIOTLBEntry *entry = g_new(AMDVIIOTLBEntry, 1);
>> +        uint64_t *key = g_new(uint64_t, 1);
>> +        uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
>> +
>>           trace_amdvi_cache_update(domid, PCI_BUS_NUM(devid), PCI_SLOT(devid),
>>                   PCI_FUNC(devid), gpa, to_cache.translated_addr);
> 
> 



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-02-20 15:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-31 17:00 [PATCH] hw/i386/amd_iommu: Don't leak memory in amdvi_update_iotlb() Peter Maydell
2024-07-31 21:07 ` Philippe Mathieu-Daudé
2025-02-17  7:26 ` Philippe Mathieu-Daudé
2025-02-20 15:55   ` Alejandro Jimenez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).