This short series introduces fixes of 2 observed isues (#1 & #2) in clone() syscall in ARC port and one enhancement (#3).
It all started from failing tst-clone1 in uClibc testsuite and during its debugging we observed another quite subtle issue which was then not only solved but better (which means shorter and simpler) implementation was made.
Alexey Brodkin (3): arc: clone: Recover PID correctly arc: clone: Fix CLONE_THREAD detection arc: clone: Simplify CLONE_THREAD detection
libc/sysdeps/linux/arc/clone.S | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
Caught by tst-getpid1 test from uClibc's test-suite.
It looks like original implementation was not correct. The code in question is supposed to recover PID of the new thread. And by no means that could happen with clone() syscall while getpid() does exactly this.
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Acked-by: Vineet Gupta vgupta@synopsys.com Reported-by: Eugeniy Paltsev paltsev@synopsys.com --- libc/sysdeps/linux/arc/clone.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libc/sysdeps/linux/arc/clone.S b/libc/sysdeps/linux/arc/clone.S index 3c1388e..dbb3fa7 100644 --- a/libc/sysdeps/linux/arc/clone.S +++ b/libc/sysdeps/linux/arc/clone.S @@ -72,7 +72,7 @@ ENTRY(clone) and_s r2, r2, r12 brne r2, r12, .Lgo_thread
- mov r8, __NR_clone + mov r8, __NR_getpid ARC_TRAP_INSN ; r0 has PID THREAD_SELF r1 ; Get to struct pthread (just before TCB) st r0, [r1, PTHREAD_PID]
For thread group case (CLONE_THREAD), the cached PID of new process/thread need not be reset. The old logic to decide that was flawed as it would be true only for exact combination of CLONE_THREAD + _VM, but would fail for CLONE_THREAD + _VM + _xyz.
More detailed tear-down of current and new code below.
Current implementation is: --------------------->8-------------------- ; r12 contains clone flags mov_s r2, CLONE_THREAD_N_VM; r2 contains bit mask and_s r2, r2, r12 ; r2 contains bit mask AND clone flags ; but r12 still contains the same flags brne r2, r12, .Lgo_thread ; here we compare modified mask with ; flags as they were and skip pthread TID/PID ; setup if r2 != r12 which happens all ; the time except clone flags were ; exactly CLONE_THREAD | CLONE_VM --------------------->8--------------------
New implementation is: --------------------->8-------------------- ; r12 contains clone flags mov_s r2, CLONE_THREAD_N_VM; r2 contains bit mask and_s r12, r12, r2 ; r12 contains clone flags AND bit mask ; i.e. we did mask all flags except ; CLONE_THREAD and CLONE_VM breq r2, r12, .Lgo_thread ; here we compare masked flags with ; target mask and if they match we skip ; pthread TID/PID setup --------------------->8--------------------
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Acked-by: Vineet Gupta vgupta@synopsys.com --- libc/sysdeps/linux/arc/clone.S | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libc/sysdeps/linux/arc/clone.S b/libc/sysdeps/linux/arc/clone.S index dbb3fa7..fc8dfcf 100644 --- a/libc/sysdeps/linux/arc/clone.S +++ b/libc/sysdeps/linux/arc/clone.S @@ -69,8 +69,8 @@ ENTRY(clone) .Lnext_clone_quirk: #ifdef RESET_PID mov_s r2, CLONE_THREAD_N_VM - and_s r2, r2, r12 - brne r2, r12, .Lgo_thread + and_s r12, r12, r2 + breq r2, r12, .Lgo_thread
mov r8, __NR_getpid ARC_TRAP_INSN ; r0 has PID
This change was inspired by similar change in glibc: https://sourceware.org/git/?p=glibc.git;a=commit;h=0cb313f7cb0e418b3d56f3a2a...
Current Linux kernel requires CLONE_VM to be set with CLONE_THREAD otherwise returning -EINVAL, see man clone2. This means we don't need to check for both CLONE_THREAD and CLONE_VM instead we may simplify code a lot and just check 1 bit (CLONE_THREAD).
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com Acked-by: Vineet Gupta vgupta@synopsys.com --- libc/sysdeps/linux/arc/clone.S | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/libc/sysdeps/linux/arc/clone.S b/libc/sysdeps/linux/arc/clone.S index fc8dfcf..3942b88 100644 --- a/libc/sysdeps/linux/arc/clone.S +++ b/libc/sysdeps/linux/arc/clone.S @@ -27,10 +27,7 @@ ; void *tls, ; int __user *child_tidptr)
-#define CLONE_VM 0x00000100 -#define CLONE_THREAD 0x00010000 #define CLONE_SETTLS 0x00080000 -#define CLONE_THREAD_N_VM (CLONE_THREAD | CLONE_VM)
ENTRY(clone) cmp r0, 0 ; @fn can't be NULL @@ -68,9 +65,7 @@ ENTRY(clone)
.Lnext_clone_quirk: #ifdef RESET_PID - mov_s r2, CLONE_THREAD_N_VM - and_s r12, r12, r2 - breq r2, r12, .Lgo_thread + bbit1 r12, 16, .Lgo_thread ; CLONE_THREAD = (1 << 16)
mov r8, __NR_getpid ARC_TRAP_INSN ; r0 has PID
Hi Alexey, Alexey Brodkin wrote,
This short series introduces fixes of 2 observed isues (#1 & #2) in clone() syscall in ARC port and one enhancement (#3).
It all started from failing tst-clone1 in uClibc testsuite and during its debugging we observed another quite subtle issue which was then not only solved but better (which means shorter and simpler) implementation was made.
Alexey Brodkin (3): arc: clone: Recover PID correctly arc: clone: Fix CLONE_THREAD detection arc: clone: Simplify CLONE_THREAD detection
Series applied and pushed, best regards Waldemar
-
Alexey Brodkin -
Waldemar Brodkorb