[1/8] ipc: fix rte_mp_request_sync memleak

Message ID 20190417143822.21276-1-herakliusz.lipiec@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [1/8] ipc: fix rte_mp_request_sync memleak |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Herakliusz Lipiec April 17, 2019, 2:38 p.m. UTC
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(-)
  

Comments

Burakov, Anatoly April 23, 2019, 8:11 a.m. UTC | #1
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>
  
Burakov, Anatoly April 23, 2019, 8:13 a.m. UTC | #2
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 :)
  

Patch

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.