Skip to contentSkip to conversation

dmapool: remove redundant NULL check for dev in dma_pool_create()

PATCH In review Daeseok Youn 2 participants 3 messages Apr 29, 2014 – May 1, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dmapool: remove redundant NULL check for dev in dma_pool_create()

"dev" cannot be NULL because it is already checked before calling dma_pool_create().

---
If dev can be NULL, it has NULL deferencing when kmalloc_node() is called after enabling CONFIG_NUMA.

 mm/dmapool.c |   26 +++++++++-----------------
 1 files changed, 9 insertions(+), 17 deletions(-)
mm/dmapool.c +8 -16
index c69781e..38dfcdd 100644
@@ -170,24 +170,16 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	retval->boundary = boundary;
 	retval->allocation = allocation;
 
-	if (dev) {
-		int ret;
+	INIT_LIST_HEAD(&retval->pools);
 
-		mutex_lock(&pools_lock);
-		if (list_empty(&dev->dma_pools))
-			ret = device_create_file(dev, &dev_attr_pools);
-		else
-			ret = 0;
-		/* note:  not currently insisting "name" be unique */
-		if (!ret)
-			list_add(&retval->pools, &dev->dma_pools);
-		else {
-			kfree(retval);
-			retval = NULL;
-		}
-		mutex_unlock(&pools_lock);
+	mutex_lock(&pools_lock);
+	if (list_empty(&dev->dma_pools) &&
+	    device_create_file(dev, &dev_attr_pools)) {
+		kfree(retval);
+		return NULL;
 	} else
-		INIT_LIST_HEAD(&retval->pools);
+		list_add(&retval->pools, &dev->dma_pools);
+	mutex_unlock(&pools_lock);
 
 	return retval;
 }
Signature · 7 lines
1.7.4.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
maintainer
#1 lore raw
Quoted 7 lines · On Tue, 29 Apr 2014 11:53:10 +0900 Daeseok Youn <[email protected]> wrote:

"dev" cannot be NULL because it is already checked before calling dma_pool_create().

---
If dev can be NULL, it has NULL deferencing when kmalloc_node() is called after enabling CONFIG_NUMA.

hm, this is unclear.

The code which handles the dev==NULL case was obviously put there deliberately, presumably with the intention of permitting drivers to call dma_pool_create() without a device*. This code is very old.

A lot of drivers call dma_pool_create() (I doubt if you audited all of them!) and perhaps there are some which use this feature and have never been run on NUMA hardware.

I think I'll apply the patch anyway because such drivers (if they exist) probably need some attending to.

I rewrote the changelog thusly:

: "dev" cannot be NULL because it is already checked before calling : dma_pool_create().
:
: If dev ever was NULL, the code would oops in dev_to_node() after enabling : CONFIG_NUMA.
:
: It is possible that some driver is using dev==NULL and has never been run : on a NUMA machine. Such a driver is probably outdated, possibly buggy and : will need some attention if it starts triggering NULL derefs.

#2 lore raw

2014-05-01 6:19 GMT+09:00, Andrew Morton <[email protected]>:

Quoted 13 lines
Quoted 7 lines · On Tue, 29 Apr 2014 11:53:10 +0900 Daeseok Youn <[email protected]> wrote:

"dev" cannot be NULL because it is already checked before calling dma_pool_create().

---
If dev can be NULL, it has NULL deferencing when kmalloc_node() is called after enabling CONFIG_NUMA.

hm, this is unclear.

Quoted 7 lines

The code which handles the dev==NULL case was obviously put there deliberately, presumably with the intention of permitting drivers to call dma_pool_create() without a device*. This code is very old.

A lot of drivers call dma_pool_create() (I doubt if you audited all of them!) and perhaps there are some which use this feature and have never been run on NUMA hardware.

Yes.. I didn't check all of callers.. sorry about that. Some drivers are checked.

Quoted 7 lines

I think I'll apply the patch anyway because such drivers (if they exist) probably need some attending to.

I rewrote the changelog thusly:

Quoted 12 lines

: "dev" cannot be NULL because it is already checked before calling : dma_pool_create().
:
: If dev ever was NULL, the code would oops in dev_to_node() after enabling : CONFIG_NUMA.
:
: It is possible that some driver is using dev==NULL and has never been run : on a NUMA machine. Such a driver is probably outdated, possibly buggy and
: will need some attention if it starts triggering NULL derefs.

Ok. Thanks for kind explanation.
Regards,
Daeseok Youn

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