From: guptap@codeaurora.org
To: Robin Murphy <robin.murphy@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
mhocko@suse.com, joro@8bytes.org, linux-mm@kvack.org,
iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
owner-linux-mm@kvack.org, stable@vger.kernel.org
Subject: Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova
Date: Tue, 02 Jun 2020 15:09:29 +0530 [thread overview]
Message-ID: <2bfb4ce3a2dfeb2d585f0308a9002feb@codeaurora.org> (raw)
In-Reply-To: <4ba082d3bb965524157704ea1ffb1ff4@codeaurora.org>
On 2020-05-26 12:49, guptap@codeaurora.org wrote:
> 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
ping?
next prev parent reply other threads:[~2020-06-02 9:39 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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
2020-06-02 9:39 ` guptap [this message]
2020-06-02 13:07 ` Robin Murphy
2020-06-03 7:37 ` 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=2bfb4ce3a2dfeb2d585f0308a9002feb@codeaurora.org \
--to=guptap@codeaurora.org \
--cc=akpm@linux-foundation.org \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).