[PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[Abdun Nihaal]

In efx_mae_enumerate_mports(), memory allocated for mae_mport_desc is
passed as a argument to efx_mae_process_mport(), but when the error path
in efx_mae_process_mport() gets executed, the memory allocated for desc
gets leaked.

Fix that by freeing the memory allocation before returning error.

Fixes: a6a15aca4207 ("sfc: enumerate mports in ef100")
Acked-by: Edward Cree <ecree.xilinx@gmail.com>
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---

v1->v2:
- Added a comment to tell that efx_mae_process_mport takes ownership of
   @desc, as suggested by Edward
- Also added Acked-by tag from Edward

Link to v1 patch:
https://patchwork.kernel.org/project/netdevbpf/patch/20251022163525.86362-1-nihaal@cse.iitm.ac.in/

 drivers/net/ethernet/sfc/mae.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/sfc/mae.c b/drivers/net/ethernet/sfc/mae.c
index 6fd0c1e9a7d5..7cfd9000f79d 100644
--- a/drivers/net/ethernet/sfc/mae.c
+++ b/drivers/net/ethernet/sfc/mae.c
@@ -1090,6 +1090,9 @@ void efx_mae_remove_mport(void *desc, void *arg)
 	kfree(mport);
 }
 
+/*
+ * Takes ownership of @desc, even if it returns an error
+ */
 static int efx_mae_process_mport(struct efx_nic *efx,
 				 struct mae_mport_desc *desc)
 {
@@ -1100,6 +1103,7 @@ static int efx_mae_process_mport(struct efx_nic *efx,
 	if (!IS_ERR_OR_NULL(mport)) {
 		netif_err(efx, drv, efx->net_dev,
 			  "mport with id %u does exist!!!\n", desc->mport_id);
+		kfree(desc);
 		return -EEXIST;
 	}
 
-- 
2.43.0

Re: [PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 433 bytes --]



On 10/23/2025 7:18 AM, Abdun Nihaal wrote:
> In efx_mae_enumerate_mports(), memory allocated for mae_mport_desc is
> passed as a argument to efx_mae_process_mport(), but when the error path
> in efx_mae_process_mport() gets executed, the memory allocated for desc
> gets leaked.
> 
> Fix that by freeing the memory allocation before returning error.
> 

Why not make the caller responsible for freeing desc on failure?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[Abdun Nihaal]

On Thu, Oct 23, 2025 at 05:48:12PM -0700, Jacob Keller wrote:
> Why not make the caller responsible for freeing desc on failure?

I put the free inside because another function efx_mae_add_mport 
called by efx_mae_process_mport was freeing desc on failure, so I
followed the same style of code, but yes making the caller responsible
would be more cleaner. I'll send a revised patch.

Re: [PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[Edward Cree]

On 24/10/2025 01:48, Jacob Keller wrote:
> On 10/23/2025 7:18 AM, Abdun Nihaal wrote:
>> In efx_mae_enumerate_mports(), memory allocated for mae_mport_desc is
>> passed as a argument to efx_mae_process_mport(), but when the error path
>> in efx_mae_process_mport() gets executed, the memory allocated for desc
>> gets leaked.
>>
>> Fix that by freeing the memory allocation before returning error.
> 
> Why not make the caller responsible for freeing desc on failure?

Since the callee takes ownership of desc on success (it stashes it in a
 table), arguably it's cleaner to have it do so in all cases; it's an
 aesthetic judgment call but I think I'd rather keep it this way and just
 fix this one failure path than change all the existing failure paths and
 the caller.
Alejandro (original author of this code) might have a different opinion
 in which case I'll defer to him but otherwise I'd say v2 is fine to apply
 as-is.

Re: [PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1037 bytes --]



On 10/24/2025 7:48 AM, Edward Cree wrote:
> On 24/10/2025 01:48, Jacob Keller wrote:
>> On 10/23/2025 7:18 AM, Abdun Nihaal wrote:
>>> In efx_mae_enumerate_mports(), memory allocated for mae_mport_desc is
>>> passed as a argument to efx_mae_process_mport(), but when the error path
>>> in efx_mae_process_mport() gets executed, the memory allocated for desc
>>> gets leaked.
>>>
>>> Fix that by freeing the memory allocation before returning error.
>>
>> Why not make the caller responsible for freeing desc on failure?
> 
> Since the callee takes ownership of desc on success (it stashes it in a
>  table), arguably it's cleaner to have it do so in all cases; it's an
>  aesthetic judgment call but I think I'd rather keep it this way and just
>  fix this one failure path than change all the existing failure paths and
>  the caller.
> Alejandro (original author of this code) might have a different opinion
>  in which case I'll defer to him but otherwise I'd say v2 is fine to apply
>  as-is.

Fair enough.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v2 net] sfc: fix potential memory leak in efx_mae_process_mport()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 23 Oct 2025 19:48:42 +0530 you wrote:
> In efx_mae_enumerate_mports(), memory allocated for mae_mport_desc is
> passed as a argument to efx_mae_process_mport(), but when the error path
> in efx_mae_process_mport() gets executed, the memory allocated for desc
> gets leaked.
> 
> Fix that by freeing the memory allocation before returning error.
> 
> [...]

Here is the summary with links:
  - [v2,net] sfc: fix potential memory leak in efx_mae_process_mport()
    https://git.kernel.org/netdev/net/c/46a499aaf8c2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html