| Message ID | 1523972123-5700-1-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
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
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
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
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
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.
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
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.
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.
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
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
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
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;
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(-) -- 2.7.4