[PATCH] iommu/arm-smmu: fix leak in arm_smmu_flush_pgtable

[PATCH] iommu/arm-smmu: fix leak in arm_smmu_flush_pgtable

[Mitchel Humpherys]

We're currently mapping a page in arm_smmu_flush_pgtable without ever
unmapping it.  Fix this by calling dma_unmap_page on the returned dma
address.  Since the only reason we're calling dma_map_page is to make
sure it actually gets flushed out to RAM, we can just call
dma_unmap_page immediately following the map.

Without this, eventually swiotlb runs out of memory and starts printing
things like:

    [   35.545076] arm-smmu d00000.arm,smmu: swiotlb buffer is full (sz: 128 bytes)

Signed-off-by: Mitchel Humpherys <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index fc13dd56953e..2ff8f35cf533 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -627,8 +627,13 @@ static void arm_smmu_flush_pgtable(void *addr, size_t size, void *cookie)
 		 * recursion here as the SMMU table walker will not be wired
 		 * through another SMMU.
 		 */
-		dma_map_page(smmu->dev, virt_to_page(addr), offset, size,
-			     DMA_TO_DEVICE);
+		dma_addr_t handle = dma_map_page(smmu->dev, virt_to_page(addr),
+						offset, size, DMA_TO_DEVICE);
+		if (handle == DMA_ERROR_CODE)
+			dev_err(smmu->dev,
+				"Couldn't flush page tables at %p!\n", addr);
+		else
+			dma_unmap_page(smmu->dev, handle, size, DMA_TO_DEVICE);
 	}
 }
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] iommu/arm-smmu: fix leak in arm_smmu_flush_pgtable

[Robin Murphy]

Hi Mitch,

On 05/03/15 00:18, Mitchel Humpherys wrote:
> We're currently mapping a page in arm_smmu_flush_pgtable without ever
> unmapping it.  Fix this by calling dma_unmap_page on the returned dma
> address.  Since the only reason we're calling dma_map_page is to make
> sure it actually gets flushed out to RAM, we can just call
> dma_unmap_page immediately following the map.
>
> Without this, eventually swiotlb runs out of memory and starts printing
> things like:
>
>      [   35.545076] arm-smmu d00000.arm,smmu: swiotlb buffer is full (sz: 128 bytes)
>

So, you have non-coherent SMMUs too ;) The real problem is that the 
SMMU's DMA mask is wrong (as it happens I've just given Will a patch to 
fix that) - this is really just doing a whole bunch of unnecessary work 
(two memory copies and two cache flushes, one of which isn't even 
flushing the right area) to hide the problem. With an appropriate DMA 
mask set, swiotlb_map_page becomes a no-op and we fall through to the 
cache flush without ever allocating anything.

Robin.

> Signed-off-by: Mitchel Humpherys <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index fc13dd56953e..2ff8f35cf533 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -627,8 +627,13 @@ static void arm_smmu_flush_pgtable(void *addr, size_t size, void *cookie)
>   		 * recursion here as the SMMU table walker will not be wired
>   		 * through another SMMU.
>   		 */
> -		dma_map_page(smmu->dev, virt_to_page(addr), offset, size,
> -			     DMA_TO_DEVICE);
> +		dma_addr_t handle = dma_map_page(smmu->dev, virt_to_page(addr),
> +						offset, size, DMA_TO_DEVICE);
> +		if (handle == DMA_ERROR_CODE)
> +			dev_err(smmu->dev,
> +				"Couldn't flush page tables at %p!\n", addr);
> +		else
> +			dma_unmap_page(smmu->dev, handle, size, DMA_TO_DEVICE);
>   	}
>   }
>
>

Re: [PATCH] iommu/arm-smmu: fix leak in arm_smmu_flush_pgtable

[Mitchel Humpherys]

On Thu, Mar 05 2015 at 02:38:45 AM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Mitch,
>
> On 05/03/15 00:18, Mitchel Humpherys wrote:
>> We're currently mapping a page in arm_smmu_flush_pgtable without ever
>> unmapping it.  Fix this by calling dma_unmap_page on the returned dma
>> address.  Since the only reason we're calling dma_map_page is to make
>> sure it actually gets flushed out to RAM, we can just call
>> dma_unmap_page immediately following the map.
>>
>> Without this, eventually swiotlb runs out of memory and starts printing
>> things like:
>>
>>      [   35.545076] arm-smmu d00000.arm,smmu: swiotlb buffer is full (sz: 128 bytes)
>>
>
> So, you have non-coherent SMMUs too ;) The real problem is that the SMMU's
> DMA mask is wrong (as it happens I've just given Will a patch to fix that)
> - this is really just doing a whole bunch of unnecessary work (two memory
> copies and two cache flushes, one of which isn't even flushing the right
> area) to hide the problem. With an appropriate DMA mask set,
> swiotlb_map_page becomes a no-op and we fall through to the cache flush
> without ever allocating anything.

Yeah I noticed that as well...  But isn't this still incorrect usage of
the API (DMA-API-HOWTO.txt seems to indicate that calls to map should
always be balanced with calls to unmap)?  What we really want to do here
is just call __dma_map_area directly, but the comment on that guy
expressly forbids it...  Not sure what's worse, abusing the DMA API or
disobeying that comment?


-Mitch

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] iommu/arm-smmu: fix leak in arm_smmu_flush_pgtable

[Will Deacon]

On Thu, Mar 05, 2015 at 05:28:02PM +0000, Mitchel Humpherys wrote:
> On Thu, Mar 05 2015 at 02:38:45 AM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> > On 05/03/15 00:18, Mitchel Humpherys wrote:
> >> We're currently mapping a page in arm_smmu_flush_pgtable without ever
> >> unmapping it.  Fix this by calling dma_unmap_page on the returned dma
> >> address.  Since the only reason we're calling dma_map_page is to make
> >> sure it actually gets flushed out to RAM, we can just call
> >> dma_unmap_page immediately following the map.
> >>
> >> Without this, eventually swiotlb runs out of memory and starts printing
> >> things like:
> >>
> >>      [   35.545076] arm-smmu d00000.arm,smmu: swiotlb buffer is full (sz: 128 bytes)
> >>
> >
> > So, you have non-coherent SMMUs too ;) The real problem is that the SMMU's
> > DMA mask is wrong (as it happens I've just given Will a patch to fix that)
> > - this is really just doing a whole bunch of unnecessary work (two memory
> > copies and two cache flushes, one of which isn't even flushing the right
> > area) to hide the problem. With an appropriate DMA mask set,
> > swiotlb_map_page becomes a no-op and we fall through to the cache flush
> > without ever allocating anything.
> 
> Yeah I noticed that as well...  But isn't this still incorrect usage of
> the API (DMA-API-HOWTO.txt seems to indicate that calls to map should
> always be balanced with calls to unmap)?  What we really want to do here
> is just call __dma_map_area directly, but the comment on that guy
> expressly forbids it...  Not sure what's worse, abusing the DMA API or
> disobeying that comment?

I'm not against adding the unmap, but the reasoning would have to change
slightly (it's not related to swiotlb) and it could wait until 4.1 since
Robin's fix actually addresses the root cause.

Will

Re: [PATCH] iommu/arm-smmu: fix leak in arm_smmu_flush_pgtable

[Robin Murphy]

On 05/03/15 17:28, Mitchel Humpherys wrote:
> On Thu, Mar 05 2015 at 02:38:45 AM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>> Hi Mitch,
>>
>> On 05/03/15 00:18, Mitchel Humpherys wrote:
>>> We're currently mapping a page in arm_smmu_flush_pgtable without ever
>>> unmapping it.  Fix this by calling dma_unmap_page on the returned dma
>>> address.  Since the only reason we're calling dma_map_page is to make
>>> sure it actually gets flushed out to RAM, we can just call
>>> dma_unmap_page immediately following the map.
>>>
>>> Without this, eventually swiotlb runs out of memory and starts printing
>>> things like:
>>>
>>>       [   35.545076] arm-smmu d00000.arm,smmu: swiotlb buffer is full (sz: 128 bytes)
>>>
>>
>> So, you have non-coherent SMMUs too ;) The real problem is that the SMMU's
>> DMA mask is wrong (as it happens I've just given Will a patch to fix that)
>> - this is really just doing a whole bunch of unnecessary work (two memory
>> copies and two cache flushes, one of which isn't even flushing the right
>> area) to hide the problem. With an appropriate DMA mask set,
>> swiotlb_map_page becomes a no-op and we fall through to the cache flush
>> without ever allocating anything.
>
> Yeah I noticed that as well...  But isn't this still incorrect usage of
> the API (DMA-API-HOWTO.txt seems to indicate that calls to map should
> always be balanced with calls to unmap)?  What we really want to do here
> is just call __dma_map_area directly, but the comment on that guy
> expressly forbids it...  Not sure what's worse, abusing the DMA API or
> disobeying that comment?

Actually, I think we've figured out a fairly nice way to make the core 
io-pgtbl code more DMA-API aware, so we can make all the appropriate 
calls for the way we're actually using the page tables - I'm having a go 
at a patch now...

Robin.