| Message ID | 52A74F24.8000805@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, 10 Dec 2013, Will Newton wrote: > 2013-12-10 Will Newton <will.newton@linaro.org> > > * sysdeps/arm/__longjmp.S: Don't apply pointer encryption > to fp register. > * sysdeps/arm/setjmp.S: Likewise. > * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add > fp to register list, remove a4. > * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD): > New macro. OK, presuming you've tested this with the glibc testsuite.
On 12/10/2013 01:06 PM, Joseph S. Myers wrote: > On Tue, 10 Dec 2013, Will Newton wrote: > >> 2013-12-10 Will Newton <will.newton@linaro.org> >> >> * sysdeps/arm/__longjmp.S: Don't apply pointer encryption >> to fp register. >> * sysdeps/arm/setjmp.S: Likewise. >> * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add >> fp to register list, remove a4. >> * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD): >> New macro. > > OK, presuming you've tested this with the glibc testsuite. > Is it really true that ruby checks the FP? I don't see such code? Can you please point it out? Cheers, Carlos.
On 10 December 2013 18:57, Carlos O'Donell <carlos@redhat.com> wrote: > On 12/10/2013 01:06 PM, Joseph S. Myers wrote: >> On Tue, 10 Dec 2013, Will Newton wrote: >> >>> 2013-12-10 Will Newton <will.newton@linaro.org> >>> >>> * sysdeps/arm/__longjmp.S: Don't apply pointer encryption >>> to fp register. >>> * sysdeps/arm/setjmp.S: Likewise. >>> * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add >>> fp to register list, remove a4. >>> * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD): >>> New macro. >> >> OK, presuming you've tested this with the glibc testsuite. >> > > Is it really true that ruby checks the FP? > > I don't see such code? Can you please point it out? In vm_core.h: 474 jmp_buf machine_regs; In vm.c: 1589 if (GET_THREAD() != th && th->machine_stack_start && th->machine_stack_end) { 1590 rb_gc_mark_machine_stack(th); 1591 rb_gc_mark_locations((VALUE *)&th->machine_regs, 1592 (VALUE *)(&th->machine_regs) + 1593 sizeof(th->machine_regs) / sizeof(VALUE)); 1594 } So it looks like a conservative GC that uses the jmp_buf as a data array to find potentially reachable objects. If fp contained a pointer to an object then the pointer encryption would render it undiscoverable and it would not get marked as live and could be collected in error. There are a number of "ifs" involved and I haven't got the testsuite running yet but it looks like a possibility.
On 12/10/2013 03:05 PM, Will Newton wrote: > On 10 December 2013 18:57, Carlos O'Donell <carlos@redhat.com> wrote: >> On 12/10/2013 01:06 PM, Joseph S. Myers wrote: >>> On Tue, 10 Dec 2013, Will Newton wrote: >>> >>>> 2013-12-10 Will Newton <will.newton@linaro.org> >>>> >>>> * sysdeps/arm/__longjmp.S: Don't apply pointer encryption >>>> to fp register. >>>> * sysdeps/arm/setjmp.S: Likewise. >>>> * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add >>>> fp to register list, remove a4. >>>> * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD): >>>> New macro. >>> >>> OK, presuming you've tested this with the glibc testsuite. >>> >> >> Is it really true that ruby checks the FP? >> >> I don't see such code? Can you please point it out? > > In vm_core.h: > > 474 jmp_buf machine_regs; > > In vm.c: > > 1589 if (GET_THREAD() != th && th->machine_stack_start && > th->machine_stack_end) { > 1590 rb_gc_mark_machine_stack(th); > 1591 rb_gc_mark_locations((VALUE *)&th->machine_regs, > 1592 (VALUE *)(&th->machine_regs) + > 1593 sizeof(th->machine_regs) / > sizeof(VALUE)); > 1594 } > > So it looks like a conservative GC that uses the jmp_buf as a data > array to find potentially reachable objects. If fp contained a pointer > to an object then the pointer encryption would render it > undiscoverable and it would not get marked as live and could be > collected in error. > > There are a number of "ifs" involved and I haven't got the testsuite > running yet but it looks like a possibility. > Thanks for pointing that out. I'll get a new glibc to the Fedora ruby maintainer and ask them to test in parallel. Cheers, Carlos.
On 12/10/2013 12:28 PM, Will Newton wrote: > > The frame pointer register is rarely used for that purpose on ARM and > applications that look at the contents of the jmp_buf may be relying > on reading the value. Ruby uses the contents of jmp_buf to find the > root set for garbage collection so relies on this pointer value being > unencrypted. > > ports/ChangeLog.arm: > > 2013-12-10 Will Newton <will.newton@linaro.org> > > * sysdeps/arm/__longjmp.S: Don't apply pointer encryption > to fp register. > * sysdeps/arm/setjmp.S: Likewise. > * sysdeps/arm/include/bits/setjmp.h (JMP_BUF_REGLIST): Add > fp to register list, remove a4. > * sysdeps/unix/sysv/linux/arm/sysdep.h: (PTR_MANGLE_LOAD): > New macro. This looks good to me. I agree that because fp might not be used for a frame pointer, and the AEABI doesn't mandate it, that it might have useful required information that the ruby GC might need. So while ruby is wrong in inspecting jmp_buf, we're actually encrypting a general register which ruby might need to to scan to correctly support dynamic GC. As David Miller points out this structure might have been on the stack and the gc would be scanning the stack looking for valid pointers and need this information. Cheers, Carlos.
diff --git a/ports/sysdeps/arm/__longjmp.S b/ports/sysdeps/arm/__longjmp.S index 894c121..aaa2d3d 100644 --- a/ports/sysdeps/arm/__longjmp.S +++ b/ports/sysdeps/arm/__longjmp.S @@ -41,14 +41,12 @@ ENTRY (__longjmp) sfi_sp sfi_breg ip, \ ldmia \B!, JMP_BUF_REGLIST #ifdef PTR_DEMANGLE - PTR_DEMANGLE (fp, a4, a3, a2) ldr a4, [ip], #4 - PTR_DEMANGLE2 (a4, a4, a3) + PTR_DEMANGLE (a4, a4, a3, a2) mov sp, a4 ldr a4, [ip], #4 PTR_DEMANGLE2 (lr, a4, a3) #else - mov fp, a4 ldr sp, [ip], #4 ldr lr, [ip], #4 #endif diff --git a/ports/sysdeps/arm/include/bits/setjmp.h b/ports/sysdeps/arm/include/bits/setjmp.h index 64505dc..7bb4f00 100644 --- a/ports/sysdeps/arm/include/bits/setjmp.h +++ b/ports/sysdeps/arm/include/bits/setjmp.h @@ -26,9 +26,8 @@ #ifndef _ISOMAC /* Register list for a ldm/stm instruction to load/store - the general registers from a __jmp_buf. The a4 register - contains fp at this point. */ -# define JMP_BUF_REGLIST {a4, v1-v6, sl} + the general registers from a __jmp_buf. */ +# define JMP_BUF_REGLIST {v1-v6, sl, fp} /* Index of __jmp_buf where the sp register resides. */ # define __JMP_BUF_SP 8 diff --git a/ports/sysdeps/arm/setjmp.S b/ports/sysdeps/arm/setjmp.S index fedd994..803591e 100644 --- a/ports/sysdeps/arm/setjmp.S +++ b/ports/sysdeps/arm/setjmp.S @@ -23,9 +23,7 @@ ENTRY (__sigsetjmp) #ifdef PTR_MANGLE - PTR_MANGLE (a4, fp, a3, ip) -#else - mov a4, fp + PTR_MANGLE_LOAD (a3, ip) #endif mov ip, r0 diff --git a/ports/sysdeps/unix/sysv/linux/arm/sysdep.h b/ports/sysdeps/unix/sysv/linux/arm/sysdep.h index 6cfe4e0..ccab57e 100644 --- a/ports/sysdeps/unix/sysv/linux/arm/sysdep.h +++ b/ports/sysdeps/unix/sysv/linux/arm/sysdep.h @@ -439,8 +439,10 @@ __local_syscall_error: \ #if (defined NOT_IN_libc && defined IS_IN_rtld) || \ (!defined SHARED && (!defined NOT_IN_libc || defined IS_IN_libpthread)) # ifdef __ASSEMBLER__ +# define PTR_MANGLE_LOAD(guard, tmp) \ + LDST_PCREL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard_local)); # define PTR_MANGLE(dst, src, guard, tmp) \ - LDST_PCREL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard_local)); \ + PTR_MANGLE_LOAD(guard, tmp); \ PTR_MANGLE2(dst, src, guard) /* Use PTR_MANGLE2 for efficiency if guard is already loaded. */ # define PTR_MANGLE2(dst, src, guard) \ @@ -457,8 +459,10 @@ extern uintptr_t __pointer_chk_guard_local attribute_relro attribute_hidden; # endif #else # ifdef __ASSEMBLER__ +# define PTR_MANGLE_LOAD(guard, tmp) \ + LDST_GLOBAL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard)); # define PTR_MANGLE(dst, src, guard, tmp) \ - LDST_GLOBAL(ldr, guard, tmp, C_SYMBOL_NAME(__pointer_chk_guard)); \ + PTR_MANGLE_LOAD(guard, tmp); \ PTR_MANGLE2(dst, src, guard) /* Use PTR_MANGLE2 for efficiency if guard is already loaded. */ # define PTR_MANGLE2(dst, src, guard) \