DPDK patches and discussions
 help / color / mirror / Atom feed
From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
To: Luc Pelletier <lucp.at.work@gmail.com>,
	"olivier.matz@6wind.com" <olivier.matz@6wind.com>,
	"jianfeng.tan@intel.com" <jianfeng.tan@intel.com>
Cc: "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 v7] eal: fix race in ctrl thread creation
Date: Wed, 7 Apr 2021 17:15:07 +0000
Message-ID: <DBAPR08MB58140ACB7FF7893A60EB6A5998759@DBAPR08MB5814.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20210407152903.130730-1-lucp.at.work@gmail.com>

<snip>

> Subject: [PATCH v7] eal: fix race in ctrl thread creation
> 
> 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>
Overall, this LGTM.

Just wondering if this should be split into 2 commits.
1) Fix the race condition with refcnt.
2) Remove the use of pthread_cancel

I have some observations below.
> ---
> 
> Hi Olivier,
> 
> Olivier, thanks for pointing out that pthread_barrier_wait acts as a full
> memory barrier; I didn't know that was explicitly specified in the
"full memory barrier" just says that the memory operations prior and after the barrier will not be mixed. It does not guarantee that the modification done by one core are 'visible' to other cores. Additional code is required to ensure that the modifications are visible. For ex: in the following code setting start_routine=NULL is not guaranteed to be visible to other cores if pthread_barrier_wait is just a full barrier.
The particular wording used in the reference is "synchronize memory with respect to other threads". I am not exactly sure what is means, but may be safe to assume it means the updates are 'visible' to other cores (as the locking APIs are part of the list and they guarantee the visibility aspects).

May be we can simplify this during 21.11 release as we will have the ability to make more invasive changes.

> documentation. I've changed it to use a regular read/write.
> 
> Hi Honnappa,
> 
> I have not changed the code to use pthread_attr_setaffinity_np because the
> attr parameter is const. I could make a copy of the provided attributes and
> use that instead, but I think this would still violate the spirit of the API and,
> AFAICT, there's no official mechanism for copying a pthread_attr_t.
Ok, thank you.

> 
> Please let me know what you think.
> 
>  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..d4e09f84b 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(&params->refcnt, 1, __ATOMIC_ACQ_REL) ==
> 0) {
> +		pthread_barrier_destroy(&params->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(&params->configured);
> -	if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
> -		pthread_barrier_destroy(&params->configured);
> -		free(params);
> -	}
> +	pthread_barrier_wait(&params->configured);
> +	start_routine = params->start_routine;
> +	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(&params->configured, NULL, 2);
> +	ret = pthread_barrier_init(&params->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)
> +		params->start_routine = NULL;
> +	pthread_barrier_wait(&params->configured);
> +	ctrl_params_free(params);
> 
> -	ret = pthread_barrier_wait(&params->configured);
> -	if (ret == PTHREAD_BARRIER_SERIAL_THREAD) {
> -		pthread_barrier_destroy(&params->configured);
> -		free(params);
> -	}
> +	if (ret != 0)
A comment here on why the pthread_join will not wait indefinitely will help. Splitting the patch will also help understand the changes better.

> +		pthread_join(*thread, NULL);
> 
> -	return 0;
> +	return -ret;
> +
> +fail_with_barrier:
> +	pthread_barrier_destroy(&params->configured);
> +
> +fail_no_barrier:
> +	free(params);
> 
> -fail:
> -	if (PTHREAD_BARRIER_SERIAL_THREAD ==
> -	    pthread_barrier_wait(&params->configured)) {
> -		pthread_barrier_destroy(&params->configured);
> -		free(params);
> -	}
> -	pthread_cancel(*thread);
> -	pthread_join(*thread, NULL);
>  	return -ret;
>  }
> 
> --
> 2.25.1


  reply	other threads:[~2021-04-07 17:15 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 [this message]
2021-04-07 15:15           ` [dpdk-dev] [PATCH v5] " Honnappa Nagarahalli
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=DBAPR08MB58140ACB7FF7893A60EB6A5998759@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