[PATCH 2/3] iommu/dma: Use correct offset in map_sg

[Magnus Damm]

Hi Robin,

On Thu, Mar 10, 2016 at 12:00 AM, Robin Murphy <robin.murphy at arm.com> wrote:
> Hi Magnus,
>
> Thanks for bringing this up...

No worries!

> On 09/03/16 07:50, Magnus Damm wrote:
>>
>> On Sat, Dec 19, 2015 at 2:01 AM, Robin Murphy <robin.murphy at arm.com>
>> 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 <robin.murphy at arm.com>
>>> ---
>>>   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.

Thanks, I agree with you about the comment! As long as the fields can
be restored everything is fine.

>> 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...

Oh well. Fixing the error case sounds good. I don't have any special
test case to trigger anything, so testing is a bit difficult for me.
Apart from that I'm happy to help - let me know if you can think of
something.

Cheers,

/ magnus