[dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL.

Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: jia.quo@intel.com
Cc: gi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 lib/librte_eal/common/include/rte_eal.h | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..abaff5164 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		return -1;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 833433229..575f8119e 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
  * This function sends a request message to the peer process, and will
  * block until receiving reply message from the peer process.
  *
- * @note The caller is responsible to free reply->replies.
+ * @note The caller is responsible to free reply->replies (even if the function
+ *    returned failure).
  *
  * @param req
  *   The req argument contains the customized request message.
-- 
2.17.2

[dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL.

Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: jia.quo@intel.com
Cc: gi.z.zhang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 lib/librte_eal/common/include/rte_eal.h | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..abaff5164 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		return -1;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 833433229..575f8119e 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
  * This function sends a request message to the peer process, and will
  * block until receiving reply message from the peer process.
  *
- * @note The caller is responsible to free reply->replies.
+ * @note The caller is responsible to free reply->replies (even if the function
+ *    returned failure).
  *
  * @param req
  *   The req argument contains the customized request message.
-- 
2.17.2

Re: [dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 17-Apr-19 3:38 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> 
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: jia.quo@intel.com
> Cc: gi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>   lib/librte_eal/common/eal_common_proc.c | 6 +++---
>   lib/librte_eal/common/include/rte_eal.h | 3 ++-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index b46d644b3..abaff5164 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
>   
>   	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
>   
> -	if (check_input(req) == false)
> -		return -1;
> -
>   	reply->nb_sent = 0;
>   	reply->nb_received = 0;
>   	reply->msgs = NULL;
>   
> +	if (check_input(req) == false)
> +		return -1;
> +
>   	if (internal_config.no_shconf) {
>   		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
>   		return 0;
> diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
> index 833433229..575f8119e 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
>    * This function sends a request message to the peer process, and will
>    * block until receiving reply message from the peer process.
>    *
> - * @note The caller is responsible to free reply->replies.
> + * @note The caller is responsible to free reply->replies (even if the function
> + *    returned failure).
>    *
>    * @param req
>    *   The req argument contains the customized request message.
> 

These patches should've been submitted as one patchset - otherwise it is 
very difficult to review it. Please resubmit as a proper patchset.

As far as the code itself goes,

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


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 17-Apr-19 3:38 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> 
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: jia.quo@intel.com
> Cc: gi.z.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
>   lib/librte_eal/common/eal_common_proc.c | 6 +++---
>   lib/librte_eal/common/include/rte_eal.h | 3 ++-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index b46d644b3..abaff5164 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
>   
>   	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
>   
> -	if (check_input(req) == false)
> -		return -1;
> -
>   	reply->nb_sent = 0;
>   	reply->nb_received = 0;
>   	reply->msgs = NULL;
>   
> +	if (check_input(req) == false)
> +		return -1;
> +
>   	if (internal_config.no_shconf) {
>   		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
>   		return 0;
> diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
> index 833433229..575f8119e 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
>    * This function sends a request message to the peer process, and will
>    * block until receiving reply message from the peer process.
>    *
> - * @note The caller is responsible to free reply->replies.
> + * @note The caller is responsible to free reply->replies (even if the function
> + *    returned failure).
>    *
>    * @param req
>    *   The req argument contains the customized request message.
> 

These patches should've been submitted as one patchset - otherwise it is 
very difficult to review it. Please resubmit as a proper patchset.

As far as the code itself goes,

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


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 17-Apr-19 3:38 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> 
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: jia.quo@intel.com
> Cc: gi.z.zhang@intel.com
> Cc: stable@dpdk.org

Also, Qi Zhang's email is wrong. Please don't write anything out 
manually, copy-paste is your friend :)


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH 1/8] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 17-Apr-19 3:38 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> 
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: jia.quo@intel.com
> Cc: gi.z.zhang@intel.com
> Cc: stable@dpdk.org

Also, Qi Zhang's email is wrong. Please don't write anything out 
manually, copy-paste is your friend :)


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2 0/8] ipc: fix possible memleaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.
Bugzilla ID: 228

Herakliusz Lipiec (8):
  ipc: fix rte_mp_request_sync memleak
  ipc: fix hotplug memleak
  ipc: fix vdev memleak
  ipc: fix vfio memleak
  ipc: fix pdump memleak
  ipc: fix tap pmd memleak
  ipc: fix net/mlx4 memleak
  ipc: fix net/mlx5 memleak

 drivers/bus/vdev/vdev.c                 | 3 +--
 drivers/net/mlx4/mlx4_mp.c              | 4 +++-
 drivers/net/mlx5/mlx5_mp.c              | 4 +++-
 drivers/net/tap/rte_eth_tap.c           | 2 ++
 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 lib/librte_eal/common/hotplug_mp.c      | 2 ++
 lib/librte_eal/common/include/rte_eal.h | 3 ++-
 lib/librte_eal/linux/eal/eal_vfio.c     | 8 ++++----
 lib/librte_pdump/rte_pdump.c            | 2 +-
 9 files changed, 21 insertions(+), 13 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v2 0/8] ipc: fix possible memleaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.
Bugzilla ID: 228

Herakliusz Lipiec (8):
  ipc: fix rte_mp_request_sync memleak
  ipc: fix hotplug memleak
  ipc: fix vdev memleak
  ipc: fix vfio memleak
  ipc: fix pdump memleak
  ipc: fix tap pmd memleak
  ipc: fix net/mlx4 memleak
  ipc: fix net/mlx5 memleak

 drivers/bus/vdev/vdev.c                 | 3 +--
 drivers/net/mlx4/mlx4_mp.c              | 4 +++-
 drivers/net/mlx5/mlx5_mp.c              | 4 +++-
 drivers/net/tap/rte_eth_tap.c           | 2 ++
 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 lib/librte_eal/common/hotplug_mp.c      | 2 ++
 lib/librte_eal/common/include/rte_eal.h | 3 ++-
 lib/librte_eal/linux/eal/eal_vfio.c     | 8 ++++----
 lib/librte_pdump/rte_pdump.c            | 2 +-
 9 files changed, 21 insertions(+), 13 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL.
Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 lib/librte_eal/common/include/rte_eal.h | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..abaff5164 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		return -1;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 833433229..575f8119e 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
  * This function sends a request message to the peer process, and will
  * block until receiving reply message from the peer process.
  *
- * @note The caller is responsible to free reply->replies.
+ * @note The caller is responsible to free reply->replies (even if the function
+ *    returned failure).
  *
  * @param req
  *   The req argument contains the customized request message.
-- 
2.17.2

[dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL.
Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 6 +++---
 lib/librte_eal/common/include/rte_eal.h | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..abaff5164 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		return -1;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 833433229..575f8119e 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -309,7 +309,8 @@ rte_mp_sendmsg(struct rte_mp_msg *msg);
  * This function sends a request message to the peer process, and will
  * block until receiving reply message from the peer process.
  *
- * @note The caller is responsible to free reply->replies.
+ * @note The caller is responsible to free reply->replies (even if the function
+ *    returned failure).
  *
  * @param req
  *   The req argument contains the customized request message.
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:43 PM
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>;
> jianfeng.tan@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak
> 
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

Acked-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks!

Re: [dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:43 PM
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>;
> jianfeng.tan@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak
> 
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

Acked-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks!

Re: [dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 23-Apr-19 6:43 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

This is being reworked for the moment, so please don't merge this just yet.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2 1/8] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 23-Apr-19 6:43 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL.
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

This is being reworked for the moment, so please don't merge this just yet.

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2 2/8] ipc: fix hotplug memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: ac9e4a17370f ("eal: support attach/detach shared device from secondary")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@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 4052a5c7f..2c8366afa 100644
--- a/lib/librte_eal/common/hotplug_mp.c
+++ b/lib/librte_eal/common/hotplug_mp.c
@@ -377,6 +377,7 @@ int eal_dev_hotplug_request_to_primary(struct eal_dev_mp_req *req)
 	ret = rte_mp_request_sync(&mp_req, &mp_reply, &ts);
 	if (ret || mp_reply.nb_received != 1) {
 		RTE_LOG(ERR, EAL, "cannot send request to primary");
+		free(mp_reply.msgs);
 		if (!ret)
 			return -1;
 		return ret;
@@ -405,6 +406,7 @@ int eal_dev_hotplug_request_to_secondary(struct eal_dev_mp_req *req)
 	ret = rte_mp_request_sync(&mp_req, &mp_reply, &ts);
 	if (ret != 0) {
 		RTE_LOG(ERR, EAL, "rte_mp_request_sync failed\n");
+		free(mp_reply.msgs);
 		return ret;
 	}
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 2/8] ipc: fix hotplug memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: ac9e4a17370f ("eal: support attach/detach shared device from secondary")
Cc: qi.z.zhang@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@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 4052a5c7f..2c8366afa 100644
--- a/lib/librte_eal/common/hotplug_mp.c
+++ b/lib/librte_eal/common/hotplug_mp.c
@@ -377,6 +377,7 @@ int eal_dev_hotplug_request_to_primary(struct eal_dev_mp_req *req)
 	ret = rte_mp_request_sync(&mp_req, &mp_reply, &ts);
 	if (ret || mp_reply.nb_received != 1) {
 		RTE_LOG(ERR, EAL, "cannot send request to primary");
+		free(mp_reply.msgs);
 		if (!ret)
 			return -1;
 		return ret;
@@ -405,6 +406,7 @@ int eal_dev_hotplug_request_to_secondary(struct eal_dev_mp_req *req)
 	ret = rte_mp_request_sync(&mp_req, &mp_reply, &ts);
 	if (ret != 0) {
 		RTE_LOG(ERR, EAL, "rte_mp_request_sync failed\n");
+		free(mp_reply.msgs);
 		return ret;
 	}
 
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 2/8] ipc: fix hotplug memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:43 PM
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>; Zhang,
> Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 2/8] ipc: fix hotplug memleak
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer even if the request returned failure. Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: ac9e4a17370f ("eal: support attach/detach shared device from
> secondary")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

Shouldn't the commit title be something like "hotplug: fix ipc memleak"?
Or even more specifically "hotplug: fix memleak on ipc failure".

For the patch itself:

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Re: [dpdk-dev] [PATCH v2 2/8] ipc: fix hotplug memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:43 PM
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>; Zhang,
> Qi Z <qi.z.zhang@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 2/8] ipc: fix hotplug memleak
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer even if the request returned failure. Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: ac9e4a17370f ("eal: support attach/detach shared device from
> secondary")
> Cc: qi.z.zhang@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

Shouldn't the commit title be something like "hotplug: fix ipc memleak"?
Or even more specifically "hotplug: fix memleak on ipc failure".

For the patch itself:

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

[dpdk-dev] [PATCH v2 3/8] ipc: fix vdev memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/bus/vdev/vdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 04f76a63f..7c43f2ddd 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -429,10 +429,9 @@ vdev_scan(void)
 			mp_rep = &mp_reply.msgs[0];
 			resp = (struct vdev_param *)mp_rep->param;
 			VDEV_LOG(INFO, "Received %d vdevs", resp->num);
-			free(mp_reply.msgs);
 		} else
 			VDEV_LOG(ERR, "Failed to request vdev from primary");
-
+		free(mp_reply.msgs);
 		/* Fall through to allow private vdevs in secondary process */
 	}
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 3/8] ipc: fix vdev memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/bus/vdev/vdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c
index 04f76a63f..7c43f2ddd 100644
--- a/drivers/bus/vdev/vdev.c
+++ b/drivers/bus/vdev/vdev.c
@@ -429,10 +429,9 @@ vdev_scan(void)
 			mp_rep = &mp_reply.msgs[0];
 			resp = (struct vdev_param *)mp_rep->param;
 			VDEV_LOG(INFO, "Received %d vdevs", resp->num);
-			free(mp_reply.msgs);
 		} else
 			VDEV_LOG(ERR, "Failed to request vdev from primary");
-
+		free(mp_reply.msgs);
 		/* Fall through to allow private vdevs in secondary process */
 	}
 
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 3/8] ipc: fix vdev memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:43 PM
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>;
> jianfeng.tan@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 3/8] ipc: fix vdev memleak
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer even if the request returned failure. Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks!

Re: [dpdk-dev] [PATCH v2 3/8] ipc: fix vdev memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:43 PM
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>;
> jianfeng.tan@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 3/8] ipc: fix vdev memleak
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer even if the request returned failure. Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks!

[dpdk-dev] [PATCH v2 4/8] ipc: fix vfio memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 19e70bb66..d293df062 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -319,8 +319,8 @@ vfio_open_group_fd(int iommu_group_num)
 			RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
 			vfio_group_fd = 0;
 		}
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	if (vfio_group_fd < 0)
 		RTE_LOG(ERR, EAL, "  cannot request group fd\n");
@@ -583,8 +583,8 @@ vfio_sync_default_container(void)
 		p = (struct vfio_mp_param *)mp_rep->param;
 		if (p->result == SOCKET_OK)
 			iommu_type_id = p->iommu_type_id;
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 	if (iommu_type_id < 0) {
 		RTE_LOG(ERR, EAL, "Could not get IOMMU type for default container\n");
 		return -1;
@@ -1050,8 +1050,8 @@ vfio_get_default_container_fd(void)
 			free(mp_reply.msgs);
 			return mp_rep->fds[0];
 		}
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	RTE_LOG(ERR, EAL, "  cannot request default container fd\n");
 	return -1;
@@ -1182,8 +1182,8 @@ rte_vfio_get_container_fd(void)
 			free(mp_reply.msgs);
 			return vfio_container_fd;
 		}
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	RTE_LOG(ERR, EAL, "  cannot request container fd\n");
 	return -1;
-- 
2.17.2

[dpdk-dev] [PATCH v2 4/8] ipc: fix vfio memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linux/eal/eal_vfio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_vfio.c b/lib/librte_eal/linux/eal/eal_vfio.c
index 19e70bb66..d293df062 100644
--- a/lib/librte_eal/linux/eal/eal_vfio.c
+++ b/lib/librte_eal/linux/eal/eal_vfio.c
@@ -319,8 +319,8 @@ vfio_open_group_fd(int iommu_group_num)
 			RTE_LOG(ERR, EAL, "  bad VFIO group fd\n");
 			vfio_group_fd = 0;
 		}
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	if (vfio_group_fd < 0)
 		RTE_LOG(ERR, EAL, "  cannot request group fd\n");
@@ -583,8 +583,8 @@ vfio_sync_default_container(void)
 		p = (struct vfio_mp_param *)mp_rep->param;
 		if (p->result == SOCKET_OK)
 			iommu_type_id = p->iommu_type_id;
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 	if (iommu_type_id < 0) {
 		RTE_LOG(ERR, EAL, "Could not get IOMMU type for default container\n");
 		return -1;
@@ -1050,8 +1050,8 @@ vfio_get_default_container_fd(void)
 			free(mp_reply.msgs);
 			return mp_rep->fds[0];
 		}
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	RTE_LOG(ERR, EAL, "  cannot request default container fd\n");
 	return -1;
@@ -1182,8 +1182,8 @@ rte_vfio_get_container_fd(void)
 			free(mp_reply.msgs);
 			return vfio_container_fd;
 		}
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	RTE_LOG(ERR, EAL, "  cannot request container fd\n");
 	return -1;
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 4/8] ipc: fix vfio memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:44 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>;
> jianfeng.tan@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 4/8] ipc: fix vfio memleak
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer even if the request returned failure. Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

For the patch itself:

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Re: [dpdk-dev] [PATCH v2 4/8] ipc: fix vfio memleak

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Herakliusz Lipiec
> Sent: Tuesday, April 23, 2019 7:44 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: dev@dpdk.org; Lipiec, Herakliusz <herakliusz.lipiec@intel.com>;
> jianfeng.tan@intel.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 4/8] ipc: fix vfio memleak
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer even if the request returned failure. Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: 83a73c5fef66 ("vfio: use generic multi-process channel")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

For the patch itself:

Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

[dpdk-dev] [PATCH v2 5/8] ipc: fix pdump memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: 660098d61f57 ("pdump: use generic multi-process channel")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-By: Reshma Pattan <reshma.pattan@intel.com>
---
 lib/librte_pdump/rte_pdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index 14744b9ff..3787c3e32 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -525,8 +525,8 @@ pdump_prepare_client_request(char *device, uint16_t queue,
 		rte_errno = resp->err_value;
 		if (!resp->err_value)
 			ret = 0;
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	if (ret < 0)
 		RTE_LOG(ERR, PDUMP,
-- 
2.17.2

[dpdk-dev] [PATCH v2 5/8] ipc: fix pdump memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: 660098d61f57 ("pdump: use generic multi-process channel")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-By: Reshma Pattan <reshma.pattan@intel.com>
---
 lib/librte_pdump/rte_pdump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index 14744b9ff..3787c3e32 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -525,8 +525,8 @@ pdump_prepare_client_request(char *device, uint16_t queue,
 		rte_errno = resp->err_value;
 		if (!resp->err_value)
 			ret = 0;
-		free(mp_reply.msgs);
 	}
+	free(mp_reply.msgs);
 
 	if (ret < 0)
 		RTE_LOG(ERR, PDUMP,
-- 
2.17.2

[dpdk-dev] [PATCH v2 6/8] ipc: fix tap pmd memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..d70412d62 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2104,6 +2104,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	if (ret < 0) {
 		TAP_LOG(ERR, "Failed to request queues from primary: %d",
 			rte_errno);
+		free(replies.msgs);
 		return -1;
 	}
 	reply = &replies.msgs[0];
@@ -2119,6 +2120,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
 
+	free(replies.msgs);
 	return 0;
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 6/8] ipc: fix tap pmd memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e9fda8cf6..d70412d62 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2104,6 +2104,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	if (ret < 0) {
 		TAP_LOG(ERR, "Failed to request queues from primary: %d",
 			rte_errno);
+		free(replies.msgs);
 		return -1;
 	}
 	reply = &replies.msgs[0];
@@ -2119,6 +2120,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
 
+	free(replies.msgs);
 	return 0;
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 7/8] ipc: fix net/mlx4 memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID:228
Fixes: 0b259b8e9655 ("net/mlx4: enable secondary process to register DMA memory")
Cc: yskoh@mellanox.com
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/mlx4/mlx4_mp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx4/mlx4_mp.c b/drivers/net/mlx4/mlx4_mp.c
index 183622453..f4cff7486 100644
--- a/drivers/net/mlx4/mlx4_mp.c
+++ b/drivers/net/mlx4/mlx4_mp.c
@@ -255,6 +255,7 @@ mlx4_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
 	if (ret) {
 		ERROR("port %u request to primary process failed",
 		      dev->data->port_id);
+		free(mp_rep.msgs);
 		return -rte_errno;
 	}
 	assert(mp_rep.nb_received == 1);
@@ -292,7 +293,8 @@ mlx4_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
 	if (ret) {
 		ERROR("port %u request to primary process failed",
 		      dev->data->port_id);
-		return -rte_errno;
+		ret = -rte_errno;
+		goto exit;
 	}
 	assert(mp_rep.nb_received == 1);
 	mp_res = &mp_rep.msgs[0];
-- 
2.17.2

[dpdk-dev] [PATCH v2 7/8] ipc: fix net/mlx4 memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID:228
Fixes: 0b259b8e9655 ("net/mlx4: enable secondary process to register DMA memory")
Cc: yskoh@mellanox.com
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/mlx4/mlx4_mp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx4/mlx4_mp.c b/drivers/net/mlx4/mlx4_mp.c
index 183622453..f4cff7486 100644
--- a/drivers/net/mlx4/mlx4_mp.c
+++ b/drivers/net/mlx4/mlx4_mp.c
@@ -255,6 +255,7 @@ mlx4_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
 	if (ret) {
 		ERROR("port %u request to primary process failed",
 		      dev->data->port_id);
+		free(mp_rep.msgs);
 		return -rte_errno;
 	}
 	assert(mp_rep.nb_received == 1);
@@ -292,7 +293,8 @@ mlx4_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
 	if (ret) {
 		ERROR("port %u request to primary process failed",
 		      dev->data->port_id);
-		return -rte_errno;
+		ret = -rte_errno;
+		goto exit;
 	}
 	assert(mp_rep.nb_received == 1);
 	mp_res = &mp_rep.msgs[0];
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 7/8] ipc: fix net/mlx4 memleak

[Yongseok Koh]

> On Apr 23, 2019, at 10:43 AM, Herakliusz Lipiec <herakliusz.lipiec@intel.com> wrote:
> 
> When sending synchronous IPC requests, the caller must free the response 
> buffer even if the request returned failure. Fix the code to correctly 
> use the IPC API.
> 
> Bugzilla ID:228
> Fixes: 0b259b8e9655 ("net/mlx4: enable secondary process to register DMA memory")
> Cc: yskoh@mellanox.com
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>

> drivers/net/mlx4/mlx4_mp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_mp.c b/drivers/net/mlx4/mlx4_mp.c
> index 183622453..f4cff7486 100644
> --- a/drivers/net/mlx4/mlx4_mp.c
> +++ b/drivers/net/mlx4/mlx4_mp.c
> @@ -255,6 +255,7 @@ mlx4_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
> 	if (ret) {
> 		ERROR("port %u request to primary process failed",
> 		      dev->data->port_id);
> +		free(mp_rep.msgs);
> 		return -rte_errno;
> 	}
> 	assert(mp_rep.nb_received == 1);
> @@ -292,7 +293,8 @@ mlx4_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
> 	if (ret) {
> 		ERROR("port %u request to primary process failed",
> 		      dev->data->port_id);
> -		return -rte_errno;
> +		ret = -rte_errno;
> +		goto exit;
> 	}
> 	assert(mp_rep.nb_received == 1);
> 	mp_res = &mp_rep.msgs[0];
> -- 
> 2.17.2
> 

Re: [dpdk-dev] [PATCH v2 7/8] ipc: fix net/mlx4 memleak

[Yongseok Koh]

> On Apr 23, 2019, at 10:43 AM, Herakliusz Lipiec <herakliusz.lipiec@intel.com> wrote:
> 
> When sending synchronous IPC requests, the caller must free the response 
> buffer even if the request returned failure. Fix the code to correctly 
> use the IPC API.
> 
> Bugzilla ID:228
> Fixes: 0b259b8e9655 ("net/mlx4: enable secondary process to register DMA memory")
> Cc: yskoh@mellanox.com
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>

> drivers/net/mlx4/mlx4_mp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_mp.c b/drivers/net/mlx4/mlx4_mp.c
> index 183622453..f4cff7486 100644
> --- a/drivers/net/mlx4/mlx4_mp.c
> +++ b/drivers/net/mlx4/mlx4_mp.c
> @@ -255,6 +255,7 @@ mlx4_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
> 	if (ret) {
> 		ERROR("port %u request to primary process failed",
> 		      dev->data->port_id);
> +		free(mp_rep.msgs);
> 		return -rte_errno;
> 	}
> 	assert(mp_rep.nb_received == 1);
> @@ -292,7 +293,8 @@ mlx4_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
> 	if (ret) {
> 		ERROR("port %u request to primary process failed",
> 		      dev->data->port_id);
> -		return -rte_errno;
> +		ret = -rte_errno;
> +		goto exit;
> 	}
> 	assert(mp_rep.nb_received == 1);
> 	mp_res = &mp_rep.msgs[0];
> -- 
> 2.17.2
> 

[dpdk-dev] [PATCH v2 8/8] ipc: fix net/mlx5 memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID:228
Fixes: 9a8ab29b84d3 ("net/mlx5: replace IPC socket with EAL API")
Fixes: c18cf501a7af ("net/mlx5: enable secondary process to register DMA memory")
Cc: yskoh@mellanox.com
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/mlx5/mlx5_mp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c
index cea74adb6..c9915b1d5 100644
--- a/drivers/net/mlx5/mlx5_mp.c
+++ b/drivers/net/mlx5/mlx5_mp.c
@@ -258,6 +258,7 @@ mlx5_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
 	if (ret) {
 		DRV_LOG(ERR, "port %u request to primary process failed",
 			dev->data->port_id);
+		free(mp_rep.msgs);
 		return -rte_errno;
 	}
 	assert(mp_rep.nb_received == 1);
@@ -295,7 +296,8 @@ mlx5_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u request to primary process failed",
 			dev->data->port_id);
-		return -rte_errno;
+		ret = -rte_errno;
+		goto exit;
 	}
 	assert(mp_rep.nb_received == 1);
 	mp_res = &mp_rep.msgs[0];
-- 
2.17.2

[dpdk-dev] [PATCH v2 8/8] ipc: fix net/mlx5 memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response 
buffer even if the request returned failure. Fix the code to correctly 
use the IPC API.

Bugzilla ID:228
Fixes: 9a8ab29b84d3 ("net/mlx5: replace IPC socket with EAL API")
Fixes: c18cf501a7af ("net/mlx5: enable secondary process to register DMA memory")
Cc: yskoh@mellanox.com
Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/mlx5/mlx5_mp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c
index cea74adb6..c9915b1d5 100644
--- a/drivers/net/mlx5/mlx5_mp.c
+++ b/drivers/net/mlx5/mlx5_mp.c
@@ -258,6 +258,7 @@ mlx5_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
 	if (ret) {
 		DRV_LOG(ERR, "port %u request to primary process failed",
 			dev->data->port_id);
+		free(mp_rep.msgs);
 		return -rte_errno;
 	}
 	assert(mp_rep.nb_received == 1);
@@ -295,7 +296,8 @@ mlx5_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u request to primary process failed",
 			dev->data->port_id);
-		return -rte_errno;
+		ret = -rte_errno;
+		goto exit;
 	}
 	assert(mp_rep.nb_received == 1);
 	mp_res = &mp_rep.msgs[0];
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 8/8] ipc: fix net/mlx5 memleak

[Yongseok Koh]

> On Apr 23, 2019, at 10:43 AM, Herakliusz Lipiec <herakliusz.lipiec@intel.com> wrote:
> 
> When sending synchronous IPC requests, the caller must free the response 
> buffer even if the request returned failure. Fix the code to correctly 
> use the IPC API.
> 
> Bugzilla ID:228
> Fixes: 9a8ab29b84d3 ("net/mlx5: replace IPC socket with EAL API")
> Fixes: c18cf501a7af ("net/mlx5: enable secondary process to register DMA memory")
> Cc: yskoh@mellanox.com
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>

> drivers/net/mlx5/mlx5_mp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c
> index cea74adb6..c9915b1d5 100644
> --- a/drivers/net/mlx5/mlx5_mp.c
> +++ b/drivers/net/mlx5/mlx5_mp.c
> @@ -258,6 +258,7 @@ mlx5_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
> 	if (ret) {
> 		DRV_LOG(ERR, "port %u request to primary process failed",
> 			dev->data->port_id);
> +		free(mp_rep.msgs);
> 		return -rte_errno;
> 	}
> 	assert(mp_rep.nb_received == 1);
> @@ -295,7 +296,8 @@ mlx5_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
> 	if (ret) {
> 		DRV_LOG(ERR, "port %u request to primary process failed",
> 			dev->data->port_id);
> -		return -rte_errno;
> +		ret = -rte_errno;
> +		goto exit;
> 	}
> 	assert(mp_rep.nb_received == 1);
> 	mp_res = &mp_rep.msgs[0];
> -- 
> 2.17.2
> 

Re: [dpdk-dev] [PATCH v2 8/8] ipc: fix net/mlx5 memleak

[Yongseok Koh]

> On Apr 23, 2019, at 10:43 AM, Herakliusz Lipiec <herakliusz.lipiec@intel.com> wrote:
> 
> When sending synchronous IPC requests, the caller must free the response 
> buffer even if the request returned failure. Fix the code to correctly 
> use the IPC API.
> 
> Bugzilla ID:228
> Fixes: 9a8ab29b84d3 ("net/mlx5: replace IPC socket with EAL API")
> Fixes: c18cf501a7af ("net/mlx5: enable secondary process to register DMA memory")
> Cc: yskoh@mellanox.com
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
Acked-by: Yongseok Koh <yskoh@mellanox.com>

> drivers/net/mlx5/mlx5_mp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_mp.c b/drivers/net/mlx5/mlx5_mp.c
> index cea74adb6..c9915b1d5 100644
> --- a/drivers/net/mlx5/mlx5_mp.c
> +++ b/drivers/net/mlx5/mlx5_mp.c
> @@ -258,6 +258,7 @@ mlx5_mp_req_mr_create(struct rte_eth_dev *dev, uintptr_t addr)
> 	if (ret) {
> 		DRV_LOG(ERR, "port %u request to primary process failed",
> 			dev->data->port_id);
> +		free(mp_rep.msgs);
> 		return -rte_errno;
> 	}
> 	assert(mp_rep.nb_received == 1);
> @@ -295,7 +296,8 @@ mlx5_mp_req_verbs_cmd_fd(struct rte_eth_dev *dev)
> 	if (ret) {
> 		DRV_LOG(ERR, "port %u request to primary process failed",
> 			dev->data->port_id);
> -		return -rte_errno;
> +		ret = -rte_errno;
> +		goto exit;
> 	}
> 	assert(mp_rep.nb_received == 1);
> 	mp_res = &mp_rep.msgs[0];
> -- 
> 2.17.2
> 

[dpdk-dev] [PATCH v3 0/2] ipc: fix possible memleaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.
Bugzilla ID: 228

v3:
- rework of the patchset
- caller is no longer responsible for freeing buffers on failure
- caller still has to free response buffers on success

Herakliusz Lipiec (2):
  ipc: fix rte_mp_request_sync memleak
  ipc: fix tap pmd memleak

 drivers/net/tap/rte_eth_tap.c           |  2 +-
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v3 0/2] ipc: fix possible memleaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.
Bugzilla ID: 228

v3:
- rework of the patchset
- caller is no longer responsible for freeing buffers on failure
- caller still has to free response buffers on success

Herakliusz Lipiec (2):
  ipc: fix rte_mp_request_sync memleak
  ipc: fix tap pmd memleak

 drivers/net/tap/rte_eth_tap.c           |  2 +-
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v3 1/2] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always freeing memory buffers on
failure.

Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..ef5eddbea 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		goto err;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
@@ -942,7 +942,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (gettimeofday(&now, NULL) < 0) {
 		RTE_LOG(ERR, EAL, "Failed to get current time\n");
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000;
@@ -954,6 +954,8 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		pthread_mutex_lock(&pending_requests.lock);
 		ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
 		pthread_mutex_unlock(&pending_requests.lock);
+		if (ret)
+			goto err;
 		return ret;
 	}
 
@@ -962,7 +964,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (!mp_dir) {
 		RTE_LOG(ERR, EAL, "Unable to open directory %s\n", mp_dir_path);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	dir_fd = dirfd(mp_dir);
@@ -972,7 +974,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 			mp_dir_path);
 		closedir(mp_dir);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	pthread_mutex_lock(&pending_requests.lock);
@@ -989,7 +991,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		 * locks on receive
 		 */
 		if (mp_request_sync(path, req, reply, &end))
-			ret = -1;
+			goto err;
 	}
 	pthread_mutex_unlock(&pending_requests.lock);
 	/* unlock the directory */
@@ -998,6 +1000,12 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	/* dir_fd automatically closed on closedir */
 	closedir(mp_dir);
 	return ret;
+
+err:
+	free(reply->msgs);
+	reply->nb_received = 0;
+	reply->msgs = NULL;
+	return -1;
 }
 
 int __rte_experimental
-- 
2.17.2

[dpdk-dev] [PATCH v3 1/2] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always freeing memory buffers on
failure.

Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..ef5eddbea 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		goto err;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
@@ -942,7 +942,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (gettimeofday(&now, NULL) < 0) {
 		RTE_LOG(ERR, EAL, "Failed to get current time\n");
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000;
@@ -954,6 +954,8 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		pthread_mutex_lock(&pending_requests.lock);
 		ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
 		pthread_mutex_unlock(&pending_requests.lock);
+		if (ret)
+			goto err;
 		return ret;
 	}
 
@@ -962,7 +964,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (!mp_dir) {
 		RTE_LOG(ERR, EAL, "Unable to open directory %s\n", mp_dir_path);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	dir_fd = dirfd(mp_dir);
@@ -972,7 +974,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 			mp_dir_path);
 		closedir(mp_dir);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	pthread_mutex_lock(&pending_requests.lock);
@@ -989,7 +991,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		 * locks on receive
 		 */
 		if (mp_request_sync(path, req, reply, &end))
-			ret = -1;
+			goto err;
 	}
 	pthread_mutex_unlock(&pending_requests.lock);
 	/* unlock the directory */
@@ -998,6 +1000,12 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	/* dir_fd automatically closed on closedir */
 	closedir(mp_dir);
 	return ret;
+
+err:
+	free(reply->msgs);
+	reply->nb_received = 0;
+	reply->msgs = NULL;
+	return -1;
 }
 
 int __rte_experimental
-- 
2.17.2

Re: [dpdk-dev] [PATCH v3 1/2] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 25-Apr-19 12:43 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always freeing memory buffers on
> failure.
> 
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

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

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 1/2] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 25-Apr-19 12:43 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always freeing memory buffers on
> failure.
> 
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

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

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v3 2/2] ipc: fix tap pmd memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response
buffer if the request was successful and reply is no longer needed.
Fix the code to correctly
use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 7f74b5dc9..3a74c2a43 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-
+        free(reply);
 	return 0;
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v3 2/2] ipc: fix tap pmd memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response
buffer if the request was successful and reply is no longer needed.
Fix the code to correctly
use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 7f74b5dc9..3a74c2a43 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-
+        free(reply);
 	return 0;
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v4 0/2] ipc: fix possible memleaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.
Bugzilla ID: 228

v3:
- rework of the patchset
- caller is no longer responsible for freeing buffers on failure
- caller still has to free response buffers on success
v4:
- fixed checkpatch issues

Herakliusz Lipiec (2):
  ipc: fix rte_mp_request_sync memleak
  ipc: fix tap pmd memleak

 drivers/net/tap/rte_eth_tap.c           |  2 +-
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v4 0/2] ipc: fix possible memleaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.
Bugzilla ID: 228

v3:
- rework of the patchset
- caller is no longer responsible for freeing buffers on failure
- caller still has to free response buffers on success
v4:
- fixed checkpatch issues

Herakliusz Lipiec (2):
  ipc: fix rte_mp_request_sync memleak
  ipc: fix tap pmd memleak

 drivers/net/tap/rte_eth_tap.c           |  2 +-
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v4 1/2] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always freeing memory buffers on
failure.

Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..ef5eddbea 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		goto err;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
@@ -942,7 +942,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (gettimeofday(&now, NULL) < 0) {
 		RTE_LOG(ERR, EAL, "Failed to get current time\n");
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000;
@@ -954,6 +954,8 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		pthread_mutex_lock(&pending_requests.lock);
 		ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
 		pthread_mutex_unlock(&pending_requests.lock);
+		if (ret)
+			goto err;
 		return ret;
 	}
 
@@ -962,7 +964,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (!mp_dir) {
 		RTE_LOG(ERR, EAL, "Unable to open directory %s\n", mp_dir_path);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	dir_fd = dirfd(mp_dir);
@@ -972,7 +974,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 			mp_dir_path);
 		closedir(mp_dir);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	pthread_mutex_lock(&pending_requests.lock);
@@ -989,7 +991,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		 * locks on receive
 		 */
 		if (mp_request_sync(path, req, reply, &end))
-			ret = -1;
+			goto err;
 	}
 	pthread_mutex_unlock(&pending_requests.lock);
 	/* unlock the directory */
@@ -998,6 +1000,12 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	/* dir_fd automatically closed on closedir */
 	closedir(mp_dir);
 	return ret;
+
+err:
+	free(reply->msgs);
+	reply->nb_received = 0;
+	reply->msgs = NULL;
+	return -1;
 }
 
 int __rte_experimental
-- 
2.17.2

[dpdk-dev] [PATCH v4 1/2] ipc: fix rte_mp_request_sync memleak

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always freeing memory buffers on
failure.

Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..ef5eddbea 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		goto err;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
@@ -942,7 +942,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (gettimeofday(&now, NULL) < 0) {
 		RTE_LOG(ERR, EAL, "Failed to get current time\n");
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000;
@@ -954,6 +954,8 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		pthread_mutex_lock(&pending_requests.lock);
 		ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
 		pthread_mutex_unlock(&pending_requests.lock);
+		if (ret)
+			goto err;
 		return ret;
 	}
 
@@ -962,7 +964,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (!mp_dir) {
 		RTE_LOG(ERR, EAL, "Unable to open directory %s\n", mp_dir_path);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	dir_fd = dirfd(mp_dir);
@@ -972,7 +974,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 			mp_dir_path);
 		closedir(mp_dir);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	pthread_mutex_lock(&pending_requests.lock);
@@ -989,7 +991,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		 * locks on receive
 		 */
 		if (mp_request_sync(path, req, reply, &end))
-			ret = -1;
+			goto err;
 	}
 	pthread_mutex_unlock(&pending_requests.lock);
 	/* unlock the directory */
@@ -998,6 +1000,12 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	/* dir_fd automatically closed on closedir */
 	closedir(mp_dir);
 	return ret;
+
+err:
+	free(reply->msgs);
+	reply->nb_received = 0;
+	reply->msgs = NULL;
+	return -1;
 }
 
 int __rte_experimental
-- 
2.17.2

Re: [dpdk-dev] [PATCH v4 1/2] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 25-Apr-19 1:48 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always freeing memory buffers on
> failure.
> 
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

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

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v4 1/2] ipc: fix rte_mp_request_sync memleak

[Burakov, Anatoly]

On 25-Apr-19 1:48 PM, Herakliusz Lipiec wrote:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always freeing memory buffers on
> failure.
> 
> Bugzilla ID: 228
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---

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

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v4 2/2] ipc: fix tap pmd memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response
buffer if the request was successful and reply is no longer needed.
Fix the code to correctly
use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 7f74b5dc9..f8a4169c5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-
+	free(reply);
 	return 0;
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v4 2/2] ipc: fix tap pmd memleak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response
buffer if the request was successful and reply is no longer needed.
Fix the code to correctly
use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 7f74b5dc9..f8a4169c5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-
+	free(reply);
 	return 0;
 }
 
-- 
2.17.2

Re: [dpdk-dev] [PATCH v4 2/2] ipc: fix tap pmd memleak

[Wiles, Keith]

> On Apr 25, 2019, at 7:48 AM, Lipiec, Herakliusz <herakliusz.lipiec@intel.com> wrote:
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer if the request was successful and reply is no longer needed.
> Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 7f74b5dc9..f8a4169c5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
> 	for (queue = 0; queue < reply_param->txq_count; queue++)
> 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
> -
> +	free(reply);
> 	return 0;
> }

Acked-by: Keith Wiles <keith.wiles@intel.com>
> 
> -- 
> 2.17.2
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v4 2/2] ipc: fix tap pmd memleak

[Wiles, Keith]

> On Apr 25, 2019, at 7:48 AM, Lipiec, Herakliusz <herakliusz.lipiec@intel.com> wrote:
> 
> When sending synchronous IPC requests, the caller must free the response
> buffer if the request was successful and reply is no longer needed.
> Fix the code to correctly
> use the IPC API.
> 
> Bugzilla ID: 228
> Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
> Cc: rasland@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 7f74b5dc9..f8a4169c5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
> 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
> 	for (queue = 0; queue < reply_param->txq_count; queue++)
> 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
> -
> +	free(reply);
> 	return 0;
> }

Acked-by: Keith Wiles <keith.wiles@intel.com>
> 
> -- 
> 2.17.2
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v4 0/2] ipc: fix possible memleaks

[Thomas Monjalon]

25/04/2019 14:48, Herakliusz Lipiec:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL and calling free everytime rte_mp_request_sync
> is used.
> 
> v2:
> - resending as patchset to make it easier to review it.

Heraliusz, it's a total mess.
There were 8 patches in v2. Why they disappeared?
The title prefixes are often wrong, so it's harder to classify them.

Should I merge all these patches?
  ipc: fix rte_mp_request_sync memleak
  ipc: fix hotplug memleak
  ipc: fix vdev memleak
  ipc: fix vfio memleak
  ipc: fix pdump memleak
  ipc: fix tap pmd memleak
  ipc: fix net/mlx4 memleak
  ipc: fix net/mlx5 memleak

Re: [dpdk-dev] [PATCH v4 0/2] ipc: fix possible memleaks

[Thomas Monjalon]

25/04/2019 14:48, Herakliusz Lipiec:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL and calling free everytime rte_mp_request_sync
> is used.
> 
> v2:
> - resending as patchset to make it easier to review it.

Heraliusz, it's a total mess.
There were 8 patches in v2. Why they disappeared?
The title prefixes are often wrong, so it's harder to classify them.

Should I merge all these patches?
  ipc: fix rte_mp_request_sync memleak
  ipc: fix hotplug memleak
  ipc: fix vdev memleak
  ipc: fix vfio memleak
  ipc: fix pdump memleak
  ipc: fix tap pmd memleak
  ipc: fix net/mlx4 memleak
  ipc: fix net/mlx5 memleak

Re: [dpdk-dev] [PATCH v4 0/2] ipc: fix possible memleaks

[Burakov, Anatoly]

On 03-May-19 9:34 AM, Thomas Monjalon wrote:
> 25/04/2019 14:48, Herakliusz Lipiec:
>> When sending multiple requests, rte_mp_request_sync
>> can succeed sending a few of those requests, but then
>> fail on a later one and in the end return with rc=-1.
>> The upper layers - e.g. device hotplug - currently
>> handles this case as if no messages were sent and no
>> memory for response buffers was allocated, which is
>> not true. Fixed by always initializing message buffer
>> to NULL and calling free everytime rte_mp_request_sync
>> is used.
>>
>> v2:
>> - resending as patchset to make it easier to review it.
> 
> Heraliusz, it's a total mess.
> There were 8 patches in v2. Why they disappeared?
> The title prefixes are often wrong, so it's harder to classify them.
> 
> Should I merge all these patches?
>    ipc: fix rte_mp_request_sync memleak
>    ipc: fix hotplug memleak
>    ipc: fix vdev memleak
>    ipc: fix vfio memleak
>    ipc: fix pdump memleak
>    ipc: fix tap pmd memleak
>    ipc: fix net/mlx4 memleak
>    ipc: fix net/mlx5 memleak
> 

Hi Thomas,

you've also skipped the following:

 > v3:
 > - rework of the patchset
 > - caller is no longer responsible for freeing buffers on failure
 > - caller still has to free response buffers on success

the v3 was where the 8 patch patchset was reworked into two-patch 
patchset, because we've changed our approach.

So no, those 8 patches should not be merged - they're superseded by v3 
(well, v4 now).

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v4 0/2] ipc: fix possible memleaks

[Burakov, Anatoly]

On 03-May-19 9:34 AM, Thomas Monjalon wrote:
> 25/04/2019 14:48, Herakliusz Lipiec:
>> When sending multiple requests, rte_mp_request_sync
>> can succeed sending a few of those requests, but then
>> fail on a later one and in the end return with rc=-1.
>> The upper layers - e.g. device hotplug - currently
>> handles this case as if no messages were sent and no
>> memory for response buffers was allocated, which is
>> not true. Fixed by always initializing message buffer
>> to NULL and calling free everytime rte_mp_request_sync
>> is used.
>>
>> v2:
>> - resending as patchset to make it easier to review it.
> 
> Heraliusz, it's a total mess.
> There were 8 patches in v2. Why they disappeared?
> The title prefixes are often wrong, so it's harder to classify them.
> 
> Should I merge all these patches?
>    ipc: fix rte_mp_request_sync memleak
>    ipc: fix hotplug memleak
>    ipc: fix vdev memleak
>    ipc: fix vfio memleak
>    ipc: fix pdump memleak
>    ipc: fix tap pmd memleak
>    ipc: fix net/mlx4 memleak
>    ipc: fix net/mlx5 memleak
> 

Hi Thomas,

you've also skipped the following:

 > v3:
 > - rework of the patchset
 > - caller is no longer responsible for freeing buffers on failure
 > - caller still has to free response buffers on success

the v3 was where the 8 patch patchset was reworked into two-patch 
patchset, because we've changed our approach.

So no, those 8 patches should not be merged - they're superseded by v3 
(well, v4 now).

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v5 0/2] ipc: fix possible memory leaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v5:
- change prefixes in commit titles.

v4:
- fixed checkpatch issues

v3:
- rework of the patchset
- caller is no longer responsible for freeing buffers on failure
- caller still has to free response buffers on success
- patchset reduced from 8 patches to 2

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.

Bugzilla ID: 228
Herakliusz Lipiec (2):
  ipc: fix memory leak in sync request
  net/tap: fix ipc related memory leak

 drivers/net/tap/rte_eth_tap.c           |  2 +-
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v5 0/2] ipc: fix possible memory leaks

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always initializing message buffer
to NULL and calling free everytime rte_mp_request_sync
is used.

v5:
- change prefixes in commit titles.

v4:
- fixed checkpatch issues

v3:
- rework of the patchset
- caller is no longer responsible for freeing buffers on failure
- caller still has to free response buffers on success
- patchset reduced from 8 patches to 2

v2:
- resending as patchset to make it easier to review it.
- changed commit message as requested.
- added bugzilla id.

Bugzilla ID: 228
Herakliusz Lipiec (2):
  ipc: fix memory leak in sync request
  net/tap: fix ipc related memory leak

 drivers/net/tap/rte_eth_tap.c           |  2 +-
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v5 1/2] ipc: fix memory leak in sync request

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always freeing memory buffers on
failure.

Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..ef5eddbea 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		goto err;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
@@ -942,7 +942,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (gettimeofday(&now, NULL) < 0) {
 		RTE_LOG(ERR, EAL, "Failed to get current time\n");
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000;
@@ -954,6 +954,8 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		pthread_mutex_lock(&pending_requests.lock);
 		ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
 		pthread_mutex_unlock(&pending_requests.lock);
+		if (ret)
+			goto err;
 		return ret;
 	}
 
@@ -962,7 +964,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (!mp_dir) {
 		RTE_LOG(ERR, EAL, "Unable to open directory %s\n", mp_dir_path);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	dir_fd = dirfd(mp_dir);
@@ -972,7 +974,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 			mp_dir_path);
 		closedir(mp_dir);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	pthread_mutex_lock(&pending_requests.lock);
@@ -989,7 +991,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		 * locks on receive
 		 */
 		if (mp_request_sync(path, req, reply, &end))
-			ret = -1;
+			goto err;
 	}
 	pthread_mutex_unlock(&pending_requests.lock);
 	/* unlock the directory */
@@ -998,6 +1000,12 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	/* dir_fd automatically closed on closedir */
 	closedir(mp_dir);
 	return ret;
+
+err:
+	free(reply->msgs);
+	reply->nb_received = 0;
+	reply->msgs = NULL;
+	return -1;
 }
 
 int __rte_experimental
-- 
2.17.2

[dpdk-dev] [PATCH v5 1/2] ipc: fix memory leak in sync request

[Herakliusz Lipiec]

When sending multiple requests, rte_mp_request_sync
can succeed sending a few of those requests, but then
fail on a later one and in the end return with rc=-1.
The upper layers - e.g. device hotplug - currently
handles this case as if no messages were sent and no
memory for response buffers was allocated, which is
not true. Fixed by always freeing memory buffers on
failure.

Bugzilla ID: 228
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index b46d644b3..ef5eddbea 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -927,13 +927,13 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 
 	RTE_LOG(DEBUG, EAL, "request: %s\n", req->name);
 
-	if (check_input(req) == false)
-		return -1;
-
 	reply->nb_sent = 0;
 	reply->nb_received = 0;
 	reply->msgs = NULL;
 
+	if (check_input(req) == false)
+		goto err;
+
 	if (internal_config.no_shconf) {
 		RTE_LOG(DEBUG, EAL, "No shared files mode enabled, IPC is disabled\n");
 		return 0;
@@ -942,7 +942,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (gettimeofday(&now, NULL) < 0) {
 		RTE_LOG(ERR, EAL, "Failed to get current time\n");
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	end.tv_nsec = (now.tv_usec * 1000 + ts->tv_nsec) % 1000000000;
@@ -954,6 +954,8 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		pthread_mutex_lock(&pending_requests.lock);
 		ret = mp_request_sync(eal_mp_socket_path(), req, reply, &end);
 		pthread_mutex_unlock(&pending_requests.lock);
+		if (ret)
+			goto err;
 		return ret;
 	}
 
@@ -962,7 +964,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	if (!mp_dir) {
 		RTE_LOG(ERR, EAL, "Unable to open directory %s\n", mp_dir_path);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	dir_fd = dirfd(mp_dir);
@@ -972,7 +974,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 			mp_dir_path);
 		closedir(mp_dir);
 		rte_errno = errno;
-		return -1;
+		goto err;
 	}
 
 	pthread_mutex_lock(&pending_requests.lock);
@@ -989,7 +991,7 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 		 * locks on receive
 		 */
 		if (mp_request_sync(path, req, reply, &end))
-			ret = -1;
+			goto err;
 	}
 	pthread_mutex_unlock(&pending_requests.lock);
 	/* unlock the directory */
@@ -998,6 +1000,12 @@ rte_mp_request_sync(struct rte_mp_msg *req, struct rte_mp_reply *reply,
 	/* dir_fd automatically closed on closedir */
 	closedir(mp_dir);
 	return ret;
+
+err:
+	free(reply->msgs);
+	reply->nb_received = 0;
+	reply->msgs = NULL;
+	return -1;
 }
 
 int __rte_experimental
-- 
2.17.2

[dpdk-dev] [PATCH v5 2/2] net/tap: fix ipc related memory leak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response
buffer if the request was successful and reply is no longer needed.
Fix the code to correctly use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 7f74b5dc9..f8a4169c5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-
+	free(reply);
 	return 0;
 }
 
-- 
2.17.2

[dpdk-dev] [PATCH v5 2/2] net/tap: fix ipc related memory leak

[Herakliusz Lipiec]

When sending synchronous IPC requests, the caller must free the response
buffer if the request was successful and reply is no longer needed.
Fix the code to correctly use the IPC API.

Bugzilla ID: 228
Fixes: c9aa56edec8e ("net/tap: access primary process queues from secondary")
Cc: rasland@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 7f74b5dc9..f8a4169c5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2118,7 +2118,7 @@ tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev)
 		process_private->rxq_fds[queue] = reply->fds[fd_iterator++];
 	for (queue = 0; queue < reply_param->txq_count; queue++)
 		process_private->txq_fds[queue] = reply->fds[fd_iterator++];
-
+	free(reply);
 	return 0;
 }
 
-- 
2.17.2

Re: [dpdk-dev] [PATCH v5 0/2] ipc: fix possible memory leaks

[Thomas Monjalon]

03/05/2019 12:28, Herakliusz Lipiec:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL and calling free everytime rte_mp_request_sync
> is used.
> 
> v5:
> - change prefixes in commit titles.

I already applied v4 with fixed titles, thanks

Re: [dpdk-dev] [PATCH v5 0/2] ipc: fix possible memory leaks

[Thomas Monjalon]

03/05/2019 12:28, Herakliusz Lipiec:
> When sending multiple requests, rte_mp_request_sync
> can succeed sending a few of those requests, but then
> fail on a later one and in the end return with rc=-1.
> The upper layers - e.g. device hotplug - currently
> handles this case as if no messages were sent and no
> memory for response buffers was allocated, which is
> not true. Fixed by always initializing message buffer
> to NULL and calling free everytime rte_mp_request_sync
> is used.
> 
> v5:
> - change prefixes in commit titles.

I already applied v4 with fixed titles, thanks