[v6] eal: fix race in ctrl thread creation
Checks
Commit Message
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,
I've made the changes as you requested. However, I'm using the atomic
built-ins for reading and writing start_routine; I think they're
required to prevent any re-reordering.
Please let me know what you think.
lib/librte_eal/common/eal_common_thread.c | 63 +++++++++++++----------
1 file changed, 35 insertions(+), 28 deletions(-)
Comments
Hi Luc,
On Wed, Apr 07, 2021 at 10:42:37AM -0400, Luc Pelletier wrote:
> 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,
>
> I've made the changes as you requested. However, I'm using the atomic
> built-ins for reading and writing start_routine; I think they're
> required to prevent any re-reordering.
>
> Please let me know what you think.
From [1], it seems that pthread_barrier_wait() is a full memory barrier.
So while not wrong, I think using atomic built-ins it is not needed.
[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
>
> lib/librte_eal/common/eal_common_thread.c | 63 +++++++++++++----------
> 1 file changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
> index 73a055902..fcb386f77 100644
> --- a/lib/librte_eal/common/eal_common_thread.c
> +++ b/lib/librte_eal/common/eal_common_thread.c
> @@ -170,25 +170,34 @@ 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(¶ms->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
> + pthread_barrier_destroy(¶ms->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;
> struct rte_thread_ctrl_params *params = arg;
> - void *(*start_routine)(void *) = params->start_routine;
> + void *(*start_routine)(void *);
> void *routine_arg = params->arg;
>
> __rte_thread_init(rte_lcore_id(), cpuset);
>
> - ret = pthread_barrier_wait(¶ms->configured);
> - if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
> - pthread_barrier_destroy(¶ms->configured);
> - free(params);
> - }
> + pthread_barrier_wait(¶ms->configured);
> + start_routine = __atomic_load_n(¶ms->start_routine, __ATOMIC_ACQUIRE);
> + ctrl_params_free(params);
> +
> + if (start_routine == NULL)
> + return NULL;
>
> return start_routine(routine_arg);
> }
> @@ -210,14 +219,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(¶ms->configured, NULL, 2);
> + ret = pthread_barrier_init(¶ms->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 +237,22 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
> }
>
> ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
> - if (ret)
> - goto fail;
> + if (ret != 0)
> + __atomic_store_n(¶ms->start_routine, NULL, __ATOMIC_RELEASE);
> + pthread_barrier_wait(¶ms->configured);
> + ctrl_params_free(params);
>
> - ret = pthread_barrier_wait(¶ms->configured);
> - if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
> - pthread_barrier_destroy(¶ms->configured);
> - free(params);
> - }
> + if (ret != 0)
> + pthread_join(*thread, NULL);
>
> - return 0;
> + return -ret;
> +
> +fail_with_barrier:
> + pthread_barrier_destroy(¶ms->configured);
> +
> +fail_no_barrier:
> + free(params);
>
> -fail:
> - if (PTHREAD_BARRIER_SERIAL_THREAD ==
> - pthread_barrier_wait(¶ms->configured)) {
> - pthread_barrier_destroy(¶ms->configured);
> - free(params);
> - }
> - pthread_cancel(*thread);
> - pthread_join(*thread, NULL);
> return -ret;
> }
>
> --
> 2.25.1
>
@@ -170,25 +170,34 @@ 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(¶ms->refcnt, 1, __ATOMIC_ACQ_REL) == 0) {
+ pthread_barrier_destroy(¶ms->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;
struct rte_thread_ctrl_params *params = arg;
- void *(*start_routine)(void *) = params->start_routine;
+ void *(*start_routine)(void *);
void *routine_arg = params->arg;
__rte_thread_init(rte_lcore_id(), cpuset);
- ret = pthread_barrier_wait(¶ms->configured);
- if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
- pthread_barrier_destroy(¶ms->configured);
- free(params);
- }
+ pthread_barrier_wait(¶ms->configured);
+ start_routine = __atomic_load_n(¶ms->start_routine, __ATOMIC_ACQUIRE);
+ ctrl_params_free(params);
+
+ if (start_routine == NULL)
+ return NULL;
return start_routine(routine_arg);
}
@@ -210,14 +219,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(¶ms->configured, NULL, 2);
+ ret = pthread_barrier_init(¶ms->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 +237,22 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name,
}
ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset);
- if (ret)
- goto fail;
+ if (ret != 0)
+ __atomic_store_n(¶ms->start_routine, NULL, __ATOMIC_RELEASE);
+ pthread_barrier_wait(¶ms->configured);
+ ctrl_params_free(params);
- ret = pthread_barrier_wait(¶ms->configured);
- if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
- pthread_barrier_destroy(¶ms->configured);
- free(params);
- }
+ if (ret != 0)
+ pthread_join(*thread, NULL);
- return 0;
+ return -ret;
+
+fail_with_barrier:
+ pthread_barrier_destroy(¶ms->configured);
+
+fail_no_barrier:
+ free(params);
-fail:
- if (PTHREAD_BARRIER_SERIAL_THREAD ==
- pthread_barrier_wait(¶ms->configured)) {
- pthread_barrier_destroy(¶ms->configured);
- free(params);
- }
- pthread_cancel(*thread);
- pthread_join(*thread, NULL);
return -ret;
}