First, `dev' is dereferenced in dev_to_node(dev), suggesting that it
must be non-null. Later `dev' is checked against NULL, suggesting
the opposite. This patch adds a NULL check before its use.
Signed-off-by: Xi Wang <[email protected]>
---
mm/dmapool.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index c5ab33b..afbf88e 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -135,6 +135,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
{
struct dma_pool *retval;
size_t allocation;
+ int node;
if (align == 0) {
align = 1;
@@ -159,7 +160,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
return NULL;
}
- retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
+ node = dev ? dev_to_node(dev) : -1;
+
+ retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, node);
if (!retval)
return retval;
--
1.7.10.4
On Mon, 5 Nov 2012 01:46:36 -0500
Xi Wang <[email protected]> wrote:
> First, `dev' is dereferenced in dev_to_node(dev), suggesting that it
> must be non-null. Later `dev' is checked against NULL, suggesting
> the opposite. This patch adds a NULL check before its use.
>
> ...
>
> @@ -159,7 +160,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> return NULL;
> }
>
> - retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
> + node = dev ? dev_to_node(dev) : -1;
> +
> + retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, node);
> if (!retval)
> return retval;
Well, the dma_pool_create() kerneldoc does not describe dev==NULL to be
acceptable usage and given the lack of oops reports, we can assume that
no code is calling this function with dev==NULL.
So I think we can just remove the code which handles dev==NULL?
On 11/5/12 3:37 PM, Andrew Morton wrote:
>
> Well, the dma_pool_create() kerneldoc does not describe dev==NULL to be
> acceptable usage and given the lack of oops reports, we can assume that
> no code is calling this function with dev==NULL.
>
> So I think we can just remove the code which handles dev==NULL?
Actually, a quick grep gives the following...
arch/arm/mach-s3c64xx/dma.c:731: dma_pool = dma_pool_create("DMA-LLI", NULL, sizeof(struct pl080s_lli), 16, 0);
drivers/usb/gadget/amd5536udc.c:3136: dev->data_requests = dma_pool_create("data_requests", NULL,
drivers/usb/gadget/amd5536udc.c:3148: dev->stp_requests = dma_pool_create("setup requests", NULL,
drivers/net/wan/ixp4xx_hss.c:973: if (!(dma_pool = dma_pool_create(DRV_NAME, NULL,
drivers/net/ethernet/xscale/ixp4xx_eth.c:1106: if (!(dma_pool = dma_pool_create(DRV_NAME, NULL,
- xi
On Mon, 05 Nov 2012 15:50:32 -0500
Xi Wang <[email protected]> wrote:
> On 11/5/12 3:37 PM, Andrew Morton wrote:
> >
> > Well, the dma_pool_create() kerneldoc does not describe dev==NULL to be
> > acceptable usage and given the lack of oops reports, we can assume that
> > no code is calling this function with dev==NULL.
> >
> > So I think we can just remove the code which handles dev==NULL?
>
> Actually, a quick grep gives the following...
>
> arch/arm/mach-s3c64xx/dma.c:731: dma_pool = dma_pool_create("DMA-LLI", NULL, sizeof(struct pl080s_lli), 16, 0);
> drivers/usb/gadget/amd5536udc.c:3136: dev->data_requests = dma_pool_create("data_requests", NULL,
> drivers/usb/gadget/amd5536udc.c:3148: dev->stp_requests = dma_pool_create("setup requests", NULL,
> drivers/net/wan/ixp4xx_hss.c:973: if (!(dma_pool = dma_pool_create(DRV_NAME, NULL,
> drivers/net/ethernet/xscale/ixp4xx_eth.c:1106: if (!(dma_pool = dma_pool_create(DRV_NAME, NULL,
>
OK, so it seems that those drivers have never been tested on a
CONFIG_NUMA kernel. whee.
So we have a large amount of code here which ostensibly supports
dev==NULL but which has not been well tested. Take a look at
dma_alloc_coherent(), dma_free_coherent() - are they safe? Unobvious.
dmam_pool_destroy() will clearly cause an oops:
devres_destroy()
->devres_remove()
->spin_lock_irqsave(&dev->devres_lock, flags);
So what to do?
I'm thinking we should disallow dev==NULL. We have a lot of code in
mm/dmapool.c which _attempts_ to support this case, but is largely
untested and obviously isn't working. I don't think it's a good idea
to try to fix up and then support this case on behalf of a handful of
scruffy drivers. It would be better to fix the drivers, then simplify
the core code. drivers/usb/gadget/amd5536udc.c can probably use
dev->gadget.dev and drivers/net/wan/ixp4xx_hss.c can probably use
port->netdev->dev, etc.
So how about we add a WARN_ON_ONCE(dev == NULL), notify the driver maintainers
and later we can remove all that mm/dmapool.c code which is trying to
handle dev==NULL?
On 11/5/12 4:26 PM, Andrew Morton wrote:
>
> OK, so it seems that those drivers have never been tested on a
> CONFIG_NUMA kernel. whee.
>
> So we have a large amount of code here which ostensibly supports
> dev==NULL but which has not been well tested. Take a look at
> dma_alloc_coherent(), dma_free_coherent() - are they safe? Unobvious.
It's probably ok to call dma_alloc_coherent()/dma_free_coherent() with a
NULL dev. Quite a few drivers do that.
> dmam_pool_destroy() will clearly cause an oops:
>
> devres_destroy()
> ->devres_remove()
> ->spin_lock_irqsave(&dev->devres_lock, flags);
Not sure if I missed anything, but I haven't found any use of
dmam_pool_destroy() in the tree..
> I'm thinking we should disallow dev==NULL. We have a lot of code in
> mm/dmapool.c which _attempts_ to support this case, but is largely
> untested and obviously isn't working. I don't think it's a good idea
> to try to fix up and then support this case on behalf of a handful of
> scruffy drivers. It would be better to fix the drivers, then simplify
> the core code. drivers/usb/gadget/amd5536udc.c can probably use
> dev->gadget.dev and drivers/net/wan/ixp4xx_hss.c can probably use
> port->netdev->dev, etc.
After more search I've still only found 4 files that invoke
dma_pool_create() with a NULL dev.
arch/arm/mach-s3c64xx/dma.c
drivers/usb/gadget/amd5536udc.c
drivers/net/wan/ixp4xx_hss.c
drivers/net/ethernet/xscale/ixp4xx_eth.c
So, yeah, we could fix those drivers instead, such as adding
"WARN_ON_ONCE(dev == NULL)" as you suggested.
- xi
A few drivers invoke dma_pool_create() with a null dev. Note that dev
is dereferenced in dev_to_node(dev), causing a null pointer dereference.
A long term solution is to disallow null dev. Once the drivers are
fixed, we can simplify the core code here. For now we add WARN_ON(!dev)
to notify the driver maintainers and avoid the null pointer dereference.
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: Xi Wang <[email protected]>
---
mm/dmapool.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index c5ab33b..bf7f8f0 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -135,6 +135,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
{
struct dma_pool *retval;
size_t allocation;
+ int node;
if (align == 0) {
align = 1;
@@ -159,7 +160,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
return NULL;
}
- retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
+ node = WARN_ON(!dev) ? -1 : dev_to_node(dev);
+
+ retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, node);
if (!retval)
return retval;
--
1.7.10.4
On Tue, 13 Nov 2012, Xi Wang wrote:
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index c5ab33b..bf7f8f0 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -135,6 +135,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> {
> struct dma_pool *retval;
> size_t allocation;
> + int node;
>
> if (align == 0) {
> align = 1;
> @@ -159,7 +160,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> return NULL;
> }
>
> - retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
> + node = WARN_ON(!dev) ? -1 : dev_to_node(dev);
> +
> + retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, node);
> if (!retval)
> return retval;
>
Begs the question why we don't just do something like this generically?
---
diff --git a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -718,7 +718,7 @@ int dev_set_name(struct device *dev, const char *name, ...);
#ifdef CONFIG_NUMA
static inline int dev_to_node(struct device *dev)
{
- return dev->numa_node;
+ return WARN_ON(!dev) ? NUMA_NO_NODE : dev->numa_node;
}
static inline void set_dev_node(struct device *dev, int node)
{
On Tue, 13 Nov 2012 15:01:34 -0800 (PST)
David Rientjes <[email protected]> wrote:
> On Tue, 13 Nov 2012, Xi Wang wrote:
>
> > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > index c5ab33b..bf7f8f0 100644
> > --- a/mm/dmapool.c
> > +++ b/mm/dmapool.c
> > @@ -135,6 +135,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> > {
> > struct dma_pool *retval;
> > size_t allocation;
> > + int node;
> >
> > if (align == 0) {
> > align = 1;
> > @@ -159,7 +160,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> > return NULL;
> > }
> >
> > - retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
> > + node = WARN_ON(!dev) ? -1 : dev_to_node(dev);
> > +
> > + retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, node);
> > if (!retval)
> > return retval;
> >
>
> Begs the question why we don't just do something like this generically?
> ---
> diff --git a/include/linux/device.h b/include/linux/device.h
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -718,7 +718,7 @@ int dev_set_name(struct device *dev, const char *name, ...);
> #ifdef CONFIG_NUMA
> static inline int dev_to_node(struct device *dev)
> {
> - return dev->numa_node;
> + return WARN_ON(!dev) ? NUMA_NO_NODE : dev->numa_node;
> }
WARN and friends can cause quite a lot of code to be generated, so they're
a rather bloat-risky thing to include in a little inlined helper
function.
On Tue, 13 Nov 2012 16:39:37 -0500
Xi Wang <[email protected]> wrote:
> A few drivers invoke dma_pool_create() with a null dev. Note that dev
> is dereferenced in dev_to_node(dev), causing a null pointer dereference.
>
> A long term solution is to disallow null dev. Once the drivers are
> fixed, we can simplify the core code here. For now we add WARN_ON(!dev)
> to notify the driver maintainers and avoid the null pointer dereference.
>
> Suggested-by: Andrew Morton <[email protected]>
I'm not sure that I really suggested doing this :(
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -135,6 +135,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> {
> struct dma_pool *retval;
> size_t allocation;
> + int node;
>
> if (align == 0) {
> align = 1;
> @@ -159,7 +160,9 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
> return NULL;
> }
>
> - retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
> + node = WARN_ON(!dev) ? -1 : dev_to_node(dev);
> +
> + retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, node);
> if (!retval)
> return retval;
We know there are a few scruffy drivers which are passing in dev==0.
Those drivers don't oops because nobody is testing them on NUMA
systems.
With this patch, the kernel will now cause runtime warnings to be
emitted from those drivers. Even on non-NUMA systems.
This is a problem! What will happen is that this code will get
released by Linus and will propagate to users mainly via distros and
eventually end-user bug reports will trickle back saying "hey, I got
this warning". Slowly people will fix the scruffy drivers and those
fixes will propagate out from Linus's tree into -stable and then into
distros and then into the end-users hands.
This is *terribly* inefficient! It's a lot of work for a lot of people
and it involves long delays.
So let's not do any of that! Let us try to get those scruffy drivers
fixed up *before* we add this warning.
As a nice side-effect of that work, we can then clean up the dmapool
code so it doesn't need to worry about handling the dev==0 special
case.
So. To start this off, can you please generate a list of the offending
drivers? Then we can hunt down the maintainers and we'll see what can be
done.
On 11/13/12 7:58 PM, Andrew Morton wrote:
> I'm not sure that I really suggested doing this :(
You suggested WARN_ON_ONCE(!dev); I changed it to WARN_ON(!dev) and
kept the NULL check..
> We know there are a few scruffy drivers which are passing in dev==0.
>
> Those drivers don't oops because nobody is testing them on NUMA
> systems.
>
> With this patch, the kernel will now cause runtime warnings to be
> emitted from those drivers. Even on non-NUMA systems.
>
> This is a problem! What will happen is that this code will get
> released by Linus and will propagate to users mainly via distros and
> eventually end-user bug reports will trickle back saying "hey, I got
> this warning". Slowly people will fix the scruffy drivers and those
> fixes will propagate out from Linus's tree into -stable and then into
> distros and then into the end-users hands.
>
> This is *terribly* inefficient! It's a lot of work for a lot of people
> and it involves long delays.
>
> So let's not do any of that! Let us try to get those scruffy drivers
> fixed up *before* we add this warning.
>
> As a nice side-effect of that work, we can then clean up the dmapool
> code so it doesn't need to worry about handling the dev==0 special
> case.
>
> So. To start this off, can you please generate a list of the offending
> drivers? Then we can hunt down the maintainers and we'll see what can be
> done.
I like this plan.
Here's the list of drivers that invoke dma_pool_create() with NULL dev,
as well as possible fixes, from previous emails.
* arch/arm/mach-s3c64xx/dma.c
Use dmac->dev for dma_pool_create() in s3c64xx_dma_init1()? Probably
need to add ->dma_pool to struct s3c64xx_dmac.
* drivers/usb/gadget/amd5536udc.c (2)
Use dev->gadget.dev or dev->pdev->dev for dma_pool_create()? Also move
the init_dma_pools() call after the assignments in udc_pci_probe().
* drivers/net/wan/ixp4xx_hss.c
* drivers/net/ethernet/xscale/ixp4xx_eth.c
Use port->netdev->dev for dma_pool_create()?
Hi,
On Wed, Nov 14, 2012 at 12:50:53AM -0500, Xi Wang wrote:
> * drivers/usb/gadget/amd5536udc.c (2)
>
> Use dev->gadget.dev or dev->pdev->dev for dma_pool_create()? Also move
> the init_dma_pools() call after the assignments in udc_pci_probe().
Makes sense to me. Do you want to provide a patch ? Make sure to Cc
myself and linux-usb ;-)
--
balbi