[PATCH] sh: mm: Fix unprotected access to struct device

[PATCH] sh: mm: Fix unprotected access to struct device

[Jacopo Mondi]

With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
operations observe dev->dma_pfn_offset") the generic DMA allocation
function on which the SH 'dma_alloc_coherent()' function relies on,
access the 'dma_pfn_offset' field of struct device.

Unfortunately the 'dma_generic_alloc_coherent()' function is called from
several places with a NULL struct device argument, halting the CPU
during the boot process.

This patch fixes the issue protecting access to dev->dma_pfn_offset,
with a trivial check for validity. It also passes a valid 'struct device'
in the 'platform_resource_setup_memory' function which is the main user
of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
(and existing) bogus users of this function they're should provide a valid
'struct device' whenever possible.

Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/sh/mm/consistent.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/sh/mm/consistent.c b/arch/sh/mm/consistent.c
index 8ce9869..b257155 100644
--- a/arch/sh/mm/consistent.c
+++ b/arch/sh/mm/consistent.c
@@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
 	void *ret, *ret_nocache;
 	int order = get_order(size);

+	WARN_ON(!dev);
+
 	gfp |= __GFP_ZERO;

 	ret = (void *)__get_free_pages(gfp, order);
@@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,

 	split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);

-	*dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset);
+	*dma_handle = virt_to_phys(ret);
+	if (dev)
+		*dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

 	return ret_nocache;
 }
@@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
 			       unsigned long attrs)
 {
 	int order = get_order(size);
-	unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
+	unsigned long pfn = (dma_handle >> PAGE_SHIFT);
 	int k;

+	WARN_ON(!dev);
+
+	if (dev)
+		pfn += dev->dma_pfn_offset;
+
 	for (k = 0; k < (1 << order); k++)
 		__free_pages(pfn_to_page(pfn + k), 0);

@@ -143,7 +152,7 @@ int __init platform_resource_setup_memory(struct platform_device *pdev,
 	if (!memsize)
 		return 0;

-	buf = dma_alloc_coherent(NULL, memsize, &dma_handle, GFP_KERNEL);
+	buf = dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);
 	if (!buf) {
 		pr_warning("%s: unable to allocate memory\n", name);
 		return -ENOMEM;
--
2.7.4

[PATCH] sh: mm: Fix unprotected access to struct device

[Jacopo Mondi]

With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
operations observe dev->dma_pfn_offset") the generic DMA allocation
function on which the SH 'dma_alloc_coherent()' function relies on,
access the 'dma_pfn_offset' field of struct device.

Unfortunately the 'dma_generic_alloc_coherent()' function is called from
several places with a NULL struct device argument, halting the CPU
during the boot process.

This patch fixes the issue protecting access to dev->dma_pfn_offset,
with a trivial check for validity. It also passes a valid 'struct device'
in the 'platform_resource_setup_memory' function which is the main user
of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
(and existing) bogus users of this function they're should provide a valid
'struct device' whenever possible.

Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/sh/mm/consistent.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/sh/mm/consistent.c b/arch/sh/mm/consistent.c
index 8ce9869..b257155 100644
--- a/arch/sh/mm/consistent.c
+++ b/arch/sh/mm/consistent.c
@@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
 	void *ret, *ret_nocache;
 	int order = get_order(size);

+	WARN_ON(!dev);
+
 	gfp |= __GFP_ZERO;

 	ret = (void *)__get_free_pages(gfp, order);
@@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,

 	split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);

-	*dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset);
+	*dma_handle = virt_to_phys(ret);
+	if (dev)
+		*dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

 	return ret_nocache;
 }
@@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
 			       unsigned long attrs)
 {
 	int order = get_order(size);
-	unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
+	unsigned long pfn = (dma_handle >> PAGE_SHIFT);
 	int k;

+	WARN_ON(!dev);
+
+	if (dev)
+		pfn += dev->dma_pfn_offset;
+
 	for (k = 0; k < (1 << order); k++)
 		__free_pages(pfn_to_page(pfn + k), 0);

@@ -143,7 +152,7 @@ int __init platform_resource_setup_memory(struct platform_device *pdev,
 	if (!memsize)
 		return 0;

-	buf = dma_alloc_coherent(NULL, memsize, &dma_handle, GFP_KERNEL);
+	buf = dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);
 	if (!buf) {
 		pr_warning("%s: unable to allocate memory\n", name);
 		return -ENOMEM;
--
2.7.4

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Thomas Petazzoni]

Hello,

On Tue, 17 Apr 2018 15:35:23 +0200, Jacopo Mondi wrote:
> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.
> 
> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
> 
> This patch fixes the issue protecting access to dev->dma_pfn_offset,
> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid
> 'struct device' whenever possible.
> 
> Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I would have done two commits here, one to fix:

  dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);

and one to switch to the WARN_ON + if(dev) model. But I don't really
care either way, so:

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Note that even with the if (dev) check, you don't avoid all possible
regressions. For example, some parts of the sh_eth driver were passing
a non-NULL struct device, but it was the wrong struct device (the one
inside struct net_device, and not the one part of struct
platform_device). I fixed that for sh_eth, but there could be other
drivers doing bogus things.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Thomas Petazzoni]

Hello,

On Tue, 17 Apr 2018 15:35:23 +0200, Jacopo Mondi wrote:
> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.
> 
> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
> 
> This patch fixes the issue protecting access to dev->dma_pfn_offset,
> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid
> 'struct device' whenever possible.
> 
> Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I would have done two commits here, one to fix:

  dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);

and one to switch to the WARN_ON + if(dev) model. But I don't really
care either way, so:

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Note that even with the if (dev) check, you don't avoid all possible
regressions. For example, some parts of the sh_eth driver were passing
a non-NULL struct device, but it was the wrong struct device (the one
inside struct net_device, and not the one part of struct
platform_device). I fixed that for sh_eth, but there could be other
drivers doing bogus things.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

Hi Thomas,

On Tue, Apr 17, 2018 at 03:54:07PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 17 Apr 2018 15:35:23 +0200, Jacopo Mondi wrote:
> > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> > operations observe dev->dma_pfn_offset") the generic DMA allocation
> > function on which the SH 'dma_alloc_coherent()' function relies on,
> > access the 'dma_pfn_offset' field of struct device.
> >
> > Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> > several places with a NULL struct device argument, halting the CPU
> > during the boot process.
> >
> > This patch fixes the issue protecting access to dev->dma_pfn_offset,
> > with a trivial check for validity. It also passes a valid 'struct device'
> > in the 'platform_resource_setup_memory' function which is the main user
> > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> > (and existing) bogus users of this function they're should provide a valid
> > 'struct device' whenever possible.
> >
> > Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> I would have done two commits here, one to fix:
>
>   dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);
>
> and one to switch to the WARN_ON + if(dev) model. But I don't really
> care either way, so:

I thought about doing the same, but as this commit is a fix to be
applied on top of v4.17-rc1, and it's likely being fast tracked as it
breaks SH architecture (at least SH7722) I thought it was good to keep
all of that in a single commit.

>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>

Thank you

> Note that even with the if (dev) check, you don't avoid all possible
> regressions. For example, some parts of the sh_eth driver were passing
> a non-NULL struct device, but it was the wrong struct device (the one
> inside struct net_device, and not the one part of struct
> platform_device). I fixed that for sh_eth, but there could be other
> drivers doing bogus things.

Well, not that much we can do here for other bogus users, right?

Thanks
   j

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

Hi Thomas,

On Tue, Apr 17, 2018 at 03:54:07PM +0200, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 17 Apr 2018 15:35:23 +0200, Jacopo Mondi wrote:
> > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> > operations observe dev->dma_pfn_offset") the generic DMA allocation
> > function on which the SH 'dma_alloc_coherent()' function relies on,
> > access the 'dma_pfn_offset' field of struct device.
> >
> > Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> > several places with a NULL struct device argument, halting the CPU
> > during the boot process.
> >
> > This patch fixes the issue protecting access to dev->dma_pfn_offset,
> > with a trivial check for validity. It also passes a valid 'struct device'
> > in the 'platform_resource_setup_memory' function which is the main user
> > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> > (and existing) bogus users of this function they're should provide a valid
> > 'struct device' whenever possible.
> >
> > Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> I would have done two commits here, one to fix:
>
>   dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);
>
> and one to switch to the WARN_ON + if(dev) model. But I don't really
> care either way, so:

I thought about doing the same, but as this commit is a fix to be
applied on top of v4.17-rc1, and it's likely being fast tracked as it
breaks SH architecture (at least SH7722) I thought it was good to keep
all of that in a single commit.

>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>

Thank you

> Note that even with the if (dev) check, you don't avoid all possible
> regressions. For example, some parts of the sh_eth driver were passing
> a non-NULL struct device, but it was the wrong struct device (the one
> inside struct net_device, and not the one part of struct
> platform_device). I fixed that for sh_eth, but there could be other
> drivers doing bogus things.

Well, not that much we can do here for other bogus users, right?

Thanks
   j

>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Geert Uytterhoeven]

Hi Jacopo,

Thanks for your patch!

On Tue, Apr 17, 2018 at 3:35 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.

accesses

> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
>
> This patch fixes the issue protecting access to dev->dma_pfn_offset,

by protecting access to the

> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid

drop "they're should"?

> 'struct device' whenever possible.

> --- a/arch/sh/mm/consistent.c
> +++ b/arch/sh/mm/consistent.c
> @@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
>         void *ret, *ret_nocache;
>         int order = get_order(size);
>
> +       WARN_ON(!dev);
> +
>         gfp |= __GFP_ZERO;
>
>         ret = (void *)__get_free_pages(gfp, order);
> @@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
>
>         split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);
>
> -       *dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset);
> +       *dma_handle = virt_to_phys(ret);
> +       if (dev)
> +               *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

I would keep the WARN_ON() and the (ideally unneeded) dev check as close
to each other as possible:

        if (!WARN_ON(!dev))
                *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

>
>         return ret_nocache;
>  }
> @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
>                                unsigned long attrs)
>  {
>         int order = get_order(size);
> -       unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
> +       unsigned long pfn = (dma_handle >> PAGE_SHIFT);
>         int k;
>
> +       WARN_ON(!dev);
> +
> +       if (dev)
> +               pfn += dev->dma_pfn_offset;

        if (!WARN_ON(!dev))
                pfn += dev->dma_pfn_offset;

> +
>         for (k = 0; k < (1 << order); k++)
>                 __free_pages(pfn_to_page(pfn + k), 0);

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Geert Uytterhoeven]

Hi Jacopo,

Thanks for your patch!

On Tue, Apr 17, 2018 at 3:35 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.

accesses

> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
>
> This patch fixes the issue protecting access to dev->dma_pfn_offset,

by protecting access to the

> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid

drop "they're should"?

> 'struct device' whenever possible.

> --- a/arch/sh/mm/consistent.c
> +++ b/arch/sh/mm/consistent.c
> @@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
>         void *ret, *ret_nocache;
>         int order = get_order(size);
>
> +       WARN_ON(!dev);
> +
>         gfp |= __GFP_ZERO;
>
>         ret = (void *)__get_free_pages(gfp, order);
> @@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
>
>         split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);
>
> -       *dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset);
> +       *dma_handle = virt_to_phys(ret);
> +       if (dev)
> +               *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

I would keep the WARN_ON() and the (ideally unneeded) dev check as close
to each other as possible:

        if (!WARN_ON(!dev))
                *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

>
>         return ret_nocache;
>  }
> @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
>                                unsigned long attrs)
>  {
>         int order = get_order(size);
> -       unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
> +       unsigned long pfn = (dma_handle >> PAGE_SHIFT);
>         int k;
>
> +       WARN_ON(!dev);
> +
> +       if (dev)
> +               pfn += dev->dma_pfn_offset;

        if (!WARN_ON(!dev))
                pfn += dev->dma_pfn_offset;

> +
>         for (k = 0; k < (1 << order); k++)
>                 __free_pages(pfn_to_page(pfn + k), 0);

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3441 bytes --]

Hi Geert,

On Tue, Apr 17, 2018 at 04:04:27PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> Thanks for your patch!
>
> On Tue, Apr 17, 2018 at 3:35 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> > operations observe dev->dma_pfn_offset") the generic DMA allocation
> > function on which the SH 'dma_alloc_coherent()' function relies on,
> > access the 'dma_pfn_offset' field of struct device.
>
> accesses
>
> > Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> > several places with a NULL struct device argument, halting the CPU
> > during the boot process.
> >
> > This patch fixes the issue protecting access to dev->dma_pfn_offset,
>
> by protecting access to the
>
> > with a trivial check for validity. It also passes a valid 'struct device'
> > in the 'platform_resource_setup_memory' function which is the main user
> > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> > (and existing) bogus users of this function they're should provide a valid
>
> drop "they're should"?
>
> > 'struct device' whenever possible.
>
> > --- a/arch/sh/mm/consistent.c
> > +++ b/arch/sh/mm/consistent.c
> > @@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
> >         void *ret, *ret_nocache;
> >         int order = get_order(size);
> >
> > +       WARN_ON(!dev);
> > +
> >         gfp |= __GFP_ZERO;
> >
> >         ret = (void *)__get_free_pages(gfp, order);
> > @@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
> >
> >         split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);
> >
> > -       *dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset);
> > +       *dma_handle = virt_to_phys(ret);
> > +       if (dev)
> > +               *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);
>
> I would keep the WARN_ON() and the (ideally unneeded) dev check as close
> to each other as possible:
>
>         if (!WARN_ON(!dev))
>                 *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

Looking at include/linux/dma-mapping.h I thought it was good to have
the WARN_ON() as early as possible in the function.

But your one looks nicer indeed!

>
> >
> >         return ret_nocache;
> >  }
> > @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
> >                                unsigned long attrs)
> >  {
> >         int order = get_order(size);
> > -       unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
> > +       unsigned long pfn = (dma_handle >> PAGE_SHIFT);
> >         int k;
> >
> > +       WARN_ON(!dev);
> > +
> > +       if (dev)
> > +               pfn += dev->dma_pfn_offset;
>
>         if (!WARN_ON(!dev))
>                 pfn += dev->dma_pfn_offset;
>
> > +
> >         for (k = 0; k < (1 << order); k++)
> >                 __free_pages(pfn_to_page(pfn + k), 0);
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I'll resend and append your and Thomas' tags.

Thanks
  j
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 3441 bytes --]

Hi Geert,

On Tue, Apr 17, 2018 at 04:04:27PM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> Thanks for your patch!
>
> On Tue, Apr 17, 2018 at 3:35 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> > operations observe dev->dma_pfn_offset") the generic DMA allocation
> > function on which the SH 'dma_alloc_coherent()' function relies on,
> > access the 'dma_pfn_offset' field of struct device.
>
> accesses
>
> > Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> > several places with a NULL struct device argument, halting the CPU
> > during the boot process.
> >
> > This patch fixes the issue protecting access to dev->dma_pfn_offset,
>
> by protecting access to the
>
> > with a trivial check for validity. It also passes a valid 'struct device'
> > in the 'platform_resource_setup_memory' function which is the main user
> > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> > (and existing) bogus users of this function they're should provide a valid
>
> drop "they're should"?
>
> > 'struct device' whenever possible.
>
> > --- a/arch/sh/mm/consistent.c
> > +++ b/arch/sh/mm/consistent.c
> > @@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
> >         void *ret, *ret_nocache;
> >         int order = get_order(size);
> >
> > +       WARN_ON(!dev);
> > +
> >         gfp |= __GFP_ZERO;
> >
> >         ret = (void *)__get_free_pages(gfp, order);
> > @@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size,
> >
> >         split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order);
> >
> > -       *dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset);
> > +       *dma_handle = virt_to_phys(ret);
> > +       if (dev)
> > +               *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);
>
> I would keep the WARN_ON() and the (ideally unneeded) dev check as close
> to each other as possible:
>
>         if (!WARN_ON(!dev))
>                 *dma_handle -= PFN_PHYS(dev->dma_pfn_offset);

Looking at include/linux/dma-mapping.h I thought it was good to have
the WARN_ON() as early as possible in the function.

But your one looks nicer indeed!

>
> >
> >         return ret_nocache;
> >  }
> > @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
> >                                unsigned long attrs)
> >  {
> >         int order = get_order(size);
> > -       unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
> > +       unsigned long pfn = (dma_handle >> PAGE_SHIFT);
> >         int k;
> >
> > +       WARN_ON(!dev);
> > +
> > +       if (dev)
> > +               pfn += dev->dma_pfn_offset;
>
>         if (!WARN_ON(!dev))
>                 pfn += dev->dma_pfn_offset;
>
> > +
> >         for (k = 0; k < (1 << order); k++)
> >                 __free_pages(pfn_to_page(pfn + k), 0);
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I'll resend and append your and Thomas' tags.

Thanks
  j
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Sergei Shtylyov]

Hello!

On 4/17/2018 4:35 PM, Jacopo Mondi wrote:

> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.
> 
> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
> 
> This patch fixes the issue protecting access to dev->dma_pfn_offset,
> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid
> 'struct device' whenever possible.
> 
> Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   arch/sh/mm/consistent.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/mm/consistent.c b/arch/sh/mm/consistent.c
> index 8ce9869..b257155 100644
> --- a/arch/sh/mm/consistent.c
> +++ b/arch/sh/mm/consistent.c
[...]
> @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
>   			       unsigned long attrs)
>   {
>   	int order = get_order(size);
> -	unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
> +	unsigned long pfn = (dma_handle >> PAGE_SHIFT);

    Parens no longer needed...

[...]

MBR, Sergei

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Sergei Shtylyov]

Hello!

On 4/17/2018 4:35 PM, Jacopo Mondi wrote:

> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.
> 
> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
> 
> This patch fixes the issue protecting access to dev->dma_pfn_offset,
> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid
> 'struct device' whenever possible.
> 
> Fixes: ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping operations observe dev->dma_pfn_offset")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>   arch/sh/mm/consistent.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/mm/consistent.c b/arch/sh/mm/consistent.c
> index 8ce9869..b257155 100644
> --- a/arch/sh/mm/consistent.c
> +++ b/arch/sh/mm/consistent.c
[...]
> @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size,
>   			       unsigned long attrs)
>   {
>   	int order = get_order(size);
> -	unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset;
> +	unsigned long pfn = (dma_handle >> PAGE_SHIFT);

    Parens no longer needed...

[...]

MBR, Sergei

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Christoph Hellwig]

On Tue, Apr 17, 2018 at 03:35:23PM +0200, Jacopo Mondi wrote:
> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.
> 
> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
> 
> This patch fixes the issue protecting access to dev->dma_pfn_offset,
> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid
> 'struct device' whenever possible.

Please fix those callers to not pass a NULL pointer instead.

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Christoph Hellwig]

On Tue, Apr 17, 2018 at 03:35:23PM +0200, Jacopo Mondi wrote:
> With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> operations observe dev->dma_pfn_offset") the generic DMA allocation
> function on which the SH 'dma_alloc_coherent()' function relies on,
> access the 'dma_pfn_offset' field of struct device.
> 
> Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> several places with a NULL struct device argument, halting the CPU
> during the boot process.
> 
> This patch fixes the issue protecting access to dev->dma_pfn_offset,
> with a trivial check for validity. It also passes a valid 'struct device'
> in the 'platform_resource_setup_memory' function which is the main user
> of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> (and existing) bogus users of this function they're should provide a valid
> 'struct device' whenever possible.

Please fix those callers to not pass a NULL pointer instead.

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

Hi Christoph,

On Wed, Apr 18, 2018 at 03:47:03AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 17, 2018 at 03:35:23PM +0200, Jacopo Mondi wrote:
> > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> > operations observe dev->dma_pfn_offset") the generic DMA allocation
> > function on which the SH 'dma_alloc_coherent()' function relies on,
> > access the 'dma_pfn_offset' field of struct device.
> >
> > Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> > several places with a NULL struct device argument, halting the CPU
> > during the boot process.
> >
> > This patch fixes the issue protecting access to dev->dma_pfn_offset,
> > with a trivial check for validity. It also passes a valid 'struct device'
> > in the 'platform_resource_setup_memory' function which is the main user
> > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> > (and existing) bogus users of this function they're should provide a valid
> > 'struct device' whenever possible.
>
> Please fix those callers to not pass a NULL pointer instead.

As long as it goes for arch/sh, the only user of dma_alloc_coherent()
is platform_resource_setup_memory(), and it has been fixed by this
patch.

Unfortunately, as Thomas pointed out, there are drivers which calls
into this with the wrong 'struct device' as the sh_eth one he had fixed.

I would then say that as long as it goes for the NULL case, we should be
fine now.

Thanks
   j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

Hi Christoph,

On Wed, Apr 18, 2018 at 03:47:03AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 17, 2018 at 03:35:23PM +0200, Jacopo Mondi wrote:
> > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping
> > operations observe dev->dma_pfn_offset") the generic DMA allocation
> > function on which the SH 'dma_alloc_coherent()' function relies on,
> > access the 'dma_pfn_offset' field of struct device.
> >
> > Unfortunately the 'dma_generic_alloc_coherent()' function is called from
> > several places with a NULL struct device argument, halting the CPU
> > during the boot process.
> >
> > This patch fixes the issue protecting access to dev->dma_pfn_offset,
> > with a trivial check for validity. It also passes a valid 'struct device'
> > in the 'platform_resource_setup_memory' function which is the main user
> > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future
> > (and existing) bogus users of this function they're should provide a valid
> > 'struct device' whenever possible.
>
> Please fix those callers to not pass a NULL pointer instead.

As long as it goes for arch/sh, the only user of dma_alloc_coherent()
is platform_resource_setup_memory(), and it has been fixed by this
patch.

Unfortunately, as Thomas pointed out, there are drivers which calls
into this with the wrong 'struct device' as the sh_eth one he had fixed.

I would then say that as long as it goes for the NULL case, we should be
fine now.

Thanks
   j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Christoph Hellwig]

On Tue, Apr 17, 2018 at 03:54:07PM +0200, Thomas Petazzoni wrote:
> 
>   dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);
> 
> and one to switch to the WARN_ON + if(dev) model. But I don't really
> care either way, so:
> 
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Yes, these should be separate patches.  And I actually hope we can
do with the NULL dev check, but that is a different sub-thread.

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Christoph Hellwig]

On Tue, Apr 17, 2018 at 03:54:07PM +0200, Thomas Petazzoni wrote:
> 
>   dma_alloc_coherent(&pdev->dev, memsize, &dma_handle, GFP_KERNEL);
> 
> and one to switch to the WARN_ON + if(dev) model. But I don't really
> care either way, so:
> 
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Yes, these should be separate patches.  And I actually hope we can
do with the NULL dev check, but that is a different sub-thread.

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Christoph Hellwig]

On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
> is platform_resource_setup_memory(), and it has been fixed by this
> patch.

Great!

> 
> Unfortunately, as Thomas pointed out, there are drivers which calls
> into this with the wrong 'struct device' as the sh_eth one he had fixed.

Yes, we'll need fixes there.  Other DMA ops implementations also look
at struct device, so they generally are buggy.

> I would then say that as long as it goes for the NULL case, we should be
> fine now.

Then I'd say skil that part, please.

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Christoph Hellwig]

On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
> is platform_resource_setup_memory(), and it has been fixed by this
> patch.

Great!

> 
> Unfortunately, as Thomas pointed out, there are drivers which calls
> into this with the wrong 'struct device' as the sh_eth one he had fixed.

Yes, we'll need fixes there.  Other DMA ops implementations also look
at struct device, so they generally are buggy.

> I would then say that as long as it goes for the NULL case, we should be
> fine now.

Then I'd say skil that part, please.

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Geert Uytterhoeven]

Hi Christoph,

On Fri, Apr 20, 2018 at 10:31 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
>> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
>> is platform_resource_setup_memory(), and it has been fixed by this
>> patch.
>
> Great!
>
>> Unfortunately, as Thomas pointed out, there are drivers which calls
>> into this with the wrong 'struct device' as the sh_eth one he had fixed.
>
> Yes, we'll need fixes there.  Other DMA ops implementations also look
> at struct device, so they generally are buggy.
>
>> I would then say that as long as it goes for the NULL case, we should be
>> fine now.
>
> Then I'd say skil that part, please.

The major reason for keeping the NULL WARN_ON() checks is to make it
obvious to the developer what is wrong, and fall back to the old behavior.

Without the checks, the kernel will just crash during early startup,
without a clue in the (missing) kernel output, usually leading to a
frustrating bisection experience (if the developer is sufficiently motivated,
at all).

Hence my vote for keeping the checks.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Geert Uytterhoeven]

Hi Christoph,

On Fri, Apr 20, 2018 at 10:31 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
>> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
>> is platform_resource_setup_memory(), and it has been fixed by this
>> patch.
>
> Great!
>
>> Unfortunately, as Thomas pointed out, there are drivers which calls
>> into this with the wrong 'struct device' as the sh_eth one he had fixed.
>
> Yes, we'll need fixes there.  Other DMA ops implementations also look
> at struct device, so they generally are buggy.
>
>> I would then say that as long as it goes for the NULL case, we should be
>> fine now.
>
> Then I'd say skil that part, please.

The major reason for keeping the NULL WARN_ON() checks is to make it
obvious to the developer what is wrong, and fall back to the old behavior.

Without the checks, the kernel will just crash during early startup,
without a clue in the (missing) kernel output, usually leading to a
frustrating bisection experience (if the developer is sufficiently motivated,
at all).

Hence my vote for keeping the checks.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Rich Felker]

On Fri, Apr 20, 2018 at 11:59:13AM +0200, Geert Uytterhoeven wrote:
> Hi Christoph,
> 
> On Fri, Apr 20, 2018 at 10:31 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
> >> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
> >> is platform_resource_setup_memory(), and it has been fixed by this
> >> patch.
> >
> > Great!
> >
> >> Unfortunately, as Thomas pointed out, there are drivers which calls
> >> into this with the wrong 'struct device' as the sh_eth one he had fixed.
> >
> > Yes, we'll need fixes there.  Other DMA ops implementations also look
> > at struct device, so they generally are buggy.
> >
> >> I would then say that as long as it goes for the NULL case, we should be
> >> fine now.
> >
> > Then I'd say skil that part, please.
> 
> The major reason for keeping the NULL WARN_ON() checks is to make it
> obvious to the developer what is wrong, and fall back to the old behavior.
> 
> Without the checks, the kernel will just crash during early startup,
> without a clue in the (missing) kernel output, usually leading to a
> frustrating bisection experience (if the developer is sufficiently motivated,
> at all).
> 
> Hence my vote for keeping the checks.

Sounds good to me.

Rich

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[Rich Felker]

On Fri, Apr 20, 2018 at 11:59:13AM +0200, Geert Uytterhoeven wrote:
> Hi Christoph,
> 
> On Fri, Apr 20, 2018 at 10:31 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
> >> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
> >> is platform_resource_setup_memory(), and it has been fixed by this
> >> patch.
> >
> > Great!
> >
> >> Unfortunately, as Thomas pointed out, there are drivers which calls
> >> into this with the wrong 'struct device' as the sh_eth one he had fixed.
> >
> > Yes, we'll need fixes there.  Other DMA ops implementations also look
> > at struct device, so they generally are buggy.
> >
> >> I would then say that as long as it goes for the NULL case, we should be
> >> fine now.
> >
> > Then I'd say skil that part, please.
> 
> The major reason for keeping the NULL WARN_ON() checks is to make it
> obvious to the developer what is wrong, and fall back to the old behavior.
> 
> Without the checks, the kernel will just crash during early startup,
> without a clue in the (missing) kernel output, usually leading to a
> frustrating bisection experience (if the developer is sufficiently motivated,
> at all).
> 
> Hence my vote for keeping the checks.

Sounds good to me.

Rich

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

Hi Christoph,

On Fri, Apr 20, 2018 at 11:59:13AM +0200, Geert Uytterhoeven wrote:
> Hi Christoph,
>
> On Fri, Apr 20, 2018 at 10:31 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
> >> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
> >> is platform_resource_setup_memory(), and it has been fixed by this
> >> patch.
> >
> > Great!
> >
> >> Unfortunately, as Thomas pointed out, there are drivers which calls
> >> into this with the wrong 'struct device' as the sh_eth one he had fixed.
> >
> > Yes, we'll need fixes there.  Other DMA ops implementations also look
> > at struct device, so they generally are buggy.
> >
> >> I would then say that as long as it goes for the NULL case, we should be
> >> fine now.
> >
> > Then I'd say skil that part, please.
>
> The major reason for keeping the NULL WARN_ON() checks is to make it
> obvious to the developer what is wrong, and fall back to the old behavior.
>
> Without the checks, the kernel will just crash during early startup,
> without a clue in the (missing) kernel output, usually leading to a
> frustrating bisection experience (if the developer is sufficiently motivated,
> at all).
>
> Hence my vote for keeping the checks.

Gentle ping as I don't see this patch being collected in v4.17-rc3

Thanks
   j

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] sh: mm: Fix unprotected access to struct device

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

Hi Christoph,

On Fri, Apr 20, 2018 at 11:59:13AM +0200, Geert Uytterhoeven wrote:
> Hi Christoph,
>
> On Fri, Apr 20, 2018 at 10:31 AM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, Apr 18, 2018 at 03:13:14PM +0200, jacopo mondi wrote:
> >> As long as it goes for arch/sh, the only user of dma_alloc_coherent()
> >> is platform_resource_setup_memory(), and it has been fixed by this
> >> patch.
> >
> > Great!
> >
> >> Unfortunately, as Thomas pointed out, there are drivers which calls
> >> into this with the wrong 'struct device' as the sh_eth one he had fixed.
> >
> > Yes, we'll need fixes there.  Other DMA ops implementations also look
> > at struct device, so they generally are buggy.
> >
> >> I would then say that as long as it goes for the NULL case, we should be
> >> fine now.
> >
> > Then I'd say skil that part, please.
>
> The major reason for keeping the NULL WARN_ON() checks is to make it
> obvious to the developer what is wrong, and fall back to the old behavior.
>
> Without the checks, the kernel will just crash during early startup,
> without a clue in the (missing) kernel output, usually leading to a
> frustrating bisection experience (if the developer is sufficiently motivated,
> at all).
>
> Hence my vote for keeping the checks.

Gentle ping as I don't see this patch being collected in v4.17-rc3

Thanks
   j

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]