From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 3BE37200 for ; Wed, 2 May 2018 15:54:22 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 May 2018 06:54:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,354,1520924400"; d="scan'208";a="36880725" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.249.170.170]) ([10.249.170.170]) by fmsmga008.fm.intel.com with ESMTP; 02 May 2018 06:54:17 -0700 To: Olivier Matz References: <1525255198-20906-1-git-send-email-jianfeng.tan@intel.com> <1525256270-23138-1-git-send-email-jianfeng.tan@intel.com> <20180502112417.shlwamchx4as4sqw@neon> Cc: dev@dpdk.org, anatoly.burakov@intel.com, thomas@monjalon.net From: "Tan, Jianfeng" Message-ID: Date: Wed, 2 May 2018 21:54:11 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20180502112417.shlwamchx4as4sqw@neon> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] eal: fix use-after-free issue on thread creation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 13:54:24 -0000 Hi Olivier, On 5/2/2018 7:24 PM, Olivier Matz wrote: > Hi Jianfeng, > > On Wed, May 02, 2018 at 10:17:50AM +0000, Jianfeng Tan wrote: >> After below commit, we encounter some strange issue: >> 1) Dead lock as described here: >> http://dpdk.org/ml/archives/dev/2018-April/099806.html >> 2) SIGSEGV issue when starting a testpmd in VM. >> >> Considering below commit changes to use dynamic memory instead of >> stack for memory barrier, we doubt it's caused by use-after-free. >> >> Fixes: 3d09a6e26d8b ("eal: fix threads block on barrier") >> >> Reported-by: Maxime Coquelin >> Reported-by: Lei Yao >> Suggested-by: Stephen Hemminger >> Signed-off-by: Jianfeng Tan >> --- >> v1->v2: >> - Destroy barrier if failure happens. >> lib/librte_eal/common/eal_common_thread.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c >> index de69452..5f0c61f 100644 >> --- a/lib/librte_eal/common/eal_common_thread.c >> +++ b/lib/librte_eal/common/eal_common_thread.c >> @@ -149,11 +149,16 @@ struct rte_thread_ctrl_params { >> >> static void *rte_thread_init(void *arg) >> { >> + int ret; >> struct rte_thread_ctrl_params *params = arg; >> void *(*start_routine)(void *) = params->start_routine; >> void *routine_arg = params->arg; >> >> - pthread_barrier_wait(¶ms->configured); >> + ret = pthread_barrier_wait(¶ms->configured); >> + if (ret == PTHREAD_BARRIER_SERIAL_THREAD) { >> + pthread_barrier_destroy(¶ms->configured); >> + free(params); >> + } >> >> return start_routine(routine_arg); >> } >> @@ -204,12 +209,16 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name, >> if (ret < 0) >> goto fail; >> >> - pthread_barrier_wait(¶ms->configured); >> - free(params); >> + ret = pthread_barrier_wait(¶ms->configured); >> + if (ret == PTHREAD_BARRIER_SERIAL_THREAD) { >> + pthread_barrier_destroy(¶ms->configured); >> + free(params); >> + } >> >> return 0; >> >> fail: >> + pthread_barrier_destroy(¶ms->configured); > I think we should have the same code than above in the fail case: > > ret = pthread_barrier_wait(¶ms->configured); > if (ret == PTHREAD_BARRIER_SERIAL_THREAD) { > pthread_barrier_destroy(¶ms->configured); > free(params); > } > > Else, the child will wait forever on the barrier on failure. Make sense. I send v3 patch according to your advice, except that not polluting ret. Thanks, Jianfeng > > This can be tested with this standalone program: > https://www.droids-corp.org/~zer0/hidden/ctrl_thread.c > > gcc -W -Wall -Werror -Wextra -pthread ctrl_thread.c > ./a.out -> fail > > gcc -W -Wall -Werror -Wextra -pthread -DFIX ctrl_thread.c > ./a.out -> ok > > > >> pthread_cancel(*thread); >> pthread_join(*thread, NULL); >> free(params); >> -- >> 2.7.4 >>