[dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak on device hotplug

[dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak on device hotplug

[Darek Stojaczyk]

rte_mp_request_sync() says that the caller is responsible
for freeing one of its parameters afterwards. EAL didn't
do that, causing a memory leak.

Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/common/hotplug_mp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/common/hotplug_mp.c b/lib/librte_eal/common/hotplug_mp.c
index 84f59d95b..9a6a88546 100644
--- a/lib/librte_eal/common/hotplug_mp.c
+++ b/lib/librte_eal/common/hotplug_mp.c
@@ -355,6 +355,7 @@ int eal_dev_hotplug_request_to_primary(struct eal_dev_mp_req *req)
 	resp = (struct eal_dev_mp_req *)mp_reply.msgs[0].param;
 	req->result = resp->result;
 
+	free(mp_reply.msgs);
 	return ret;
 }
 
@@ -397,6 +398,7 @@ int eal_dev_hotplug_request_to_secondary(struct eal_dev_mp_req *req)
 		}
 	}
 
+	free(mp_reply.msgs);
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak on device hotplug

[Burakov, Anatoly]

On 25-Oct-18 11:46 AM, Darek Stojaczyk wrote:
> rte_mp_request_sync() says that the caller is responsible
> for freeing one of its parameters afterwards. EAL didn't
> do that, causing a memory leak.
> 
> Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
>   lib/librte_eal/common/hotplug_mp.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_eal/common/hotplug_mp.c b/lib/librte_eal/common/hotplug_mp.c
> index 84f59d95b..9a6a88546 100644
> --- a/lib/librte_eal/common/hotplug_mp.c
> +++ b/lib/librte_eal/common/hotplug_mp.c
> @@ -355,6 +355,7 @@ int eal_dev_hotplug_request_to_primary(struct eal_dev_mp_req *req)
>   	resp = (struct eal_dev_mp_req *)mp_reply.msgs[0].param;
>   	req->result = resp->result;
>   
> +	free(mp_reply.msgs);
>   	return ret;
>   }
>   
> @@ -397,6 +398,7 @@ int eal_dev_hotplug_request_to_secondary(struct eal_dev_mp_req *req)
>   		}
>   	}
>   
> +	free(mp_reply.msgs);
>   	return 0;
>   }
>   
> 

This is correct but incomplete. There are also numerous error conditions 
which check for number of received responses to be a particular number, 
and if the number don't match, we just exit without freeing memory. 
Those errors need to free the memory as well.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2] eal: fix IPC memleak on device hotplug

[Darek Stojaczyk]

rte_mp_request_sync() says that the caller is responsible
for freeing one of its parameters afterwards. EAL didn't
do that, causing a memory leak.

Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
Cc: qi.z.zhang@intel.com
Cc: anatoly.burakov@intel.com

Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
---
 lib/librte_eal/common/hotplug_mp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_eal/common/hotplug_mp.c b/lib/librte_eal/common/hotplug_mp.c
index 84f59d95b..b68e4cabb 100644
--- a/lib/librte_eal/common/hotplug_mp.c
+++ b/lib/librte_eal/common/hotplug_mp.c
@@ -355,6 +355,7 @@ int eal_dev_hotplug_request_to_primary(struct eal_dev_mp_req *req)
 	resp = (struct eal_dev_mp_req *)mp_reply.msgs[0].param;
 	req->result = resp->result;
 
+	free(mp_reply.msgs);
 	return ret;
 }
 
@@ -379,6 +380,7 @@ int eal_dev_hotplug_request_to_secondary(struct eal_dev_mp_req *req)
 
 	if (mp_reply.nb_sent != mp_reply.nb_received) {
 		RTE_LOG(ERR, EAL, "not all secondary reply\n");
+		free(mp_reply.msgs);
 		return -1;
 	}
 
@@ -397,6 +399,7 @@ int eal_dev_hotplug_request_to_secondary(struct eal_dev_mp_req *req)
 		}
 	}
 
+	free(mp_reply.msgs);
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak on device hotplug

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Friday, October 26, 2018 4:22 PM
> To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak
> on device hotplug
> 
> <SNIP>
>
> This is correct but incomplete. There are also numerous error conditions
> which check for number of received responses to be a particular number,
> and if the number don't match, we just exit without freeing memory.
> Those errors need to free the memory as well.
> 

Yup, thanks. I pushed v2 with one extra free() in the function notifying secondary processes.
The function which notifies the primary process has a similar error check -
 - `if (mp_reply.nb_received == 1)`  - but I figured if there's more than 1 primary process
replying, then the memory leak is your smallest problem.

> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak on device hotplug

[Zhang, Qi Z]

> -----Original Message-----
> From: Stojaczyk, Dariusz
> Sent: Monday, October 29, 2018 7:02 AM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak on
> device hotplug
> 
> > -----Original Message-----
> > From: Burakov, Anatoly
> > Sent: Friday, October 26, 2018 4:22 PM
> > To: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_mp_request_sync() memleak
> > on device hotplug
> >
> > <SNIP>
> >
> > This is correct but incomplete. There are also numerous error
> > conditions which check for number of received responses to be a
> > particular number, and if the number don't match, we just exit without
> freeing memory.
> > Those errors need to free the memory as well.
> >
> 
> Yup, thanks. I pushed v2 with one extra free() in the function notifying
> secondary processes.
> The function which notifies the primary process has a similar error check -
>  - `if (mp_reply.nb_received == 1)`  - but I figured if there's more than 1
> primary process replying, then the memory leak is your smallest problem.


Good capture!
Thanks for fix this.

> 
> > --
> > Thanks,
> > Anatoly

Re: [dpdk-dev] [PATCH v2] eal: fix IPC memleak on device hotplug

[Burakov, Anatoly]

On 29-Oct-18 11:47 AM, Darek Stojaczyk wrote:
> rte_mp_request_sync() says that the caller is responsible
> for freeing one of its parameters afterwards. EAL didn't
> do that, causing a memory leak.
> 
> Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> Cc: qi.z.zhang@intel.com
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] eal: fix IPC memleak on device hotplug

[Thomas Monjalon]

30/10/2018 11:15, Burakov, Anatoly:
> On 29-Oct-18 11:47 AM, Darek Stojaczyk wrote:
> > rte_mp_request_sync() says that the caller is responsible
> > for freeing one of its parameters afterwards. EAL didn't
> > do that, causing a memory leak.
> > 
> > Fixes: 244d5130719c ("eal: enable hotplug on multi-process")
> > Cc: qi.z.zhang@intel.com
> > Cc: anatoly.burakov@intel.com
> > 
> > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks