Re: 答复: [musl] Subject: [PATCH] pthread: Fix bug that pthread_create may cause priority inversion

Submitted by Rich Felker on Sept. 11, 2019, 5:29 p.m.

Details

Message ID 20190911172919.GZ9017@brightrain.aerifal.cx
State New
Series "Subject: [PATCH] pthread: Fix bug that pthread_create may cause priority inversion"
Headers show

Commit Message

Rich Felker Sept. 11, 2019, 5:29 p.m.
On Wed, Sep 11, 2019 at 09:52:00AM -0400, Rich Felker wrote:
> On Wed, Sep 11, 2019 at 01:38:38PM +0000, zhaohang (F) wrote:
> > Thank you Rich for your patch. It helps me a lot.
> > 
> > But I find that 'return 0' is used to let child thread exit. In that
> > case, a bad thing will happen that the return address of child
> > thread maybe undefined, if caller set prio of child unsuccessfully.
> 
> The code in __clone is supposed to perform SYS_exit if the start
> function returns; this actually matters for users of the public
> clone() function, I think.

I found the problem -- when clone.s is built as thumb, mov lr,pc is
invalid for saving the return address (it omits the thumb-mode bit).
I have a patch I'll push soon, attached. Thanks again for the report!

Rich
From 05870abeaac0588fb9115cfd11f96880a0af2108 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Wed, 11 Sep 2019 13:13:57 -0400
Subject: [PATCH 1/2] fix code path where child function returns in arm __clone
 built as thumb

mov lr,pc is not a valid way to save the return address in thumb mode
since it omits the thumb bit. use a chain of bl and bx to emulate blx.
this could be avoided by converting to a .S file with preprocessor
conditions to use blx if available, but the time cost here is
dominated by the syscall anyway.

while making this change, also remove the remnants of support for
pre-bx ISA levels. commit 9f290a49bf9ee247d540d3c83875288a7991699c
removed the hack from the parent code paths, but left the unnecessary
code in the child. keeping it would require rewriting two code paths
rather than one, and is useless for reasons described in that commit.
---
 src/thread/arm/clone.s | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/thread/arm/clone.s b/src/thread/arm/clone.s
index e16b1326..bb0965da 100644
--- a/src/thread/arm/clone.s
+++ b/src/thread/arm/clone.s
@@ -20,13 +20,9 @@  __clone:
 	bx lr
 
 1:	mov r0,r6
-	tst r5,#1
-	bne 1f
-	mov lr,pc
-	mov pc,r5
+	bl 3f
 2:	mov r7,#1
 	svc 0
-
-1:	mov lr,pc
-	bx r5
 	b 2b
+
+3:	bx r5

Comments

zhaohang (F) Sept. 16, 2019, 2:27 a.m.
Thanks for your reply

-----邮件原件-----
发件人: Rich Felker [mailto:dalias@aerifal.cx] 代表 Rich Felker
发送时间: 2019年9月12日 1:29
收件人: musl@lists.openwall.com
主题: Re: [musl] Re: 答复: [musl] Subject: [PATCH] pthread: Fix bug that pthread_create may cause priority inversion

On Wed, Sep 11, 2019 at 09:52:00AM -0400, Rich Felker wrote:
> On Wed, Sep 11, 2019 at 01:38:38PM +0000, zhaohang (F) wrote:

> > Thank you Rich for your patch. It helps me a lot.

> > 

> > But I find that 'return 0' is used to let child thread exit. In that 

> > case, a bad thing will happen that the return address of child 

> > thread maybe undefined, if caller set prio of child unsuccessfully.

> 

> The code in __clone is supposed to perform SYS_exit if the start 

> function returns; this actually matters for users of the public

> clone() function, I think.


I found the problem -- when clone.s is built as thumb, mov lr,pc is invalid for saving the return address (it omits the thumb-mode bit).
I have a patch I'll push soon, attached. Thanks again for the report!

Rich