mm/util.c: add a none zero check of "len"

PATCH NAK Pan Xinhui 2 participants 3 messages Jan 19, 2015 – Jan 22, 2015

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Pan Xinhui

author

2015-01-19 08:20 UTC #0 lore raw

PATCH 1/1 mm/util.c: add a none zero check of "len"

Although this check should have been done by caller. But as it's exported to others, It's better to add a none zero check of "len" like other functions.

Signed-off-by xinhuix.pan <[email protected]>

---
 mm/util.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/util.c +5 -0

index fec39d4..3dc2873 100644
@@ -72,6 +72,9 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
 {
 	void *p;
 
+	if (unlikely(!len))
+		return ERR_PTR(-EINVAL);
+
 	p = kmalloc_track_caller(len, gfp);
 	if (p)
 		memcpy(p, src, len);

@@ -91,6 +94,8 @@ void *memdup_user(const void __user *src, size_t len)
 {
 	void *p;
 
+	if (unlikely(!len))
+		return ERR_PTR(-EINVAL);
 	/*
 	 * Always use GFP_KERNEL, since copy_from_user() can sleep and
 	 * cause pagefault, which makes it pointless to use GFP_NOFS

Signature · 7 lines

1.9.1

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

David Rientjes

NAK

2015-01-21 23:09 UTC #1 lore raw

Quoted 23 lines · Pan Xinhui

Although this check should have been done by caller. But as it's exported to others,
It's better to add a none zero check of "len" like other functions.
Signed-off-by xinhuix.pan <[email protected]>
---
 mm/util.c | 5 +++++
 1 file changed, 5 insertions(+)
mm/util.c +2 -0
index fec39d4..3dc2873 100644
@@ -72,6 +72,9 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
 {
 	void *p;
 +	if (unlikely(!len))
+		return ERR_PTR(-EINVAL);
+
 	p = kmalloc_track_caller(len, gfp);
 	if (p)
 		memcpy(p, src, len);

mm/util.c void *memdup_user(const void __user *src, size_t len) · line 100

Quoted 7 lines

mm/util.c +1 -0

@@ -91,6 +94,8 @@ void *memdup_user(const void __user *src, size_t len)
 {
 	void *p;
 +	if (unlikely(!len))
+		return ERR_PTR(-EINVAL);
 	/*
 	 * Always use GFP_KERNEL, since copy_from_user() can sleep and
 	 * cause pagefault, which makes it pointless to use GFP_NOFS

Nack, there's no need for this since both slab and slub check for ZERO_OR_NULL_PTR() and kmalloc_slab() will return ZERO_SIZE_PTR in these cases. kmemdup() would then return NULL, which is appropriate since it doesn't return an ERR_PTR() even when memory cannot be allocated. memdup_user() would return -ENOMEM for size == 0, which would arguably be the wrong return value, but I don't think we need to slow down either of these library functions to check for something as stupid as duplicating size == 0.

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

Pan Xinhui

author

2015-01-22 02:29 UTC #2 lore raw

Quoted 33 lines · David Rientjes

Quoted 23 lines · Pan Xinhui
Although this check should have been done by caller. But as it's exported to others,
It's better to add a none zero check of "len" like other functions.
Signed-off-by xinhuix.pan <[email protected]>
---
  mm/util.c | 5 +++++
  1 file changed, 5 insertions(+)
mm/util.c +2 -0
index fec39d4..3dc2873 100644
@@ -72,6 +72,9 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
  {
  	void *p;
  +	if (unlikely(!len))
+		return ERR_PTR(-EINVAL);
+
  	p = kmalloc_track_caller(len, gfp);
  	if (p)
  		memcpy(p, src, len);
mm/util.c void *memdup_user(const void __user *src, size_t len) · line 100
Quoted 7 lines
mm/util.c +1 -0
@@ -91,6 +94,8 @@ void *memdup_user(const void __user *src, size_t len)
  {
  	void *p;
  +	if (unlikely(!len))
+		return ERR_PTR(-EINVAL);
  	/*
  	 * Always use GFP_KERNEL, since copy_from_user() can sleep and
  	 * cause pagefault, which makes it pointless to use GFP_NOFS

Quoted 9 lines

Nack, there's no need for this since both slab and slub check for ZERO_OR_NULL_PTR() and kmalloc_slab() will return ZERO_SIZE_PTR in these cases. kmemdup() would then return NULL, which is appropriate since it doesn't return an ERR_PTR() even when memory cannot be allocated. memdup_user() would return -ENOMEM for size == 0, which would arguably be the wrong return value, but I don't think we need to slow down either of these library functions to check for something as stupid as duplicating size == 0.

Hi, David
Thanks for your reply :)
But let me explain it to you as I think it's not stupid to do a duplicate check.
1) if size is zero, kmalloc_track_caller will return ZERO_SIZE_PTR, and the value is 0x10, that makes the next line if (p) meaningless. panic will hit. Actually we have hit this panic in our tests. So make this "len" check is needed in my opinion.

2) yes, you point out that the called should have done the check before call these two functions. However we can make the codes more simpler. The caller will be able to skip the check of "len",

before applying this patch, the code may be

if (size == 0){
	//do something else. mostly this is an error.
} else {
	p = kmemdup(src, len, flags);
	if (IS_ERR(p))
	....
}

after applying this patch, the code may be
p = kmemdup(src, len, flags);
if (IS_ERR(p)) {
.....
}
......

we can handle these errors more simpler :) And I have reviewed most functions who will call kmemdup/memdup_user. some of them do a len == 0 check, but some didn't.

in my opinion, it should always be an error to pass len of 0 to them, so my patch don't broke anything.

3) People always know some lib functions, like strcpy, is not safe to call, so we must do a null check. But here, and now, I think it is our duty to do such check. Not all users(who calls these functions) know the fact that "these functions behave like strcpy".

Thanks for your reply again, David :)
I appreciate that you will give me some advices and your opinions.

Thanks.
xinhui