eal: fix thread race in control thread creation

PATCH Superseded Tyler Retzlaff 3 participants 9 messages Feb 27, 2023 – Mar 9, 2023

Share & tools

Tyler Retzlaff

author

2023-02-27 17:17 UTC #0 lore raw

PATCH 1/1 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.

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 <[email protected]>

---
 lib/eal/common/eal_common_thread.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

lib/eal/common/eal_common_thread.c +6 -4

index edb9d4e..079a385 100644
@@ -256,7 +256,7 @@ static int ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return params->ret;
+		return 1;
 	}
 
 	__atomic_store_n(&params->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;
 
 	if (ctrl_thread_init(arg) != 0)
 		return NULL;
 
-	return start_routine(params->arg);
+	return start_routine(start_arg);
 }
 
 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

Signature · 1 line

1.8.3.1

David Marchand

Reviewed-by on v1 1/1

2023-03-01 08:19 UTC #1 lore raw

Quoted 5 lines · Tyler Retzlaff

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.

Quoted 7 lines

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

For*

Quoted 4 lines

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

For*

Quoted 4 lines

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

For*

Quoted 7 lines

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 <[email protected]>

Reviewed-by David Marchand <[email protected]>

Thanks Tyler.

Signature · 1 line

David Marchand

Tyler Retzlaff

author

2023-03-01 21:09 UTC #2 lore raw

PATCH v2 1/1 eal: fix thread race in control thread creation

In reply to Tyler Retzlaff

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.

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 <[email protected]>

Reviewed-by David Marchand <[email protected]>

---
 lib/eal/common/eal_common_thread.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

lib/eal/common/eal_common_thread.c +6 -4

index edb9d4e..079a385 100644
@@ -256,7 +256,7 @@ static int ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return params->ret;
+		return 1;
 	}
 
 	__atomic_store_n(&params->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;
 
 	if (ctrl_thread_init(arg) != 0)
 		return NULL;
 
-	return start_routine(params->arg);
+	return start_routine(start_arg);
 }
 
 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

Signature · 1 line

1.8.3.1

Honnappa Nagarahalli

2023-03-02 01:30 UTC #3 lore raw

Quoted 9 lines

-----Original Message-----
From: Tyler Retzlaff <[email protected]>
Sent: Wednesday, March 1, 2023 3:10 PM
To: [email protected]; [email protected]
Cc: [email protected]; Tyler Retzlaff <[email protected]>
Subject: [PATCH v2] eal: fix thread race in control thread creation

Quoted 3 lines

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. See more comments below.

Quoted 40 lines

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 <[email protected]>
Reviewed-by David Marchand <[email protected]>
---
 lib/eal/common/eal_common_thread.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
lib/eal/common/eal_common_thread.c +1 -1
index edb9d4e..079a385 100644
@@ -256,7 +256,7 @@ static int ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return params->ret;
+		return 1;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,

lib/eal/common/eal_common_thread.c static int ctrl_thread_init(void *arg) static void · line 271

Quoted 4 lines

lib/eal/common/eal_common_thread.c +1 -0

@@ -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

lib/eal/common/eal_common_thread.c static int ctrl_thread_init(void *arg) · line 277

Quoted 6 lines

lib/eal/common/eal_common_thread.c +1 -1

 	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.

Quoted 18 lines

lib/eal/common/eal_common_thread.c +3 -3

 }
 
 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

Tyler Retzlaff

author

2023-03-02 01:46 UTC #4 lore raw

Quoted 9 lines · Honnappa Nagarahalli

Quoted 9 lines
-----Original Message-----
From: Tyler Retzlaff <[email protected]>
Sent: Wednesday, March 1, 2023 3:10 PM
To: [email protected]; [email protected]
Cc: [email protected]; Tyler Retzlaff <[email protected]>
Subject: [PATCH v2] eal: fix thread race in control thread creation

Quoted 4 lines

Quoted 3 lines
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.

Quoted 49 lines

See more comments below.
Quoted 40 lines
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 <[email protected]>
Reviewed-by David Marchand <[email protected]>
---
 lib/eal/common/eal_common_thread.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
lib/eal/common/eal_common_thread.c +1 -1
index edb9d4e..079a385 100644
@@ -256,7 +256,7 @@ static int ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return params->ret;
+		return 1;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
lib/eal/common/eal_common_thread.c static int ctrl_thread_init(void *arg) static void · line 271
Quoted 4 lines
lib/eal/common/eal_common_thread.c +1 -0
@@ -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
Quoted 1 line

Quoted 6 lines

lib/eal/common/eal_common_thread.c static int ctrl_thread_init(void *arg) · line 277
Quoted 5 lines
lib/eal/common/eal_common_thread.c +1 -1
 	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.

Quoted 19 lines

}

static uint32_t control_thread_start(void *arg) {

lib/eal/common/eal_common_thread.c +3 -3

 	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

Honnappa Nagarahalli

2023-03-02 04:08 UTC #5 lore raw

<snip>

Quoted 10 lines

Quoted 5 lines
Quoted 4 lines
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.

Yes

Quoted 58 lines

Quoted 12 lines
See more comments below.
Quoted 9 lines
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.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 41 lines
Quoted 31 lines
For control_thread_start if ctrl_thread_init fails just return 0 instead of loading
params->ret, since the value returned is unused when params->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 <[email protected]>
Reviewed-by David Marchand <[email protected]>
---
 lib/eal/common/eal_common_thread.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
lib/eal/common/eal_common_thread.c +1 -1
index edb9d4e..079a385 100644
@@ -256,7 +256,7 @@ static int ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return params->ret;
+		return 1;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
lib/eal/common/eal_common_thread.c static int ctrl_thread_init(void *arg) static · line 271
Quoted 4 lines
lib/eal/common/eal_common_thread.c +1 -0
@@ -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
Quoted 4 lines
lib/eal/common/eal_common_thread.c +0 -0
 	if (ctrl_thread_init(arg) != 0)
 		return NULL;
 

Quoted 6 lines

Quoted 3 lines
lib/eal/common/eal_common_thread.c static int ctrl_thread_init(void *arg) static · line 275
Quoted 2 lines
lib/eal/common/eal_common_thread.c +1 -1
-	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.

Yes, that's what I thought, it is a small size.

Quoted 23 lines

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.

Quoted 19 lines

lib/eal/common/eal_common_thread.c +3 -3

 }
 
 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

David Marchand

2023-03-07 13:53 UTC #6 lore raw

Quoted 14 lines · Honnappa Nagarahalli

Quoted 9 lines
Quoted 6 lines
Quoted 5 lines
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.
Yes, that's what I thought, it is a small size.
Quoted 3 lines
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.

Hum, how would it look like?
- the parent thread would free params if the child thread fails to set affinity,
- the child thread would free params otherwise,

Is this what you propose?

This works, but we have to be extra careful if any change is done in this part later.

Signature · 1 line

David Marchand

Tyler Retzlaff

author

2023-03-07 17:54 UTC #7 lore raw

Quoted 17 lines · David Marchand

Quoted 14 lines · Honnappa Nagarahalli
Quoted 9 lines
Quoted 6 lines
Quoted 5 lines
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.
Yes, that's what I thought, it is a small size.
Quoted 3 lines
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.

Quoted 8 lines

Hum, how would it look like?
- the parent thread would free params if the child thread fails to set affinity,
- the child thread would free params otherwise,
Is this what you propose?
This works, but we have to be extra careful if any change is done in this part later.

i think the fix i've already put up is kind of trivially verifiable as correct, if we just need a fix then it works.

Quoted 4 lines

Signature · 2 lines
-- 
David Marchand

David Marchand

Applied

2023-03-09 09:01 UTC #8 lore raw

In reply to Tyler Retzlaff

Quoted 24 lines · Tyler Retzlaff

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.
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.

Quoted 4 lines

Fixes 878b7468eacb ("eal: add platform agnostic control thread API")
Signed-off-by Tyler Retzlaff <[email protected]>
Reviewed-by David Marchand <[email protected]>

Honnappa reached out offlist and proposed to send the discussed rework as a followup patch.
So I'll take this fix as is for now.

Applied, thanks Tyler.

Signature · 1 line

David Marchand