From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6A050A0A0C for ; Wed, 7 Apr 2021 14:48:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5A477140ECF; Wed, 7 Apr 2021 14:48:14 +0200 (CEST) Received: from mail-qv1-f44.google.com (mail-qv1-f44.google.com [209.85.219.44]) by mails.dpdk.org (Postfix) with ESMTP id 5CE984013F; Wed, 7 Apr 2021 14:48:11 +0200 (CEST) Received: by mail-qv1-f44.google.com with SMTP id h3so5877098qvr.10; Wed, 07 Apr 2021 05:48:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=iZsxoaWH5vHf6yp4SHCefnkLc5PVlsZ0IgGxHXuktTE=; b=UVGvr6bbZNxTnwQ+32cywtxLfGm79IFyDygRtzA7tnRoVmxuccQZWloOQFPVp735St A0h08b6jHazPUPujFYb77zQcLZgOuAricIP0FQTFoB3mrfd/jHSRljLaTr81xKqyhmLD 7IX8N6wc4RWc56nh9yzPvVGq1Nnfprqnl0poMfVw28mXZh2oMvjlsZOzym5EoYNNezxl etTgeC5JIX1Pm3oKwR8/lq7SlnhJSa8S6bdjWSiAL59f8XGmeb0rgUe8Iue6RiSo1uUj cJdt3Km6VlyDhJYxPXQIjCJVkAIssCR9Zywkvm1Iqcnaw1mndZPTNZ3Xg6mdSrDxi7yD 0TcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=iZsxoaWH5vHf6yp4SHCefnkLc5PVlsZ0IgGxHXuktTE=; b=oF4Fe6wHI3K6Qb8BTJb51GK91vuv/1PKu1N6DEVFbyYid08hj1z7fHmVbAx8oBHf/2 MwEwfVxumDeUJWDKNmvX6/vcmV+zBDNGpUag06nANOKkXINNKrcvs0fNmyNuVTzX87PL Dc/yGrzdG1fc5bIlaB96cIodYJjEl+090hD5CnFtjbigHpe+mhw/id9uj1smZpKMP09L xXfjxtlbOWMOZWHge+ESR/VGFAAPcBpSBNiXIkBhrCYag4Q3QhFvonJGv0MZGh9iZ9gu TOfvK4K9Z6+dVWwsUJrPZjsrhnikOMAK1MSKKoirsCQuRCXqX5Pt52fwD0PcR9bH30lb knQg== X-Gm-Message-State: AOAM532IXTR/+rCkkzDkOCjbvmc6KtYbJxf32wWwSXueHrC7HOFXG6SQ 0pIEZ1gKrC2eUIp+aOnOTU4= X-Google-Smtp-Source: ABdhPJwe3RzX5EA2DFVWKsRLfr0hrIGAkXyMMFcJeO1k4wbeZYPCuZUpJdeYEnFgHHoZ2YinsASevg== X-Received: by 2002:a0c:9b82:: with SMTP id o2mr3050695qve.47.1617799690325; Wed, 07 Apr 2021 05:48:10 -0700 (PDT) Received: from localhost.localdomain (bras-base-hullpq2034w-grc-18-74-15-213-92.dsl.bell.ca. [74.15.213.92]) by smtp.gmail.com with ESMTPSA id b15sm16809201qtr.58.2021.04.07.05.48.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Apr 2021 05:48:09 -0700 (PDT) From: Luc Pelletier To: olivier.matz@6wind.com, jianfeng.tan@intel.com, Honnappa.Nagarahalli@arm.com Cc: dev@dpdk.org, Luc Pelletier , stable@dpdk.org Date: Wed, 7 Apr 2021 08:35:30 -0400 Message-Id: <20210407123529.123899-1-lucp.at.work@gmail.com> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-stable] [PATCH v4] eal: fix race in ctrl thread creation X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" 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 --- 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(¶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; @@ -184,11 +191,8 @@ static void *ctrl_thread_init(void *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); + 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(¶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 +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(¶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); - } + return -ret; - return 0; +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