From patchwork Wed Apr 7 20:16:04 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luc Pelletier X-Patchwork-Id: 90820 X-Patchwork-Delegate: david.marchand@redhat.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EBDC0A0A0F; Wed, 7 Apr 2021 22:26:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A88B9140F81; Wed, 7 Apr 2021 22:26:22 +0200 (CEST) Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by mails.dpdk.org (Postfix) with ESMTP id 4E0C6140F7F; Wed, 7 Apr 2021 22:26:21 +0200 (CEST) Received: by mail-qt1-f182.google.com with SMTP id x9so14751547qto.8; Wed, 07 Apr 2021 13:26:21 -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=5vS+BVSyrRgO5Q6IVHgdfSLPFiao1qp6vZGA1gmn7a0=; b=lDu74IngwDYMq3AITX1+VZKcZef3zlX8ba6E+/o0fPyFdIy95dHci4QT5s7PYCg92d 6zOSacQL0XTbSdnG8FauQCpphtlaEa73lHEkKbqI1pzum1mdOsLC8wDDG74CaVNxE4Kt MPQk2cBuqtgxq6JNFtFIDm6w4UYP2/tQeDMegTdFBgPTSuxm3mTNTGH2R/9WM+0JpKRQ 64IVivvkMuPZx3X6+dtjkQo2cUFL26JOFMpFWAVFrjXUtPo9scRazVXR1fe6knZcjcOS uWyUllgbOGuugkf/Hxj49XCHdx9Jdcah/nwt0aP+fpF1Y/euLVZwelxbEYADCujlq8yr EJuA== 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=5vS+BVSyrRgO5Q6IVHgdfSLPFiao1qp6vZGA1gmn7a0=; b=L8Dpe8Z2UqBnTX87JkMQnWnX62ZtLf/OKqtYd8RuRafHCGymQJGyNqv7muKJ+5JjKV 31rK1EYVHayVHX0kaXNE+MbRdrq68vaqVuwqsIKLSha8BCaIL4HZXBM8QxD1fnWBlUlB 5UlRwv1D4bUBbAp8vO9WAy6REn9xWVdQK78OkyDokeHiUib7kGz59Q5IylQgzvtyy+gg hKvr9SgJDtXRessGcIz+HMfDBT5+FQ1ZBomVs8GhiCQCWQcah3ZhpkRB286MqFgL3h38 drA/HrQ2L8CO/nm7DSmBLwq6ynFeokN82i40vR/qm+UqgdNt899TTtBMd3W1UiyOet6W NDBg== X-Gm-Message-State: AOAM530CvVw2CH7CmK5M4z2XIH0xTyYDeGWyMkjO1WPGyS+G0wHphORj 7hbU7H/EgsUH+bj2DCCH/ks= X-Google-Smtp-Source: ABdhPJy/wX5Hz+o70Ya65+qhvbluBva51sZBjpRGjFe9KGk29oTGw05AafC1vThr/yjtf8M1RmmN/A== X-Received: by 2002:a05:622a:15d4:: with SMTP id d20mr4303144qty.85.1617827180791; Wed, 07 Apr 2021 13:26:20 -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 7sm19532060qkm.64.2021.04.07.13.26.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Apr 2021 13:26:20 -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 16:16:04 -0400 Message-Id: <20210407201603.149234-1-lucp.at.work@gmail.com> In-Reply-To: References: MIME-Version: 1.0 Subject: [dpdk-dev] [PATCH 1/2] eal: fix race in ctrl thread creation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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: 3a0d465 ("eal: fix use-after-free on control thread creation") Cc: jianfeng.tan@intel.com Cc: stable@dpdk.org Signed-off-by: Luc Pelletier Acked-by: Olivier Matz Reviewed-by: Honnappa Nagarahalli --- Hi Olivier, Hi Honnappa, As discussed, I've split the changes into 2 patches. This first commit fixes the race condition but leaves in the pthread_cancel call. lib/librte_eal/common/eal_common_thread.c | 49 +++++++++++++---------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c index 73a055902..3347e91bf 100644 --- a/lib/librte_eal/common/eal_common_thread.c +++ b/lib/librte_eal/common/eal_common_thread.c @@ -170,11 +170,19 @@ 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 +192,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,15 +215,18 @@ 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_create(thread, attr, ctrl_thread_init, (void *)params); + ret = pthread_barrier_init(¶ms->configured, NULL, 2); if (ret != 0) { free(params); return -ret; } + ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params); + if (ret != 0) + goto fail; + if (name != NULL) { ret = rte_thread_setname(*thread, name); if (ret < 0) @@ -227,25 +235,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) + goto fail_cancel; - 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 0; -fail: - if (PTHREAD_BARRIER_SERIAL_THREAD == - pthread_barrier_wait(¶ms->configured)) { - pthread_barrier_destroy(¶ms->configured); - free(params); - } +fail_cancel: pthread_cancel(*thread); pthread_join(*thread, NULL); + +fail: + pthread_barrier_destroy(¶ms->configured); + free(params); + return -ret; } From patchwork Wed Apr 7 20:16:06 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luc Pelletier X-Patchwork-Id: 90821 X-Patchwork-Delegate: david.marchand@redhat.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6F2D2A0A0F; Wed, 7 Apr 2021 22:28:56 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2710A140F81; Wed, 7 Apr 2021 22:28:56 +0200 (CEST) Received: from mail-qv1-f47.google.com (mail-qv1-f47.google.com [209.85.219.47]) by mails.dpdk.org (Postfix) with ESMTP id 54EA9140F7F; Wed, 7 Apr 2021 22:28:55 +0200 (CEST) Received: by mail-qv1-f47.google.com with SMTP id t13so3717089qvs.7; Wed, 07 Apr 2021 13:28:55 -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=VMz1HXOh0o6zKNzAz8TyY9AAt3vcJaxmFlm9CSQviWI=; b=jWFn1ve1sKtsEsdEUyqNRwHDgZa7wH4oWGPUPcnkS2yMUaaW/5sb/CWkcbhcjcw0rm orGSH+r4BDfFUy9SiGuhgcsJIViF3WI8iF2G7KNLQA1HkVAGqaNSxWRxt4exC6DDJRxH u9cTP3uL91v5IbASihId7UWmJJ55FS5agZYDERGuY3aR3LRJ+9ab7VYS17PYkizY1ZEc BZ9wCVu3LX65o+6N1xGyXALA3TenqFA5mK2cEDblZ3B0A06gc8Ak4MnrqLnjZfU+eU9U MyXTxAnf8Fq3YZVQhDJtOOOqsReTUdrGt9eBTItpg/ieWrd2U5QsPAooJ3jkFnPz3xAn aF/g== 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=VMz1HXOh0o6zKNzAz8TyY9AAt3vcJaxmFlm9CSQviWI=; b=q6qdsTQejQxFVzosd6yt6nD598bGMtrMiiqUTNoeqyurN17roikX4skvSygAnooeZx prHPxcokxmdNmNUv6YVet0dqSZjocEDc0gcqm4tsnq5eDB10qtMpVPaYD/z+Lu835Spn zqofhEMBMZHb+ZZud+I8qNlzPSjackEqoG5uBSpJFXoVaJ/UjxY8p+Ne1G6pGtGtLlbi rJabhCOjSMsxckZJZqHDxHuTpk42wb5V4wzc3EhoAKbwrxklzWbnGBesabjAYl/p5WjJ NMvQy9zJNLSSpMHhEM6SC5Re+CU+fAGadQNHsf7NNDnygNad4biu4CCDifN8OqmAQxo8 uQMA== X-Gm-Message-State: AOAM530ROrCxHCHMG+gOi7sCY96NJjemLGX5B7XQpRt+rgc/ar23wj5K 85N4HIuJMiM35EVN5iNLs2A= X-Google-Smtp-Source: ABdhPJway+0jJTXGHvEhXFtGHrSg+GXGtlPTlD6He4td5ajEw4JS3Wp1ZhY1TGDlYjP0DV5gMb1a2Q== X-Received: by 2002:a05:6214:f0d:: with SMTP id gw13mr5402477qvb.33.1617827334877; Wed, 07 Apr 2021 13:28:54 -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 7sm19532060qkm.64.2021.04.07.13.28.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Apr 2021 13:28:54 -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 16:16:06 -0400 Message-Id: <20210407201603.149234-2-lucp.at.work@gmail.com> In-Reply-To: References: MIME-Version: 1.0 Subject: [dpdk-dev] [PATCH 2/2] eal: fix hang in ctrl thread creation error logic X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" The affinity of a control thread is set after it has been launched. If setting the affinity fails, pthread_cancel is called followed by a call to pthread_join, which can hang forever if the thread's start routine doesn't call a pthread cancellation point. This patch modifies the logic so that the control thread exits gracefully if the affinity cannot be set successfully and removes the call to pthread_cancel. Fixes: 6383d26 ("eal: set name when creating a control thread") Cc: olivier.matz@6wind.com Cc: stable@dpdk.org Signed-off-by: Luc Pelletier Acked-by: Olivier Matz Reviewed-by: Honnappa Nagarahalli Acked-by: Olivier Matz Reviewed-by: Honnappa Nagarahalli --- Hi Olivier, Hi Honnappa, As discussed, I've split the changes into 2 patches. This second commit removes the pthread_cancel call which could result in a hang on join, if the ctrl thread routine didn't call a cancellation point. lib/librte_eal/common/eal_common_thread.c | 29 +++++++++++++---------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c index 3347e91bf..03dbcd9e8 100644 --- a/lib/librte_eal/common/eal_common_thread.c +++ b/lib/librte_eal/common/eal_common_thread.c @@ -187,14 +187,18 @@ static void *ctrl_thread_init(void *arg) 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); pthread_barrier_wait(¶ms->configured); + start_routine = params->start_routine; ctrl_params_free(params); + if (start_routine == NULL) + return NULL; + return start_routine(routine_arg); } @@ -218,14 +222,12 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name, params->refcnt = 2; ret = pthread_barrier_init(¶ms->configured, NULL, 2); - if (ret != 0) { - free(params); - return -ret; - } + if (ret != 0) + goto fail_no_barrier; ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params); if (ret != 0) - goto fail; + goto fail_with_barrier; if (name != NULL) { ret = rte_thread_setname(*thread, name); @@ -236,19 +238,22 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name, ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset); if (ret != 0) - goto fail_cancel; + params->start_routine = NULL; pthread_barrier_wait(¶ms->configured); ctrl_params_free(params); - return 0; + if (ret != 0) + /* start_routine has been set to NULL above; */ + /* ctrl thread will exit immediately */ + pthread_join(*thread, NULL); -fail_cancel: - pthread_cancel(*thread); - pthread_join(*thread, NULL); + return -ret; -fail: +fail_with_barrier: pthread_barrier_destroy(¶ms->configured); + +fail_no_barrier: free(params); return -ret;