[PATCH] mm: fix NULL checking in dma_pool_create()

[PATCH] mm: fix NULL checking in dma_pool_create()

[Xi Wang]

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 <xi.wang@gmail.com>
---
 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

[PATCH] mm: fix NULL checking in dma_pool_create()

[Xi Wang]

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 <xi.wang@gmail.com>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Andrew Morton]

On Mon,  5 Nov 2012 01:46:36 -0500
Xi Wang <xi.wang@gmail.com> 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?

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Andrew Morton]

On Mon,  5 Nov 2012 01:46:36 -0500
Xi Wang <xi.wang@gmail.com> 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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Xi Wang]

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

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Xi Wang]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Andrew Morton]

On Mon, 05 Nov 2012 15:50:32 -0500
Xi Wang <xi.wang@gmail.com> 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?

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Andrew Morton]

On Mon, 05 Nov 2012 15:50:32 -0500
Xi Wang <xi.wang@gmail.com> 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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Xi Wang]

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

Re: [PATCH] mm: fix NULL checking in dma_pool_create()

[Xi Wang]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2] mm: fix null dev in dma_pool_create()

[Xi Wang]

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 <akpm@linux-foundation.org>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 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

[PATCH v2] mm: fix null dev in dma_pool_create()

[Xi Wang]

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 <akpm@linux-foundation.org>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[David Rientjes]

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)
 {

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[David Rientjes]

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)
 {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Andrew Morton]

On Tue, 13 Nov 2012 15:01:34 -0800 (PST)
David Rientjes <rientjes@google.com> 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.

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Andrew Morton]

On Tue, 13 Nov 2012 15:01:34 -0800 (PST)
David Rientjes <rientjes@google.com> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Andrew Morton]

On Tue, 13 Nov 2012 16:39:37 -0500
Xi Wang <xi.wang@gmail.com> 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 <akpm@linux-foundation.org>

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.

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Andrew Morton]

On Tue, 13 Nov 2012 16:39:37 -0500
Xi Wang <xi.wang@gmail.com> 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 <akpm@linux-foundation.org>

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Xi Wang]

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()?

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Xi Wang]

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()?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Felipe Balbi]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] mm: fix null dev in dma_pool_create()

[Felipe Balbi]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]