On 2020-05-22 07:25, guptap@xxxxxxxxxxxxxx wrote:Sorry for delayed response, it was a long weekend.
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 aThis condition would arise only if driver calling dma_unmap/free_* with 0
symptom of another bug elsewhere, namely some driver calling
dma_unmap_* or dma_free_* with the wrong address or size in the first
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.