[PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[wu000273]

From: Qiushi Wu <wu000273@umn.edu>

In function nfp_abm_vnic_set_mac, pointer nsp is allocated by nfp_nsp_open.
But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,
which can lead to a memory leak bug. Thus add a call of the function
“nfp_nsp_close” for the completion of the exception handling.

Fixes: f6e71efdf9fb1 ("nfp: abm: look up MAC addresses via management FW")
Signed-off-by: Qiushi Wu <wu000273@umn.edu>
---
 drivers/net/ethernet/netronome/nfp/abm/main.c | 21 ++++++++++---------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/abm/main.c b/drivers/net/ethernet/netronome/nfp/abm/main.c
index 9183b3e85d21..f196789f62fe 100644
--- a/drivers/net/ethernet/netronome/nfp/abm/main.c
+++ b/drivers/net/ethernet/netronome/nfp/abm/main.c
@@ -265,8 +265,7 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm *abm, struct nfp_net *nn,
 
 	if (id > pf->eth_tbl->count) {
 		nfp_warn(pf->cpp, "No entry for persistent MAC address\n");
-		eth_hw_addr_random(nn->dp.netdev);
-		return;
+		goto generate_random_address;
 	}
 
 	snprintf(hwinfo, sizeof(hwinfo), "eth%u.mac.pf%u",
@@ -276,14 +275,13 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm *abm, struct nfp_net *nn,
 	if (IS_ERR(nsp)) {
 		nfp_warn(pf->cpp, "Failed to access the NSP for persistent MAC address: %ld\n",
 			 PTR_ERR(nsp));
-		eth_hw_addr_random(nn->dp.netdev);
-		return;
+		goto generate_random_address;
 	}
 
 	if (!nfp_nsp_has_hwinfo_lookup(nsp)) {
 		nfp_warn(pf->cpp, "NSP doesn't support PF MAC generation\n");
-		eth_hw_addr_random(nn->dp.netdev);
-		return;
+		nfp_nsp_close(nsp);
+		goto generate_random_address;
 	}
 
 	err = nfp_nsp_hwinfo_lookup(nsp, hwinfo, sizeof(hwinfo));
@@ -291,8 +289,7 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm *abm, struct nfp_net *nn,
 	if (err) {
 		nfp_warn(pf->cpp, "Reading persistent MAC address failed: %d\n",
 			 err);
-		eth_hw_addr_random(nn->dp.netdev);
-		return;
+		goto generate_random_address;
 	}
 
 	if (sscanf(hwinfo, "%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx",
@@ -300,12 +297,16 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm *abm, struct nfp_net *nn,
 		   &mac_addr[3], &mac_addr[4], &mac_addr[5]) != 6) {
 		nfp_warn(pf->cpp, "Can't parse persistent MAC address (%s)\n",
 			 hwinfo);
-		eth_hw_addr_random(nn->dp.netdev);
-		return;
+		goto generate_random_address;
 	}
 
 	ether_addr_copy(nn->dp.netdev->dev_addr, mac_addr);
 	ether_addr_copy(nn->dp.netdev->perm_addr, mac_addr);
+	return;
+
+generate_random_address:
+	eth_hw_addr_random(nn->dp.netdev);
+	return;
 }
 
 static int
-- 
2.17.1

Re: [PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

> … Thus add a call of the function
> “nfp_nsp_close” for the completion of the exception handling.

I suggest to mention also the addition of a jump target because of
a Linux coding style concern.


…
> +++ b/drivers/net/ethernet/netronome/nfp/abm/main.c
…
> @@ -300,12 +297,16 @@ nfp_abm_vnic_set_mac(struct nfp_pf *pf, struct nfp_abm *abm, struct nfp_net *nn,
…
> +generate_random_address:
> +	eth_hw_addr_random(nn->dp.netdev);
> +	return;
>  }
…

I recommend to apply the check “RETURN_VOID” once more.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=0e698dfa282211e414076f9dc7e83c1c288314fd#n4866

Regards,
Markus

Re: [PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

> But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,

Do you distinguish between releasing items and the role of such a pointer?


> which can lead to a memory leak bug.

Would you like to reconsider the usage of the word “can” for
such change descriptions?

Regards,
Markus

Re: [PATCH v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Jakub Kicinski]

On Sun,  3 May 2020 15:49:32 -0500 wu000273@umn.edu wrote:
> From: Qiushi Wu <wu000273@umn.edu>
> 
> In function nfp_abm_vnic_set_mac, pointer nsp is allocated by nfp_nsp_open.
> But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,
> which can lead to a memory leak bug. Thus add a call of the function
> “nfp_nsp_close” for the completion of the exception handling.
> 
> Fixes: f6e71efdf9fb1 ("nfp: abm: look up MAC addresses via management FW")
> Signed-off-by: Qiushi Wu <wu000273@umn.edu>

This just makes the code longer. v1 was perfectly fine, thanks for the
fix. 

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

> By the way, is there anything else that I need to improve for this patch?

I became curious if you would like to adjust further details from
the change description.
Other contributors might care less for presented concerns.

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

> Thanks for your feedback, and yes, I'd like to further adjust the description details
> to make the patch more clear and better.

Thanks for such a positive response.


> But because Jakub seems to prefer v1, so I'm somehow confused

Such a view can be reasonable.
The change acceptance varies between involved contributors.


> if I should submit a new version based on v1

Such a choice depends also on your willingness to clarify and improve
the software situation by better commit messages.


> or based on the version that has an addition of a jump target?

I propose that you can offer patch series with two update steps (for example)
according to the affected function implementation.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=47cf1b422e6093aee2a3e55d5e162112a2c69870#n102

1. Add a missing function call for the completion of the desired resource clean-up.

2. Adjust a bit of exception handling code so that it can be better reused
   at the end of this function.
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=47cf1b422e6093aee2a3e55d5e162112a2c69870#n450


I am curious if you are going to answer (my) previous questions and suggestions
(in constructive ways).

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

>> Do you distinguish between releasing items and the role of such a pointer?
> I didn't distinguish these. 

How do you think about to clarify this aspect a bit more (according to computer science)
besides your subsequent constructive feedback?

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

> What do you think about changing:
> "But when nfp_nsp_has_hwinfo_lookup fail, the pointer is not released,.."
> to
…
> or
> "But when nfp_nsp_has_hwinfo_lookup fail,

I became curious about a related wording variant.

  But when a call of the function “…” failed,


> NSP resource is not cleaned up and unlocked."

I find such information also nicer. (The abbreviation “NSP” might need
another bit of clarification.)

I imagine there might be interests (eventually related to computer science)
to measure the corresponding object sizes because of a missed function call
and offer a more precise information in the commit message
(depending on the willingness to invest efforts in such a data determination).

Will such considerations become relevant for any subsequent
software development approaches?

Regards,
Markus

Re: [v3] nfp: abm: Fix incomplete release of system resources in nfp_abm_vnic_set_mac()

[Markus Elfring]

> I'm curious if I could still modify these commit message information for the v1 patch,
> which has already been applied and queued up?

The maintainer found the provided information good enough.
Thus he committed the software correction with the subject
“nfp: abm: fix a memory leak bug” on 2020-05-04.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/netronome/nfp/abm/main.c?id=bd4af432cc71b5fbfe4833510359a6ad3ada250d

So this change will probably be published “forever” since then.

I got the impression that the corresponding patch review contains helpful information.
I am curious then if it might affect the adjustment of related patches.


>> Will such considerations become relevant for any subsequent
>> software development approaches?
>
> Sorry, I actually don't familiar with these.

I am informed in the way that you can participate in university research groups.
Thus I assumed that you would like to add recent insights
from computer science areas.
I imagined that the bug report (combined with a patch) was triggered by
an evolving source code analysis approach which will be explained
in another research paper. Is such a view appropriate?
https://github.com/umnsec/cheq/

Regards,
Markus

Re: [v3] nfp: abm: University research groups?

[Markus Elfring]

> > I imagined that the bug report (combined with a patch) was triggered by
> > an evolving source code analysis approach which will be explained
> > in another research paper. Is such a view appropriate?
> > https://github.com/umnsec/cheq/
>
> Could you elaborate more on "university research groups?"

You are working together for the publishing of some papers which will
eventually be presented at conferences.
You might build additional relationships and participate in further work groups.


> We are continuously working on automated kernel analysis

This is good to know.


> to improve the unfortunately very buggy kernel.

There are various software development challenges to consider.

Have you got a desire to express connections to recent research results
also in commit messages?

Regards,
Markus