IOMMU Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iommu/dma: limit iova free size to unmmaped iova
@ 2020-05-21 11:30 Prakash Gupta
  2020-05-21 18:49 ` Andrew Morton
  2020-05-21 20:16 ` Robin Murphy
  0 siblings, 2 replies; 7+ messages in thread
From: Prakash Gupta @ 2020-05-21 11:30 UTC (permalink / raw)
  To: akpm, mhocko, joro; +Cc: linux-mm, iommu, linux-kernel, Prakash Gupta

Limit the iova size while freeing based on unmapped size. In absence of
this even with unmap failure, invalid iova is pushed to iova rcache and
subsequently can cause panic while rcache magazine is freed.

Signed-off-by: Prakash Gupta <guptap@codeaurora.org>

:100644 100644 4959f5df21bd 098f7d377e04 M	drivers/iommu/dma-iommu.c

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4959f5df21bd..098f7d377e04 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -472,7 +472,8 @@ static void __iommu_dma_unmap(struct device *dev, dma_addr_t dma_addr,
 
 	if (!cookie->fq_domain)
 		iommu_tlb_sync(domain, &iotlb_gather);
-	iommu_dma_free_iova(cookie, dma_addr, size);
+	if (unmapped)
+		iommu_dma_free_iova(cookie, dma_addr, unmapped);
 }
 
 static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

* Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
  2020-05-21 11:30 [PATCH] iommu/dma: limit iova free size to unmmaped iova Prakash Gupta
@ 2020-05-21 18:49 ` Andrew Morton
  2020-05-25 14:28   ` Joerg Roedel
  2020-05-21 20:16 ` Robin Murphy
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Morton @ 2020-05-21 18:49 UTC (permalink / raw)
  To: Prakash Gupta; +Cc: linux-mm, mhocko, iommu, linux-kernel

On Thu, 21 May 2020 17:00:04 +0530 Prakash Gupta <guptap@codeaurora.org> wrote:

> Limit the iova size while freeing based on unmapped size. In absence of
> this even with unmap failure, invalid iova is pushed to iova rcache and
> subsequently can cause panic while rcache magazine is freed.
> 
> Signed-off-by: Prakash Gupta <guptap@codeaurora.org>
> 

I think we need a cc:stable here?

> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -472,7 +472,8 @@ static void __iommu_dma_unmap(struct device *dev, dma_addr_t dma_addr,
>  
>  	if (!cookie->fq_domain)
>  		iommu_tlb_sync(domain, &iotlb_gather);
> -	iommu_dma_free_iova(cookie, dma_addr, size);
> +	if (unmapped)
> +		iommu_dma_free_iova(cookie, dma_addr, unmapped);
>  }
>  
>  static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,

I'll assume that Joerg will handle this fix?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

* Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
  2020-05-21 11:30 [PATCH] iommu/dma: limit iova free size to unmmaped iova Prakash Gupta
  2020-05-21 18:49 ` Andrew Morton
@ 2020-05-21 20:16 ` Robin Murphy
  2020-05-22  6:25   ` guptap
  1 sibling, 1 reply; 7+ messages in thread
From: Robin Murphy @ 2020-05-21 20:16 UTC (permalink / raw)
  To: Prakash Gupta, akpm, mhocko, joro; +Cc: linux-mm, iommu, linux-kernel

On 2020-05-21 12:30, Prakash Gupta wrote:
> Limit the iova size while freeing based on unmapped size. In absence of
> this even with unmap failure, invalid iova is pushed to iova rcache and
> subsequently can cause panic while rcache magazine is freed.

Can you elaborate on that panic?

> Signed-off-by: Prakash Gupta <guptap@codeaurora.org>
> 
> :100644 100644 4959f5df21bd 098f7d377e04 M	drivers/iommu/dma-iommu.c
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 4959f5df21bd..098f7d377e04 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -472,7 +472,8 @@ static void __iommu_dma_unmap(struct device *dev, dma_addr_t dma_addr,
>   
>   	if (!cookie->fq_domain)
>   		iommu_tlb_sync(domain, &iotlb_gather);
> -	iommu_dma_free_iova(cookie, dma_addr, size);
> +	if (unmapped)
> +		iommu_dma_free_iova(cookie, dma_addr, unmapped);

Frankly, if any part of the unmap fails then things have gone 
catastrophically wrong already, but either way this isn't right. The 
IOVA API doesn't support partial freeing - an IOVA *must* be freed with 
its original size, or not freed at all, otherwise it will corrupt the 
state of the rcaches and risk a cascade of further misbehaviour for 
future callers.

TBH my gut feeling here is that you're really just trying to treat a 
symptom of another bug elsewhere, namely some driver calling dma_unmap_* 
or dma_free_* with the wrong address or size in the first place.

Robin.

>   }
>   
>   static dma_addr_t __iommu_dma_map(struct device *dev, phys_addr_t phys,
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

* Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
  2020-05-21 20:16 ` Robin Murphy
@ 2020-05-22  6:25   ` guptap
  2020-05-22  9:24     ` Robin Murphy
  0 siblings, 1 reply; 7+ messages in thread
From: guptap @ 2020-05-22  6:25 UTC (permalink / raw)
  To: Robin Murphy, Andrew Morton
  Cc: mhocko, owner-linux-mm, linux-kernel, stable, linux-mm, iommu

On 2020-05-22 01:46, Robin Murphy wrote:
> On 2020-05-21 12:30, Prakash Gupta wrote:
>> Limit the iova size while freeing based on unmapped size. In absence 
>> of
>> this even with unmap failure, invalid iova is pushed to iova rcache 
>> and
>> subsequently can cause panic while rcache magazine is freed.
> 
> Can you elaborate on that panic?
> 
We have seen couple of stability issues around this.
Below is one such example:

kernel BUG at kernel/msm-4.19/drivers/iommu/iova.c:904!
iova_magazine_free_pfns
iova_rcache_insert
free_iova_fast
__iommu_unmap_page
iommu_dma_unmap_page


It turned out an iova pfn 0 got into iova_rcache. One possibility I see 
is
where client unmap with invalid dma_addr. The unmap call will fail and 
warn on
and still try to free iova. This will cause invalid pfn to be inserted 
into
rcache. As and when the magazine with invalid pfn will be freed
private_find_iova() will return NULL for invalid iova and meet bug 
condition.

>> Signed-off-by: Prakash Gupta <guptap@codeaurora.org>
>> 
>> :100644 100644 4959f5df21bd 098f7d377e04 M	drivers/iommu/dma-iommu.c
>> 
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 4959f5df21bd..098f7d377e04 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -472,7 +472,8 @@ static void __iommu_dma_unmap(struct device *dev, 
>> dma_addr_t dma_addr,
>>     	if (!cookie->fq_domain)
>>   		iommu_tlb_sync(domain, &iotlb_gather);
>> -	iommu_dma_free_iova(cookie, dma_addr, size);
>> +	if (unmapped)
>> +		iommu_dma_free_iova(cookie, dma_addr, unmapped);
> 
> Frankly, if any part of the unmap fails then things have gone
> catastrophically wrong already, but either way this isn't right. The
> IOVA API doesn't support partial freeing - an IOVA *must* be freed
> with its original size, or not freed at all, otherwise it will corrupt
> the state of the rcaches and risk a cascade of further misbehaviour
> for future callers.
> 
I agree, we shouldn't be freeing the partial iova. Instead just making
sure if unmap was successful should be sufficient before freeing iova. 
So change
can instead be something like this:

-	iommu_dma_free_iova(cookie, dma_addr, size);
+	if (unmapped)
+		iommu_dma_free_iova(cookie, dma_addr, size);

> TBH my gut feeling here is that you're really just trying to treat a
> symptom of another bug elsewhere, namely some driver calling
> dma_unmap_* or dma_free_* with the wrong address or size in the first
> place.
> 
This condition would arise only if driver calling dma_unmap/free_* with 
0
iova_pfn. This will be flagged with a warning during unmap but will 
trigger
panic later on while doing unrelated dma_map/unmap_*. If unmapped has 
already
failed for invalid iova, there is no reason we should consider this as 
valid
iova and free. This part should be fixed.

On 2020-05-22 00:19, Andrew Morton wrote:
> I think we need a cc:stable here?
> 
Added now.

Thanks,
Prakash
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

* Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
  2020-05-22  6:25   ` guptap
@ 2020-05-22  9:24     ` Robin Murphy
  2020-05-26  7:19       ` guptap
  0 siblings, 1 reply; 7+ messages in thread
From: Robin Murphy @ 2020-05-22  9:24 UTC (permalink / raw)
  To: guptap, Andrew Morton
  Cc: mhocko, owner-linux-mm, linux-kernel, stable, linux-mm, iommu

On 2020-05-22 07:25, guptap@codeaurora.org wrote:
> On 2020-05-22 01:46, Robin Murphy wrote:
>> On 2020-05-21 12:30, Prakash Gupta wrote:
>>> Limit the iova size while freeing based on unmapped size. In absence of
>>> this even with unmap failure, invalid iova is pushed to iova rcache and
>>> subsequently can cause panic while rcache magazine is freed.
>>
>> Can you elaborate on that panic?
>>
> We have seen couple of stability issues around this.
> Below is one such example:
> 
> kernel BUG at kernel/msm-4.19/drivers/iommu/iova.c:904!
> iova_magazine_free_pfns
> iova_rcache_insert
> free_iova_fast
> __iommu_unmap_page
> iommu_dma_unmap_page

OK, so it's not some NULL dereference or anything completely unexpected, 
that's good.

> It turned out an iova pfn 0 got into iova_rcache. One possibility I see is
> where client unmap with invalid dma_addr. The unmap call will fail and 
> warn on
> and still try to free iova. This will cause invalid pfn to be inserted into
> rcache. As and when the magazine with invalid pfn will be freed
> private_find_iova() will return NULL for invalid iova and meet bug 
> condition.

That would indeed be a bug in whatever driver made the offending 
dma_unmap call.

>>> Signed-off-by: Prakash Gupta <guptap@codeaurora.org>
>>>
>>> :100644 100644 4959f5df21bd 098f7d377e04 M    drivers/iommu/dma-iommu.c
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 4959f5df21bd..098f7d377e04 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -472,7 +472,8 @@ static void __iommu_dma_unmap(struct device *dev, 
>>> dma_addr_t dma_addr,
>>>         if (!cookie->fq_domain)
>>>           iommu_tlb_sync(domain, &iotlb_gather);
>>> -    iommu_dma_free_iova(cookie, dma_addr, size);
>>> +    if (unmapped)
>>> +        iommu_dma_free_iova(cookie, dma_addr, unmapped);
>>
>> Frankly, if any part of the unmap fails then things have gone
>> catastrophically wrong already, but either way this isn't right. The
>> IOVA API doesn't support partial freeing - an IOVA *must* be freed
>> with its original size, or not freed at all, otherwise it will corrupt
>> the state of the rcaches and risk a cascade of further misbehaviour
>> for future callers.
>>
> I agree, we shouldn't be freeing the partial iova. Instead just making
> sure if unmap was successful should be sufficient before freeing iova. 
> So change
> can instead be something like this:
> 
> -    iommu_dma_free_iova(cookie, dma_addr, size);
> +    if (unmapped)
> +        iommu_dma_free_iova(cookie, dma_addr, size);
> 
>> TBH my gut feeling here is that you're really just trying to treat a
>> symptom of another bug elsewhere, namely some driver calling
>> dma_unmap_* or dma_free_* with the wrong address or size in the first
>> place.
>>
> This condition would arise only if driver calling dma_unmap/free_* with 0
> iova_pfn. This will be flagged with a warning during unmap but will trigger
> panic later on while doing unrelated dma_map/unmap_*. If unmapped has 
> already
> failed for invalid iova, there is no reason we should consider this as 
> valid
> iova and free. This part should be fixed.

I disagree. In general, if drivers call the DMA API incorrectly it is 
liable to lead to data loss, memory corruption, and various other 
unpleasant misbehaviour - it is not the DMA layer's job to attempt to 
paper over driver bugs.

There *is* an argument for downgrading the BUG_ON() in 
iova_magazine_free_pfns() to a WARN_ON(), since frankly it isn't a 
sufficiently serious condition to justify killing the whole machine 
immediately, but NAK to bodging the iommu-dma mid-layer to "fix" that. A 
serious bug already happened elsewhere, so trying to hide the fallout 
really doesn't help anyone.

Robin.

> On 2020-05-22 00:19, Andrew Morton wrote:
>> I think we need a cc:stable here?
>>
> Added now.
> 
> Thanks,
> Prakash
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

* Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
  2020-05-21 18:49 ` Andrew Morton
@ 2020-05-25 14:28   ` Joerg Roedel
  0 siblings, 0 replies; 7+ messages in thread
From: Joerg Roedel @ 2020-05-25 14:28 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-mm, iommu, mhocko, linux-kernel, Prakash Gupta

On Thu, May 21, 2020 at 11:49:06AM -0700, Andrew Morton wrote:
> I'll assume that Joerg will handle this fix?

Yes, I will when it turns out necessary.


Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

* Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
  2020-05-22  9:24     ` Robin Murphy
@ 2020-05-26  7:19       ` guptap
  0 siblings, 0 replies; 7+ messages in thread
From: guptap @ 2020-05-26  7:19 UTC (permalink / raw)
  To: Robin Murphy
  Cc: mhocko, owner-linux-mm, linux-kernel, stable, linux-mm, iommu,
	Andrew Morton

On 2020-05-22 14:54, Robin Murphy wrote:
> On 2020-05-22 07:25, guptap@codeaurora.org wrote:
>> On 2020-05-22 01:46, Robin Murphy wrote:
>>> On 2020-05-21 12:30, Prakash Gupta wrote:
>> I agree, we shouldn't be freeing the partial iova. Instead just making
>> sure if unmap was successful should be sufficient before freeing iova. 
>> So change
>> can instead be something like this:
>> 
>> -    iommu_dma_free_iova(cookie, dma_addr, size);
>> +    if (unmapped)
>> +        iommu_dma_free_iova(cookie, dma_addr, size);
>> 
>>> TBH my gut feeling here is that you're really just trying to treat a
>>> symptom of another bug elsewhere, namely some driver calling
>>> dma_unmap_* or dma_free_* with the wrong address or size in the first
>>> place.
>>> 
>> This condition would arise only if driver calling dma_unmap/free_* 
>> with 0
>> iova_pfn. This will be flagged with a warning during unmap but will 
>> trigger
>> panic later on while doing unrelated dma_map/unmap_*. If unmapped has 
>> already
>> failed for invalid iova, there is no reason we should consider this as 
>> valid
>> iova and free. This part should be fixed.
> 
> I disagree. In general, if drivers call the DMA API incorrectly it is
> liable to lead to data loss, memory corruption, and various other
> unpleasant misbehaviour - it is not the DMA layer's job to attempt to
> paper over driver bugs.
> 
> There *is* an argument for downgrading the BUG_ON() in
> iova_magazine_free_pfns() to a WARN_ON(), since frankly it isn't a
> sufficiently serious condition to justify killing the whole machine
> immediately, but NAK to bodging the iommu-dma mid-layer to "fix" that.
> A serious bug already happened elsewhere, so trying to hide the
> fallout really doesn't help anyone.
> 
Sorry for delayed response, it was a long weekend.
I agree that invalid DMA API call can result in unexpected issues and 
client
should fix it, but then the present behavior makes it difficult to catch 
cases
when driver is making wrong DMA API calls. When invalid iova pfn is 
passed it
doesn't fail then and there, though DMA layer is aware of iova being 
invalid. It
fails much after that in the context of an valid map/unmap, with 
BUG_ON().

Downgrading BUG_ON() to WARN_ON() in iova_magazine_free_pfns() will not 
help
much as invalid iova will cause NULL pointer dereference.

I see no reason why DMA layer wants to free an iova for which unmapped 
failed.
IMHO queuing an invalid iova (which already failed unmap) to rcache 
which
eventually going to crash the system looks like iommu-dma layer issue.

Thanks,
Prakash
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21 11:30 [PATCH] iommu/dma: limit iova free size to unmmaped iova Prakash Gupta
2020-05-21 18:49 ` Andrew Morton
2020-05-25 14:28   ` Joerg Roedel
2020-05-21 20:16 ` Robin Murphy
2020-05-22  6:25   ` guptap
2020-05-22  9:24     ` Robin Murphy
2020-05-26  7:19       ` guptap

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git