From mboxrd@z Thu Jan 1 00:00:00 1970 From: Magnus Damm Subject: Re: [PATCH 2/3] iommu/dma: Use correct offset in map_sg Date: Thu, 10 Mar 2016 16:47:14 +0900 Message-ID: References: <9a84191ed813c23db7901f8c73f514d081495bdf.1450457730.git.robin.murphy@arm.com> <4812a34857b081e45c36d7e887840f3675da74dc.1450457730.git.robin.murphy@arm.com> <56E03A83.9050909@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56E03A83.9050909-5wv7dgnIgG8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Robin Murphy Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: iommu@lists.linux-foundation.org Hi Robin, On Thu, Mar 10, 2016 at 12:00 AM, Robin Murphy 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 >> 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 >>> --- >>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: magnus.damm@gmail.com (Magnus Damm) Date: Thu, 10 Mar 2016 16:47:14 +0900 Subject: [PATCH 2/3] iommu/dma: Use correct offset in map_sg In-Reply-To: <56E03A83.9050909@arm.com> References: <9a84191ed813c23db7901f8c73f514d081495bdf.1450457730.git.robin.murphy@arm.com> <4812a34857b081e45c36d7e887840f3675da74dc.1450457730.git.robin.murphy@arm.com> <56E03A83.9050909@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Robin, On Thu, Mar 10, 2016 at 12:00 AM, Robin Murphy 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 >> 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 >>> --- >>> 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