IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: guptap@codeaurora.org
To: Robin Murphy <robin.murphy@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: mhocko@suse.com, owner-linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	linux-mm@kvack.org, iommu@lists.linux-foundation.org
Subject: Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
Date: Fri, 22 May 2020 11:55:44 +0530
Message-ID: <90662ef3123dbf2e93f9718ee5cc14a7@codeaurora.org> (raw)
In-Reply-To: <7aaa8dcc-6a47-f256-431d-2a1b034b4076@arm.com>

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

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 11:30 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 [this message]
2020-05-22  9:24     ` Robin Murphy
2020-05-26  7:19       ` guptap

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=90662ef3123dbf2e93f9718ee5cc14a7@codeaurora.org \
    --to=guptap@codeaurora.org \
    --cc=akpm@linux-foundation.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=owner-linux-mm@kvack.org \
    --cc=robin.murphy@arm.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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