[dpdk-dev] [PATCH v4] eal: fix race in ctrl thread creation

[Luc Pelletier <lucp.at.work@gmail.com>]

The creation of control threads uses a pthread barrier for
synchronization. This patch fixes a race condition where the pthread
barrier could get destroyed while one of the threads has not yet
returned from the pthread_barrier_wait function, which could result in
undefined behaviour.

Fixes: 3a0d465d4c53 ("eal: fix use-after-free on control thread creation")
Cc: jianfeng.tan@intel.com
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---

Hi Olivier,
Hi Honnappa,

Thanks for your comments Honnappa. You make a very good point about the
cancellation point requirements. I've removed the pthread_cancel and
pthread_join calls. I've also added back the barrier to make sure the
control thread function only runs after the affinity has been set. If
setting the affinity fails, the control thread will run without getting
cancelled, but rte_ctrl_thread_create will return the error returned by
pthread_set_affinity_np.

I have also eliminated the duplication of the logic to decrement the
reference count and free the barrier & memory.

What do you think of the changes now?

 lib/librte_eal/common/eal_common_thread.c | 51 +++++++++++------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 73a055902..2a8811582 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -170,11 +170,18 @@ struct rte_thread_ctrl_params {
 	void *(*start_routine)(void *);
 	void *arg;
 	pthread_barrier_t configured;
+	unsigned int refcnt;
 };
 
+static void ctrl_params_free(struct rte_thread_ctrl_params* params) {
+	if (__atomic_sub_fetch(&params->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
+		pthread_barrier_destroy(&params->configured);
+		free(params);
+	}
+}
+
 static void *ctrl_thread_init(void *arg)
 {
-	int ret;
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
@@ -184,11 +191,8 @@ static void *ctrl_thread_init(void *arg)
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 
-	ret = pthread_barrier_wait(&params->configured);
-	if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
-		pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
+	pthread_barrier_wait(&params->configured);
+	ctrl_params_free(params);
 
 	return start_routine(routine_arg);
 }
@@ -210,14 +214,15 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 
 	params->start_routine = start_routine;
 	params->arg = arg;
+	params->refcnt = 2;
 
-	pthread_barrier_init(&params->configured, NULL, 2);
+	ret = pthread_barrier_init(&params->configured, NULL, 2);
+	if (ret != 0)
+		goto fail_no_barrier;
 
 	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
-	if (ret != 0) {
-		free(params);
-		return -ret;
-	}
+	if (ret != 0)
+		goto fail_with_barrier;
 
 	if (name != NULL) {
 		ret = rte_thread_setname(*thread, name);
@@ -227,25 +232,17 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
 	}
 
 	ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
-	if (ret)
-		goto fail;
+	pthread_barrier_wait(&params->configured);
+	ctrl_params_free(params);
 
-	ret = pthread_barrier_wait(&params->configured);
-	if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
-		pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
+	return -ret;
 
-	return 0;
+fail_with_barrier:
+	pthread_barrier_destroy(&params->configured);
+
+fail_no_barrier:
+	free(params);
 
-fail:
-	if (PTHREAD_BARRIER_SERIAL_THREAD ==
-	    pthread_barrier_wait(&params->configured)) {
-		pthread_barrier_destroy(&params->configured);
-		free(params);
-	}
-	pthread_cancel(*thread);
-	pthread_join(*thread, NULL);
 	return -ret;
 }
 
-- 
2.25.1