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

[dpdk-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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

Re: [dpdk-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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

Re: [dpdk-stable] [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-stable] [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-stable] [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-stable] [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-stable] [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

[dpdk-stable] [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-stable] [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