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 0F3B9A0A0E for ; Wed, 7 Apr 2021 17:53:28 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 03B70140F24; Wed, 7 Apr 2021 17:53:28 +0200 (CEST) Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) by mails.dpdk.org (Postfix) with ESMTP id 7FA3C406A3; Wed, 7 Apr 2021 17:53:25 +0200 (CEST) Received: by mail-qv1-f54.google.com with SMTP id x27so9213162qvd.2; Wed, 07 Apr 2021 08:53:25 -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=YO0HZMgYV6c7sZIb0pJ+CCGGgOkUZYJiH8YyTOBekMo=; b=bHgTD/o4jVW/8H6v3Bk/bA2bsfOIp6G2TfcYhYdD/XzUDeWV0el4RhGS9nqwAPam1h UjUT7lP+CEICnCXn1GrYYM/iVjrqJzS1pfTWjheuIe/st5jLvkz9EfGyFK7ugNesu4Hq MHRCcTSNwITU03+u/habTZa/RWPtiF3IcyiQTDXad9y2WpH4b2Qpko9YR0/rHm/6giNz v5JLnTf1fyQ1Y533ALqBDtEFb7JvjBmot1lDEoV4g76OGXwakjn2ZQKZCbJukirmPOSA eBa5vxfNUV5fxPyU7Kdzi0pTlegrmnpB03rC0X1I21rgumejix1ZvEJZTwtbraD551dM rO7g== 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=YO0HZMgYV6c7sZIb0pJ+CCGGgOkUZYJiH8YyTOBekMo=; b=EfSE+wFm7aEduIbOmU743dkrTBe34Yehi84ynE9Nzd+9fIkIWhxL02CIqO3Fn/NJvP WiiqjJ4H2ZbXjoe7kbImvFgstvbWBFded1ETUdUBQ6dvPvOAsLyy6zFAOi61bBg0JMKb 2cVn8FHQgbUsKGf6RJgsy1C1Jle9Qpy0K/hPZDizFf41kimGWBlIFMcfhrSK4zveKTB/ A17KD1TMb6u2OiMks/3swuEAvkV/03UhR/kQ/fHZ+HSn8X0TV27YyZisCGy36xZPXGJh TN97cQMlgMKQFE/nTMKDZAU3nxbxUNE3cK/w8/UhqlbHtuverNkbQ5+gSi7L4iX7rl+F dBxA== X-Gm-Message-State: AOAM532zIVA0ydbNukLaVq9hc1aQS4JUVxRihPiIctyQBESBG2fuzJMm YGaS1Ekjas2yhCYs27+JbZs= X-Google-Smtp-Source: ABdhPJxliryYrdleyIJJBHPkeNevykkkO+ZHHgsfm5B+S0QklMItmhXDWtfnVps0gezrvqAnKp1AoA== X-Received: by 2002:ad4:5bc7:: with SMTP id t7mr4202802qvt.43.1617810804890; Wed, 07 Apr 2021 08:53:24 -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 66sm19167731qkk.18.2021.04.07.08.53.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Apr 2021 08:53:24 -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 11:29:04 -0400 Message-Id: <20210407152903.130730-1-lucp.at.work@gmail.com> In-Reply-To: <20210407145722.GN1650@platinum> References: <20210407145722.GN1650@platinum> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-stable] [PATCH v7] 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, Olivier, thanks for pointing out that pthread_barrier_wait acts as a full memory barrier; I didn't know that was explicitly specified in the documentation. I've changed it to use a regular read/write. Hi Honnappa, I have not changed the code to use pthread_attr_setaffinity_np because the attr parameter is const. I could make a copy of the provided attributes and use that instead, but I think this would still violate the spirit of the API and, AFAICT, there's no official mechanism for copying a pthread_attr_t. Please let me know what you think. 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..d4e09f84b 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 = params->start_routine; + 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) + params->start_routine = NULL; + 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