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 <[email protected]>
---
arch/arm64/mm/dma-mapping.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
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)) {
--
2.13.0
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)) {
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
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.
--
Catalin
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.
--
.Olav
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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.
--
.Olav
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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?
--
.Olav
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
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.
--
Catalin