[PATCH v2 1/1] ARM: signal: sigreturn_codes should be endian neutral to work in BE8

Thu Aug 15 06:57:58 EDT 2013

On Wed, Aug 14, 2013 at 12:08:54AM -0700, Victor Kamensky wrote:
> In case of BE8 kernel data is in BE order whereas code stays in LE
> order. Move sigreturn_codes to separate .S file and use proper
> assembler mnemonics for these code snippets. In this case compiler
> will take care of proper instructions byteswaps for BE8 case.
> Change assumes that sufficiently Thumb-capable tools are used to
> build kernel.
> 
> Problem was discovered during ltp testing of BE system: all rt_sig*
> tests failed. Tested against the same tests in both BE and LE modes.

This looks sensible to me, but I have a few comments -- see below.

Cheers
---Dave

> 
> Signed-off-by: Victor Kamensky <victor.kamensky at linaro.org>
> ---
>  arch/arm/kernel/Makefile          |  3 +-
>  arch/arm/kernel/signal.c          | 29 +++---------------
>  arch/arm/kernel/sigreturn_codes.S | 64 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 70 insertions(+), 26 deletions(-)
>  create mode 100644 arch/arm/kernel/sigreturn_codes.S
> 
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 86d10dd..0722155 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -17,7 +17,8 @@ CFLAGS_REMOVE_return_address.o = -pg
>  
>  obj-y		:= elf.o entry-common.o irq.o opcodes.o \
>  		   process.o ptrace.o return_address.o \
> -		   setup.o signal.o stacktrace.o sys_arm.o time.o traps.o
> +		   setup.o signal.o sigreturn_codes.o \
> +		   stacktrace.o sys_arm.o time.o traps.o
>  
>  obj-$(CONFIG_ATAGS)		+= atags_parse.o
>  obj-$(CONFIG_ATAGS_PROC)	+= atags_proc.o
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index ab33042..cdcbf04 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -21,29 +21,8 @@
>  #include <asm/unistd.h>
>  #include <asm/vfp.h>
>  
> -/*
> - * For ARM syscalls, we encode the syscall number into the instruction.
> - */
> -#define SWI_SYS_SIGRETURN	(0xef000000|(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> -#define SWI_SYS_RT_SIGRETURN	(0xef000000|(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE))
> -
> -/*
> - * With EABI, the syscall number has to be loaded into r7.
> - */
> -#define MOV_R7_NR_SIGRETURN	(0xe3a07000 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> -#define MOV_R7_NR_RT_SIGRETURN	(0xe3a07000 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> -
> -/*
> - * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> - * need two 16-bit instructions.
> - */
> -#define SWI_THUMB_SIGRETURN	(0xdf00 << 16 | 0x2700 | (__NR_sigreturn - __NR_SYSCALL_BASE))
> -#define SWI_THUMB_RT_SIGRETURN	(0xdf00 << 16 | 0x2700 | (__NR_rt_sigreturn - __NR_SYSCALL_BASE))
> -
> -static const unsigned long sigreturn_codes[7] = {
> -	MOV_R7_NR_SIGRETURN,    SWI_SYS_SIGRETURN,    SWI_THUMB_SIGRETURN,
> -	MOV_R7_NR_RT_SIGRETURN, SWI_SYS_RT_SIGRETURN, SWI_THUMB_RT_SIGRETURN,
> -};
> +extern const unsigned long sigreturn_codes[];

Could be [_NR_SIGRETURN_CODES] -- see below.

Then you can revert to using sizeof(sigreturn_codes), as previously.

> +extern const int sigreturn_codes_size;

Might go away too

>  
>  static unsigned long signal_return_offset;
>  
> @@ -639,10 +618,10 @@ struct page *get_signal_page(void)
>  	 * Copy signal return handlers into the vector page, and
>  	 * set sigreturn to be a pointer to these.
>  	 */
> -	memcpy(addr + offset, sigreturn_codes, sizeof(sigreturn_codes));
> +	memcpy(addr + offset, sigreturn_codes, sigreturn_codes_size);
>  
>  	ptr = (unsigned long)addr + offset;
> -	flush_icache_range(ptr, ptr + sizeof(sigreturn_codes));
> +	flush_icache_range(ptr, ptr + sigreturn_codes_size);
>  
>  	return page;
>  }
> diff --git a/arch/arm/kernel/sigreturn_codes.S b/arch/arm/kernel/sigreturn_codes.S
> new file mode 100644
> index 0000000..3ac1540
> --- /dev/null
> +++ b/arch/arm/kernel/sigreturn_codes.S
> @@ -0,0 +1,64 @@
> +/*
> + * sigreturn_codes.S - code sinpets for sigreturn syscalls
> + *
> + * Created by:	Victor Kamensky, 2013-08-13
> + * Copyright:	(C) 2013  Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/unistd.h>
> +
> +/*
> + * For ARM syscalls, we encode the syscall number into the instruction.
> + * With EABI, the syscall number has to be loaded into r7. As result
> + * ARM syscall sequence snippet will have move and svc in .arm encoding
> + *
> + * For Thumb syscalls, we pass the syscall number via r7.  We therefore
> + * need two 16-bit instructions in .thumb encoding
> + */
> +
> +	.text

Maybe this should be in .rodata instead.  We only copy the instructions
from here... if we ever execute them here, something has gone wrong
somewhere.

This won't affect link-time swabbing.

> +	.global sigreturn_codes
> +	.type	sigreturn_codes, #object
> +
> +sigreturn_codes:

The above 3 lines can be replaced with ENTRY()...ENDPROC().

(.type / ENDPROC() isn't really required here, though.  I would suggest
omitting them, since although we assemble this as code, we really access
it as data)

> +	/* ARM sigreturn syscall code snippet */
> +	.arm

.arm can insert padding for alignment purposes.  To be on the safe side,
I suggest moving the first .arm just before the sigreturn_codes label.
This will avoid strange surprises if extra code is inserted into this file
later.

> +	mov	r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#(__NR_sigreturn)|(__NR_OABI_SYSCALL_BASE)

The "svc" mnemonic was invented for ARMv6.

Since all platforms rely on this code, including v4/v5, we should use
the old "swi" mnemonic throughout this file instead.

> +
> +	/* Thumb sigreturn syscall code snippet */
> +	.thumb

We should add a comment warning the reader that these instructions are
not executed in-place, and are expected to have a precise, fixed layout.

> +	movs	r7, #(__NR_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#0
> +
> +	/* ARM sigreturn_rt syscall code snippet */
> +	.arm
> +	mov	r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#(__NR_rt_sigreturn)|(__NR_OABI_SYSCALL_BASE)
> +
> +	/* Thumb sigreturn_rt syscall code snippet */
> +	.thumb
> +	movs	r7, #(__NR_rt_sigreturn - __NR_SYSCALL_BASE)
> +	svc	#0
> +
> +	.arm
> +	.space	4

Nico, do you know what the spare word at the end of this array was for?
Is it ABI, or just an accidental off-by-one?

It was introduced in commit fcca538b83f2984095f15f0f90f6c7686e3a32d4
([ARM] 3270/1: ARM EABI: fix sigreturn and rt_sigreturn), but I can't
see clearly why it's needed.

> +__sigreturn_codes_end:
> +	.size	sigreturn_codes, . - sigreturn_codes
> +
> +	.section .rodata
> +	.global	sigreturn_codes_size
> +	.type	sigreturn_codes_size, #object
> +	.size	sigreturn_codes_size, 4
> +
> +sigreturn_codes_size:
> +	.long __sigreturn_codes_end - sigreturn_codes

The correct size is statically fixed by hard-coded assumptions in
signal.c -- we don't want to hide mis-edits by pretending to adapt to
size changes.

So instead of the above lines you could do something like this:

arch/arm/include/asm/signal.h:
#define _NR_SIGRETURN_CODES 7 /* or mabye 6? */
#define _SIGRETURN_CODES_SIZE (4 * _NR_SIGRETURN_CODES)

sigreturn_codes.S:
#include <asm/signal.h>

/* ... */
	svc	#0

	.if	. - sigreturn_codes != _SIGRETURN_CODES_SIZE
	.error	"Incorrect size for sigreturn_codes"
	.endif

Cheers
---Dave