Skip to contentSkip to conversation

mm/vmalloc.c: check the addr first

PATCH In review Hui Su 2 participants 3 messages Sep 27, 2020 – Oct 8, 2020

Subsystems (2)
Memory management vmalloc
lore
Share & tools
lore

Hui Su

author
#0 lore raw

PATCH 1/1 mm/vmalloc.c: check the addr first

As the comments said, if @addr is NULL, no operation is performed, check the addr first in vfree() and vfree_atomic() maybe a better choice.

Signed-off-by Hui Su <[email protected]>
---
 mm/vmalloc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
mm/vmalloc.c +6 -5
index be4724b916b3..1cf50749a209 100644
@@ -2305,10 +2305,11 @@ void vfree_atomic(const void *addr)
 {
 	BUG_ON(in_nmi());
 
-	kmemleak_free(addr);
-
 	if (!addr)
 		return;
+
+	kmemleak_free(addr);
+
 	__vfree_deferred(addr);
 }
 

@@ -2340,13 +2341,13 @@ void vfree(const void *addr)
 {
 	BUG_ON(in_nmi());
 
+	if (!addr)
+		return;
+
 	kmemleak_free(addr);
 
 	might_sleep_if(!in_interrupt());
 
-	if (!addr)
-		return;
-
 	__vfree(addr);
 }
 EXPORT_SYMBOL(vfree);
Signature · 1 line
2.25.1
#1 lore raw
Quoted 3 lines · Hui Su

As the comments said, if @addr is NULL, no operation is performed, check the addr first in vfree() and vfree_atomic() maybe a better choice.

I don't see how this change helps anything. kmemleak_free() checks addr so no danger there. Also kmemleak_free() contains a pr_debug() which some might find useful.

Ira

Quoted 46 lines
Signed-off-by Hui Su <[email protected]>
---
 mm/vmalloc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
mm/vmalloc.c +6 -5
index be4724b916b3..1cf50749a209 100644
@@ -2305,10 +2305,11 @@ void vfree_atomic(const void *addr)
 {
 	BUG_ON(in_nmi());
 
-	kmemleak_free(addr);
-
 	if (!addr)
 		return;
+
+	kmemleak_free(addr);
+
 	__vfree_deferred(addr);
 }
 

@@ -2340,13 +2341,13 @@ void vfree(const void *addr)
 {
 	BUG_ON(in_nmi());
 
+	if (!addr)
+		return;
+
 	kmemleak_free(addr);
 
 	might_sleep_if(!in_interrupt());
 
-	if (!addr)
-		return;
-
 	__vfree(addr);
 }
 EXPORT_SYMBOL(vfree);
Signature · 1 line
2.25.1
author
#2 lore raw
Quoted 11 lines · Ira Weiny
Quoted 3 lines · Hui Su

As the comments said, if @addr is NULL, no operation is performed, check the addr first in vfree() and vfree_atomic() maybe a better choice.

I don't see how this change helps anything. kmemleak_free() checks addr so no danger there. Also kmemleak_free() contains a pr_debug() which some might find useful.

Ira

Thanks for your explanations, Ira.

Firstly, as you have said, the kmemleak_free() only unregister a previously registered object(which shouldn't be NULL), and kmemleak_free() alse check the ptr whether is NULL, it does no danger here.

Secondly, as you said kmemleak_free() contains a pr_debug() which will be useful, and i don't think so because this patch only filter out the NULL pointer case, which may not be useful?(maybe not right?)

Thirdly, let's see kimage_file_prepare_segments() and kimage_file_post_load_cleanup() in kernel/kexec_file.c.

kimage_file_prepare_segments() uses 'out' label in several places, when reading files from kernel_fd or initrd_fd or memdup_user cmdline_buf.
The out label will run kimage_file_post_load_cleanup(), which will do vfree() for ->kernel_buf and ->initrd_buf and ->cmdline_buf and so on...

Supposing the arch_kexec_kernel_image_probe() failed, and it goto out, then it still vfree(->initrd_buf). In this case we can return faster if we check param addr first in vfree().

In this case maybe we should use more Precise labels?

All above is my opinion, thanks.

Quoted 46 lines
Quoted 46 lines
Signed-off-by Hui Su <[email protected]>
---
 mm/vmalloc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
mm/vmalloc.c +6 -5
index be4724b916b3..1cf50749a209 100644
@@ -2305,10 +2305,11 @@ void vfree_atomic(const void *addr)
 {
 	BUG_ON(in_nmi());
 
-	kmemleak_free(addr);
-
 	if (!addr)
 		return;
+
+	kmemleak_free(addr);
+
 	__vfree_deferred(addr);
 }
 

@@ -2340,13 +2341,13 @@ void vfree(const void *addr)
 {
 	BUG_ON(in_nmi());
 
+	if (!addr)
+		return;
+
 	kmemleak_free(addr);
 
 	might_sleep_if(!in_interrupt());
 
-	if (!addr)
-		return;
-
 	__vfree(addr);
 }
 EXPORT_SYMBOL(vfree);
Signature · 1 line
2.25.1