usb: dwc3: free dwc->regset on dwc3_debugfs_exit

PATCH In review [email protected] 2 participants 14 messages Apr 8, 2016 – Apr 12, 2016

Subsystems (2)

USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

[email protected]

author

2016-04-08 09:52 UTC #0 lore raw

PATCH 1/1 usb: dwc3: free dwc->regset on dwc3_debugfs_exit

From: "Du, Changbin" <[email protected]>

dwc->regset is allocated on dwc3_debugfs_init, and should be released on dwc3_debugfs_exit.

Signed-off-by Du, Changbin <[email protected]>

---
This patch is seperated from patch set:
[PATCH v2 0/3] Improvement, fix and new entry for dwc3 debugfs
Because it is not related to other 2 patches.

---
 drivers/usb/dwc3/debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/usb/dwc3/debugfs.c +3 -0

index 9ac37fe..9eeb444 100644
@@ -687,4 +687,7 @@ void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
 	dwc->root = NULL;
+
+	kfree(dwc->regset);
+	dwc->regset = NULL;
 }

Signature · 1 line

2.5.0

Felipe Balbi

2016-04-11 08:08 UTC #1 lore raw

Quoted 20 lines · [email protected] writes:

From: "Du, Changbin" <[email protected]>
dwc->regset is allocated on dwc3_debugfs_init, and should be released on dwc3_debugfs_exit.
Signed-off-by Du, Changbin <[email protected]>
---
This patch is seperated from patch set:
[PATCH v2 0/3] Improvement, fix and new entry for dwc3 debugfs
Because it is not related to other 2 patches.
---
 drivers/usb/dwc3/debugfs.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/usb/dwc3/debugfs.c +0 -0
index 9ac37fe..9eeb444 100644

drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 691

Quoted 5 lines

drivers/usb/dwc3/debugfs.c +2 -0

@@ -687,4 +687,7 @@ void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
 	dwc->root = NULL;
+
+	kfree(dwc->regset);

we also need a kfree() on dwc3_debugfs_init().

drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 692

Quoted 1 line

drivers/usb/dwc3/debugfs.c +1 -0
+	dwc->regset = NULL;

setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit() when removing the driver.

Signature · 1 line

balbi

Du, Changbin

author

2016-04-11 11:09 UTC #2 lore raw

Quoted 4 lines

Quoted 4 lines
drivers/usb/dwc3/debugfs.c +0 -0
index 9ac37fe..9eeb444 100644

Quoted 8 lines

drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 691
Quoted 6 lines
drivers/usb/dwc3/debugfs.c +2 -0
@@ -687,4 +687,7 @@ void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
 	dwc->root = NULL;
+
+	kfree(dwc->regset);
we also need a kfree() on dwc3_debugfs_init().

This patch is based on the patch set
[PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void> So, they do has dependency. :)

Quoted 7 lines

drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 692
Quoted 1 line
drivers/usb/dwc3/debugfs.c +1 -0
+	dwc->regset = NULL;
setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit() when removing the driver.
--
Balbi

I'd like keep this line even it is unnecessary, because It is a good habit to Avoid wild pointers. Just like the dwc->root = NULL.

Thanks,
Du, Changbin

Felipe Balbi

2016-04-11 11:15 UTC #3 lore raw

Quoted 12 lines · Changbin

Quoted 4 lines
Quoted 4 lines
drivers/usb/dwc3/debugfs.c +0 -0
index 9ac37fe..9eeb444 100644
Quoted 8 lines
drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 691
Quoted 6 lines
drivers/usb/dwc3/debugfs.c +2 -0
@@ -687,4 +687,7 @@ void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
 	dwc->root = NULL;
+
+	kfree(dwc->regset);
we also need a kfree() on dwc3_debugfs_init().

Quoted 3 lines

This patch is based on the patch set
[PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void> So, they do has dependency. :)

I _did_ mention on previous set that fixes and new-features shouldn't have dependencies between them. That was the whole point of splitting the series in two.

Quoted 9 lines

Quoted 7 lines
drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 692
Quoted 1 line
drivers/usb/dwc3/debugfs.c +1 -0
+	dwc->regset = NULL;
setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit() when removing the driver.
--
Balbi
I'd like keep this line even it is unnecessary, because It is a good habit to Avoid wild pointers. Just like the dwc->root = NULL.

there won't be any wild pointers here, we'll free struct dwc3 *dwc itself.

Signature · 1 line

balbi

Du, Changbin

author

2016-04-11 11:39 UTC #4 lore raw

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 692
Quoted 2 lines
drivers/usb/dwc3/debugfs.c +1 -0
+	dwc->regset = NULL;
setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit() when removing the driver.
--
Balbi

Quoted 7 lines

Quoted 2 lines
I'd like keep this line even it is unnecessary, because It is a good habit to Avoid wild pointers. Just like the dwc->root = NULL.
there won't be any wild pointers here, we'll free struct dwc3 *dwc itself.
--
Balbi

I agree the dwc will be freed in current code. But the 'free' logical is out of the debugfs code. They should be treat as some logical independent. Per this point, I still think set pointer to null is not bad. For example, if dwc3 core code invoke dwc3_debugfs_exit twice by mistake(just an example case, not really), then no crash/impact for the second call.

Thanks,
Du, Changbin

Felipe Balbi

2016-04-11 12:11 UTC #5 lore raw

Hi,

Quoted 13 lines · Changbin

Quoted 13 lines
Quoted 10 lines
Quoted 8 lines
drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 692
Quoted 2 lines
drivers/usb/dwc3/debugfs.c +1 -0
+	dwc->regset = NULL;
setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit() when removing the driver.
--
Balbi
I'd like keep this line even it is unnecessary, because It is a good habit to Avoid wild pointers. Just like the dwc->root = NULL.
there won't be any wild pointers here, we'll free struct dwc3 *dwc itself.

Quoted 7 lines

Quoted 2 lines
--
Balbi
I agree the dwc will be freed in current code. But the 'free' logical is out of the debugfs code. They should be treat as some logical independent. Per this point, I still think set pointer to null is not bad. For example, if dwc3 core code invoke dwc3_debugfs_exit twice by mistake(just an example case, not really), then no crash/impact for the second call.

the second call should crash because it's clearly wrong ;-) If dwc3 ever calls dwc3_debugfs_exit() twice, it really deserves to crash. It's something so wrong that we want the verbosity and urgency of a kernel oops to make sure we fix it ASAP.

If, however, we set it to null, it might be years before we notice anything's wrong.

Signature · 1 line

balbi

Du, Changbin

author

2016-04-12 02:10 UTC #6 lore raw

Quoted 33 lines

Hi,
Quoted 4 lines · Changbin
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/usb/dwc3/debugfs.c void dwc3_debugfs_exit(struct dwc3 *dwc) · line 692
Quoted 2 lines
drivers/usb/dwc3/debugfs.c +1 -0
+	dwc->regset = NULL;
setting regset to NULL is unnecessary. We only call
dwc3_debugfs_exit()
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
when removing the driver.
--
Balbi
I'd like keep this line even it is unnecessary, because It is a good habit to Avoid wild pointers. Just like the dwc->root = NULL.
Quoted 6 lines
Quoted 4 lines
there won't be any wild pointers here, we'll free struct dwc3 *dwc itself.
--
Balbi
I agree the dwc will be freed in current code. But the 'free' logical is out of the debugfs code. They should be treat as some logical independent.
Per
Quoted 1 line
this point, I still think set pointer to null is not bad. For example, if dwc3
core
Quoted 1 line
code invoke dwc3_debugfs_exit twice by mistake(just an example case,
not
Quoted 1 line
really), then no crash/impact for the second call.
the second call should crash because it's clearly wrong ;-) If dwc3 ever calls dwc3_debugfs_exit() twice, it really deserves to crash. It's something so wrong that we want the verbosity and urgency of a kernel oops to make sure we fix it ASAP.

Quoted 5 lines

If, however, we set it to null, it might be years before we notice anything's wrong.
--
Balbi

Hmm, I agree from this point. I will combine this patch with other two patches (due to their dependency). And I'd like remove the 'dwc->root=NULL' as well, Is it ok for you?

Thx,
Du, Changbin

Felipe Balbi

2016-04-12 06:20 UTC #7 lore raw

Hi Changbin,

Quoted 33 lines · Changbin

Quoted 33 lines
Hi,
Quoted 4 lines · Changbin
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
+	dwc->regset = NULL;
setting regset to NULL is unnecessary. We only call
dwc3_debugfs_exit()
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
when removing the driver.
--
Balbi
I'd like keep this line even it is unnecessary, because It is a good habit to Avoid wild pointers. Just like the dwc->root = NULL.
Quoted 6 lines
Quoted 4 lines
there won't be any wild pointers here, we'll free struct dwc3 *dwc itself.
--
Balbi
I agree the dwc will be freed in current code. But the 'free' logical is out of the debugfs code. They should be treat as some logical independent.
Per
Quoted 1 line
this point, I still think set pointer to null is not bad. For example, if dwc3
core
Quoted 1 line
code invoke dwc3_debugfs_exit twice by mistake(just an example case,
not
Quoted 1 line
really), then no crash/impact for the second call.
the second call should crash because it's clearly wrong ;-) If dwc3 ever calls dwc3_debugfs_exit() twice, it really deserves to crash. It's something so wrong that we want the verbosity and urgency of a kernel oops to make sure we fix it ASAP.

Quoted 8 lines

Quoted 5 lines
If, however, we set it to null, it might be years before we notice anything's wrong.
--
Balbi
Hmm, I agree from this point. I will combine this patch with other two patches (due to their dependency). And I'd like remove the 'dwc->root=NULL' as well,

you are creating a dependency that doesn't exist. Please stop that. You should have two separate branches based on v4.6-rc3 (or, if you prefer, one based on my testing/fixes and another based on my testing/next). On one branch you have *only* $subject and you fix *all* the memory leaks. On the other branch you have the other two patches.

Ignore the fact that we might have a conflict, that's for git (and maintainers) to handle when they happen.

Again, don't create dependencies between fixes for the -rc cycle and changes for the next merge window.

Quoted 1 line

Is it ok for you?

yeah, please remove root = NULL as that's completely unnecessary, but split these patches in separate branches and fix all memory leaks.

Signature · 1 line

balbi

Du, Changbin

author

2016-04-12 06:52 UTC #8 lore raw

Hi, Balbi,

Quoted 11 lines

Quoted 1 line
Hmm, I agree from this point. I will combine this patch with other two
patches
Quoted 1 line
(due to their dependency). And I'd like remove the 'dwc->root=NULL' as
well,
you are creating a dependency that doesn't exist. Please stop that. You should have two separate branches based on v4.6-rc3 (or, if you prefer, one based on my testing/fixes and another based on my testing/next). On one branch you have *only* $subject and you fix *all* the memory leaks. On the other branch you have the other two patches.

Quoted 6 lines

Ignore the fact that we might have a conflict, that's for git (and maintainers) to handle when they happen.
Again, don't create dependencies between fixes for the -rc cycle and changes for the next merge window.

Thanks for dedicated explanation. I was concern about the conflict. Now it is very clear for me to handle such situation.

Quoted 7 lines

Quoted 1 line
Is it ok for you?
yeah, please remove root = NULL as that's completely unnecessary, but split these patches in separate branches and fix all memory leaks.
--
balbi

Felipe Balbi

2016-04-12 08:08 UTC #9 lore raw

Hi,

Quoted 13 lines · Changbin

Hi, Balbi,
Quoted 11 lines
Quoted 1 line
Hmm, I agree from this point. I will combine this patch with other two
patches
Quoted 1 line
(due to their dependency). And I'd like remove the 'dwc->root=NULL' as
well,
you are creating a dependency that doesn't exist. Please stop that. You should have two separate branches based on v4.6-rc3 (or, if you prefer, one based on my testing/fixes and another based on my testing/next). On one branch you have *only* $subject and you fix *all* the memory leaks. On the other branch you have the other two patches.

Quoted 7 lines

Quoted 6 lines
Ignore the fact that we might have a conflict, that's for git (and maintainers) to handle when they happen.
Again, don't create dependencies between fixes for the -rc cycle and changes for the next merge window.
Thanks for dedicated explanation. I was concern about the conflict.

yeah, no problem ;-)

Quoted 1 line

Now it is very clear for me to handle such situation.

good :-) glad we could sort it out.

Signature · 1 line

balbi

[email protected]

author

2016-04-12 08:25 UTC #10 lore raw

PATCH v2 1/1 usb: dwc3: fix memory leak of dwc->regset

In reply to Felipe Balbi

From: "Du, Changbin" <[email protected]>

dwc->regset is allocated on dwc3_debugfs_init, and should be released on init failure or dwc3_debugfs_exit. Btw, The line "dwc->root = NULL" is unnecessary, so remove it.

Signed-off-by Du, Changbin <[email protected]>

---
v2:
Title changed;
free dwc->regset on failure path.

---
 drivers/usb/dwc3/debugfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/usb/dwc3/debugfs.c +3 -2

index 9ac37fe..abd8889 100644
@@ -678,7 +678,8 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
 
 err1:
 	debugfs_remove_recursive(root);
-
+	if (!dwc->regset)
+		kfree(dwc->regset);
 err0:
 	return ret;
 }

@@ -686,5 +687,5 @@ err0:
 void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
-	dwc->root = NULL;
+	kfree(dwc->regset);
 }

Signature · 1 line

2.5.0

Felipe Balbi

2016-04-12 08:28 UTC #11 lore raw

Re: [PATCH v2] usb: dwc3: fix memory leak of dwc->regset

Hi,

Quoted 21 lines · [email protected] writes:

From: "Du, Changbin" <[email protected]>
dwc->regset is allocated on dwc3_debugfs_init, and should be released on init failure or dwc3_debugfs_exit. Btw, The line "dwc->root = NULL" is unnecessary, so remove it.
Signed-off-by Du, Changbin <[email protected]>
---
v2:
Title changed;
free dwc->regset on failure path.
---
 drivers/usb/dwc3/debugfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/usb/dwc3/debugfs.c +0 -0
index 9ac37fe..abd8889 100644

drivers/usb/dwc3/debugfs.c int dwc3_debugfs_init(struct dwc3 *dwc) · line 682

Quoted 6 lines

drivers/usb/dwc3/debugfs.c +2 -1

@@ -678,7 +678,8 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
 
 err1:
 	debugfs_remove_recursive(root);
-
+	if (!dwc->regset)
+		kfree(dwc->regset);

IOW:

if regset is NULL, free NULL.

This check is wrong and unnecessary ;-) kfree(NULL) is safe.

Signature · 1 line

balbi

[email protected]

author

2016-04-12 08:35 UTC #12 lore raw

PATCH v3 1/1 usb: dwc3: fix memory leak of dwc->regset

From: "Du, Changbin" <[email protected]>

dwc->regset is allocated on dwc3_debugfs_init, and should be released on init failure or dwc3_debugfs_exit. Btw, The line "dwc->root = NULL" is unnecessary, so remove it.

Signed-off-by Du, Changbin <[email protected]>

---
v3:
remove unnecessary if(!NULL) for free
v2:
Title changed;
free dwc->regset on failure path.

---
 drivers/usb/dwc3/debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/usb/dwc3/debugfs.c +2 -2

index 9ac37fe..07d99eb 100644
@@ -678,7 +678,7 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
 
 err1:
 	debugfs_remove_recursive(root);
-
+	kfree(dwc->regset);
 err0:
 	return ret;
 }

@@ -686,5 +686,5 @@ err0:
 void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
-	dwc->root = NULL;
+	kfree(dwc->regset);
 }

Signature · 1 line

2.5.0

Felipe Balbi

2016-04-12 10:11 UTC #13 lore raw

Re: [PATCH v3] usb: dwc3: fix memory leak of dwc->regset

Hi,

Quoted 8 lines · [email protected] writes:

From: "Du, Changbin" <[email protected]>
dwc->regset is allocated on dwc3_debugfs_init, and should be released on init failure or dwc3_debugfs_exit. Btw, The line "dwc->root = NULL" is unnecessary, so remove it.
Signed-off-by Du, Changbin <[email protected]>
---

I've modifed the patch a little bit just to make sure the error labels are easy to follow:

commit c7232c5fdcc5e1b6fc39b498b58573553ebb43da

Author: Du, Changbin <[email protected]>
Date:   Tue Apr 12 16:24:34 2016 +0800

usb: dwc3: fix memory leak of dwc->regset

dwc->regset is allocated on dwc3_debugfs_init, and should be released on init failure or dwc3_debugfs_exit. Btw, The line "dwc->root = NULL" is unnecessary, so remove it.

Signed-off-by Du, Changbin <[email protected]>

[ [email protected] : add another err label for the new
error condition ]

Signed-off-by Felipe Balbi <[email protected]>

drivers/usb/dwc3/debugfs.c +8 -5

index 9ac37fe1b6a7..cebf9e38b60a 100644
@@ -645,7 +645,7 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
 	file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
 	if (!file) {
 		ret = -ENOMEM;
-		goto err1;
+		goto err2;
 	}
 
 	if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {

@@ -653,7 +653,7 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
 				dwc, &dwc3_mode_fops);
 		if (!file) {
 			ret = -ENOMEM;
-			goto err1;
+			goto err2;
 		}
 	}

@@ -663,19 +663,22 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
 				dwc, &dwc3_testmode_fops);
 		if (!file) {
 			ret = -ENOMEM;
-			goto err1;
+			goto err2;
 		}
 
 		file = debugfs_create_file("link_state", S_IRUGO | S_IWUSR, root,
 				dwc, &dwc3_link_state_fops);
 		if (!file) {
 			ret = -ENOMEM;
-			goto err1;
+			goto err2;
 		}
 	}
 
 	return 0;
 
+err2:
+	kfree(dwc->regset);
+
 err1:
 	debugfs_remove_recursive(root);

@@ -686,5 +689,5 @@ err0:
 void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
 	debugfs_remove_recursive(dwc->root);
-	dwc->root = NULL;
+	kfree(dwc->regset);
 }

patch is longer but functionally equivalent.

Signature · 1 line

balbi