From: Robin Murphy <robin.murphy@arm.com> To: Marc Orr <marcorr@google.com> Cc: hch@lst.de, m.szyprowski@samsung.com, Jianxiong Gao <jxgao@google.com>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] dma: mark unmapped DMA scatter/gather invalid Date: Mon, 11 Jan 2021 20:39:35 +0000 [thread overview] Message-ID: <bbf6f07c-369b-e470-78ff-815cfb4dbf92@arm.com> (raw) In-Reply-To: <CAA03e5HLN_iPzXRTJ_Dvx5h8KP-Qi+=6u+9mHnaoB9oBMMWs7Q@mail.gmail.com> On 2021-01-11 18:03, Marc Orr wrote: >> On 2021-01-11 15:43, Marc Orr wrote: > > minus stable@vger.kernel.org, per gregkh@'s email. > >>> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c >>> index 0a4881e59aa7..3d9b17fe5771 100644 >>> --- a/kernel/dma/direct.c >>> +++ b/kernel/dma/direct.c >>> @@ -374,9 +374,11 @@ void dma_direct_unmap_sg(struct device *dev, struct scatterlist *sgl, >>> struct scatterlist *sg; >>> int i; >>> >>> - for_each_sg(sgl, sg, nents, i) >>> + for_each_sg(sgl, sg, nents, i) { >>> dma_direct_unmap_page(dev, sg->dma_address, sg_dma_len(sg), dir, >>> attrs); >>> + sg->dma_address = DMA_MAPPING_ERROR; >> >> There are more DMA API backends than just dma-direct, so while this >> might help paper over bugs when SWIOTLB is in use, it's not going to >> have any effect when those same bugs are hit under other circumstances. >> Once again, the moral of the story is that effort is better spent just >> fixing the bugs ;) > > Thanks for the quick feedback. What is the correct fix? I understand > the first half. The NVMe driver should be updated to not call unmap on > an address that has already been unmapped within the DMA direct code. > Where I'm less certain is how to communicate to the NVMe driver that > the mapping failed. In particular, the NVMe code explicitly checks if > the first DMA address in the scatter/gather list is set to > DMA_MAPPING_ERROR. Thus, don't we need to update the DMA direct code > to propagate DMA_MAPPING_ERROR back up to the driver, via the > scatter/gather struct? Erm, you check the return value of dma_map_sg(). If it's zero, the request failed; if it's nonzero, that's how many DMA segments you now have to process. See Documentation/core-api/dma-api.rst. The only guarantee offered about the state of the scatterlist itself is that if it is successfully mapped, then the dma_address and dma_length fields are valid for that many segments, and if that is fewer than the total number of physical segments then the next one after the final DMA segment will have a dma_length of 0. In particular there are no guarantees at all about the state if the mapping was unsuccessful. If a driver is failing to keep track of the success/failure status and later down the line trying to guess what to do with a list that may or may not have been mapped, then frankly that driver should be redesigned because that is a terrible anti-pattern. At the very very least it should explicitly encode its own "known bad" state upon failure that it can then reliably recognise later. Robin. > I skimmed arch/arm/mm/dma-mapping.c, just now. I can see that this > code sets the address within the scatter/gather struct to > DMA_MAPPING_ERROR before trying to map an IO address and write it into > the struct. Is this a good example to follow? > > Thanks, > Marc >
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Marc Orr <marcorr@google.com> Cc: linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, Jianxiong Gao <jxgao@google.com>, hch@lst.de Subject: Re: [PATCH] dma: mark unmapped DMA scatter/gather invalid Date: Mon, 11 Jan 2021 20:39:35 +0000 [thread overview] Message-ID: <bbf6f07c-369b-e470-78ff-815cfb4dbf92@arm.com> (raw) In-Reply-To: <CAA03e5HLN_iPzXRTJ_Dvx5h8KP-Qi+=6u+9mHnaoB9oBMMWs7Q@mail.gmail.com> On 2021-01-11 18:03, Marc Orr wrote: >> On 2021-01-11 15:43, Marc Orr wrote: > > minus stable@vger.kernel.org, per gregkh@'s email. > >>> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c >>> index 0a4881e59aa7..3d9b17fe5771 100644 >>> --- a/kernel/dma/direct.c >>> +++ b/kernel/dma/direct.c >>> @@ -374,9 +374,11 @@ void dma_direct_unmap_sg(struct device *dev, struct scatterlist *sgl, >>> struct scatterlist *sg; >>> int i; >>> >>> - for_each_sg(sgl, sg, nents, i) >>> + for_each_sg(sgl, sg, nents, i) { >>> dma_direct_unmap_page(dev, sg->dma_address, sg_dma_len(sg), dir, >>> attrs); >>> + sg->dma_address = DMA_MAPPING_ERROR; >> >> There are more DMA API backends than just dma-direct, so while this >> might help paper over bugs when SWIOTLB is in use, it's not going to >> have any effect when those same bugs are hit under other circumstances. >> Once again, the moral of the story is that effort is better spent just >> fixing the bugs ;) > > Thanks for the quick feedback. What is the correct fix? I understand > the first half. The NVMe driver should be updated to not call unmap on > an address that has already been unmapped within the DMA direct code. > Where I'm less certain is how to communicate to the NVMe driver that > the mapping failed. In particular, the NVMe code explicitly checks if > the first DMA address in the scatter/gather list is set to > DMA_MAPPING_ERROR. Thus, don't we need to update the DMA direct code > to propagate DMA_MAPPING_ERROR back up to the driver, via the > scatter/gather struct? Erm, you check the return value of dma_map_sg(). If it's zero, the request failed; if it's nonzero, that's how many DMA segments you now have to process. See Documentation/core-api/dma-api.rst. The only guarantee offered about the state of the scatterlist itself is that if it is successfully mapped, then the dma_address and dma_length fields are valid for that many segments, and if that is fewer than the total number of physical segments then the next one after the final DMA segment will have a dma_length of 0. In particular there are no guarantees at all about the state if the mapping was unsuccessful. If a driver is failing to keep track of the success/failure status and later down the line trying to guess what to do with a list that may or may not have been mapped, then frankly that driver should be redesigned because that is a terrible anti-pattern. At the very very least it should explicitly encode its own "known bad" state upon failure that it can then reliably recognise later. Robin. > I skimmed arch/arm/mm/dma-mapping.c, just now. I can see that this > code sets the address within the scatter/gather struct to > DMA_MAPPING_ERROR before trying to map an IO address and write it into > the struct. Is this a good example to follow? > > Thanks, > Marc > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-01-11 20:40 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-11 15:43 [PATCH] dma: mark unmapped DMA scatter/gather invalid Marc Orr 2021-01-11 15:43 ` Marc Orr via iommu 2021-01-11 16:08 ` Robin Murphy 2021-01-11 16:08 ` Robin Murphy 2021-01-11 18:03 ` Marc Orr 2021-01-11 18:03 ` Marc Orr via iommu 2021-01-11 20:39 ` Robin Murphy [this message] 2021-01-11 20:39 ` Robin Murphy 2021-01-11 21:36 ` Marc Orr via iommu 2021-01-11 21:36 ` Marc Orr 2021-01-11 16:15 ` Greg KH 2021-01-11 16:15 ` Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=bbf6f07c-369b-e470-78ff-815cfb4dbf92@arm.com \ --to=robin.murphy@arm.com \ --cc=hch@lst.de \ --cc=iommu@lists.linux-foundation.org \ --cc=jxgao@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=marcorr@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.