Re: [PATCH v6 00/21] ILP32 for ARM64

Re: [PATCH v6 00/21] ILP32 for ARM64

[Szabolcs Nagy]

On 24/05/16 01:04, Yury Norov wrote:
> This version is based on kernel v4.6.
> It works with glibc-2.23, and tested with LTP.
> 
...
> ILP32 glibc branch is available here:
> https://github.com/norov/glibc/tree/ilp32-2.23
> 
> It is tested with this series with no major downsides. I will send it to 
> glibc-alpha soon, after final revise. Please review and comment it as well.

i spotted one __ilp32__ vs __ILP32__ typo in the glibc code,
i can review it in detail when there is a cleaned up patch set.

in general the approach seems ok, the ugly part is when lp64
and ilp32 share code, but ilp32 needs some tweaks compared to
the current code (e.g. x vs w regs in asm, long changed to
long long in syscalls, different relocations etc) those will
be hard to review. the naming is sometimes _be_ilp32 sometimes
ilp32_be, but let's hope there will be no new abi variant to
confuse this further.

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[David Miller]

From: Yury Norov <ynorov@caviumnetworks.com>
Date: Tue, 24 May 2016 03:04:30 +0300

> +To clear that top halves, automatic wrappers are introduced. They clear all
> +required registers before passing control to regular syscall handler.

Why have one of these for every single compat system call, rather than
simply clearing the top half of all of these registers unconditionally
in the 32-bit system call trap before the system call is invoked?

That's what we do on sparc64.

And with that, you only need wrappers for the case where there needs
to be proper sign extention of a 32-bit signed argument.

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[David Miller]

From: Yury Norov <ynorov@caviumnetworks.com>
Date: Wed, 25 May 2016 23:03:27 +0300

> On Wed, May 25, 2016 at 12:30:17PM -0700, David Miller wrote:
>> From: Yury Norov <ynorov@caviumnetworks.com>
>> Date: Tue, 24 May 2016 03:04:30 +0300
>> 
>> > +To clear that top halves, automatic wrappers are introduced. They clear all
>> > +required registers before passing control to regular syscall handler.
>> 
>> Why have one of these for every single compat system call, rather than
>> simply clearing the top half of all of these registers unconditionally
>> in the 32-bit system call trap before the system call is invoked?
>> 
>> That's what we do on sparc64.
>> 
>> And with that, you only need wrappers for the case where there needs
>> to be proper sign extention of a 32-bit signed argument.
> 
> It was discussed as one of possible solutions. The downside of it is
> that we cannot pass 64-bit types (like off_t) in single register.

Wrappers can be added for the cases where you'd like to do that.

> The other downside is that we clear top halves for every single
> syscall, and it looks excessive. So, from spark64 and s390 approaches
> we choosed second.

It's like 4 cpu cycles even on crappy sparc64 cpus which only dual
issue. :)

And that's a pretty low cost for the benefits if you ask me.

Re: [PATCH 18/23] arm64: ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it

[Arnd Bergmann]

On Tuesday, May 24, 2016 3:04:47 AM CEST Yury Norov wrote:
> +static unsigned long compat_sys_mmap2(compat_uptr_t addr, compat_size_t len,
> +       int prot, int flags, int fd, off_t pgoff)
> +{
> +       if (pgoff & (~PAGE_MASK >> 12))
> +               return -EINVAL;
> +
> +       return sys_mmap_pgoff(addr, len, prot, flags, fd,
> +                      pgoff >> (PAGE_SHIFT - 12));
> +}
> +
> +static unsigned long compat_sys_pread64(unsigned int fd,
> +               compat_uptr_t __user *ubuf, compat_size_t count, off_t offset)
> +{
> +       return sys_pread64(fd, (char *) ubuf, count, offset);
> +}
> +
> +static unsigned long compat_sys_pwrite64(unsigned int fd,
> +               compat_uptr_t __user *ubuf, compat_size_t count, off_t offset)
> +{
> +       return sys_pwrite64(fd, (char *) ubuf, count, offset);
> +}
> 

The use of compat_uptr_t seems inconsistent here, and neither of the two
ways of doing it is what we do elsewhere. compat_uptr_t is meant to
be a scalar 32-bit type that gets converted into a pointer using the
compat_ptr() macro, so compat_sys_mmap2 should not use compat_ptr_t
(we don't access it as a pointer in mmap_pgoff) but compat_ulong_t,
and compat_sys_pread64() should have a compat_uptr_t argument, not
pointer to compat_uptr_t.

	Arnd

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Arnd Bergmann]

On Wednesday, May 25, 2016 1:21:45 PM CEST David Miller wrote:
> From: Yury Norov <ynorov@caviumnetworks.com>
> Date: Wed, 25 May 2016 23:03:27 +0300
> 
> > On Wed, May 25, 2016 at 12:30:17PM -0700, David Miller wrote:
> >> From: Yury Norov <ynorov@caviumnetworks.com>
> >> Date: Tue, 24 May 2016 03:04:30 +0300
> >> 
> >> > +To clear that top halves, automatic wrappers are introduced. They clear all
> >> > +required registers before passing control to regular syscall handler.
> >> 
> >> Why have one of these for every single compat system call, rather than
> >> simply clearing the top half of all of these registers unconditionally
> >> in the 32-bit system call trap before the system call is invoked?
> >> 
> >> That's what we do on sparc64.
> >> 
> >> And with that, you only need wrappers for the case where there needs
> >> to be proper sign extention of a 32-bit signed argument.
> > 
> > It was discussed as one of possible solutions. The downside of it is
> > that we cannot pass 64-bit types (like off_t) in single register.
> 
> Wrappers can be added for the cases where you'd like to do that.

If we clear the upper halves on the initial entry, we can't use a wrapper
to restore them, so would have to instead pass them as register
pairs as we do on the other 32-bit architectures.

> > The other downside is that we clear top halves for every single
> > syscall, and it looks excessive. So, from spark64 and s390 approaches
> > we choosed second.
> 
> It's like 4 cpu cycles even on crappy sparc64 cpus which only dual
> issue. :)
> 
> And that's a pretty low cost for the benefits if you ask me.

To clarify what we are talking about: These syscalls that normally
pass 64-bit arguments as register pairs are intentionally overridden
to make them faster on ilp32 mode compare to other compat modes:

+#define compat_sys_fadvise64_64        sys_fadvise64_64
+#define compat_sys_fallocate           sys_fallocate
+#define compat_sys_ftruncate64         sys_ftruncate
+#define compat_sys_lookup_dcookie      sys_lookup_dcookie
+#define compat_sys_readahead           sys_readahead
+#define compat_sys_sync_file_range     sys_sync_file_range
+#define compat_sys_truncate64          sys_truncate
+#define sys_llseek                     sys_lseek
+static unsigned long compat_sys_pread64(unsigned int fd,
+               compat_uptr_t __user *ubuf, compat_size_t count, off_t offset)
+{
+       return sys_pread64(fd, (char *) ubuf, count, offset);
+}
+
+static unsigned long compat_sys_pwrite64(unsigned int fd,
+               compat_uptr_t __user *ubuf, compat_size_t count, off_t offset)
+{
+       return sys_pwrite64(fd, (char *) ubuf, count, offset);
+}

If we use the normal calling conventions, we could remove these overrides
along with the respective special-case handling in glibc. None of them
look particularly performance-sensitive, but I could be wrong there.

	Arnd

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 25 May 2016 22:47:33 +0200

> If we use the normal calling conventions, we could remove these overrides
> along with the respective special-case handling in glibc. None of them
> look particularly performance-sensitive, but I could be wrong there.

You could set the lowest bit in the system call entry pointer to indicate
the upper-half clears should be elided.

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Arnd Bergmann]

On Wednesday, May 25, 2016 1:50:39 PM CEST David Miller wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed, 25 May 2016 22:47:33 +0200
> 
> > If we use the normal calling conventions, we could remove these overrides
> > along with the respective special-case handling in glibc. None of them
> > look particularly performance-sensitive, but I could be wrong there.
> 
> You could set the lowest bit in the system call entry pointer to indicate
> the upper-half clears should be elided.

Right, but that would introduce an extra conditional branch in the syscall
hotpath, and likely eliminate the gains from passing the loff_t arguments
in a single register instead of a pair.

	Arnd

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 25 May 2016 23:01:06 +0200

> On Wednesday, May 25, 2016 1:50:39 PM CEST David Miller wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> Date: Wed, 25 May 2016 22:47:33 +0200
>> 
>> > If we use the normal calling conventions, we could remove these overrides
>> > along with the respective special-case handling in glibc. None of them
>> > look particularly performance-sensitive, but I could be wrong there.
>> 
>> You could set the lowest bit in the system call entry pointer to indicate
>> the upper-half clears should be elided.
> 
> Right, but that would introduce an extra conditional branch in the syscall
> hotpath, and likely eliminate the gains from passing the loff_t arguments
> in a single register instead of a pair.

Ok, then, how much are you really gaining from avoiding a 'shift' and
an 'or' to build the full 64-bit value?  3 cycles?  Maybe 4?

And the executing the wrappers, those have a non-trivial cost too.

Cost wise, this seems like it all cancels out in the end, but what
do I know?

Re: [PATCH 16/23] arm64: ilp32: introduce binfmt_ilp32.c

[Zhangjian (Bamvor)]

Hi, yury

The coredump is usable in our platform. It miss the following definition:
+#define compat_elf_greg_t	elf_greg_t
+#define compat_elf_gregset_t	elf_gregset_t

And it leads to the wrong register save in core dump. After apply this patch,
gdb could debug core dump files.

Here is the full patch:
 From 102624840aa5dacdd1bbfe3b390290f52f530ea2 Mon Sep 17 00:00:00 2001
From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
Date: Thu, 26 May 2016 21:00:16 +0800
Subject: [PATCH hulk-4.1-next] arm64: ilp32: fix coredump issue

ILP32 use aarch64 register and 32bit signal struct which means it
could not make use of the existing compat_elf_prstatus/elf_prstatus
and compat_elf_prpsinfo/elf_prpsinfo.

This patch fix this issue by introducing the different
compat_elf_greg_t, compat_elf_gregset_t for aarch64 ilp32 and aarch32
el0.

Tested pass on huawei's hardware in bigendian.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
---
  arch/arm64/include/asm/elf.h     | 14 +++++++-------
  arch/arm64/kernel/binfmt_elf32.c |  3 +++
  arch/arm64/kernel/binfmt_ilp32.c |  8 +++++++-
  arch/arm64/kernel/ptrace.c       | 20 ++++++++++----------
  4 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index 0106d18..9019441 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -154,18 +154,18 @@ extern int arch_setup_additional_pages(struct linux_binprm *bprm,
  				       int uses_interp);

  /* 1GB of VA */
-#define STACK_RND_MASK			(is_compat_task() ? \
-						0x7ff >> (PAGE_SHIFT - 12) : \
-						0x3ffff >> (PAGE_SHIFT - 12))
+#define STACK_RND_MASK		(is_compat_task() ? \
+					0x7ff >> (PAGE_SHIFT - 12) : \
+					0x3ffff >> (PAGE_SHIFT - 12))

  #ifdef CONFIG_COMPAT

-#define COMPAT_ELF_ET_DYN_BASE		(2 * TASK_SIZE_32 / 3)
+#define COMPAT_ELF_ET_DYN_BASE	(2 * TASK_SIZE_32 / 3)

  /* AArch32 registers. */
-#define COMPAT_ELF_NGREG		18
-typedef unsigned int			compat_elf_greg_t;
-typedef compat_elf_greg_t		compat_elf_gregset_t[COMPAT_ELF_NGREG];
+#define COMPAT_ELF_NGREG	18
+typedef unsigned int		compat_a32_elf_greg_t;
+typedef compat_a32_elf_greg_t	compat_a32_elf_gregset_t[COMPAT_ELF_NGREG];

  #endif /* CONFIG_COMPAT */

diff --git a/arch/arm64/kernel/binfmt_elf32.c b/arch/arm64/kernel/binfmt_elf32.c
index 7b9b445..f75253c 100644
--- a/arch/arm64/kernel/binfmt_elf32.c
+++ b/arch/arm64/kernel/binfmt_elf32.c
@@ -31,4 +31,7 @@ struct linux_binprm;
  extern int aarch32_setup_vectors_page(struct linux_binprm *bprm,
  				      int uses_interp);

+#define compat_elf_greg_t	compat_a32_elf_greg_t
+#define compat_elf_gregset_t	compat_a32_elf_gregset_t
+
  #include "../../../fs/compat_binfmt_elf.c"
diff --git a/arch/arm64/kernel/binfmt_ilp32.c b/arch/arm64/kernel/binfmt_ilp32.c
index b827a9a..01baf83 100644
--- a/arch/arm64/kernel/binfmt_ilp32.c
+++ b/arch/arm64/kernel/binfmt_ilp32.c
@@ -2,7 +2,9 @@
   * Support for ILP32 Linux/aarch64 ELF binaries.
   */

-#include <linux/elfcore-compat.h>
+#include <linux/elf.h>
+#include <linux/elfcore.h>
+#include <linux/compat.h>
  #include <linux/time.h>

  #undef	ELF_CLASS
@@ -30,9 +32,13 @@
   * The machine-dependent core note format types are defined in elfcore-compat.h,
   * which requires asm/elf.h to define compat_elf_gregset_t et al.
   */
+#define compat_elf_greg_t	elf_greg_t
+#define compat_elf_gregset_t	elf_gregset_t
  #define elf_prstatus	compat_elf_prstatus
  #define elf_prpsinfo	compat_elf_prpsinfo

+#include <linux/elfcore-compat.h>
+
  /*
   * Compat version of cputime_to_compat_timeval, perhaps this
   * should be an inline in <linux/compat.h>.
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 5c86135..9784c77 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -913,8 +913,8 @@ static const struct user_regset aarch32_regsets[] = {
  	[REGSET_COMPAT_GPR] = {
  		.core_note_type = NT_PRSTATUS,
  		.n = COMPAT_ELF_NGREG,
-		.size = sizeof(compat_elf_greg_t),
-		.align = sizeof(compat_elf_greg_t),
+		.size = sizeof(compat_a32_elf_greg_t),
+		.align = sizeof(compat_a32_elf_greg_t),
  		.get = compat_gpr_get,
  		.set = compat_gpr_set
  	},
@@ -947,7 +947,7 @@ static int compat_ptrace_read_user(struct task_struct *tsk, compat_ulong_t off,
  		tmp = tsk->mm->start_data;
  	else if (off == COMPAT_PT_TEXT_END_ADDR)
  		tmp = tsk->mm->end_code;
-	else if (off < sizeof(compat_elf_gregset_t))
+	else if (off < sizeof(compat_a32_elf_gregset_t))
  		return copy_regset_to_user(tsk, &user_aarch32_view,
  					   REGSET_COMPAT_GPR, off,
  					   sizeof(compat_ulong_t), ret);
@@ -968,7 +968,7 @@ static int compat_ptrace_write_user(struct task_struct *tsk, compat_ulong_t off,
  	if (off & 3 || off >= COMPAT_USER_SZ)
  		return -EIO;

-	if (off >= sizeof(compat_elf_gregset_t))
+	if (off >= sizeof(compat_a32_elf_gregset_t))
  		return 0;

  	set_fs(KERNEL_DS);
@@ -1116,9 +1116,11 @@ static long compat_a32_ptrace(struct task_struct *child, compat_long_t request,
  	unsigned long addr = caddr;
  	unsigned long data = cdata;
  	void __user *datap = compat_ptr(data);
+	unsigned int pr_reg_size = sizeof(compat_a32_elf_gregset_t);
  	int ret;

  	switch (request) {
+
  		case PTRACE_PEEKUSR:
  			ret = compat_ptrace_read_user(child, addr, datap);
  			break;
@@ -1130,17 +1132,15 @@ static long compat_a32_ptrace(struct task_struct *child, compat_long_t request,
  		case COMPAT_PTRACE_GETREGS:
  			ret = copy_regset_to_user(child,
  						  &user_aarch32_view,
-						  REGSET_COMPAT_GPR,
-						  0, sizeof(compat_elf_gregset_t),
-						  datap);
+						  REGSET_COMPAT_GPR, 0,
+						  pr_reg_size, datap);
  			break;

  		case COMPAT_PTRACE_SETREGS:
  			ret = copy_regset_from_user(child,
  						    &user_aarch32_view,
-						    REGSET_COMPAT_GPR,
-						    0, sizeof(compat_elf_gregset_t),
-						    datap);
+						    REGSET_COMPAT_GPR, 0,
+						    pr_reg_size, datap);
  			break;

  		case COMPAT_PTRACE_GET_THREAD_AREA:
-- 
1.8.4.5


On 2016/5/24 8:04, Yury Norov wrote:
> to handle ILP32 binaries
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
>   arch/arm64/kernel/Makefile       |  1 +
>   arch/arm64/kernel/binfmt_ilp32.c | 91 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 92 insertions(+)
>   create mode 100644 arch/arm64/kernel/binfmt_ilp32.c
>
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 6bc9738..9dfdf86 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -28,6 +28,7 @@ $(obj)/%.stub.o: $(obj)/%.o FORCE
>   arm64-obj-$(CONFIG_AARCH32_EL0)		+= sys32.o kuser32.o signal32.o 	\
>   					   sys_compat.o entry32.o		\
>   					   ../../arm/kernel/opcodes.o binfmt_elf32.o
> +arm64-obj-$(CONFIG_ARM64_ILP32)		+= binfmt_ilp32.o
>   arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>   arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
>   arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
> diff --git a/arch/arm64/kernel/binfmt_ilp32.c b/arch/arm64/kernel/binfmt_ilp32.c
> new file mode 100644
> index 0000000..a934fd4
> --- /dev/null
> +++ b/arch/arm64/kernel/binfmt_ilp32.c
> @@ -0,0 +1,91 @@
> +/*
> + * Support for ILP32 Linux/aarch64 ELF binaries.
> + */
> +
> +#include <linux/elfcore-compat.h>
> +#include <linux/time.h>
> +
> +#undef	ELF_CLASS
> +#define ELF_CLASS	ELFCLASS32
> +
> +#undef	elfhdr
> +#undef	elf_phdr
> +#undef	elf_shdr
> +#undef	elf_note
> +#undef	elf_addr_t
> +#define elfhdr		elf32_hdr
> +#define elf_phdr	elf32_phdr
> +#define elf_shdr	elf32_shdr
> +#define elf_note	elf32_note
> +#define elf_addr_t	Elf32_Addr
> +
> +/*
> + * Some data types as stored in coredump.
> + */
> +#define user_long_t		compat_long_t
> +#define user_siginfo_t		compat_siginfo_t
> +#define copy_siginfo_to_user	copy_siginfo_to_user32
> +
> +/*
> + * The machine-dependent core note format types are defined in elfcore-compat.h,
> + * which requires asm/elf.h to define compat_elf_gregset_t et al.
> + */
> +#define elf_prstatus	compat_elf_prstatus
> +#define elf_prpsinfo	compat_elf_prpsinfo
> +
> +/*
> + * Compat version of cputime_to_compat_timeval, perhaps this
> + * should be an inline in <linux/compat.h>.
> + */
> +static void cputime_to_compat_timeval(const cputime_t cputime,
> +				      struct compat_timeval *value)
> +{
> +	struct timeval tv;
> +	cputime_to_timeval(cputime, &tv);
> +	value->tv_sec = tv.tv_sec;
> +	value->tv_usec = tv.tv_usec;
> +}
> +
> +#undef cputime_to_timeval
> +#define cputime_to_timeval cputime_to_compat_timeval
> +
> +/* AARCH64 ILP32 EABI. */
> +#undef elf_check_arch
> +#define elf_check_arch(x)		(((x)->e_machine == EM_AARCH64)	\
> +					&& (x)->e_ident[EI_CLASS] == ELFCLASS32)
> +
> +#undef SET_PERSONALITY
> +#define SET_PERSONALITY(ex)						\
> +do {									\
> +	set_thread_flag(TIF_32BIT_AARCH64);				\
> +	clear_thread_flag(TIF_32BIT);					\
> +} while (0)
> +
> +#undef ARCH_DLINFO
> +#define ARCH_DLINFO							\
> +do {									\
> +	NEW_AUX_ENT(AT_SYSINFO_EHDR,					\
> +		    (elf_addr_t)(long)current->mm->context.vdso);	\
> +} while (0)
> +
> +#ifdef __AARCH64EB__
> +#define COMPAT_ELF_PLATFORM		("aarch64_be:ilp32")
> +#else
> +#define COMPAT_ELF_PLATFORM		("aarch64:ilp32")
> +#endif
> +
> +#undef ELF_HWCAP
> +#undef ELF_HWCAP2
> +#define ELF_HWCAP			((u32) elf_hwcap)
> +#define ELF_HWCAP2			((u32) (elf_hwcap >> 32))
> +
> +/*
> + * Rename a few of the symbols that binfmt_elf.c will define.
> + * These are all local so the names don't really matter, but it
> + * might make some debugging less confusing not to duplicate them.
> + */
> +#define elf_format		compat_elf_format
> +#define init_elf_binfmt		init_compat_elf_binfmt
> +#define exit_elf_binfmt		exit_compat_elf_binfmt
> +
> +#include "../../../fs/binfmt_elf.c"
>

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Szabolcs Nagy]

On 26/05/16 15:20, Catalin Marinas wrote:
> While writing the above, I realised the current ILP32 patches still miss
> on converting pointers passed from user space (unless I got myself
> confused in macros). The new __SC_WRAP() and COMPAT_SYSCALL_WRAPx()
> macros take care of zero or sign extension via __SC_COMPAT_CAST().
> However, we have two more existing cases which I don't see covered:
> 
> a) Native syscalls taking a pointer argument and invoked directly from
>    ILP32. For example, sys_read() takes a pointer but I don't see any
>    __SC_WRAP added by patch 5
> 
> b) Current compat syscalls taking a pointer argument. For example,
>    compat_sys_vmsplice() gets the iov32 pointer and the compiler assumes
>    it is a 64-bit variable. I don't see where the upper half is zeroed
> 

on x32 sign/zero extension is currently left to userspace,
which is difficult to deal with, (long long)arg does the
wrong thing for pointer args.

> We can solve (a) by adding more __SC_WRAP annotations in the generic
> unistd.h. For (b), we would need an __SC_DELOUSE with a bit of penalty
> on AArch32/compat support where it isn't needed. So maybe davem has a
> point on the overall impact of always zeroing the upper half of the
> arguments ;) (both from a performance and maintainability perspective).
> I guess this part of the ABI is still up for discussion.
> 

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[David Miller]

From: Catalin Marinas <catalin.marinas@arm.com>
Date: Thu, 26 May 2016 15:20:58 +0100

> We can solve (a) by adding more __SC_WRAP annotations in the generic
> unistd.h.
 ...

I really think it's much more robust to clear the tops of the registers
by default.  Then you won't be auditing constantly and adding more and
more wrappers.

You can't even quantify the performance gains for me in any precise
way.  Whatever you gain by avoiding the 64-bit
decompostion/reconstitution for those few system calls with 64-bit
registers, you are losing by calling the wrappers for more common
system calls, more often.

"it's more natural to pass 64-bit values in a register" is not a clear
justification for this change.

This looks way over engineered to me.

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Arnd Bergmann]

On Friday, May 27, 2016 8:03:57 AM CEST Heiko Carstens wrote:
> > > > > Cost wise, this seems like it all cancels out in the end, but what
> > > > > do I know?
> > > > 
> > > > I think you know something, and I also think Heiko and other s390 guys
> > > > know something as well. So I'd like to listen their arguments here.
> 
> If it comes to 64 bit arguments for compat system calls: s390 also has an
> x32-like ABI extension which allows user space to use full 64 bit
> registers. As far as I know hardly anybody ever made use of that.
> 
> However even if that would be widely used, to me it wouldn't make sense to
> add new compat system calls which allow 64 bit arguments, simply because
> something like
> 
> c = (u32)a | (u64)b << 32;
> 
> can be done with a single 1-cycle instruction. It's just not worth the
> extra effort to maintain additional system call variants.

For reference, both tile and mips also have separate 32-bit ABIs that are
only used on 64-bit kernels (aside from the normal 32-bit ABI). Tile
does it like s390 and passes 64-bit arguments as pairs, while MIPS
and x86 and pass them as single registers.

Tile is very similar to arm64 because it also uses the generic system
call table, which I think is a good argument to keep them in sync.

	Arnd

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Arnd Bergmann]

On Friday, May 27, 2016 10:30:52 AM CEST Catalin Marinas wrote:
> On Fri, May 27, 2016 at 10:42:59AM +0200, Arnd Bergmann wrote:
> > On Friday, May 27, 2016 8:03:57 AM CEST Heiko Carstens wrote:
> > > > > > > Cost wise, this seems like it all cancels out in the end, but what
> > > > > > > do I know?
> > > > > > 
> > > > > > I think you know something, and I also think Heiko and other s390 guys
> > > > > > know something as well. So I'd like to listen their arguments here.
> > > 
> > > If it comes to 64 bit arguments for compat system calls: s390 also has an
> > > x32-like ABI extension which allows user space to use full 64 bit
> > > registers. As far as I know hardly anybody ever made use of that.
> > > 
> > > However even if that would be widely used, to me it wouldn't make sense to
> > > add new compat system calls which allow 64 bit arguments, simply because
> > > something like
> > > 
> > > c = (u32)a | (u64)b << 32;
> > > 
> > > can be done with a single 1-cycle instruction. It's just not worth the
> > > extra effort to maintain additional system call variants.
> > 
> > For reference, both tile and mips also have separate 32-bit ABIs that are
> > only used on 64-bit kernels (aside from the normal 32-bit ABI). Tile
> > does it like s390 and passes 64-bit arguments as pairs, while MIPS
> > and x86 and pass them as single registers.
> 
> AFAIK, x32 also requires that the upper half of a 64-bit reg is zeroed
> by the user when a 32-bit value is passed. We could require the same on
> AArch64/ILP32 but I'm a bit uneasy on trusting a multitude of C
> libraries on this.

It's not about trusting a C library, it's about ensuring malicious code
cannot pass argumentst that the kernel code assumes will never happen.

	Arnd

Re: [PATCH v6 00/21] ILP32 for ARM64

[Szabolcs Nagy]

On 02/06/16 20:03, Yury Norov wrote:
> On Tue, May 24, 2016 at 03:04:29AM +0300, Yury Norov wrote:
>> ILP32 glibc branch is available here:
>> https://github.com/norov/glibc/tree/ilp32-2.23
>>
> 
> So for AARCH64/ILP32 we turn next types to 64-bit in glibc:
> #define __INO_T_TYPE            __UQUAD_TYPE
> #define __OFF_T_TYPE            __SQUAD_TYPE
> #define __BLKCNT_T_TYPE         __SQUAD_TYPE
> #define __FSBLKCNT_T_TYPE       __UQUAD_TYPE
> #define __FSFILCNT_T_TYPE       __UQUAD_TYPE
> 
> And define:
> # define __INO_T_MATCHES_INO64_T                1
> # define __OFF_T_MATCHES_OFF64_T                1
> # define __BLKCNT_T_MATCHES_BLKCNT64_T          1
> # define __FSBLKCNT_T_MATCHES_FSBLKCNT64_T      1
> # define __FSFILCNT_T_MATCHES_FSFILCNT_T        1
> 
> If so, stat and statfs  structures for ilp32 are turning the same as
> for lp64. And so we'd handle related syscalls with native lp64
> handlers (wrapped, to zero top halves) in kernel. 
> 
> And we don't need stat64 for ilp32.
> 
> Did I miss something? Is everything correct?
> 
> OFF_T is turned to 64-bit quite smoothly, others make applications
> crash with segfault. Now I'm in deep debugging.
> 

based on previous discussions, non-trivial glibc changes may
be needed to make 64bit fs apis the default on a 32bit abi.
http://sourceware.org/ml/libc-alpha/2016-05/msg00337.html
http://sourceware.org/ml/libc-alpha/2016-05/msg00356.html

but i guess if you consistently fix the 32bit assumptions
then it should work.

> https://github.com/norov/glibc/commits/ilp32-dev
> https://github.com/norov/linux/commits/ilp32
> 
> Yury.
> 

Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext

[Zhangjian (Bamvor)]

Hi,

I found an issue of unwind with the following code. The correct backtrace
should be:
(gdb) where
#0 0x004004d0 in my_sig (sig=11) at test_force3.c:16
#1 <signal handler called>
#2 func2 (num=0) at test_force3.c:22
#3 0x00400540 in func1 (num=1) at test_force3.c:28
#4 0x00400574 in main (argc=1, argv=0xffd7bc04) at test_force3.c:33

Without my patch, the backtrace is:
(gdb) where
#0 0x00400490 in my_sig (sig=11) at test_force3.c:16
#1 <signal handler called>
#2 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33

With my patch which fix the wrong frame pointer(setup_return calculate the offset
of fp through ilp32_sigframe instead of sigfreame), the backtrace is:
(gdb) where
#0 0x00400490 in my_sig (sig=11) at test_force3.c:16
#1 <signal handler called>
#2 func1 () at test_force3.c:28
#3 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33

I am not sure there is still some issue in kernel. But it seem that the gdb of ilp32
does not work correctly when unwind without framepointer.

The test code is:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <signal.h>

void my_sig(int sig)
{
	printf("sig=%d\n", sig);
	*(int *)0 = 0x0;
}


void func2()
{
	*(int *)0 = 0x0;
}

void func1()
{
	func2();
}

int main(int argc, char **argv)
{
	signal(11, my_sig);
	func1();
	return 0;
}


The full patch is as follows:

 From 7e364a765097f57aed2d73f94c1688c2e7343e79 Mon Sep 17 00:00:00 2001
From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
Date: Sat, 4 Jun 2016 14:30:05 +0800
Subject: [PATCH] arm64: ilp32: fix for wrong fp offset when calculate the
  new fp

ILP32 define its own sigframe(ilp32_sigframe) because of the
difference uc_context. setup_return do not use ilp32 specific
sigframe to calculate the new offset of fp which lead to wrong
fp in signal handler. At this circumstance, gdb backtrace will miss
one item:
(gdb) where

It should be:
(gdb) where

The test code is as follows:

void my_sig(int sig)
{
         printf("sig=%d\n", sig);
         *(int *)0 = 0x0;
}

void func2(int num)
{
         printf("%s: %d\n", __FUNCTION__, num);
         *(int *)0 = 0x0;
         func2(num-1);
}

void func1(int num)
{
         printf("%s\n", __FUNCTION__);
         func2(num - 1);
}

int main(int argc, char **argv)
{
         signal(11, my_sig);
         func1(argc);
         return 0;
}

This patch fix this by passing the correct offset of fp to
setup_return.
Test pass on both ILP32 and LP64 in aarch64 EE.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
---
  arch/arm64/include/asm/signal_common.h | 3 ++-
  arch/arm64/kernel/signal.c             | 9 +++++----
  arch/arm64/kernel/signal_ilp32.c       | 4 ++--
  3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/signal_common.h b/arch/arm64/include/asm/signal_common.h
index de93c71..a5d7b63 100644
--- a/arch/arm64/include/asm/signal_common.h
+++ b/arch/arm64/include/asm/signal_common.h
@@ -29,6 +29,7 @@ int setup_sigcontex(struct sigcontext __user *uc_mcontext,
  		    struct pt_regs *regs);
  int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sf);
  void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
-			void __user *frame, off_t sigframe_off, int usig);
+			void __user *frame, off_t sigframe_off, off_t fp_off,
+			int usig);

  #endif /* __ASM_SIGNAL_COMMON_H */
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 038bebe..e66a6e9 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -256,14 +256,14 @@ static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig,
  }

  void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
-			 void __user *frame, off_t sigframe_off, int usig)
+			 void __user *frame, off_t sigframe_off, off_t fp_off,
+			 int usig)
  {
  	__sigrestore_t sigtramp;

  	regs->regs[0] = usig;
  	regs->sp = (unsigned long)frame;
-	regs->regs[29] = regs->sp + sigframe_off +
-		offsetof(struct sigframe, fp);
+	regs->regs[29] = regs->sp + sigframe_off + fp_off;
  	regs->pc = (unsigned long)ka->sa.sa_handler;

  	if (ka->sa.sa_flags & SA_RESTORER)
@@ -294,7 +294,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
  	err |= setup_sigframe(&frame->sig, regs, set);
  	if (err == 0) {
  		setup_return(regs, &ksig->ka, frame,
-			offsetof(struct rt_sigframe, sig), usig);
+			offsetof(struct rt_sigframe, sig),
+			offsetof(struct sigframe, fp), usig);
  		if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
  			err |= copy_siginfo_to_user(&frame->info, &ksig->info);
  			regs->regs[1] = (unsigned long)&frame->info;
diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
index a8ea73e..9030f14 100644
--- a/arch/arm64/kernel/signal_ilp32.c
+++ b/arch/arm64/kernel/signal_ilp32.c
@@ -147,7 +147,6 @@ static struct ilp32_rt_sigframe __user *ilp32_get_sigframe(struct ksignal *ksig,
  	struct ilp32_rt_sigframe __user *frame;

  	sp = sp_top = sigsp(regs->sp, ksig);
-
  	sp = (sp - sizeof(struct ilp32_rt_sigframe)) & ~15;
  	frame = (struct ilp32_rt_sigframe __user *)sp;

@@ -183,7 +182,8 @@ int ilp32_setup_rt_frame(int usig, struct ksignal *ksig,
  	err |= setup_ilp32_sigframe(&frame->sig, regs, set);
  	if (err == 0) {
  		setup_return(regs, &ksig->ka, frame,
-			     offsetof(struct ilp32_rt_sigframe, sig), usig);
+			     offsetof(struct ilp32_rt_sigframe, sig),
+			     offsetof(struct ilp32_sigframe, fp), usig);
  		regs->regs[1] = (unsigned long)&frame->info;
  		regs->regs[2] = (unsigned long)&frame->sig.uc;
  	}
-- 
1.8.4.5

Regards

Bamvor

Re: [PATCH 17/23] arm64: ptrace: handle ptrace_request differently for aarch32 and ilp32

[Yury Norov]

On Wed, Jun 08, 2016 at 09:34:09AM +0800, zhouchengming wrote:
> On 2016/5/24 8:04, Yury Norov wrote:
> >Here new aarch32 ptrace syscall handler is introsuced to avoid run-time
> >detection of the task type.
> >
> >Signed-off-by: Yury Norov<ynorov@caviumnetworks.com>

[...]

> Hello, I found ilp32 will use sys_ptrace, not compat_sys_ptrace. So I write
> a little patch to see if can solve the problem correctly.
> 
> Thanks.
> 
> From f6156236df578bb05c4a17e7f9776ceaf8f7afe6 Mon Sep 17 00:00:00 2001
> From: Zhou Chengming <zhouchengming1@huawei.com>
> Date: Wed, 8 Jun 2016 09:46:23 +0800
> Subject: [PATCH] ilp32: use compat_sys_ptrace instead of sys_ptrace
> 
> When we analyze a testcase of ptrace that failed on ilp32, we found
> the syscall that the ilp32 uses is sys_ptrace, not compat_sys_ptrace.
> Because in include/uapi/asm-generic/unistd.h it's defined like:
> __SYSCALL(__NR_ptrace, sys_ptrace)
> So we change it to __SC_COMP(__NR_ptrace, sys_ptrace, compat_sys_ptrace),
> let compat tasks use the compat_sys_ptrace.
> 
> Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
> ---
>  include/uapi/asm-generic/unistd.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/asm-generic/unistd.h
> b/include/uapi/asm-generic/unistd.h
> index 2862d2e..50ee770 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -364,7 +364,7 @@ __SC_WRAP(__NR_syslog, sys_syslog)
> 
>  /* kernel/ptrace.c */
>  #define __NR_ptrace 117
> -__SYSCALL(__NR_ptrace, sys_ptrace)
> +__SC_COMP(__NR_ptrace, sys_ptrace, compat_sys_ptrace)
> 
>  /* kernel/sched/core.c */
>  #define __NR_sched_setparam 118
> -- 
> 1.7.7
> 

Hi Zhou,

Thank you for the catch.

Could you also show the test that is failed for you. It should
probably be sent to LTP maillist.

I'm not sure your fix correct as it affects other architectures that
use standard unistd.h. I think it's better to redirect the syscall in
arch/arm64/kernel/sys_ilp32.c with corresponding definition.

Yury

Re: [PATCH 13/23] arm64: introduce is_a32_task and is_a32_thread (for AArch32 compat)

[Zhangjian (Bamvor)]

Hi, Yury

On 2016/5/24 8:04, Yury Norov wrote:
> Based on patch of Andrew Pinski.
>
> This patch introduces is_a32_compat_task and is_a32_thread so it is
> easier to say this is a a32 specific thread or a generic compat thread/task.
> Corresponding functions are located in <asm/is_compat.h> to avoid mess in
> headers.
>
> Some files include both <linux/compat.h> and <asm/compat.h>,
> and this is wrong because <linux/compat.h> has <asm/compat.h> already
> included. It was fixed too.
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Andrew Pinski <Andrew.Pinski@caviumnetworks.com>
> Reviewed-by: David Daney <ddaney@caviumnetworks.com>
> ---
>   arch/arm64/include/asm/compat.h      | 19 ++----------
>   arch/arm64/include/asm/elf.h         | 10 +++----
>   arch/arm64/include/asm/ftrace.h      |  2 +-
>   arch/arm64/include/asm/is_compat.h   | 58 ++++++++++++++++++++++++++++++++++++
>   arch/arm64/include/asm/memory.h      |  3 +-
>   arch/arm64/include/asm/processor.h   |  5 ++--
>   arch/arm64/include/asm/syscall.h     |  2 +-
>   arch/arm64/include/asm/thread_info.h |  2 +-
>   arch/arm64/kernel/hw_breakpoint.c    | 10 +++----
>   arch/arm64/kernel/perf_regs.c        |  2 +-
>   arch/arm64/kernel/process.c          |  7 ++---
>   arch/arm64/kernel/ptrace.c           | 11 ++++---
>   arch/arm64/kernel/signal.c           |  4 +--
>   arch/arm64/kernel/traps.c            |  3 +-
>   14 files changed, 91 insertions(+), 47 deletions(-)
>   create mode 100644 arch/arm64/include/asm/is_compat.h
>
[...]
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 12f8a00..a66a0f7 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -26,6 +26,7 @@
>   #include <linux/types.h>
>   #include <asm/bug.h>
>   #include <asm/sizes.h>
> +#include <asm/is_compat.h>
>
>   /*
>    * Allow for constants defined here to be used from assembly code
> @@ -61,7 +62,7 @@
>
>   #ifdef CONFIG_COMPAT
>   #define TASK_SIZE_32		UL(0x100000000)
> -#define TASK_SIZE		(test_thread_flag(TIF_32BIT) ? \
> +#define TASK_SIZE		(is_compat_task() ?		\
>   				TASK_SIZE_32 : TASK_SIZE_64)
>   #define TASK_SIZE_OF(tsk)	(test_tsk_thread_flag(tsk, TIF_32BIT) ? \
>   				TASK_SIZE_32 : TASK_SIZE_64)
Should we update or delete this macro?
  #define TASK_SIZE_OF(tsk) (is_compat_task() ? \
TASK_SIZE_32 : TASK_SIZE_64)

x86, sparc, mips, ppc, parisc, s390 define its own version. But
"include/linux/sched.h" will define it if
TASK_SIZE_OF does not exist:
#ifndef TASK_SIZE_OF
#define TASK_SIZE_OF(tsk)       TASK_SIZE
#endif


Regards

Bamvor

Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext

[Zhangjian (Bamvor)]

ping

On 2016/6/4 19:34, Zhangjian (Bamvor) wrote:
> Hi,
>
> I found an issue of unwind with the following code. The correct backtrace
> should be:
> (gdb) where
> #0 0x004004d0 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 func2 (num=0) at test_force3.c:22
> #3 0x00400540 in func1 (num=1) at test_force3.c:28
> #4 0x00400574 in main (argc=1, argv=0xffd7bc04) at test_force3.c:33
>
> Without my patch, the backtrace is:
> (gdb) where
> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
>
> With my patch which fix the wrong frame pointer(setup_return calculate the offset
> of fp through ilp32_sigframe instead of sigfreame), the backtrace is:
> (gdb) where
> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 func1 () at test_force3.c:28
> #3 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
>
> I am not sure there is still some issue in kernel. But it seem that the gdb of ilp32
> does not work correctly when unwind without framepointer.
>
> The test code is:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <signal.h>
>
> void my_sig(int sig)
> {
>      printf("sig=%d\n", sig);
>      *(int *)0 = 0x0;
> }
>
>
> void func2()
> {
>      *(int *)0 = 0x0;
> }
>
> void func1()
> {
>      func2();
> }
>
> int main(int argc, char **argv)
> {
>      signal(11, my_sig);
>      func1();
>      return 0;
> }
>
>
> The full patch is as follows:
>
>  From 7e364a765097f57aed2d73f94c1688c2e7343e79 Mon Sep 17 00:00:00 2001
> From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> Date: Sat, 4 Jun 2016 14:30:05 +0800
> Subject: [PATCH] arm64: ilp32: fix for wrong fp offset when calculate the
>   new fp
>
> ILP32 define its own sigframe(ilp32_sigframe) because of the
> difference uc_context. setup_return do not use ilp32 specific
> sigframe to calculate the new offset of fp which lead to wrong
> fp in signal handler. At this circumstance, gdb backtrace will miss
> one item:
> (gdb) where
>
> It should be:
> (gdb) where
>
> The test code is as follows:
>
> void my_sig(int sig)
> {
>          printf("sig=%d\n", sig);
>          *(int *)0 = 0x0;
> }
>
> void func2(int num)
> {
>          printf("%s: %d\n", __FUNCTION__, num);
>          *(int *)0 = 0x0;
>          func2(num-1);
> }
>
> void func1(int num)
> {
>          printf("%s\n", __FUNCTION__);
>          func2(num - 1);
> }
>
> int main(int argc, char **argv)
> {
>          signal(11, my_sig);
>          func1(argc);
>          return 0;
> }
>
> This patch fix this by passing the correct offset of fp to
> setup_return.
> Test pass on both ILP32 and LP64 in aarch64 EE.
>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> ---
>   arch/arm64/include/asm/signal_common.h | 3 ++-
>   arch/arm64/kernel/signal.c             | 9 +++++----
>   arch/arm64/kernel/signal_ilp32.c       | 4 ++--
>   3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/signal_common.h b/arch/arm64/include/asm/signal_common.h
> index de93c71..a5d7b63 100644
> --- a/arch/arm64/include/asm/signal_common.h
> +++ b/arch/arm64/include/asm/signal_common.h
> @@ -29,6 +29,7 @@ int setup_sigcontex(struct sigcontext __user *uc_mcontext,
>               struct pt_regs *regs);
>   int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sf);
>   void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> -            void __user *frame, off_t sigframe_off, int usig);
> +            void __user *frame, off_t sigframe_off, off_t fp_off,
> +            int usig);
>
>   #endif /* __ASM_SIGNAL_COMMON_H */
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 038bebe..e66a6e9 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -256,14 +256,14 @@ static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig,
>   }
>
>   void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> -             void __user *frame, off_t sigframe_off, int usig)
> +             void __user *frame, off_t sigframe_off, off_t fp_off,
> +             int usig)
>   {
>       __sigrestore_t sigtramp;
>
>       regs->regs[0] = usig;
>       regs->sp = (unsigned long)frame;
> -    regs->regs[29] = regs->sp + sigframe_off +
> -        offsetof(struct sigframe, fp);
> +    regs->regs[29] = regs->sp + sigframe_off + fp_off;
>       regs->pc = (unsigned long)ka->sa.sa_handler;
>
>       if (ka->sa.sa_flags & SA_RESTORER)
> @@ -294,7 +294,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>       err |= setup_sigframe(&frame->sig, regs, set);
>       if (err == 0) {
>           setup_return(regs, &ksig->ka, frame,
> -            offsetof(struct rt_sigframe, sig), usig);
> +            offsetof(struct rt_sigframe, sig),
> +            offsetof(struct sigframe, fp), usig);
>           if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
>               err |= copy_siginfo_to_user(&frame->info, &ksig->info);
>               regs->regs[1] = (unsigned long)&frame->info;
> diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
> index a8ea73e..9030f14 100644
> --- a/arch/arm64/kernel/signal_ilp32.c
> +++ b/arch/arm64/kernel/signal_ilp32.c
> @@ -147,7 +147,6 @@ static struct ilp32_rt_sigframe __user *ilp32_get_sigframe(struct ksignal *ksig,
>       struct ilp32_rt_sigframe __user *frame;
>
>       sp = sp_top = sigsp(regs->sp, ksig);
> -
>       sp = (sp - sizeof(struct ilp32_rt_sigframe)) & ~15;
>       frame = (struct ilp32_rt_sigframe __user *)sp;
>
> @@ -183,7 +182,8 @@ int ilp32_setup_rt_frame(int usig, struct ksignal *ksig,
>       err |= setup_ilp32_sigframe(&frame->sig, regs, set);
>       if (err == 0) {
>           setup_return(regs, &ksig->ka, frame,
> -                 offsetof(struct ilp32_rt_sigframe, sig), usig);
> +                 offsetof(struct ilp32_rt_sigframe, sig),
> +                 offsetof(struct ilp32_sigframe, fp), usig);
>           regs->regs[1] = (unsigned long)&frame->info;
>           regs->regs[2] = (unsigned long)&frame->sig.uc;
>       }

Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext

[Zhangjian (Bamvor)]

Hi, Yury


Here is another print issue in this patch:

On 2016/5/24 8:04, Yury Norov wrote:
> From: Andrew Pinski <apinski@cavium.com>
>
> ILP32 uses AARCH32 compat structures and syscall handlers for signals.
> But ILP32 struct rt_sigframe  and ucontext differs from both LP64 and
> AARCH32. So some specific mechanism is needed to take care of it.
>
[...]
> diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
> new file mode 100644
> index 0000000..841e8f8
> --- /dev/null
> +++ b/arch/arm64/kernel/signal_ilp32.c
> @@ -0,0 +1,192 @@
> +/*
[...]
> +asmlinkage long ilp32_sys_rt_sigreturn(struct pt_regs *regs)
> +{
> +	struct ilp32_rt_sigframe __user *frame;
> +
> +	/* Always make any pending restarted system calls return -EINTR */
> +	current->restart_block.fn = do_no_restart_syscall;
> +
> +	/*
> +	 * Since we stacked the signal on a 128-bit boundary,
> +	 * then 'sp' should be word aligned here.  If it's
> +	 * not, then the user is trying to mess with us.
> +	 */
> +	if (regs->sp & 15)
> +		goto badframe;
> +
> +	frame = (struct ilp32_rt_sigframe __user *)regs->sp;
> +
> +	if (!access_ok(VERIFY_READ, frame, sizeof (*frame)))
> +		goto badframe;
> +
> +	if (restore_ilp32_sigframe(regs, &frame->sig))
> +		goto badframe;
> +
> +	if (compat_restore_altstack(&frame->sig.uc.uc_stack))
> +		goto badframe;
> +
> +	return regs->regs[0];
> +
> +badframe:
> +	if (show_unhandled_signals)
> +		pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%08llx sp=%08llx\n",
> +				    current->comm, task_pid_nr(current), __func__,
> +				    regs->pc, regs->compat_sp);
It should be sp instead of compat_sp. The latter one is used by aarch32 EE.

Regards

Bamvor
> +	force_sig(SIGSEGV, current);
> +	return 0;
> +}
> +

Re: [PATCH 13/23] arm64: introduce is_a32_task and is_a32_thread (for AArch32 compat)

[Zhangjian (Bamvor)]

On 2016/6/12 20:21, Zhangjian (Bamvor) wrote:
> Hi, Yury
>
> On 2016/5/24 8:04, Yury Norov wrote:
>> Based on patch of Andrew Pinski.
>>
>> This patch introduces is_a32_compat_task and is_a32_thread so it is
>> easier to say this is a a32 specific thread or a generic compat thread/task.
>> Corresponding functions are located in <asm/is_compat.h> to avoid mess in
>> headers.
>>
>> Some files include both <linux/compat.h> and <asm/compat.h>,
>> and this is wrong because <linux/compat.h> has <asm/compat.h> already
>> included. It was fixed too.
>>
>> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Andrew Pinski <Andrew.Pinski@caviumnetworks.com>
>> Reviewed-by: David Daney <ddaney@caviumnetworks.com>
>> ---
>>   arch/arm64/include/asm/compat.h      | 19 ++----------
>>   arch/arm64/include/asm/elf.h         | 10 +++----
>>   arch/arm64/include/asm/ftrace.h      |  2 +-
>>   arch/arm64/include/asm/is_compat.h   | 58 ++++++++++++++++++++++++++++++++++++
>>   arch/arm64/include/asm/memory.h      |  3 +-
>>   arch/arm64/include/asm/processor.h   |  5 ++--
>>   arch/arm64/include/asm/syscall.h     |  2 +-
>>   arch/arm64/include/asm/thread_info.h |  2 +-
>>   arch/arm64/kernel/hw_breakpoint.c    | 10 +++----
>>   arch/arm64/kernel/perf_regs.c        |  2 +-
>>   arch/arm64/kernel/process.c          |  7 ++---
>>   arch/arm64/kernel/ptrace.c           | 11 ++++---
>>   arch/arm64/kernel/signal.c           |  4 +--
>>   arch/arm64/kernel/traps.c            |  3 +-
>>   14 files changed, 91 insertions(+), 47 deletions(-)
>>   create mode 100644 arch/arm64/include/asm/is_compat.h
>>
> [...]
>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> index 12f8a00..a66a0f7 100644
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -26,6 +26,7 @@
>>   #include <linux/types.h>
>>   #include <asm/bug.h>
>>   #include <asm/sizes.h>
>> +#include <asm/is_compat.h>
>>
>>   /*
>>    * Allow for constants defined here to be used from assembly code
>> @@ -61,7 +62,7 @@
>>
>>   #ifdef CONFIG_COMPAT
>>   #define TASK_SIZE_32        UL(0x100000000)
>> -#define TASK_SIZE        (test_thread_flag(TIF_32BIT) ? \
>> +#define TASK_SIZE        (is_compat_task() ?        \
>>                   TASK_SIZE_32 : TASK_SIZE_64)
>>   #define TASK_SIZE_OF(tsk)    (test_tsk_thread_flag(tsk, TIF_32BIT) ? \
>>                   TASK_SIZE_32 : TASK_SIZE_64)
> Should we update or delete this macro?
>   #define TASK_SIZE_OF(tsk) (is_compat_task() ? \
> TASK_SIZE_32 : TASK_SIZE_64)
Sorry it should be:
#define TASK_SIZE_OF(tsk)       ((is_a32_compat_thread(task_thread_info(tsk)) \
                                   || is_ilp32_compat_thread(task_thread_info(tsk))) ? \
                                 TASK_SIZE_32 : TASK_SIZE_64)

> x86, sparc, mips, ppc, parisc, s390 define its own version. But
> "include/linux/sched.h" will define it if
> TASK_SIZE_OF does not exist:
> #ifndef TASK_SIZE_OF
> #define TASK_SIZE_OF(tsk)       TASK_SIZE
> #endif
>
>
> Regards
>
> Bamvor
>

Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext

[Zhangjian (Bamvor)]

Hi,

On 2016/6/4 19:34, Zhangjian (Bamvor) wrote:
> Hi,
>
> I found an issue of unwind with the following code. The correct backtrace
> should be:
> (gdb) where
> #0 0x004004d0 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 func2 (num=0) at test_force3.c:22
> #3 0x00400540 in func1 (num=1) at test_force3.c:28
> #4 0x00400574 in main (argc=1, argv=0xffd7bc04) at test_force3.c:33
>
> Without my patch, the backtrace is:
> (gdb) where
> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
>
> With my patch which fix the wrong frame pointer(setup_return calculate the offset
> of fp through ilp32_sigframe instead of sigfreame), the backtrace is:
> (gdb) where
> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 func1 () at test_force3.c:28
> #3 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
>
> I am not sure there is still some issue in kernel. But it seem that the gdb of ilp32
> does not work correctly when unwind without framepointer.
I confirm that the reason why gdb could not unwind the func2 is gdb do not get the
correct offset of uc_mcontext.
And it seems that the kernel part is a reasonable fix for me.

Regards

Bamvor
>
> The test code is:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <signal.h>
>
> void my_sig(int sig)
> {
>      printf("sig=%d\n", sig);
>      *(int *)0 = 0x0;
> }
>
>
> void func2()
> {
>      *(int *)0 = 0x0;
> }
>
> void func1()
> {
>      func2();
> }
>
> int main(int argc, char **argv)
> {
>      signal(11, my_sig);
>      func1();
>      return 0;
> }
>
>
> The full patch is as follows:
>
>  From 7e364a765097f57aed2d73f94c1688c2e7343e79 Mon Sep 17 00:00:00 2001
> From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> Date: Sat, 4 Jun 2016 14:30:05 +0800
> Subject: [PATCH] arm64: ilp32: fix for wrong fp offset when calculate the
>   new fp
>
> ILP32 define its own sigframe(ilp32_sigframe) because of the
> difference uc_context. setup_return do not use ilp32 specific
> sigframe to calculate the new offset of fp which lead to wrong
> fp in signal handler. At this circumstance, gdb backtrace will miss
> one item:
> (gdb) where
>
> It should be:
> (gdb) where
>
> The test code is as follows:
>
> void my_sig(int sig)
> {
>          printf("sig=%d\n", sig);
>          *(int *)0 = 0x0;
> }
>
> void func2(int num)
> {
>          printf("%s: %d\n", __FUNCTION__, num);
>          *(int *)0 = 0x0;
>          func2(num-1);
> }
>
> void func1(int num)
> {
>          printf("%s\n", __FUNCTION__);
>          func2(num - 1);
> }
>
> int main(int argc, char **argv)
> {
>          signal(11, my_sig);
>          func1(argc);
>          return 0;
> }
>
> This patch fix this by passing the correct offset of fp to
> setup_return.
> Test pass on both ILP32 and LP64 in aarch64 EE.
>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> ---
>   arch/arm64/include/asm/signal_common.h | 3 ++-
>   arch/arm64/kernel/signal.c             | 9 +++++----
>   arch/arm64/kernel/signal_ilp32.c       | 4 ++--
>   3 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/signal_common.h b/arch/arm64/include/asm/signal_common.h
> index de93c71..a5d7b63 100644
> --- a/arch/arm64/include/asm/signal_common.h
> +++ b/arch/arm64/include/asm/signal_common.h
> @@ -29,6 +29,7 @@ int setup_sigcontex(struct sigcontext __user *uc_mcontext,
>               struct pt_regs *regs);
>   int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sf);
>   void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> -            void __user *frame, off_t sigframe_off, int usig);
> +            void __user *frame, off_t sigframe_off, off_t fp_off,
> +            int usig);
>
>   #endif /* __ASM_SIGNAL_COMMON_H */
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 038bebe..e66a6e9 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -256,14 +256,14 @@ static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig,
>   }
>
>   void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> -             void __user *frame, off_t sigframe_off, int usig)
> +             void __user *frame, off_t sigframe_off, off_t fp_off,
> +             int usig)
>   {
>       __sigrestore_t sigtramp;
>
>       regs->regs[0] = usig;
>       regs->sp = (unsigned long)frame;
> -    regs->regs[29] = regs->sp + sigframe_off +
> -        offsetof(struct sigframe, fp);
> +    regs->regs[29] = regs->sp + sigframe_off + fp_off;
>       regs->pc = (unsigned long)ka->sa.sa_handler;
>
>       if (ka->sa.sa_flags & SA_RESTORER)
> @@ -294,7 +294,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>       err |= setup_sigframe(&frame->sig, regs, set);
>       if (err == 0) {
>           setup_return(regs, &ksig->ka, frame,
> -            offsetof(struct rt_sigframe, sig), usig);
> +            offsetof(struct rt_sigframe, sig),
> +            offsetof(struct sigframe, fp), usig);
>           if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
>               err |= copy_siginfo_to_user(&frame->info, &ksig->info);
>               regs->regs[1] = (unsigned long)&frame->info;
> diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
> index a8ea73e..9030f14 100644
> --- a/arch/arm64/kernel/signal_ilp32.c
> +++ b/arch/arm64/kernel/signal_ilp32.c
> @@ -147,7 +147,6 @@ static struct ilp32_rt_sigframe __user *ilp32_get_sigframe(struct ksignal *ksig,
>       struct ilp32_rt_sigframe __user *frame;
>
>       sp = sp_top = sigsp(regs->sp, ksig);
> -
>       sp = (sp - sizeof(struct ilp32_rt_sigframe)) & ~15;
>       frame = (struct ilp32_rt_sigframe __user *)sp;
>
> @@ -183,7 +182,8 @@ int ilp32_setup_rt_frame(int usig, struct ksignal *ksig,
>       err |= setup_ilp32_sigframe(&frame->sig, regs, set);
>       if (err == 0) {
>           setup_return(regs, &ksig->ka, frame,
> -                 offsetof(struct ilp32_rt_sigframe, sig), usig);
> +                 offsetof(struct ilp32_rt_sigframe, sig),
> +                 offsetof(struct ilp32_sigframe, fp), usig);
>           regs->regs[1] = (unsigned long)&frame->info;
>           regs->regs[2] = (unsigned long)&frame->sig.uc;
>       }

Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext

[Yury Norov]

Hi Bamvor,

Sorry, I missed this patch.

On Sat, Jun 04, 2016 at 07:34:32PM +0800, Zhangjian (Bamvor) wrote:
> Hi,
> 
> I found an issue of unwind with the following code. The correct backtrace
> should be:
> (gdb) where
> #0 0x004004d0 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 func2 (num=0) at test_force3.c:22
> #3 0x00400540 in func1 (num=1) at test_force3.c:28
> #4 0x00400574 in main (argc=1, argv=0xffd7bc04) at test_force3.c:33
> 
> Without my patch, the backtrace is:
> (gdb) where
> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
> 
> With my patch which fix the wrong frame pointer(setup_return calculate the offset
> of fp through ilp32_sigframe instead of sigfreame), the backtrace is:
> (gdb) where
> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
> #1 <signal handler called>
> #2 func1 () at test_force3.c:28
> #3 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
> 
> I am not sure there is still some issue in kernel. But it seem that the gdb of ilp32
> does not work correctly when unwind without framepointer.
> 
> The test code is:
> 
> From 7e364a765097f57aed2d73f94c1688c2e7343e79 Mon Sep 17 00:00:00 2001
> From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> Date: Sat, 4 Jun 2016 14:30:05 +0800
> Subject: [PATCH] arm64: ilp32: fix for wrong fp offset when calculate the
>  new fp
> 
> ILP32 define its own sigframe(ilp32_sigframe) because of the
> difference uc_context. setup_return do not use ilp32 specific
> sigframe to calculate the new offset of fp which lead to wrong
> fp in signal handler. At this circumstance, gdb backtrace will miss
> one item:
> (gdb) where
> 
> It should be:
> (gdb) where
> 
> The test code is as follows:
> 
> void my_sig(int sig)
> {
>         printf("sig=%d\n", sig);
>         *(int *)0 = 0x0;
> }
> 
> void func2(int num)
> {
>         printf("%s: %d\n", __FUNCTION__, num);
>         *(int *)0 = 0x0;
>         func2(num-1);
> }
> 
> void func1(int num)
> {
>         printf("%s\n", __FUNCTION__);
>         func2(num - 1);
> }
> 
> int main(int argc, char **argv)
> {
>         signal(11, my_sig);
>         func1(argc);
>         return 0;
> }
> 
> This patch fix this by passing the correct offset of fp to
> setup_return.
> Test pass on both ILP32 and LP64 in aarch64 EE.
> 
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> ---
>  arch/arm64/include/asm/signal_common.h | 3 ++-
>  arch/arm64/kernel/signal.c             | 9 +++++----
>  arch/arm64/kernel/signal_ilp32.c       | 4 ++--
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/signal_common.h b/arch/arm64/include/asm/signal_common.h
> index de93c71..a5d7b63 100644
> --- a/arch/arm64/include/asm/signal_common.h
> +++ b/arch/arm64/include/asm/signal_common.h
> @@ -29,6 +29,7 @@ int setup_sigcontex(struct sigcontext __user *uc_mcontext,
>  		    struct pt_regs *regs);
>  int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sf);
>  void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> -			void __user *frame, off_t sigframe_off, int usig);
> +			void __user *frame, off_t sigframe_off, off_t fp_off,
> +			int usig); 
> 
>  #endif /* __ASM_SIGNAL_COMMON_H */
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 038bebe..e66a6e9 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -256,14 +256,14 @@ static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig,
>  }
> 
>  void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> -			 void __user *frame, off_t sigframe_off, int usig)
> +			 void __user *frame, off_t sigframe_off, off_t fp_off,
> +			 int usig)
>  {
>  	__sigrestore_t sigtramp;
> 
>  	regs->regs[0] = usig;
>  	regs->sp = (unsigned long)frame;
> -	regs->regs[29] = regs->sp + sigframe_off +
> -		offsetof(struct sigframe, fp);
> +	regs->regs[29] = regs->sp + sigframe_off + fp_off;

I think you are right here. The only nitpick is what for we send 2
offsets just to add one to another inside setup_return()?
We can do like this:

        void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
        			 void __user *frame, off_t fp_off, int usig)
        {
        	__sigrestore_t sigtramp;
        
        	regs->regs[0] = usig;
        	regs->sp = (unsigned long)frame;
        	regs->regs[29] = regs->sp + fp_off;
                [...]
        }

Where fp_off calculation is done by caller. 

	setup_return(regs, &ksig->ka, frame,
		offsetof(struct rt_sigframe, sig) + offsetof(struct sigframe, fp),
                usig);

For me it's more clear to understand what happens with this approach.
I don't think struct rt_sigframe will grow, but we can even introduce
some helper for it:
        #define RT_SIGFRAME_FP_POS (offsetof(struct rt_sigframe, sig) + offsetof(struct sigframe, fp))

If no objections, I'll apply your patch with my fix in next series.

>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> 
>  	if (ka->sa.sa_flags & SA_RESTORER)
> @@ -294,7 +294,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>  	err |= setup_sigframe(&frame->sig, regs, set);
>  	if (err == 0) {
>  		setup_return(regs, &ksig->ka, frame,
> -			offsetof(struct rt_sigframe, sig), usig);
> +			offsetof(struct rt_sigframe, sig),
> +			offsetof(struct sigframe, fp), usig);
>  		if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
>  			err |= copy_siginfo_to_user(&frame->info, &ksig->info);
>  			regs->regs[1] = (unsigned long)&frame->info;
> diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
> index a8ea73e..9030f14 100644
> --- a/arch/arm64/kernel/signal_ilp32.c
> +++ b/arch/arm64/kernel/signal_ilp32.c
> @@ -147,7 +147,6 @@ static struct ilp32_rt_sigframe __user *ilp32_get_sigframe(struct ksignal *ksig,
>  	struct ilp32_rt_sigframe __user *frame;
> 
>  	sp = sp_top = sigsp(regs->sp, ksig);
> -
>  	sp = (sp - sizeof(struct ilp32_rt_sigframe)) & ~15;
>  	frame = (struct ilp32_rt_sigframe __user *)sp;
> 
> @@ -183,7 +182,8 @@ int ilp32_setup_rt_frame(int usig, struct ksignal *ksig,
>  	err |= setup_ilp32_sigframe(&frame->sig, regs, set);
>  	if (err == 0) {
>  		setup_return(regs, &ksig->ka, frame,
> -			     offsetof(struct ilp32_rt_sigframe, sig), usig);
> +			     offsetof(struct ilp32_rt_sigframe, sig),
> +			     offsetof(struct ilp32_sigframe, fp), usig);
>  		regs->regs[1] = (unsigned long)&frame->info;
>  		regs->regs[2] = (unsigned long)&frame->sig.uc;
>  	}
> -- 
> 1.8.4.5
> 
> Regards
> 
> Bamvor
> 
> 

Re: [PATCH 13/23] arm64: introduce is_a32_task and is_a32_thread (for AArch32 compat)

[Yury Norov]

On Sun, Jun 12, 2016 at 09:08:28PM +0800, Zhangjian (Bamvor) wrote:
> >>  #ifdef CONFIG_COMPAT
> >>  #define TASK_SIZE_32        UL(0x100000000)
> >>-#define TASK_SIZE        (test_thread_flag(TIF_32BIT) ? \
> >>+#define TASK_SIZE        (is_compat_task() ?        \
> >>                  TASK_SIZE_32 : TASK_SIZE_64)
> >>  #define TASK_SIZE_OF(tsk)    (test_tsk_thread_flag(tsk, TIF_32BIT) ? \
> >>                  TASK_SIZE_32 : TASK_SIZE_64)
> >Should we update or delete this macro?
> >  #define TASK_SIZE_OF(tsk) (is_compat_task() ? \
> >TASK_SIZE_32 : TASK_SIZE_64)
> Sorry it should be:
> #define TASK_SIZE_OF(tsk)       ((is_a32_compat_thread(task_thread_info(tsk)) \
>                                   || is_ilp32_compat_thread(task_thread_info(tsk))) ? \
>                                 TASK_SIZE_32 : TASK_SIZE_64)

Thank you. I know about this, but always forget to fix. )
I think we'd introduce is_compat_thread() as well.

> >x86, sparc, mips, ppc, parisc, s390 define its own version. But
> >"include/linux/sched.h" will define it if
> >TASK_SIZE_OF does not exist:
> >#ifndef TASK_SIZE_OF
> >#define TASK_SIZE_OF(tsk)       TASK_SIZE
> >#endif
> >
> >
> >Regards
> >
> >Bamvor
> >

Re: [PATCH 16/23] arm64: ilp32: introduce binfmt_ilp32.c

[Zhangjian (Bamvor)]

Hi, Yury

On 2016/5/24 8:04, Yury Norov wrote:
> to handle ILP32 binaries
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
>   arch/arm64/kernel/Makefile       |  1 +
>   arch/arm64/kernel/binfmt_ilp32.c | 91 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 92 insertions(+)
>   create mode 100644 arch/arm64/kernel/binfmt_ilp32.c
>
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 6bc9738..9dfdf86 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -28,6 +28,7 @@ $(obj)/%.stub.o: $(obj)/%.o FORCE
>   arm64-obj-$(CONFIG_AARCH32_EL0)		+= sys32.o kuser32.o signal32.o 	\
>   					   sys_compat.o entry32.o		\
>   					   ../../arm/kernel/opcodes.o binfmt_elf32.o
> +arm64-obj-$(CONFIG_ARM64_ILP32)		+= binfmt_ilp32.o
>   arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>   arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
>   arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
> diff --git a/arch/arm64/kernel/binfmt_ilp32.c b/arch/arm64/kernel/binfmt_ilp32.c
> new file mode 100644
> index 0000000..a934fd4
> --- /dev/null
> +++ b/arch/arm64/kernel/binfmt_ilp32.c
> @@ -0,0 +1,91 @@
> +/*
> + * Support for ILP32 Linux/aarch64 ELF binaries.
> + */
> +
> +#include <linux/elfcore-compat.h>
> +#include <linux/time.h>
> +
> +#undef	ELF_CLASS
> +#define ELF_CLASS	ELFCLASS32
> +
> +#undef	elfhdr
> +#undef	elf_phdr
> +#undef	elf_shdr
> +#undef	elf_note
> +#undef	elf_addr_t
> +#define elfhdr		elf32_hdr
> +#define elf_phdr	elf32_phdr
> +#define elf_shdr	elf32_shdr
> +#define elf_note	elf32_note
> +#define elf_addr_t	Elf32_Addr
> +
> +/*
> + * Some data types as stored in coredump.
> + */
> +#define user_long_t		compat_long_t
> +#define user_siginfo_t		compat_siginfo_t
> +#define copy_siginfo_to_user	copy_siginfo_to_user32
> +
> +/*
> + * The machine-dependent core note format types are defined in elfcore-compat.h,
> + * which requires asm/elf.h to define compat_elf_gregset_t et al.
> + */
> +#define elf_prstatus	compat_elf_prstatus
> +#define elf_prpsinfo	compat_elf_prpsinfo
> +
> +/*
> + * Compat version of cputime_to_compat_timeval, perhaps this
> + * should be an inline in <linux/compat.h>.
> + */
> +static void cputime_to_compat_timeval(const cputime_t cputime,
> +				      struct compat_timeval *value)
> +{
> +	struct timeval tv;
> +	cputime_to_timeval(cputime, &tv);
> +	value->tv_sec = tv.tv_sec;
> +	value->tv_usec = tv.tv_usec;
> +}
> +
> +#undef cputime_to_timeval
> +#define cputime_to_timeval cputime_to_compat_timeval
> +
> +/* AARCH64 ILP32 EABI. */
> +#undef elf_check_arch
> +#define elf_check_arch(x)		(((x)->e_machine == EM_AARCH64)	\
> +					&& (x)->e_ident[EI_CLASS] == ELFCLASS32)
> +
> +#undef SET_PERSONALITY
> +#define SET_PERSONALITY(ex)						\
> +do {									\
> +	set_thread_flag(TIF_32BIT_AARCH64);				\
> +	clear_thread_flag(TIF_32BIT);					\
> +} while (0)
> +
> +#undef ARCH_DLINFO
> +#define ARCH_DLINFO							\
> +do {									\
> +	NEW_AUX_ENT(AT_SYSINFO_EHDR,					\
> +		    (elf_addr_t)(long)current->mm->context.vdso);	\
> +} while (0)
> +
> +#ifdef __AARCH64EB__
> +#define COMPAT_ELF_PLATFORM		("aarch64_be:ilp32")
> +#else
> +#define COMPAT_ELF_PLATFORM		("aarch64:ilp32")
> +#endif
fs/binfmt_elf.c use ELF_PLATFORM instead of the COMPAT one. Should we define
ELF_PLATFORM directly?
#undef ELF_PLATFORM
#ifdef __AARCH64EB__
#define ELF_PLATFORM            ("aarch64_be:ilp32")
#else
#define ELF_PLATFORM            ("aarch64:ilp32")
#endif

Regards

Bamvor
> +
> +#undef ELF_HWCAP
> +#undef ELF_HWCAP2
> +#define ELF_HWCAP			((u32) elf_hwcap)
> +#define ELF_HWCAP2			((u32) (elf_hwcap >> 32))
> +
> +/*
> + * Rename a few of the symbols that binfmt_elf.c will define.
> + * These are all local so the names don't really matter, but it
> + * might make some debugging less confusing not to duplicate them.
> + */
> +#define elf_format		compat_elf_format
> +#define init_elf_binfmt		init_compat_elf_binfmt
> +#define exit_elf_binfmt		exit_compat_elf_binfmt
> +
> +#include "../../../fs/binfmt_elf.c"
>

Re: [PATCH 16/23] arm64: ilp32: introduce binfmt_ilp32.c

[Zhangjian (Bamvor)]

Hi, again

I found another issue in binfmt_ilp32.c. We are using the ELF_ET_DYN_BASE
for ilp32 application. The default ELF_ET_DYN_BASE is calculated by
TASK_SIZE_64. IIUC, we should define the following things in binfmt_ilp32.c
which is the same value as aarch32:

+#undef ELF_ET_DYN_BASE
+#define ELF_ET_DYN_BASE COMPAT_ELF_ET_DYN_BASE

Note that the ilp32 library works without this patch. After read code and
debug, the address is corrected in get_unmapped_area. I suspect find_vma
fix this wrong address.

Ideas?

Regards

Bamvor

On 2016/6/13 11:05, Zhangjian (Bamvor) wrote:
> Hi, Yury
>
> On 2016/5/24 8:04, Yury Norov wrote:
>> to handle ILP32 binaries
>>
>> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
>> ---
>>   arch/arm64/kernel/Makefile       |  1 +
>>   arch/arm64/kernel/binfmt_ilp32.c | 91 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 92 insertions(+)
>>   create mode 100644 arch/arm64/kernel/binfmt_ilp32.c
>>
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 6bc9738..9dfdf86 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -28,6 +28,7 @@ $(obj)/%.stub.o: $(obj)/%.o FORCE
>>   arm64-obj-$(CONFIG_AARCH32_EL0)        += sys32.o kuser32.o signal32.o     \
>>                          sys_compat.o entry32.o        \
>>                          ../../arm/kernel/opcodes.o binfmt_elf32.o
>> +arm64-obj-$(CONFIG_ARM64_ILP32)        += binfmt_ilp32.o
>>   arm64-obj-$(CONFIG_FUNCTION_TRACER)    += ftrace.o entry-ftrace.o
>>   arm64-obj-$(CONFIG_MODULES)        += arm64ksyms.o module.o
>>   arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)    += module-plts.o
>> diff --git a/arch/arm64/kernel/binfmt_ilp32.c b/arch/arm64/kernel/binfmt_ilp32.c
>> new file mode 100644
>> index 0000000..a934fd4
>> --- /dev/null
>> +++ b/arch/arm64/kernel/binfmt_ilp32.c
>> @@ -0,0 +1,91 @@
>> +/*
>> + * Support for ILP32 Linux/aarch64 ELF binaries.
>> + */
>> +
>> +#include <linux/elfcore-compat.h>
>> +#include <linux/time.h>
>> +
>> +#undef    ELF_CLASS
>> +#define ELF_CLASS    ELFCLASS32
>> +
>> +#undef    elfhdr
>> +#undef    elf_phdr
>> +#undef    elf_shdr
>> +#undef    elf_note
>> +#undef    elf_addr_t
>> +#define elfhdr        elf32_hdr
>> +#define elf_phdr    elf32_phdr
>> +#define elf_shdr    elf32_shdr
>> +#define elf_note    elf32_note
>> +#define elf_addr_t    Elf32_Addr
>> +
>> +/*
>> + * Some data types as stored in coredump.
>> + */
>> +#define user_long_t        compat_long_t
>> +#define user_siginfo_t        compat_siginfo_t
>> +#define copy_siginfo_to_user    copy_siginfo_to_user32
>> +
>> +/*
>> + * The machine-dependent core note format types are defined in elfcore-compat.h,
>> + * which requires asm/elf.h to define compat_elf_gregset_t et al.
>> + */
>> +#define elf_prstatus    compat_elf_prstatus
>> +#define elf_prpsinfo    compat_elf_prpsinfo
>> +
>> +/*
>> + * Compat version of cputime_to_compat_timeval, perhaps this
>> + * should be an inline in <linux/compat.h>.
>> + */
>> +static void cputime_to_compat_timeval(const cputime_t cputime,
>> +                      struct compat_timeval *value)
>> +{
>> +    struct timeval tv;
>> +    cputime_to_timeval(cputime, &tv);
>> +    value->tv_sec = tv.tv_sec;
>> +    value->tv_usec = tv.tv_usec;
>> +}
>> +
>> +#undef cputime_to_timeval
>> +#define cputime_to_timeval cputime_to_compat_timeval
>> +
>> +/* AARCH64 ILP32 EABI. */
>> +#undef elf_check_arch
>> +#define elf_check_arch(x)        (((x)->e_machine == EM_AARCH64)    \
>> +                    && (x)->e_ident[EI_CLASS] == ELFCLASS32)
>> +
>> +#undef SET_PERSONALITY
>> +#define SET_PERSONALITY(ex)                        \
>> +do {                                    \
>> +    set_thread_flag(TIF_32BIT_AARCH64);                \
>> +    clear_thread_flag(TIF_32BIT);                    \
>> +} while (0)
>> +
>> +#undef ARCH_DLINFO
>> +#define ARCH_DLINFO                            \
>> +do {                                    \
>> +    NEW_AUX_ENT(AT_SYSINFO_EHDR,                    \
>> +            (elf_addr_t)(long)current->mm->context.vdso);    \
>> +} while (0)
>> +
>> +#ifdef __AARCH64EB__
>> +#define COMPAT_ELF_PLATFORM        ("aarch64_be:ilp32")
>> +#else
>> +#define COMPAT_ELF_PLATFORM        ("aarch64:ilp32")
>> +#endif
> fs/binfmt_elf.c use ELF_PLATFORM instead of the COMPAT one. Should we define
> ELF_PLATFORM directly?
> #undef ELF_PLATFORM
> #ifdef __AARCH64EB__
> #define ELF_PLATFORM            ("aarch64_be:ilp32")
> #else
> #define ELF_PLATFORM            ("aarch64:ilp32")
> #endif
>
> Regards
>
> Bamvor
>> +
>> +#undef ELF_HWCAP
>> +#undef ELF_HWCAP2
>> +#define ELF_HWCAP            ((u32) elf_hwcap)
>> +#define ELF_HWCAP2            ((u32) (elf_hwcap >> 32))
>> +
>> +/*
>> + * Rename a few of the symbols that binfmt_elf.c will define.
>> + * These are all local so the names don't really matter, but it
>> + * might make some debugging less confusing not to duplicate them.
>> + */
>> +#define elf_format        compat_elf_format
>> +#define init_elf_binfmt        init_compat_elf_binfmt
>> +#define exit_elf_binfmt        exit_compat_elf_binfmt
>> +
>> +#include "../../../fs/binfmt_elf.c"
>>
>

Re: [PATCH 01/23] all: syscall wrappers: add documentation

[Yury Norov]

Hi Catalin, David, all

> COMPAT_SYSCALL_WRAP2(creat, ...):
> 	mov	w0, w0
> 	b	<sys_creat>
> 
> > > Cost wise, this seems like it all cancels out in the end, but what
> > > do I know?
> > 
> > I think you know something, and I also think Heiko and other s390 guys
> > know something as well. So I'd like to listen their arguments here.
> > 
> > For me spark64 way is looking reasonable only because it's really simple
> > and takes less coding. I'll try it on some branch and share here what happened.
> 
> The kernel code will definitely look simpler ;). It would be good to see
> if there actually is any performance impact. Even with 16 more cycles on
> syscall entry, would they be lost in the noise? You don't need a full
> implementation, just some dummy mov x0, x0 on the entry path.
> 
> -- 
> Catalin

I wrote a simple test:

        struct timeval start, end;
        unsigned long long ut;

        int main()
        {
                gettimeofday(&start, NULL);

                for (int i = 1000000; i; i--)
                        syscall(__NR_getrusage, 100 /* EINVAL */, NULL);

                gettimeofday(&end, NULL);
                
                ut = (end.tv_sec - start.tv_sec) * 1000000ULL
                        + end.tv_usec - start.tv_usec;

                printf("%lld\n", ut);

                exit(EXIT_SUCCESS);
        }

In kernel there's minimal overhead:
diff --git a/kernel/sys.c b/kernel/sys.c
index 89d5be4..003d5ad 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1634,6 +1634,17 @@ COMPAT_SYSCALL_DEFINE2(getrusage, int, who,
struct compat_rusage __user *, ru)
{
        struct rusage r;
         
+       asm volatile (
+       "       mov w0, w0      \n"
+       "       mov w1, w1      \n"
+       "       mov w2, w2      \n"
+       "       mov w3, w3      \n"
+       "       mov w4, w4      \n"
+       "       mov w5, w5      \n"
+       "       mov w6, w6      \n"
+       "       mov w7, w7      \n"
+       );
+
        if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN &&
            who != RUSAGE_THREAD)
                return -EINVAL;

On QEMU:
With MOVs:      W/O MOVs:
832015          814564
840639          803165
830482          813116
832895          802928
832083          832658
834461          802993
829405          812465
846677          822651
828409          803393
836845          821470
828716          801044
831620          821301
825423          800278
829946          821476

We have 83 mS vs 81 mS, ~2.6% of performance degradation.
And I can show bigger numbers if I'll use asm svc instead of
syscall() wrapper which increases time as well. 

It's definitely more than 0, but not so big anyway. For syscalls
with heavy payload it will be non-measurable. So the choice
is still there. Should we use wrappers and save 2.5% of syscall
performance. Or clear top-halves unconditionally and win in simplicity?

If QEMU is looking non-representative, I can run test on real
hardware, but it takes a time, and I think will end up with similar
results.

Latest kernel with wrappers and library are here:
https://github.com/norov/linux/commits/ilp32
https://github.com/norov/glibc/commits/ilp32-dev

BTW, notice the change in ABI: syscalls that take stat and statfs
structures now routed to (wrapped) native handlers, after switching
userspace to use 64-bit off_t, ino_t, blkcnt_t, fsblkcnt_t and
fsfilcnt_t types.

Yury.

Re: [PATCH 16/23] arm64: ilp32: introduce binfmt_ilp32.c

[Yury Norov]

On Thu, May 26, 2016 at 09:49:42PM +0800, Zhangjian (Bamvor) wrote:
> Hi, yury
> 
> The coredump is usable in our platform. It miss the following definition:
> +#define compat_elf_greg_t	elf_greg_t
> +#define compat_elf_gregset_t	elf_gregset_t
> 
> And it leads to the wrong register save in core dump. After apply this patch,
> gdb could debug core dump files.
> 
> Here is the full patch:
> From 102624840aa5dacdd1bbfe3b390290f52f530ea2 Mon Sep 17 00:00:00 2001
> From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> Date: Thu, 26 May 2016 21:00:16 +0800
> Subject: [PATCH hulk-4.1-next] arm64: ilp32: fix coredump issue
> 
> ILP32 use aarch64 register and 32bit signal struct which means it
> could not make use of the existing compat_elf_prstatus/elf_prstatus
> and compat_elf_prpsinfo/elf_prpsinfo.
> 
> This patch fix this issue by introducing the different
> compat_elf_greg_t, compat_elf_gregset_t for aarch64 ilp32 and aarch32
> el0.
> 
> Tested pass on huawei's hardware in bigendian.
> 
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
> ---
>  arch/arm64/include/asm/elf.h     | 14 +++++++-------
>  arch/arm64/kernel/binfmt_elf32.c |  3 +++
>  arch/arm64/kernel/binfmt_ilp32.c |  8 +++++++-
>  arch/arm64/kernel/ptrace.c       | 20 ++++++++++----------
>  4 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
> index 0106d18..9019441 100644
> --- a/arch/arm64/include/asm/elf.h
> +++ b/arch/arm64/include/asm/elf.h
> @@ -154,18 +154,18 @@ extern int arch_setup_additional_pages(struct linux_binprm *bprm,
>  				       int uses_interp);
> 
>  /* 1GB of VA */
> -#define STACK_RND_MASK			(is_compat_task() ? \
> -						0x7ff >> (PAGE_SHIFT - 12) : \
> -						0x3ffff >> (PAGE_SHIFT - 12))
> +#define STACK_RND_MASK		(is_compat_task() ? \
> +					0x7ff >> (PAGE_SHIFT - 12) : \
> +					0x3ffff >> (PAGE_SHIFT - 12))
> 
>  #ifdef CONFIG_COMPAT
> 
> -#define COMPAT_ELF_ET_DYN_BASE		(2 * TASK_SIZE_32 / 3)
> +#define COMPAT_ELF_ET_DYN_BASE	(2 * TASK_SIZE_32 / 3)
> 
>  /* AArch32 registers. */
> -#define COMPAT_ELF_NGREG		18
> -typedef unsigned int			compat_elf_greg_t;
> -typedef compat_elf_greg_t		compat_elf_gregset_t[COMPAT_ELF_NGREG];
> +#define COMPAT_ELF_NGREG	18
> +typedef unsigned int		compat_a32_elf_greg_t;
> +typedef compat_a32_elf_greg_t	compat_a32_elf_gregset_t[COMPAT_ELF_NGREG];
> 
>  #endif /* CONFIG_COMPAT */
> 
> diff --git a/arch/arm64/kernel/binfmt_elf32.c b/arch/arm64/kernel/binfmt_elf32.c
> index 7b9b445..f75253c 100644
> --- a/arch/arm64/kernel/binfmt_elf32.c
> +++ b/arch/arm64/kernel/binfmt_elf32.c
> @@ -31,4 +31,7 @@ struct linux_binprm;
>  extern int aarch32_setup_vectors_page(struct linux_binprm *bprm,
>  				      int uses_interp);
> 
> +#define compat_elf_greg_t	compat_a32_elf_greg_t
> +#define compat_elf_gregset_t	compat_a32_elf_gregset_t
> +
>  #include "../../../fs/compat_binfmt_elf.c"
> diff --git a/arch/arm64/kernel/binfmt_ilp32.c b/arch/arm64/kernel/binfmt_ilp32.c
> index b827a9a..01baf83 100644
> --- a/arch/arm64/kernel/binfmt_ilp32.c
> +++ b/arch/arm64/kernel/binfmt_ilp32.c
> @@ -2,7 +2,9 @@
>   * Support for ILP32 Linux/aarch64 ELF binaries.
>   */
> 
> -#include <linux/elfcore-compat.h>
> +#include <linux/elf.h>
> +#include <linux/elfcore.h>
> +#include <linux/compat.h>
>  #include <linux/time.h>
> 
>  #undef	ELF_CLASS
> @@ -30,9 +32,13 @@
>   * The machine-dependent core note format types are defined in elfcore-compat.h,
>   * which requires asm/elf.h to define compat_elf_gregset_t et al.
>   */
> +#define compat_elf_greg_t	elf_greg_t
> +#define compat_elf_gregset_t	elf_gregset_t
>  #define elf_prstatus	compat_elf_prstatus
>  #define elf_prpsinfo	compat_elf_prpsinfo
> 
> +#include <linux/elfcore-compat.h>
> +
>  /*
>   * Compat version of cputime_to_compat_timeval, perhaps this
>   * should be an inline in <linux/compat.h>.
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 5c86135..9784c77 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -913,8 +913,8 @@ static const struct user_regset aarch32_regsets[] = {
>  	[REGSET_COMPAT_GPR] = {
>  		.core_note_type = NT_PRSTATUS,
>  		.n = COMPAT_ELF_NGREG,
> -		.size = sizeof(compat_elf_greg_t),
> -		.align = sizeof(compat_elf_greg_t),
> +		.size = sizeof(compat_a32_elf_greg_t),
> +		.align = sizeof(compat_a32_elf_greg_t),
>  		.get = compat_gpr_get,
>  		.set = compat_gpr_set
>  	},
> @@ -947,7 +947,7 @@ static int compat_ptrace_read_user(struct task_struct *tsk, compat_ulong_t off,
>  		tmp = tsk->mm->start_data;
>  	else if (off == COMPAT_PT_TEXT_END_ADDR)
>  		tmp = tsk->mm->end_code;
> -	else if (off < sizeof(compat_elf_gregset_t))
> +	else if (off < sizeof(compat_a32_elf_gregset_t))
>  		return copy_regset_to_user(tsk, &user_aarch32_view,
>  					   REGSET_COMPAT_GPR, off,
>  					   sizeof(compat_ulong_t), ret);
> @@ -968,7 +968,7 @@ static int compat_ptrace_write_user(struct task_struct *tsk, compat_ulong_t off,
>  	if (off & 3 || off >= COMPAT_USER_SZ)
>  		return -EIO;
> 
> -	if (off >= sizeof(compat_elf_gregset_t))
> +	if (off >= sizeof(compat_a32_elf_gregset_t))
>  		return 0;
> 
>  	set_fs(KERNEL_DS);
> @@ -1116,9 +1116,11 @@ static long compat_a32_ptrace(struct task_struct *child, compat_long_t request,
>  	unsigned long addr = caddr;
>  	unsigned long data = cdata;
>  	void __user *datap = compat_ptr(data);
> +	unsigned int pr_reg_size = sizeof(compat_a32_elf_gregset_t);
>  	int ret;
> 
>  	switch (request) {
> +
>  		case PTRACE_PEEKUSR:
>  			ret = compat_ptrace_read_user(child, addr, datap);
>  			break;
> @@ -1130,17 +1132,15 @@ static long compat_a32_ptrace(struct task_struct *child, compat_long_t request,
>  		case COMPAT_PTRACE_GETREGS:
>  			ret = copy_regset_to_user(child,
>  						  &user_aarch32_view,
> -						  REGSET_COMPAT_GPR,
> -						  0, sizeof(compat_elf_gregset_t),
> -						  datap);
> +						  REGSET_COMPAT_GPR, 0,
> +						  pr_reg_size, datap);
>  			break;
> 
>  		case COMPAT_PTRACE_SETREGS:
>  			ret = copy_regset_from_user(child,
>  						    &user_aarch32_view,
> -						    REGSET_COMPAT_GPR,
> -						    0, sizeof(compat_elf_gregset_t),
> -						    datap);
> +						    REGSET_COMPAT_GPR, 0,
> +						    pr_reg_size, datap);
>  			break;
> 
>  		case COMPAT_PTRACE_GET_THREAD_AREA:

Hi Bamvor,

I didn't work much with ilp32 debugging and coredumps yet, but what I
see in your patch is an attempt to use elf_gregset_t in compat_elf_prstatus
for ilp32 instead of compat_elf_gregset_t.

I think we can do it simpler. That's what pahole shows for binfmt_ilp32.o
after applying the attached patch:

struct compat_elf_prstatus {
        struct compat_elf_siginfo  pr_info;              /*     0    12 */
        short int                  pr_cursig;            /*    12     2 */

        /* XXX 2 bytes hole, try to pack */

        compat_ulong_t             pr_sigpend;           /*    16     4 */
        compat_ulong_t             pr_sighold;           /*    20     4 */
        compat_pid_t               pr_pid;               /*    24     4 */
        compat_pid_t               pr_ppid;              /*    28     4 */
        compat_pid_t               pr_pgrp;              /*    32     4 */
        compat_pid_t               pr_sid;               /*    36     4 */
        struct compat_timeval      pr_utime;             /*    40     8 */
        struct compat_timeval      pr_stime;             /*    48     8 */
        struct compat_timeval      pr_cutime;            /*    56     8 */
        /* --- cacheline 1 boundary (64 bytes) --- */
        struct compat_timeval      pr_cstime;            /*    64     8 */
        elf_gregset_t              pr_reg;               /*    72   272 */
        /* --- cacheline 5 boundary (320 bytes) was 24 bytes ago --- */
        compat_int_t               pr_fpvalid;           /*   344     4 */

        /* size: 352, cachelines: 6, members: 14 */
        /* sum members: 346, holes: 1, sum holes: 2 */
        /* padding: 4 */
        /* last cacheline: 32 bytes */
};

Did I miss something?
---
 arch/arm64/include/asm/elf.h     | 6 ++++++
 arch/arm64/kernel/binfmt_ilp32.c | 1 +
 2 files changed, 7 insertions(+)

diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index a967726..4dcbcec 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -174,10 +174,16 @@ extern int arch_setup_additional_pages(struct linux_binprm *bprm,
 
 #define COMPAT_ELF_ET_DYN_BASE		(2 * TASK_SIZE_32 / 3)
 
+#ifndef USE_AARCH64_GREG
 /* AArch32 registers. */
 #define COMPAT_ELF_NGREG		18
 typedef unsigned int			compat_elf_greg_t;
 typedef compat_elf_greg_t		compat_elf_gregset_t[COMPAT_ELF_NGREG];
+#else /* AArch64 registers for AARCH64/ILP32 */
+#define COMPAT_ELF_NGREG	ELF_NGREG
+#define compat_elf_greg_t	elf_greg_t
+#define compat_elf_gregset_t	elf_gregset_t
+#endif
 
 /* AArch32 EABI. */
 #define EF_ARM_EABI_MASK		0xff000000
diff --git a/arch/arm64/kernel/binfmt_ilp32.c b/arch/arm64/kernel/binfmt_ilp32.c
index b2bd590..416b3f5 100644
--- a/arch/arm64/kernel/binfmt_ilp32.c
+++ b/arch/arm64/kernel/binfmt_ilp32.c
@@ -1,6 +1,7 @@
 /*
  * Support for ILP32 Linux/aarch64 ELF binaries.
  */
+#define USE_AARCH64_GREG
 
 #include <linux/elfcore-compat.h>
 #include <linux/time.h>
-- 
2.7.4

Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext

[Zhangjian (Bamvor)]

Hi,

On 2016/6/13 1:44, Yury Norov wrote:
> Hi Bamvor,
>
> Sorry, I missed this patch.
>
> On Sat, Jun 04, 2016 at 07:34:32PM +0800, Zhangjian (Bamvor) wrote:
>> Hi,
>>
>> I found an issue of unwind with the following code. The correct backtrace
>> should be:
>> (gdb) where
>> #0 0x004004d0 in my_sig (sig=11) at test_force3.c:16
>> #1 <signal handler called>
>> #2 func2 (num=0) at test_force3.c:22
>> #3 0x00400540 in func1 (num=1) at test_force3.c:28
>> #4 0x00400574 in main (argc=1, argv=0xffd7bc04) at test_force3.c:33
>>
>> Without my patch, the backtrace is:
>> (gdb) where
>> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
>> #1 <signal handler called>
>> #2 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
>>
>> With my patch which fix the wrong frame pointer(setup_return calculate the offset
>> of fp through ilp32_sigframe instead of sigfreame), the backtrace is:
>> (gdb) where
>> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16
>> #1 <signal handler called>
>> #2 func1 () at test_force3.c:28
>> #3 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33
>>
>> I am not sure there is still some issue in kernel. But it seem that the gdb of ilp32
>> does not work correctly when unwind without framepointer.
>>
>> The test code is:
>>
>>  From 7e364a765097f57aed2d73f94c1688c2e7343e79 Mon Sep 17 00:00:00 2001
>> From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
>> Date: Sat, 4 Jun 2016 14:30:05 +0800
>> Subject: [PATCH] arm64: ilp32: fix for wrong fp offset when calculate the
>>   new fp
>>
>> ILP32 define its own sigframe(ilp32_sigframe) because of the
>> difference uc_context. setup_return do not use ilp32 specific
>> sigframe to calculate the new offset of fp which lead to wrong
>> fp in signal handler. At this circumstance, gdb backtrace will miss
>> one item:
>> (gdb) where
>>
>> It should be:
>> (gdb) where
>>
>> The test code is as follows:
>>
>> void my_sig(int sig)
>> {
>>          printf("sig=%d\n", sig);
>>          *(int *)0 = 0x0;
>> }
>>
>> void func2(int num)
>> {
>>          printf("%s: %d\n", __FUNCTION__, num);
>>          *(int *)0 = 0x0;
>>          func2(num-1);
>> }
>>
>> void func1(int num)
>> {
>>          printf("%s\n", __FUNCTION__);
>>          func2(num - 1);
>> }
>>
>> int main(int argc, char **argv)
>> {
>>          signal(11, my_sig);
>>          func1(argc);
>>          return 0;
>> }
>>
>> This patch fix this by passing the correct offset of fp to
>> setup_return.
>> Test pass on both ILP32 and LP64 in aarch64 EE.
>>
>> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com>
>> ---
>>   arch/arm64/include/asm/signal_common.h | 3 ++-
>>   arch/arm64/kernel/signal.c             | 9 +++++----
>>   arch/arm64/kernel/signal_ilp32.c       | 4 ++--
>>   3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/signal_common.h b/arch/arm64/include/asm/signal_common.h
>> index de93c71..a5d7b63 100644
>> --- a/arch/arm64/include/asm/signal_common.h
>> +++ b/arch/arm64/include/asm/signal_common.h
>> @@ -29,6 +29,7 @@ int setup_sigcontex(struct sigcontext __user *uc_mcontext,
>>   		    struct pt_regs *regs);
>>   int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sf);
>>   void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
>> -			void __user *frame, off_t sigframe_off, int usig);
>> +			void __user *frame, off_t sigframe_off, off_t fp_off,
>> +			int usig);
>>
>>   #endif /* __ASM_SIGNAL_COMMON_H */
>> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> index 038bebe..e66a6e9 100644
>> --- a/arch/arm64/kernel/signal.c
>> +++ b/arch/arm64/kernel/signal.c
>> @@ -256,14 +256,14 @@ static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig,
>>   }
>>
>>   void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
>> -			 void __user *frame, off_t sigframe_off, int usig)
>> +			 void __user *frame, off_t sigframe_off, off_t fp_off,
>> +			 int usig)
>>   {
>>   	__sigrestore_t sigtramp;
>>
>>   	regs->regs[0] = usig;
>>   	regs->sp = (unsigned long)frame;
>> -	regs->regs[29] = regs->sp + sigframe_off +
>> -		offsetof(struct sigframe, fp);
>> +	regs->regs[29] = regs->sp + sigframe_off + fp_off;
>
> I think you are right here. The only nitpick is what for we send 2
> offsets just to add one to another inside setup_return()?
> We can do like this:
>
>          void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
>          			 void __user *frame, off_t fp_off, int usig)
>          {
>          	__sigrestore_t sigtramp;
>
>          	regs->regs[0] = usig;
>          	regs->sp = (unsigned long)frame;
>          	regs->regs[29] = regs->sp + fp_off;
>                  [...]
>          }
>
> Where fp_off calculation is done by caller.
>
> 	setup_return(regs, &ksig->ka, frame,
> 		offsetof(struct rt_sigframe, sig) + offsetof(struct sigframe, fp),
>                  usig);
>
> For me it's more clear to understand what happens with this approach.
> I don't think struct rt_sigframe will grow, but we can even introduce
> some helper for it:
>          #define RT_SIGFRAME_FP_POS (offsetof(struct rt_sigframe, sig) + offsetof(struct sigframe, fp))
>
> If no objections, I'll apply your patch with my fix in next series.
Sure. Thanks.

Regards

Bamvor
>
>>   	regs->pc = (unsigned long)ka->sa.sa_handler;
>>
>>   	if (ka->sa.sa_flags & SA_RESTORER)
>> @@ -294,7 +294,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>>   	err |= setup_sigframe(&frame->sig, regs, set);
>>   	if (err == 0) {
>>   		setup_return(regs, &ksig->ka, frame,
>> -			offsetof(struct rt_sigframe, sig), usig);
>> +			offsetof(struct rt_sigframe, sig),
>> +			offsetof(struct sigframe, fp), usig);
>>   		if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
>>   			err |= copy_siginfo_to_user(&frame->info, &ksig->info);
>>   			regs->regs[1] = (unsigned long)&frame->info;
>> diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c
>> index a8ea73e..9030f14 100644
>> --- a/arch/arm64/kernel/signal_ilp32.c
>> +++ b/arch/arm64/kernel/signal_ilp32.c
>> @@ -147,7 +147,6 @@ static struct ilp32_rt_sigframe __user *ilp32_get_sigframe(struct ksignal *ksig,
>>   	struct ilp32_rt_sigframe __user *frame;
>>
>>   	sp = sp_top = sigsp(regs->sp, ksig);
>> -
>>   	sp = (sp - sizeof(struct ilp32_rt_sigframe)) & ~15;
>>   	frame = (struct ilp32_rt_sigframe __user *)sp;
>>
>> @@ -183,7 +182,8 @@ int ilp32_setup_rt_frame(int usig, struct ksignal *ksig,
>>   	err |= setup_ilp32_sigframe(&frame->sig, regs, set);
>>   	if (err == 0) {
>>   		setup_return(regs, &ksig->ka, frame,
>> -			     offsetof(struct ilp32_rt_sigframe, sig), usig);
>> +			     offsetof(struct ilp32_rt_sigframe, sig),
>> +			     offsetof(struct ilp32_sigframe, fp), usig);
>>   		regs->regs[1] = (unsigned long)&frame->info;
>>   		regs->regs[2] = (unsigned long)&frame->sig.uc;
>>   	}
>> --
>> 1.8.4.5
>>
>> Regards
>>
>> Bamvor
>>
>>

Re: [PATCH 17/23] arm64: ptrace: handle ptrace_request differently for aarch32 and ilp32

[Bamvor Zhang]

Hi, Chengming

On Sat, Jun 25, 2016 at 5:36 PM, zhouchengming
<zhouchengming1@huawei.com> wrote:
> On 2016/6/9 1:00, Yury Norov wrote:
>>
>> On Wed, Jun 08, 2016 at 09:34:09AM +0800, zhouchengming wrote:
>>>
>>> On 2016/5/24 8:04, Yury Norov wrote:
>>>>
>>>> Here new aarch32 ptrace syscall handler is introsuced to avoid run-time
>>>> detection of the task type.
>>>>
>>>> Signed-off-by: Yury Norov<ynorov@caviumnetworks.com>
>>
>>
>> [...]
>>
>>> Hello, I found ilp32 will use sys_ptrace, not compat_sys_ptrace. So I
>>> write
>>> a little patch to see if can solve the problem correctly.
>>>
>>> Thanks.
>>>
>>>  From f6156236df578bb05c4a17e7f9776ceaf8f7afe6 Mon Sep 17 00:00:00 2001
>>> From: Zhou Chengming<zhouchengming1@huawei.com>
>>> Date: Wed, 8 Jun 2016 09:46:23 +0800
>>> Subject: [PATCH] ilp32: use compat_sys_ptrace instead of sys_ptrace
>>>
>>> When we analyze a testcase of ptrace that failed on ilp32, we found
>>> the syscall that the ilp32 uses is sys_ptrace, not compat_sys_ptrace.
>>> Because in include/uapi/asm-generic/unistd.h it's defined like:
>>> __SYSCALL(__NR_ptrace, sys_ptrace)
>>> So we change it to __SC_COMP(__NR_ptrace, sys_ptrace, compat_sys_ptrace),
>>> let compat tasks use the compat_sys_ptrace.
>>>
>>> Signed-off-by: Zhou Chengming<zhouchengming1@huawei.com>
>>> ---
>>>   include/uapi/asm-generic/unistd.h |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/include/uapi/asm-generic/unistd.h
>>> b/include/uapi/asm-generic/unistd.h
>>> index 2862d2e..50ee770 100644
>>> --- a/include/uapi/asm-generic/unistd.h
>>> +++ b/include/uapi/asm-generic/unistd.h
>>> @@ -364,7 +364,7 @@ __SC_WRAP(__NR_syslog, sys_syslog)
>>>
>>>   /* kernel/ptrace.c */
>>>   #define __NR_ptrace 117
>>> -__SYSCALL(__NR_ptrace, sys_ptrace)
>>> +__SC_COMP(__NR_ptrace, sys_ptrace, compat_sys_ptrace)
>>>
>>>   /* kernel/sched/core.c */
>>>   #define __NR_sched_setparam 118
>>> --
>>> 1.7.7
>>>
>>
>> Hi Zhou,
>>
>> Thank you for the catch.
>>
>> Could you also show the test that is failed for you. It should
>> probably be sent to LTP maillist.
>>
>> I'm not sure your fix correct as it affects other architectures that
>> use standard unistd.h. I think it's better to redirect the syscall in
>> arch/arm64/kernel/sys_ilp32.c with corresponding definition.
>>
>> Yury
>>
>> .
>>
>
> Sorry, I missed this mail. Thanks for your reply. :)
> I attach the testcase file of ptrace that failed on ilp32.
> I also think it's better to redirect the syscall in ilp32, so I changed
> the patch.

Thanks for your patch. But Yury has already sent an new series this week
which define ptrace to compat one.

It seems that Yury do not take GET/SETSIGMASK into account. You
could share your test case and patches at this point.

Best wishes

Bamvor

[1] https://www.mail-archive.com/linux-doc@vger.kernel.org/msg03811.html
>
>
> From 7e692ba1adf02c2a2f125836f5222f455c9ffe56 Mon Sep 17 00:00:00 2001
> From: Zhou Chengming <zhouchengming1@huawei.com>
> Date: Sat, 25 Jun 2016 18:02:51 +0800
> Subject: [PATCH] ilp32 should use compat_sys_ptrace
>
> The file include/uapi/asm-generic/unistd.h defines this:
> __SYSCALL(__NR_ptrace, sys_ptrace)
> It may cause some ptrace tests failed on ilp32. So we redirect the ptrace
> syscall in arch/arm64/kernel/sys_ilp32.c with corresponding definition.
>
> Signed-off-by: Zhou Chengming <zhouchengming1@huawei.com>
> ---
>  arch/arm64/kernel/sys_ilp32.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm64/kernel/sys_ilp32.c b/arch/arm64/kernel/sys_ilp32.c
> index d85fe94..06d5e1b 100644
> --- a/arch/arm64/kernel/sys_ilp32.c
> +++ b/arch/arm64/kernel/sys_ilp32.c
> @@ -46,6 +46,9 @@
>  asmlinkage long ilp32_sys_rt_sigreturn_wrapper(void);
>  #define compat_sys_rt_sigreturn        ilp32_sys_rt_sigreturn_wrapper
>
> +/* ilp32 should use compat_sys_ptrace */
> +#define sys_ptrace                    compat_sys_ptrace
> +
>  #include <asm/syscall.h>
>
>  #undef __SYSCALL
> --
> 1.7.7
>
>
>



-- 
-----------------------------------------
   arm64, kernel. opensuse
   blog: http://aarch64.me
-----------------------------------------