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 14491A0579; Wed, 7 Apr 2021 16:57:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 84643140F72; Wed, 7 Apr 2021 16:57:25 +0200 (CEST) Received: from mail-wr1-f42.google.com (mail-wr1-f42.google.com [209.85.221.42]) by mails.dpdk.org (Postfix) with ESMTP id E71D0140F6C for ; Wed, 7 Apr 2021 16:57:23 +0200 (CEST) Received: by mail-wr1-f42.google.com with SMTP id a12so8544179wrq.13 for ; Wed, 07 Apr 2021 07:57:23 -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=JHBz2IDUedOf/Q3T+x2Ie2v/876ufxhEjikxzeESUSU=; b=HDC4E2eDh/GBY4WjVyCaPu+mfpYc7zUzD2vuthBIof7rSyjBgrOUYvMxaNTCbHNpED 3wAaiZfJAL4usoBlUO9fKWNiju32iua/3SeQxTCv0VodPCQWHhg3aq4l5pi+l2pKoyWU 7FILThxAuCCRC6x2GfhXO+nBOmgP5dhlyrwXd2zHjk8FzJTo7LtHtDSXrOK+n0wJKu0j VbGj0HJSaoBI8n9Ue5Qjr81EN1X2hHNrtGTWcmwF5DAPhZW7m83Z+6uP5RAqDFZBEQYH a8PRq+orS+ZGwT+P+emJU4Gq/eZhQLgTQMJn6sV+ZhRUIuVI9wUZMzfjWjKC8ZMnw6zY v6LA== 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=JHBz2IDUedOf/Q3T+x2Ie2v/876ufxhEjikxzeESUSU=; b=JsxqvOs5nDAHiKgsPbUWigXXtqR6HLIJjZf3w7YGjkBL3I9QCAZJtrqh+tnaWxMr2X D0JaJtfUczNlztS4R/dL36G0bNKWpW0Nh3jXVGX0plmGwOyz9mMqWwNhCiCXTsEXzGHi HXOUyuH1JWMejaUIqvcIhgwJi4xRd1eIRcfviGK6Zak6qJs4TMmVPWLpEJmnAMoznv4n Z0zehSSPQ/cQqAk8Ed2/f7pIl1YK7/40u+x77WH7eCU5G+izkwOpqz4VdaOOg8wSPjxr Fg4but1DGssz7wOLYtwV01cRykiQrb6CoDxUUJpZ+tg09lphThvn+o0iR8mcnTe/0CWL EP/Q== X-Gm-Message-State: AOAM532yf63N1d4omK5pvawUseXbhGHJO18FHKcJW9N9QORJunfo2zDb xWYhR5Gb/+LmgzuPemiNsfCNHw== X-Google-Smtp-Source: ABdhPJw4267TkTO9OVg6ap1WupYsVfSNECq1QUDHjeDhKsG054NhefqiV2hntUffEopD/bSKS99Jjw== X-Received: by 2002:adf:db49:: with SMTP id f9mr4893016wrj.394.1617807443630; Wed, 07 Apr 2021 07:57:23 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id j30sm43818659wrj.62.2021.04.07.07.57.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Apr 2021 07:57:23 -0700 (PDT) Date: Wed, 7 Apr 2021 16:57:22 +0200 From: Olivier Matz To: Luc Pelletier Cc: jianfeng.tan@intel.com, Honnappa.Nagarahalli@arm.com, dev@dpdk.org, stable@dpdk.org Message-ID: <20210407145722.GN1650@platinum> References: <20210407133105.GL1650@platinum> <20210407144236.128365-1-lucp.at.work@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210407144236.128365-1-lucp.at.work@gmail.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v6] 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" 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 > --- > > 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 >