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 68A7AA0C40 for ; Wed, 7 Apr 2021 15:31:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 552DE140F31; Wed, 7 Apr 2021 15:31:10 +0200 (CEST) Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by mails.dpdk.org (Postfix) with ESMTP id 5D6664013F for ; Wed, 7 Apr 2021 15:31:07 +0200 (CEST) Received: by mail-wr1-f48.google.com with SMTP id a12so8230322wrq.13 for ; Wed, 07 Apr 2021 06:31:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=VirE6s8kn1O3tSXwb7l5FGCtjeKrxCL+7L+14sgPTD8=; b=gRqdVpBRsE/5DTYUGl6atLvbBCkObktknA+Q5eVEpQ2Nc4BI/3t97/lJfxV/18W1wF Apo+IdeMKeJ6hghABJz4jBQ8Ent87kwS9bbweL/3NUd58uu8V4aGlB+WPxrRtuKdPEFj VgMxPr3k0dnE6PuIHyuHMkqwF7xiyXWXq9r917uqiB4Y1AHXljuKfTN7n4jQrQlly2yN KuflYH6aTvN/o8FsOIghC9CSiOc+TgJ+cO2FT7Ap1L81by2lga1jbbqnkW0AkKBc+XXU 0VGeX2xlgkgz7a7IpstBpoZC7mLPtidNkSMPfsQdWPPa01hbIXaEn0dvujsVhTtCFqCh HS8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=VirE6s8kn1O3tSXwb7l5FGCtjeKrxCL+7L+14sgPTD8=; b=GuNurYQrjZfmPXSH4wK3LKQo4yP5bryyHWI1LY5kmoAwnMnzMnlvKzXrXknolsqTGd R3IrA3gsO72iO6g2FYPdWARVJsOY6JdXB/5CnF+l6mjiSIHR2nyxkJ2D24ZyuR36iauJ X8+ZAXZKGODmr7e2QokYpcZuzvHdCYr/Wzlx3FE10X79FQziO0SNr4936VlDLjgpnj6A ylydipZcLzykWhBenfyP9kZP7HBZK2QyAgRcQY3y+gvZQvCNs0BK+JT51G56ewk0iMZL f0N5lNL6VpYrYQt0c5RUG+2WG2l7v2ktFUD4gHxGRPjkQ5E2w+lvGi7J32A5W5fAmH1w FvZA== X-Gm-Message-State: AOAM530Ql5x2BodN8OAmPPGIuX2CrQCLbYwiD/brT5xagscmNJuoDmi6 VcUU1U1Y8McgT6B0ZEn9iluU/Q== X-Google-Smtp-Source: ABdhPJwh031fohG6/xDMYBzlLdMemiVgLsV2LtlY1ILlIi9jLc7uB03AjYiebP81mvi3Ot1qAYM1hA== X-Received: by 2002:a5d:452b:: with SMTP id j11mr4553679wra.228.1617802267024; Wed, 07 Apr 2021 06:31:07 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id x12sm29943794wrr.7.2021.04.07.06.31.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Apr 2021 06:31:06 -0700 (PDT) Date: Wed, 7 Apr 2021 15:31:05 +0200 From: Olivier Matz To: Luc Pelletier Cc: jianfeng.tan@intel.com, Honnappa.Nagarahalli@arm.com, dev@dpdk.org, stable@dpdk.org Message-ID: <20210407133105.GL1650@platinum> References: <20210407123529.123899-1-lucp.at.work@gmail.com> <20210407125322.124327-1-lucp.at.work@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210407125322.124327-1-lucp.at.work@gmail.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-stable] [PATCH v5] 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" Hi Luc, On Wed, Apr 07, 2021 at 08:53:23AM -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 > --- > > Same as v4 except that I fixed 2 minor style issues flagged by patchwork. > > lib/librte_eal/common/eal_common_thread.c | 52 +++++++++++------------ > 1 file changed, 25 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..c1044e795 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,14 +215,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 +233,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; I think not killing the thread when pthread_setaffinity_np() returns an error is not very understandable from the API user point of view. What about doing this on top of your patch? The idea is to set start_routine to NULL before the barrier if pthread_setaffinity_np() failed. So there is no need to cancel the thread, it will exit by itself. @@ -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); } @@ -233,10 +237,18 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name, } ret = pthread_setaffinity_np(*thread, sizeof(*cpuset), cpuset); + if (ret != 0) + params->start_routine = NULL; + pthread_barrier_wait(¶ms->configured); ctrl_params_free(params); - return -ret; + if (ret != 0) { + pthread_join(*thread, NULL); + return -ret; + } + + return 0; fail_with_barrier: pthread_barrier_destroy(¶ms->configured); Regards, Olivier