Hi Magnus,
Thanks for bringing this up...
On 09/03/16 07:50, Magnus Damm wrote:
On Sat, Dec 19, 2015 at 2:01 AM, Robin Murphy <[email protected]> wrote:
When mapping a non-page-aligned scatterlist entry, we copy the original
offset to the output DMA address before aligning it to hand off to
iommu_map_sg(), then later adding the IOVA page address portion to get
the final mapped address. However, when the IOVA page size is smaller
than the CPU page size, it is the offset within the IOVA page we want,
not that within the CPU page, which can easily be larger than an IOVA
page and thus result in an incorrect final address.
Fix the bug by taking only the IOVA-aligned part of the offset as the
basis of the DMA address, not the whole thing.
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/dma-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 982e716..03811e3 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -458,7 +458,7 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist
*sg,
size_t s_length = s->length;
size_t pad_len = (mask - iova_len + 1) & mask;
- sg_dma_address(s) = s->offset;
+ sg_dma_address(s) = s_offset;
sg_dma_len(s) = s_length;
s->offset -= s_offset;
s_length = iova_align(iovad, s_length + s_offset);
--
1.9.1
Hi Robin,
Thanks a lot for your fix! While I don't have any doubt that your
patch fixes a real issue I wonder if another update is needed.
Depending on what is expected perhaps just the comment above the code
wants an update or maybe the "un-swizzling" needs more work. With this
patch applied the code looks semi-complete to me at this point.
Currently the comment just above the hunk says:
/*
* Work out how much IOVA space we need, and align the segments to
* IOVA granules for the IOMMU driver to handle. With some clever
* trickery we can modify the list in-place, but reversibly, by
* hiding the original data in the as-yet-unused DMA fields.
*/
With your fix the "original data" is no longer stored in the unused
DMA fields.
OK, so we're now moving some of the data rather than taking a literal
copy, but the point remains that we're not throwing any information away
- we can move the remainder back again if necessary. As far as I'm
concerned the comment is still valid, but if it's open to
misinterpretation I can try rephrasing it.
Instead the s_offset value is stored as modified in
sg_dma_address() which in turn will make the iommu_dma_map_sg()
function return with modified sg->s_offset both on success and
failure.
Perhaps this is intentional design, or maybe __invalidate_sg() and
__finalize_sg() both need to support roll back? Any ideas?
What's missing is that some idiot forgot about the hard-to-exercise
failure path and didn't update __invalidate_sg() to match. I'll get
right on that...
Robin.
Thanks,
/ magnus
My untested hack to support roll back on top of next-20160308 does
something like this...
--- 0001/drivers/iommu/dma-iommu.c
+++ work/drivers/iommu/dma-iommu.c 2016-03-09 16:33:21.250513000 +0900
@@ -392,7 +392,7 @@ void iommu_dma_unmap_page(struct device
* Handling IOVA concatenation can come later, if needed
*/
static int __finalise_sg(struct device *dev, struct scatterlist *sg, int
nents,
- dma_addr_t dma_addr)
+ dma_addr_t dma_addr, struct iova_domain *iovad)
{
struct scatterlist *s;
int i;
@@ -405,7 +405,7 @@ static int __finalise_sg(struct device *
s->offset = s_offset;
s->length = s_length;
- sg_dma_address(s) = dma_addr + s_offset;
+ sg_dma_address(s) = dma_addr + iova_offset(iovad, s_offset);
dma_addr += s_dma_len;
}
return i;
@@ -455,11 +455,13 @@ int iommu_dma_map_sg(struct device *dev,
* hiding the original data in the as-yet-unused DMA fields.
*/
for_each_sg(sg, s, nents, i) {
- size_t s_offset = iova_offset(iovad, s->offset);
+ size_t s_offset = s->offset;
size_t s_length = s->length;
sg_dma_address(s) = s_offset;
sg_dma_len(s) = s_length;
+
+ s_offset = iova_offset(iovad, s_offset);
s->offset -= s_offset;
s_length = iova_align(iovad, s_length + s_offset);
s->length = s_length;
@@ -494,7 +496,7 @@ int iommu_dma_map_sg(struct device *dev,
if (iommu_map_sg(domain, dma_addr, sg, nents, prot) < iova_len)
goto out_free_iova;
- return __finalise_sg(dev, sg, nents, dma_addr);
+ return __finalise_sg(dev, sg, nents, dma_addr, iovad);
out_free_iova:
__free_iova(iovad, iova);
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
