From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> To: Olivier Matz <olivier.matz@6wind.com>, Luc Pelletier <lucp.at.work@gmail.com> Cc: "jianfeng.tan@intel.com" <jianfeng.tan@intel.com>, "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>, nd <nd@arm.com>, Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>, nd <nd@arm.com> Subject: Re: [dpdk-dev] [PATCH v5] eal: fix race in ctrl thread creation Date: Wed, 7 Apr 2021 15:15:53 +0000 Message-ID: <DBAPR08MB581490C7F81791C485E062F698759@DBAPR08MB5814.eurprd08.prod.outlook.com> (raw) In-Reply-To: <20210407133105.GL1650@platinum> <snip> > > 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 <lucp.at.work@gmail.com> > > --- > > > > 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. Agree. > > 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. How about using the pthread_attr_setaffinity_np API? It is deviating from the documentation of the 'rte_ctrl_thread_create'. But, from the user perspective, the behavior should not change. This way we do not have to handle the error after the thread is launched. > > @@ -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
next prev parent reply other threads:[~2021-04-07 15:16 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-24 13:04 [dpdk-dev] [PATCH] eal: fix possible UB on creation of ctrl thread Luc Pelletier 2021-03-25 11:27 ` [dpdk-dev] [PATCH v2] eal: fix race in ctrl thread creation Olivier Matz 2021-03-25 14:42 ` Luc Pelletier 2021-04-02 4:34 ` Honnappa Nagarahalli 2021-04-06 15:57 ` [dpdk-dev] [PATCH v3] eal: fix possible UB on creation of ctrl thread Luc Pelletier 2021-04-06 16:15 ` [dpdk-dev] [PATCH v3] eal: fix race in ctrl thread creation Luc Pelletier 2021-04-06 21:10 ` Honnappa Nagarahalli 2021-04-07 12:35 ` [dpdk-dev] [PATCH v4] " Luc Pelletier 2021-04-07 12:53 ` [dpdk-dev] [PATCH v5] " Luc Pelletier 2021-04-07 13:22 ` Luc Pelletier 2021-04-07 13:31 ` Olivier Matz 2021-04-07 14:42 ` [dpdk-dev] [PATCH v6] " Luc Pelletier 2021-04-07 14:57 ` Olivier Matz 2021-04-07 15:29 ` [dpdk-dev] [PATCH v7] " Luc Pelletier 2021-04-07 17:15 ` Honnappa Nagarahalli 2021-04-07 15:15 ` Honnappa Nagarahalli [this message] 2021-04-07 20:16 ` [dpdk-dev] [PATCH 1/2] " Luc Pelletier 2021-04-08 14:17 ` Olivier Matz 2021-04-08 17:06 ` Honnappa Nagarahalli 2021-04-07 20:16 ` [dpdk-dev] [PATCH 2/2] eal: fix hang in ctrl thread creation error logic Luc Pelletier 2021-04-08 14:20 ` Olivier Matz 2021-04-08 18:01 ` Luc Pelletier 2021-04-09 8:13 ` David Marchand 2021-04-08 17:07 ` Honnappa Nagarahalli 2021-04-09 14:34 ` [dpdk-dev] [dpdk-stable] " David Marchand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=DBAPR08MB581490C7F81791C485E062F698759@DBAPR08MB5814.eurprd08.prod.outlook.com \ --to=honnappa.nagarahalli@arm.com \ --cc=dev@dpdk.org \ --cc=jianfeng.tan@intel.com \ --cc=lucp.at.work@gmail.com \ --cc=nd@arm.com \ --cc=olivier.matz@6wind.com \ --cc=stable@dpdk.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git