Skip to contentSkip to conversation

vhost: NULL vs ERR_PTR bug

PATCH In review Dan Carpenter 4 participants 9 messages Jul 15, 2015

Subsystems (4)
KVM Kernel janitors virtio core virtio host (vhost)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 vhost: NULL vs ERR_PTR bug

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>
drivers/vhost/vhost.c +1 -1
index a9fe859..99d613b 100644
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
+			return NULL;
 	}
 	return n;
 }
maintainer
#1 lore raw
Quoted 6 lines · Dan Carpenter

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>

Ouch. Thanks a lot for noticing this, and it's a good thing you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

Quoted 14 lines
drivers/vhost/vhost.c +1 -1
index a9fe859..99d613b 100644
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
+			return NULL;
 	}
 	return n;
 }
#2 lore raw

Am 15.07.2015 13:28, schrieb Michael S. Tsirkin:

Quoted 5 lines · Dan Carpenter
Quoted 4 lines

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Quoted 7 lines
Quoted 2 lines
Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>

Ouch. Thanks a lot for noticing this, and it's a good thing you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

Is this function needed at all ?

e.g. I followed vzalloc() to __vmalloc_node_range()

a check for size=0 seems to be done there.

re,
wh

Quoted 19 lines
Quoted 6 lines
drivers/vhost/vhost.c +0 -0
index a9fe859..99d613b 100644

drivers/vhost/vhost.c static void *vhost_kvzalloc(unsigned long size) · line 692

Quoted 8 lines
drivers/vhost/vhost.c +1 -1
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
+			return NULL;
 	}
 	return n;
 }

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#3 lore raw
Quoted 2 lines · walter harms

Is this function needed at all ?

It tries to kmalloc() memory and if it can't then it tries to vmalloc() it. There are a bunch of these functions. Eventually someone should put one in a common header.

regards,
dan carpenter

#4 lore raw
In reply to walter harms
Quoted 16 lines · walter harms

Am 15.07.2015 13:28, schrieb Michael S. Tsirkin:

Quoted 5 lines · Dan Carpenter
Quoted 4 lines

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Quoted 7 lines
Quoted 2 lines
Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>

Ouch. Thanks a lot for noticing this, and it's a good thing you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

Quoted 5 lines

Is this function needed at all ?

e.g. I followed vzalloc() to __vmalloc_node_range()

a check for size=0 seems to be done there.

At least it looks like one could return the result of vzalloc directly?

julia

Quoted 29 lines

re,
wh

Quoted 10 lines
Quoted 10 lines
drivers/vhost/vhost.c +0 -1
index a9fe859..99d613b 100644
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
Quoted 9 lines

drivers/vhost/vhost.c static void *vhost_kvzalloc(unsigned long size) · line 692

Quoted 4 lines
drivers/vhost/vhost.c +1 -0
+			return NULL;
 	}
 	return n;
 }

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw
Quoted 3 lines · Julia Lawall

At least it looks like one could return the result of vzalloc directly?

julia

That's a true... I was tempted to re-write it a bit. I think one of the Lustre people are eventually going to export this function and we will delete this one.

regards,
dan carpenter

#6 lore raw
In reply to walter harms
Quoted 16 lines · walter harms

Am 15.07.2015 13:28, schrieb Michael S. Tsirkin:

Quoted 5 lines · Dan Carpenter
Quoted 4 lines

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Quoted 7 lines
Quoted 2 lines
Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>

Ouch. Thanks a lot for noticing this, and it's a good thing you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

Quoted 5 lines

Is this function needed at all ?

e.g. I followed vzalloc() to __vmalloc_node_range()

a check for size==0 seems to be done there.

At least it looks like one could return the result of vzalloc directly?

julia

Quoted 29 lines

re,
wh

Quoted 10 lines
Quoted 10 lines
drivers/vhost/vhost.c +0 -1
index a9fe859..99d613b 100644
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
Quoted 9 lines

drivers/vhost/vhost.c static void *vhost_kvzalloc(unsigned long size) · line 692

Quoted 4 lines
drivers/vhost/vhost.c +1 -0
+			return NULL;
 	}
 	return n;
 }

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw
In reply to Michael S. Tsirkin

Am 15.07.2015 13:28, schrieb Michael S. Tsirkin:

Quoted 5 lines · Dan Carpenter
Quoted 4 lines

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Quoted 7 lines
Quoted 2 lines
Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>

Ouch. Thanks a lot for noticing this, and it's a good thing you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

Is this function needed at all ?

e.g. I followed vzalloc() to __vmalloc_node_range()

a check for size==0 seems to be done there.

re,
wh

Quoted 19 lines
Quoted 6 lines
drivers/vhost/vhost.c +0 -0
index a9fe859..99d613b 100644

drivers/vhost/vhost.c static void *vhost_kvzalloc(unsigned long size) · line 692

Quoted 8 lines
drivers/vhost/vhost.c +1 -1
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
+			return NULL;
 	}
 	return n;
 }

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#8 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter

There is only one caller for vhost_kvzalloc() and it expects NULL on allocation failure. Most people would probably expect that so let's change ERR_PTR(-ENOMEM) to NULL.

Fixes 4de7255f7d2b ('vhost: extend memory regions allocation to vmalloc')
Signed-off-by Dan Carpenter <[email protected]>

Ouch. Thanks a lot for noticing this, and it's a good thing you did before Linus merged it.
I'll squash this into Igor's patch and redo my pull request.

Quoted 14 lines
drivers/vhost/vhost.c +1 -1
index a9fe859..99d613b 100644
@@ -686,7 +686,7 @@ static void *vhost_kvzalloc(unsigned long size)
 	if (!n) {
 		n = vzalloc(size);
 		if (!n)
-			return ERR_PTR(-ENOMEM);
+			return NULL;
 	}
 	return n;
 }