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 84CD741DB0; Thu, 2 Mar 2023 02:46:01 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5908641151; Thu, 2 Mar 2023 02:46:01 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 8FE684021F for ; Thu, 2 Mar 2023 02:45:59 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id D1736209FE08; Wed, 1 Mar 2023 17:45:58 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D1736209FE08 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1677721558; bh=qXDKZOaHJmXq73sRB8TLbxwJ9CY8ZI7WEDK6u5cyN3A=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=NTx3GRcGmHwbW83k0G28vmQ6yCukAP5Tr/eT9Dsd4OHlolG+juCwckt7fMfWASjxr 50qKG1wegxEk6qZNGgGe9NZ/ODp33f4KdOmwMmsBaszL5mL48ShXpYRCM//UAsUECN jhut7avUFxgp/eIRbVFriJc8Fmm0xbe6YkYr95Dk= Date: Wed, 1 Mar 2023 17:45:58 -0800 From: Tyler Retzlaff To: Honnappa Nagarahalli Cc: "dev@dpdk.org" , "david.marchand@redhat.com" , "thomas@monjalon.net" , nd Subject: Re: [PATCH v2] eal: fix thread race in control thread creation Message-ID: <20230302014558.GA9271@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1677518230-1194-1-git-send-email-roretzla@linux.microsoft.com> <1677704982-2643-1-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 On Thu, Mar 02, 2023 at 01:30:13AM +0000, Honnappa Nagarahalli wrote: > > > > -----Original Message----- > > From: Tyler Retzlaff > > Sent: Wednesday, March 1, 2023 3:10 PM > > To: dev@dpdk.org; david.marchand@redhat.com > > Cc: thomas@monjalon.net; Tyler Retzlaff > > Subject: [PATCH v2] eal: fix thread race in control thread creation > > > > When ctrl_thread_init transitions params->ctrl_thread_status from > > CTRL_THREAD_LAUNCHING the creating thread and new thread may run > > concurrently leading to unsynchronized access to params. > IMO, the code will be simpler if we did not free 'params' in 'rte_thread_create_control'/'rte_ctrl_thread_create'. We could avoid creating the local copies of start_routine and the arg. You mean in the success case i assume? it still has to be free'd if rte_thread_create fails. > See more comments below. > > > > > This permits races for both the failure and success paths after ctrl_thread_status > > is stored. > > * params->ret may be loaded in ctrl_thread_init failure path > > * params->arg may be loaded in ctrl_thread_start or > > control_thread_start when calling start_routine. > > > > For ctrl_thread_init remove the params->ret load and just return 1 since it is > > only interpreted as a indicator of success / failure of ctrl_thread_init. > > > > For {ctrl,control}_thread_start store param->arg in stack allocated storage prior > > to calling ctrl_thread_init and use the copy when calling start_routine. > > > > For control_thread_start if ctrl_thread_init fails just return 0 instead of loading > > params->ret, since the value returned is unused when ctrl_thread_status is set > > to CTRL_THREAD_ERROR when ctrl_thread_init fails. > > > > Fixes: 878b7468eacb ("eal: add platform agnostic control thread API") > > > > Signed-off-by: Tyler Retzlaff > > Reviewed-by: David Marchand > > --- > > lib/eal/common/eal_common_thread.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/lib/eal/common/eal_common_thread.c > > b/lib/eal/common/eal_common_thread.c > > index edb9d4e..079a385 100644 > > --- a/lib/eal/common/eal_common_thread.c > > +++ b/lib/eal/common/eal_common_thread.c > > @@ -256,7 +256,7 @@ static int ctrl_thread_init(void *arg) > > if (params->ret != 0) { > > __atomic_store_n(¶ms->ctrl_thread_status, > > CTRL_THREAD_ERROR, __ATOMIC_RELEASE); > > - return params->ret; > > + return 1; > > } > > > > __atomic_store_n(¶ms->ctrl_thread_status, > > @@ -268,23 +268,25 @@ static int ctrl_thread_init(void *arg) static void > > *ctrl_thread_start(void *arg) { > > struct rte_thread_ctrl_params *params = arg; > > + void *start_arg = params->arg; > > void *(*start_routine)(void *) = params->u.ctrl_start_routine; > These copies can be avoided, code will be much simpler > > > > > if (ctrl_thread_init(arg) != 0) > > return NULL; > > > > - return start_routine(params->arg); > > + return start_routine(start_arg); > We can free 'params' here after 'start_routine' returns. I guess it doesn't matter if the allocation is retained for the duration of start_routine() which could be ~long. David/Honnappah let me know what you decide. if you'd prefer to change to honnappah's suggestion i'll put a new version up. > > > } > > > > static uint32_t control_thread_start(void *arg) { > > struct rte_thread_ctrl_params *params = arg; > > + void *start_arg = params->arg; > > rte_thread_func start_routine = params->u.control_start_routine; > > > > if (ctrl_thread_init(arg) != 0) > > - return params->ret; > > + return 0; > > > > - return start_routine(params->arg); > > + return start_routine(start_arg); > > } > > > > int > > -- > > 1.8.3.1