gcov: cleanup an allocation in new_node()

PATCH In review Dan Carpenter 2 participants 4 messages Sep 11, 2012 – Sep 17, 2012

Subsystems (2)

Kernel janitors Gcov based kernel profiling

lore

Share & tools

lore

Dan Carpenter

author

2012-09-11 11:13 UTC #0 lore raw

PATCH 1/1 gcov: cleanup an allocation in new_node()

1) kcalloc(1, ...) can be replaced with kzalloc().
2) There was a small leak if the debugfs_create_dir() fails.
3) I removed a pr_warning("out of memory\n") because kmalloc() already prints a more useful failure message in the unlikely case of an allocation failure.

Signed-off-by Dan Carpenter <[email protected]>

kernel/gcov/fs.c +5 -4

index 9bd0934..132168f 100644
@@ -437,7 +437,7 @@ static struct gcov_node *new_node(struct gcov_node *parent,
 	if (!node)
 		goto err_nomem;
 	if (info) {
-		node->loaded_info = kcalloc(1, sizeof(struct gcov_info *),
+		node->loaded_info = kzalloc(sizeof(struct gcov_info *),
 					   GFP_KERNEL);
 		if (!node->loaded_info)
 			goto err_nomem;

@@ -451,8 +451,7 @@ static struct gcov_node *new_node(struct gcov_node *parent,
 		node->dentry = debugfs_create_dir(node->name, parent->dentry);
 	if (!node->dentry) {
 		pr_warning("could not create file\n");
-		kfree(node);
-		return NULL;
+		goto err_info;
 	}
 	if (info)
 		add_links(node, parent->dentry);

@@ -461,9 +460,11 @@ static struct gcov_node *new_node(struct gcov_node *parent,
 
 	return node;
 
+err_info:
+	if (info)
+		kfree(node->loaded_info);
 err_nomem:
 	kfree(node);
-	pr_warning("out of memory\n");
 	return NULL;
 }

Peter Oberparleiter

2012-09-13 12:43 UTC #1 lore raw

Quoted 13 lines · Dan Carpenter

1) kcalloc(1, ...) can be replaced with kzalloc().
2) There was a small leak if the debugfs_create_dir() fails.
3) I removed a pr_warning("out of memory\n") because kmalloc() already prints a more useful failure message in the unlikely case of an allocation failure.
Signed-off-by Dan Carpenter <[email protected]>
kernel/gcov/fs.c +0 -0
index 9bd0934..132168f 100644

kernel/gcov/fs.c static struct gcov_node *new_node(struct gcov_node *parent, · line 441

Quoted 6 lines

kernel/gcov/fs.c +1 -1

@@ -437,7 +437,7 @@ static struct gcov_node *new_node(struct gcov_node *parent,
  	if (!node)
  		goto err_nomem;
  	if (info) {
-		node->loaded_info = kcalloc(1, sizeof(struct gcov_info *),
+		node->loaded_info = kzalloc(sizeof(struct gcov_info *),
  					   GFP_KERNEL);

I'd like to keep using kcalloc here since node->loaded_info is an array.

kernel/gcov/fs.c static struct gcov_node *new_node(struct gcov_node *parent, · line 454

Quoted 9 lines

kernel/gcov/fs.c +1 -2

  		if (!node->loaded_info)
  			goto err_nomem;

@@ -451,8 +451,7 @@ static struct gcov_node *new_node(struct gcov_node *parent,
  		node->dentry = debugfs_create_dir(node->name, parent->dentry);
  	if (!node->dentry) {
  		pr_warning("could not create file\n");
-		kfree(node);
-		return NULL;
+		goto err_info;

I agree that this is a potential memory leak - good find!

kernel/gcov/fs.c static struct gcov_node *new_node(struct gcov_node *parent, · line 465

Quoted 10 lines

kernel/gcov/fs.c +3 -0

  	}
  	if (info)
  		add_links(node, parent->dentry);

@@ -461,9 +460,11 @@ static struct gcov_node *new_node(struct gcov_node *parent,
 
  	return node;
 
+err_info:
+	if (info)
+		kfree(node->loaded_info);

The 'if' here is not necessary: node->loaded_info is either zero (kfree tolerates NULL) or requires freeing. With the removal of the pr_warning below, the additional kfree can also be added to err_nomem (no need for another label) though then it should probably also be renamed.

kernel/gcov/fs.c static struct gcov_node *new_node(struct gcov_node *parent, · line 470

Quoted 7 lines

kernel/gcov/fs.c +0 -1

  err_nomem:
  	kfree(node);
-	pr_warning("out of memory\n");
  	return NULL;
  }

Regards,
Peter Oberparleiter

Signature · 2 lines

Peter Oberparleiter
Linux on System z Development - IBM Germany

Dan Carpenter

author

2012-09-14 06:52 UTC #2 lore raw

PATCH v2 1/1 gcov: add a kfree() on error to new_node()

We should free "node->loaded_info" on this error path. We'd never care in real life but it makes the static checkers happy.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: some style changes.

kernel/gcov/fs.c +3 -2

index 9bd0934..7ba47f7 100644
@@ -451,8 +451,7 @@ static struct gcov_node *new_node(struct gcov_node *parent,
 		node->dentry = debugfs_create_dir(node->name, parent->dentry);
 	if (!node->dentry) {
 		pr_warning("could not create file\n");
-		kfree(node);
-		return NULL;
+		goto err_info;
 	}
 	if (info)
 		add_links(node, parent->dentry);

@@ -461,6 +460,8 @@ static struct gcov_node *new_node(struct gcov_node *parent,
 
 	return node;
 
+err_info:
+	kfree(node->loaded_info);
 err_nomem:
 	kfree(node);
 	pr_warning("out of memory\n");

Peter Oberparleiter

Reviewed-by Reviewed-by on v2 1/1

2012-09-17 07:58 UTC #3 lore raw

Re: [patch v2] gcov: add a kfree() on error to new_node()

Quoted 7 lines · Dan Carpenter

We should free "node->loaded_info" on this error path. We'd never care in real life but it makes the static checkers happy.
Signed-off-by Dan Carpenter <[email protected]>
---
v2: some style changes.

Signed-off-by Peter Oberparleiter <[email protected]>

Reviewed-by Peter Oberparleiter <[email protected]>

Thanks for sending this patch.

Andrew, since there is no tree for gcov patches, could you queue this for upstream inclusion?

Regards,
Peter Oberparleiter

---
We should free "node->loaded_info" on this error path. We'd never care in real life but it makes the static checkers happy.

Signed-off-by Dan Carpenter <[email protected]>

Signed-off-by Peter Oberparleiter <[email protected]>

Reviewed-by Peter Oberparleiter <[email protected]>

---
v2: some style changes.

kernel/gcov/fs.c +0 -0

index 9bd0934..7ba47f7 100644
@@ -451,8 +451,7 @@ static struct gcov_node *new_node(struct gcov_node

*parent,
node->dentry = debugfs_create_dir(node->name, parent->dentry);
if (!node->dentry) {
pr_warning("could not create file\n");
- kfree(node);
- return NULL;
+ goto err_info;
}
if (info)
add_links(node, parent->dentry);
@@ -461,6 +460,8 @@ static struct gcov_node *new_node(struct gcov_node *parent,

return node;

+err_info:
+ kfree(node->loaded_info);
err_nomem:
kfree(node);
pr_warning("out of memory\n");