Re: [PATCH] iommu/dma: limit iova free size to unmmaped iova

From: guptap
Date: Tue May 26 2020 - 03:19:32 EST


On 2020-05-22 14:54, Robin Murphy wrote:
On 2020-05-22 07:25, guptap@xxxxxxxxxxxxxx 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