DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ipc: fix use-after-free on failed send
@ 2018-11-20 15:23 Anatoly Burakov
  2018-11-20 16:18 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
  0 siblings, 1 reply; 3+ messages in thread
From: Anatoly Burakov @ 2018-11-20 15:23 UTC (permalink / raw)
  To: dev; +Cc: thomas

Previous fix for rte_panic has moved setting of alarm before
sending the message. This means that whether we send a message,
the alarm would still trigger. The comment noted that cleanup
would happen in the alarm handler, but that's not what actually
happened - instead, in the event of failed send we freed the
memory in-place, before putting the request on the queue.

This works OK when the message is sent, but when sending the
message fails, the alarm would still trigger with a pointer
argument that points to non-existent memory, and cause
memory corruption.

There probably is a "proper" fix for this issue, with correct
handling of sent vs. unsent requests, however it would be
simpler just to sacrifice the sent request in the (extremely
unlikely) event of alarm set failing. The other process would
still send a response, but it will be ignored by the sender.

Fixes: 45e5f49e87fb ("ipc: remove panic in async request")

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_proc.c | 36 ++++++++-----------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
index f65ef56c0..375e98709 100644
--- a/lib/librte_eal/common/eal_common_proc.c
+++ b/lib/librte_eal/common/eal_common_proc.c
@@ -827,20 +827,17 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
 		goto fail;
 	}
 
-	/*
-	 * set the alarm before sending message. there are two possible error
-	 * scenarios to consider here:
-	 *
-	 * - if the alarm set fails, we free the memory right there
-	 * - if the alarm set succeeds but sending message fails, then the alarm
-	 *   will trigger and clean up the memory
-	 *
-	 * Even if the alarm triggers too early (i.e. immediately), we're still
-	 * holding the lock to pending requests queue, so the interrupt thread
-	 * will just spin until we release the lock, and either release the
-	 * memory, or doesn't find any pending requests in the queue because we
-	 * never added any due to send message failure.
-	 */
+	ret = send_msg(dst, req, MP_REQ);
+	if (ret < 0) {
+		RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n",
+			dst, req->name);
+		ret = -1;
+		goto fail;
+	} else if (ret == 0) {
+		ret = 0;
+		goto fail;
+	}
+	/* if alarm set fails, we simply ignore the reply */
 	if (rte_eal_alarm_set(ts->tv_sec * 1000000 + ts->tv_nsec / 1000,
 			      async_reply_handle, pending_req) < 0) {
 		RTE_LOG(ERR, EAL, "Fail to set alarm for request %s:%s\n",
@@ -848,17 +845,6 @@ mp_request_async(const char *dst, struct rte_mp_msg *req,
 		ret = -1;
 		goto fail;
 	}
-
-	ret = send_msg(dst, req, MP_REQ);
-	if (ret < 0) {
-		RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n",
-			dst, req->name);
-		ret = -1;
-		goto fail;
-	} else if (ret == 0) {
-		ret = 0;
-		goto fail;
-	}
 	TAILQ_INSERT_TAIL(&pending_requests.requests, pending_req, next);
 
 	param->user_reply.nb_sent++;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-11-22 22:10 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20 15:23 [dpdk-dev] [PATCH] ipc: fix use-after-free on failed send Anatoly Burakov
2018-11-20 16:18 ` [dpdk-dev] [PATCH v2] " Anatoly Burakov
2018-11-22 22:09   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).