* [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-17 13:35 ` Jacopo Mondi
0 siblings, 0 replies; 26+ messages in thread
From: Jacopo Mondi @ 2018-04-17 13:35 UTC (permalink / raw)
To: ysato, dalias, thomas.petazzoni, robin.murphy
Cc: Jacopo Mondi, geert, linux-renesas-soc, linux-sh, linux-kernel
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 13:35 ` Jacopo Mondi
@ 2018-04-17 13:54 ` Thomas Petazzoni
-1 siblings, 0 replies; 26+ messages in thread
From: Thomas Petazzoni @ 2018-04-17 13:54 UTC (permalink / raw)
To: Jacopo Mondi
Cc: ysato, dalias, thomas.petazzoni, robin.murphy, geert,
linux-renesas-soc, linux-sh, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-17 13:54 ` Thomas Petazzoni
0 siblings, 0 replies; 26+ messages in thread
From: Thomas Petazzoni @ 2018-04-17 13:54 UTC (permalink / raw)
To: Jacopo Mondi
Cc: ysato, dalias, thomas.petazzoni, robin.murphy, geert,
linux-renesas-soc, linux-sh, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 13:54 ` Thomas Petazzoni
@ 2018-04-17 13:59 ` jacopo mondi
-1 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-04-17 13:59 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy,
geert, linux-renesas-soc, linux-sh, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-17 13:59 ` jacopo mondi
0 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-04-17 13:59 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy,
geert, linux-renesas-soc, linux-sh, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 13:54 ` Thomas Petazzoni
@ 2018-04-20 8:30 ` Christoph Hellwig
-1 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2018-04-20 8:30 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy,
geert, linux-renesas-soc, linux-sh, linux-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-20 8:30 ` Christoph Hellwig
0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2018-04-20 8:30 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy,
geert, linux-renesas-soc, linux-sh, linux-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 13:35 ` Jacopo Mondi
@ 2018-04-17 14:04 ` Geert Uytterhoeven
-1 siblings, 0 replies; 26+ messages in thread
From: Geert Uytterhoeven @ 2018-04-17 14:04 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Yoshinori Sato, Rich Felker, Thomas Petazzoni, Robin Murphy,
Linux-Renesas, Linux-sh list, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-17 14:04 ` Geert Uytterhoeven
0 siblings, 0 replies; 26+ messages in thread
From: Geert Uytterhoeven @ 2018-04-17 14:04 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Yoshinori Sato, Rich Felker, Thomas Petazzoni, Robin Murphy,
Linux-Renesas, Linux-sh list, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 14:04 ` Geert Uytterhoeven
@ 2018-04-17 14:20 ` jacopo mondi
-1 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-04-17 14:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jacopo Mondi, Yoshinori Sato, Rich Felker, Thomas Petazzoni,
Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-17 14:20 ` jacopo mondi
0 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-04-17 14:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jacopo Mondi, Yoshinori Sato, Rich Felker, Thomas Petazzoni,
Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 13:35 ` Jacopo Mondi
@ 2018-04-18 9:13 ` Sergei Shtylyov
-1 siblings, 0 replies; 26+ messages in thread
From: Sergei Shtylyov @ 2018-04-18 9:13 UTC (permalink / raw)
To: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy
Cc: geert, linux-renesas-soc, linux-sh, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-18 9:13 ` Sergei Shtylyov
0 siblings, 0 replies; 26+ messages in thread
From: Sergei Shtylyov @ 2018-04-18 9:13 UTC (permalink / raw)
To: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy
Cc: geert, linux-renesas-soc, linux-sh, linux-kernel
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-17 13:35 ` Jacopo Mondi
@ 2018-04-18 10:47 ` Christoph Hellwig
-1 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2018-04-18 10:47 UTC (permalink / raw)
To: Jacopo Mondi
Cc: ysato, dalias, thomas.petazzoni, robin.murphy, geert,
linux-renesas-soc, linux-sh, linux-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-18 10:47 ` Christoph Hellwig
0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2018-04-18 10:47 UTC (permalink / raw)
To: Jacopo Mondi
Cc: ysato, dalias, thomas.petazzoni, robin.murphy, geert,
linux-renesas-soc, linux-sh, linux-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-18 10:47 ` Christoph Hellwig
@ 2018-04-18 13:13 ` jacopo mondi
-1 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-04-18 13:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy,
geert, linux-renesas-soc, linux-sh, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-18 13:13 ` jacopo mondi
0 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-04-18 13:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jacopo Mondi, ysato, dalias, thomas.petazzoni, robin.murphy,
geert, linux-renesas-soc, linux-sh, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-18 13:13 ` jacopo mondi
@ 2018-04-20 8:31 ` Christoph Hellwig
-1 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2018-04-20 8:31 UTC (permalink / raw)
To: jacopo mondi
Cc: Christoph Hellwig, Jacopo Mondi, ysato, dalias, thomas.petazzoni,
robin.murphy, geert, linux-renesas-soc, linux-sh, linux-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-20 8:31 ` Christoph Hellwig
0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2018-04-20 8:31 UTC (permalink / raw)
To: jacopo mondi
Cc: Christoph Hellwig, Jacopo Mondi, ysato, dalias, thomas.petazzoni,
robin.murphy, geert, linux-renesas-soc, linux-sh, linux-kernel
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-20 8:31 ` Christoph Hellwig
@ 2018-04-20 9:59 ` Geert Uytterhoeven
-1 siblings, 0 replies; 26+ messages in thread
From: Geert Uytterhoeven @ 2018-04-20 9:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: jacopo mondi, Jacopo Mondi, Yoshinori Sato, Rich Felker,
Thomas Petazzoni, Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-20 9:59 ` Geert Uytterhoeven
0 siblings, 0 replies; 26+ messages in thread
From: Geert Uytterhoeven @ 2018-04-20 9:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: jacopo mondi, Jacopo Mondi, Yoshinori Sato, Rich Felker,
Thomas Petazzoni, Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-20 9:59 ` Geert Uytterhoeven
@ 2018-04-20 14:56 ` Rich Felker
-1 siblings, 0 replies; 26+ messages in thread
From: Rich Felker @ 2018-04-20 14:56 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Christoph Hellwig, jacopo mondi, Jacopo Mondi, Yoshinori Sato,
Thomas Petazzoni, Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-04-20 14:56 ` Rich Felker
0 siblings, 0 replies; 26+ messages in thread
From: Rich Felker @ 2018-04-20 14:56 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Christoph Hellwig, jacopo mondi, Jacopo Mondi, Yoshinori Sato,
Thomas Petazzoni, Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] sh: mm: Fix unprotected access to struct device
2018-04-20 9:59 ` Geert Uytterhoeven
@ 2018-05-02 7:41 ` jacopo mondi
-1 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-05-02 7:41 UTC (permalink / raw)
To: hch
Cc: Geert Uytterhoeven, Jacopo Mondi, Yoshinori Sato, Rich Felker,
Thomas Petazzoni, Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] sh: mm: Fix unprotected access to struct device
@ 2018-05-02 7:41 ` jacopo mondi
0 siblings, 0 replies; 26+ messages in thread
From: jacopo mondi @ 2018-05-02 7:41 UTC (permalink / raw)
To: hch
Cc: Geert Uytterhoeven, Jacopo Mondi, Yoshinori Sato, Rich Felker,
Thomas Petazzoni, Robin Murphy, Linux-Renesas, Linux-sh list,
Linux Kernel Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread