| Message ID | 20170610194110.27712-1-ohaugan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 10, 2017 at 12:41:10PM -0700, Olav Haugan wrote: > @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, > bool coherent = is_device_dma_coherent(dev); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This re-introduces an instance that you say you're getting rid of... > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, false); > > + if (!dev) { > + WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); > + return NULL; > + } > + > size = PAGE_ALIGN(size); > > if (!coherent && !gfpflags_allow_blocking(flags)) {
On Sat, Jun 10, 2017 at 12:41:10PM -0700, Olav Haugan wrote: > @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, > bool coherent = is_device_dma_coherent(dev); > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, false); > > + if (!dev) { > + WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); > + return NULL; > + } > + > size = PAGE_ALIGN(size); > > if (!coherent && !gfpflags_allow_blocking(flags)) { > @@ -192,8 +188,13 @@ static void __dma_free(struct device *dev, size_t size, > void *vaddr, dma_addr_t dma_handle, > unsigned long attrs) > { > - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > + void *swiotlb_addr; > > + if (!dev) { > + WARN_ONCE(1, "Use an actual device structure for DMA free\n"); > + return; > + } > + swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); I don't think we need the checks anymore. With commit 1dccb598df54 ("arm64: simplify dma_get_ops") __generic_dma_ops() returns dummy_dma_ops when dev == NULL, so the above __dma_alloc/__dma_free functions would not be called.
On 17-06-12 13:29:04, Catalin Marinas wrote: > On Sat, Jun 10, 2017 at 12:41:10PM -0700, Olav Haugan wrote: > > @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, > > bool coherent = is_device_dma_coherent(dev); > > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, false); > > > > + if (!dev) { > > + WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); > > + return NULL; > > + } > > + > > size = PAGE_ALIGN(size); > > > > if (!coherent && !gfpflags_allow_blocking(flags)) { > > @@ -192,8 +188,13 @@ static void __dma_free(struct device *dev, size_t size, > > void *vaddr, dma_addr_t dma_handle, > > unsigned long attrs) > > { > > - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > > + void *swiotlb_addr; > > > > + if (!dev) { > > + WARN_ONCE(1, "Use an actual device structure for DMA free\n"); > > + return; > > + } > > + swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > > I don't think we need the checks anymore. With commit 1dccb598df54 > ("arm64: simplify dma_get_ops") __generic_dma_ops() returns > dummy_dma_ops when dev == NULL, so the above __dma_alloc/__dma_free > functions would not be called. Ah, you are right. I will remove the checks then! Thanks.
On 17-06-10 23:03:54, Russell King - ARM Linux wrote: > On Sat, Jun 10, 2017 at 12:41:10PM -0700, Olav Haugan wrote: > > @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, > > bool coherent = is_device_dma_coherent(dev); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > This re-introduces an instance that you say you're getting rid of... > The ARM64 version of is_device_dma_coherent() checks for !dev already...But anyway I will completely remove the !dev checks in the alloc/free functions since as Catalin pointed out it is coverted elsewhere.
On 17-06-12 13:29:04, Catalin Marinas wrote: > On Sat, Jun 10, 2017 at 12:41:10PM -0700, Olav Haugan wrote: > > @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, > > bool coherent = is_device_dma_coherent(dev); > > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, false); > > > > + if (!dev) { > > + WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); > > + return NULL; > > + } > > + > > size = PAGE_ALIGN(size); > > > > if (!coherent && !gfpflags_allow_blocking(flags)) { > > @@ -192,8 +188,13 @@ static void __dma_free(struct device *dev, size_t size, > > void *vaddr, dma_addr_t dma_handle, > > unsigned long attrs) > > { > > - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > > + void *swiotlb_addr; > > > > + if (!dev) { > > + WARN_ONCE(1, "Use an actual device structure for DMA free\n"); > > + return; > > + } > > + swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > > I don't think we need the checks anymore. With commit 1dccb598df54 > ("arm64: simplify dma_get_ops") __generic_dma_ops() returns > dummy_dma_ops when dev == NULL, so the above __dma_alloc/__dma_free > functions would not be called. > We don't need the check in is_device_dma_coherent() either then right?
On Mon, Jun 12, 2017 at 01:50:28PM -0700, Olav Haugan wrote: > On 17-06-12 13:29:04, Catalin Marinas wrote: > > On Sat, Jun 10, 2017 at 12:41:10PM -0700, Olav Haugan wrote: > > > @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, > > > bool coherent = is_device_dma_coherent(dev); > > > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, false); > > > > > > + if (!dev) { > > > + WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); > > > + return NULL; > > > + } > > > + > > > size = PAGE_ALIGN(size); > > > > > > if (!coherent && !gfpflags_allow_blocking(flags)) { > > > @@ -192,8 +188,13 @@ static void __dma_free(struct device *dev, size_t size, > > > void *vaddr, dma_addr_t dma_handle, > > > unsigned long attrs) > > > { > > > - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > > > + void *swiotlb_addr; > > > > > > + if (!dev) { > > > + WARN_ONCE(1, "Use an actual device structure for DMA free\n"); > > > + return; > > > + } > > > + swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); > > > > I don't think we need the checks anymore. With commit 1dccb598df54 > > ("arm64: simplify dma_get_ops") __generic_dma_ops() returns > > dummy_dma_ops when dev == NULL, so the above __dma_alloc/__dma_free > > functions would not be called. > > We don't need the check in is_device_dma_coherent() either then right? Right. The only user of this function outside arch/arm64/mm/dma-mapping.c is Xen (shared with arm32 under arch/arm/xen/) but since arm32 doesn't do this NULL check either, we should be fine.
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c index 3216e098c058..a4b65773711d 100644 --- a/arch/arm64/mm/dma-mapping.c +++ b/arch/arm64/mm/dma-mapping.c @@ -95,11 +95,6 @@ static void *__dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, gfp_t flags, unsigned long attrs) { - if (dev == NULL) { - WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); - return NULL; - } - if (IS_ENABLED(CONFIG_ZONE_DMA) && dev->coherent_dma_mask <= DMA_BIT_MASK(32)) flags |= GFP_DMA; @@ -128,10 +123,6 @@ static void __dma_free_coherent(struct device *dev, size_t size, bool freed; phys_addr_t paddr = dma_to_phys(dev, dma_handle); - if (dev == NULL) { - WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); - return; - } freed = dma_release_from_contiguous(dev, phys_to_page(paddr), @@ -149,6 +140,11 @@ static void *__dma_alloc(struct device *dev, size_t size, bool coherent = is_device_dma_coherent(dev); pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, false); + if (!dev) { + WARN_ONCE(1, "Use an actual device structure for DMA allocation\n"); + return NULL; + } + size = PAGE_ALIGN(size); if (!coherent && !gfpflags_allow_blocking(flags)) { @@ -192,8 +188,13 @@ static void __dma_free(struct device *dev, size_t size, void *vaddr, dma_addr_t dma_handle, unsigned long attrs) { - void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); + void *swiotlb_addr; + if (!dev) { + WARN_ONCE(1, "Use an actual device structure for DMA free\n"); + return; + } + swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle)); size = PAGE_ALIGN(size); if (!is_device_dma_coherent(dev)) {
The current null-pointer check in __dma_alloc_coherent and __dma_free_coherent is pretty much useless since we are dereferencing the pointer before checking for null. Check for null-pointer before the actual dereferencing of the pointer. Signed-off-by: Olav Haugan <ohaugan@codeaurora.org> --- arch/arm64/mm/dma-mapping.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)