| Message ID | 1450215766-14765-14-git-send-email-ynorov@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Yury Norov <ynorov@caviumnetworks.com> writes: > From: Jan Dakinevich <jan.dakinevich@gmail.com> > > statfs64, fstat64 and mmap_pgoff has wrappers that needed both by aarch32 and Typo: s/fstat64/fstatfs64/ Andreas.
On Thu, Dec 17, 2015 at 03:38:16PM +0100, Andreas Schwab wrote: > Yury Norov <ynorov@caviumnetworks.com> writes: > > > From: Jan Dakinevich <jan.dakinevich@gmail.com> > > > > statfs64, fstat64 and mmap_pgoff has wrappers that needed both by aarch32 and > > Typo: s/fstat64/fstatfs64/ > > Andreas. Thank you.
On Wed, Dec 16, 2015 at 12:42:39AM +0300, Yury Norov wrote: > statfs64, fstat64 and mmap_pgoff has wrappers that needed both by aarch32 and > ilp32 to workaround some issues. Here we create common file to share aarch32 > workarounds to with ilp32 code. [...] > --- /dev/null > +++ b/arch/arm64/kernel/entry32-common.S > @@ -0,0 +1,37 @@ > +#include <linux/linkage.h> > +#include <linux/const.h> > + > +#include <asm/assembler.h> > +#include <asm/asm-offsets.h> > +#include <asm/errno.h> > +#include <asm/page.h> > + > +ENTRY(compat_sys_statfs64_wrapper) > + mov w3, #84 > + cmp w1, #88 > + csel w1, w3, w1, eq > + b compat_sys_statfs64 > +ENDPROC(compat_sys_statfs64_wrapper) > + > +ENTRY(compat_sys_fstatfs64_wrapper) > + mov w3, #84 > + cmp w1, #88 > + csel w1, w3, w1, eq > + b compat_sys_fstatfs64 > +ENDPROC(compat_sys_fstatfs64_wrapper) I'm not convinced we need these wrappers for ILP32. They've been introduced on arch/arm many years ago by commit Fixes: 713c481519f1 ([ARM] 3108/2: old ABI compat: statfs64 and fstatfs64) to deal with user space passing a size of 88 (the EABI size of struct compat_statfs64 without the packing and alignment attribute). Since that commit, the sizeof(struct compat_statfs64) is 84 already. This should be the case with the new ILP32 exported headers (no backwards compatibility), so user space should never pass 88 as size. Therefore we could call compat_sys_(f)statfs64 directly without wrappers.
On Tuesday 22 December 2015, Catalin Marinas wrote: > > + > > +ENTRY(compat_sys_statfs64_wrapper) > > + mov w3, #84 > > + cmp w1, #88 > > + csel w1, w3, w1, eq > > + b compat_sys_statfs64 > > +ENDPROC(compat_sys_statfs64_wrapper) > > + > > +ENTRY(compat_sys_fstatfs64_wrapper) > > + mov w3, #84 > > + cmp w1, #88 > > + csel w1, w3, w1, eq > > + b compat_sys_fstatfs64 > > +ENDPROC(compat_sys_fstatfs64_wrapper) > > I'm not convinced we need these wrappers for ILP32. They've been > introduced on arch/arm many years ago by commit Fixes: 713c481519f1 > ([ARM] 3108/2: old ABI compat: statfs64 and fstatfs64) to deal with user > space passing a size of 88 (the EABI size of struct compat_statfs64 > without the packing and alignment attribute). Since that commit, the > sizeof(struct compat_statfs64) is 84 already. This should be the case > with the new ILP32 exported headers (no backwards compatibility), so > user space should never pass 88 as size. Therefore we could call > compat_sys_(f)statfs64 directly without wrappers. That means we have to set ARCH_PACK_STATFS64 in the arm64 header files though, and propagate the OABI alignment to arm64/ilp32 as well, rather than using the 88-byte version that every other 32-bit architecture except for x86-32 and arm32 has. Another option would be to set "#define __statfs_word __u64" and use the 64-bit statfs call, instead of compat_sys_statfs64, but that in turn requires special-casing statfs in libc. Arnd
On Tue, Dec 22, 2015 at 12:25:15PM +0000, Catalin Marinas wrote: > On Wed, Dec 16, 2015 at 12:42:39AM +0300, Yury Norov wrote: > > statfs64, fstat64 and mmap_pgoff has wrappers that needed both by aarch32 and > > ilp32 to workaround some issues. Here we create common file to share aarch32 > > workarounds to with ilp32 code. > [...] > > --- /dev/null > > +++ b/arch/arm64/kernel/entry32-common.S > > @@ -0,0 +1,37 @@ > > +#include <linux/linkage.h> > > +#include <linux/const.h> > > + > > +#include <asm/assembler.h> > > +#include <asm/asm-offsets.h> > > +#include <asm/errno.h> > > +#include <asm/page.h> > > + > > +ENTRY(compat_sys_statfs64_wrapper) > > + mov w3, #84 > > + cmp w1, #88 > > + csel w1, w3, w1, eq > > + b compat_sys_statfs64 > > +ENDPROC(compat_sys_statfs64_wrapper) > > + > > +ENTRY(compat_sys_fstatfs64_wrapper) > > + mov w3, #84 > > + cmp w1, #88 > > + csel w1, w3, w1, eq > > + b compat_sys_fstatfs64 > > +ENDPROC(compat_sys_fstatfs64_wrapper) > > I'm not convinced we need these wrappers for ILP32. They've been > introduced on arch/arm many years ago by commit Fixes: 713c481519f1 > ([ARM] 3108/2: old ABI compat: statfs64 and fstatfs64) to deal with user > space passing a size of 88 (the EABI size of struct compat_statfs64 > without the packing and alignment attribute). Since that commit, the > sizeof(struct compat_statfs64) is 84 already. This should be the case > with the new ILP32 exported headers (no backwards compatibility), so > user space should never pass 88 as size. Therefore we could call > compat_sys_(f)statfs64 directly without wrappers. > With current glibc, sizeof(struct compat_statfs64) is 88, so I added wrappers just to make this couple of syscalls work. AFAIR, glibc doesn't use exported headers here, so we'd change it. Maybe Andrew will share more details. > -- > Catalin
On Tue, Dec 22, 2015 at 10:44:14PM +0100, Arnd Bergmann wrote: > On Tuesday 22 December 2015, Catalin Marinas wrote: > > > + > > > +ENTRY(compat_sys_statfs64_wrapper) > > > + mov w3, #84 > > > + cmp w1, #88 > > > + csel w1, w3, w1, eq > > > + b compat_sys_statfs64 > > > +ENDPROC(compat_sys_statfs64_wrapper) > > > + > > > +ENTRY(compat_sys_fstatfs64_wrapper) > > > + mov w3, #84 > > > + cmp w1, #88 > > > + csel w1, w3, w1, eq > > > + b compat_sys_fstatfs64 > > > +ENDPROC(compat_sys_fstatfs64_wrapper) > > > > I'm not convinced we need these wrappers for ILP32. They've been > > introduced on arch/arm many years ago by commit Fixes: 713c481519f1 > > ([ARM] 3108/2: old ABI compat: statfs64 and fstatfs64) to deal with user > > space passing a size of 88 (the EABI size of struct compat_statfs64 > > without the packing and alignment attribute). Since that commit, the > > sizeof(struct compat_statfs64) is 84 already. This should be the case > > with the new ILP32 exported headers (no backwards compatibility), so > > user space should never pass 88 as size. Therefore we could call > > compat_sys_(f)statfs64 directly without wrappers. > > That means we have to set ARCH_PACK_STATFS64 in the arm64 header files > though, and propagate the OABI alignment to arm64/ilp32 as well, rather > than using the 88-byte version that every other 32-bit architecture > except for x86-32 and arm32 has. Yuri replied that for EABI glibc, sizeof(struct statfs64) is already 88. If that's correct and the packing attribute is ignored by glibc, we could drop ARCH_PACK_COMPAT_STATFS64 as well (OABI not supported by arm64). But I would be slightly worried since glibc is not the only user of the kernel ABI. For ILP32, I think we can skip defining ARCH_PACK_STATFS64 (of course, only if __ILP32__) and state that sizeof(struct statfs64) is 88 (unpacked). In which case we need the wrappers above to be able to reuse the compat_sys_statfs64 code. > Another option would be to set "#define __statfs_word __u64" and use > the 64-bit statfs call, instead of compat_sys_statfs64, but that in turn > requires special-casing statfs in libc. I wouldn't go this route as we kind of agreed that ILP32 should look like any other 32-bit ABI.
On Wednesday 23 December 2015, Catalin Marinas wrote: > > That means we have to set ARCH_PACK_STATFS64 in the arm64 header files > > though, and propagate the OABI alignment to arm64/ilp32 as well, rather > > than using the 88-byte version that every other 32-bit architecture > > except for x86-32 and arm32 has. > > Yuri replied that for EABI glibc, sizeof(struct statfs64) is already 88. > If that's correct and the packing attribute is ignored by glibc, we > could drop ARCH_PACK_COMPAT_STATFS64 as well (OABI not supported by > arm64). But I would be slightly worried since glibc is not the only user > of the kernel ABI. It looks like glibc has its own definition of 'struct statfs64', which is incompatible with the one in the kernel headers for ARM EABI. However, there are other libc implementations besides glibc, and we can't assume that they all do it the same way, so we clearly have to keep using the wrapper for ARM EABI. For ARM64/ILP32, we are probably better off defining it the same way in kernel and libc without that wrapper. > For ILP32, I think we can skip defining ARCH_PACK_STATFS64 (of course, > only if __ILP32__) and state that sizeof(struct statfs64) is 88 > (unpacked). In which case we need the wrappers above to be able to reuse > the compat_sys_statfs64 code. > > > Another option would be to set "#define __statfs_word __u64" and use > > the 64-bit statfs call, instead of compat_sys_statfs64, but that in turn > > requires special-casing statfs in libc. > > I wouldn't go this route as we kind of agreed that ILP32 should look > like any other 32-bit ABI. It's really tricky then: in order to support EABI binaries from a libc that uses the kernel headers with the OABI compatible definition, we must not copy the 88 byte structure to user space, because that would overwrite user space stack data, and that in turn means we have to set ARCH_PACK_COMPAT_STATFS64, but that in turn prevents us from using the generic 32-bit syscall ABI for the arm64/ilp32 fstatfs64 call. It seems that today, put_compat_statfs64() doesn't actually use the size argument, and it just copies the individual fields, which is fine either way. This means we could turn around the logic in the arm32 wrapper, remove ARCH_PACK_COMPAT_STATFS64, and make the ilp32 code call directly into compat_sys_fstatfs64(), but it would be a bit fragile, as we rely on put_compat_statfs64() not actually writing the padding fields that the native do_statfs64() writes. If someone changed them to both use copy_to_user, we'd silently introduce data corruption on rarely used libc implementations. Arnd
On Wed, Dec 23, 2015 at 09:41:54PM +0100, Arnd Bergmann wrote: > On Wednesday 23 December 2015, Catalin Marinas wrote: > > > That means we have to set ARCH_PACK_STATFS64 in the arm64 header files > > > though, and propagate the OABI alignment to arm64/ilp32 as well, rather > > > than using the 88-byte version that every other 32-bit architecture > > > except for x86-32 and arm32 has. > > > > Yuri replied that for EABI glibc, sizeof(struct statfs64) is already 88. > > If that's correct and the packing attribute is ignored by glibc, we > > could drop ARCH_PACK_COMPAT_STATFS64 as well (OABI not supported by > > arm64). But I would be slightly worried since glibc is not the only user > > of the kernel ABI. > > It looks like glibc has its own definition of 'struct statfs64', which > is incompatible with the one in the kernel headers for ARM EABI. > > However, there are other libc implementations besides glibc, and we > can't assume that they all do it the same way, so we clearly have to > keep using the wrapper for ARM EABI. For ARM64/ILP32, we are probably > better off defining it the same way in kernel and libc without that > wrapper. > > > For ILP32, I think we can skip defining ARCH_PACK_STATFS64 (of course, > > only if __ILP32__) and state that sizeof(struct statfs64) is 88 > > (unpacked). In which case we need the wrappers above to be able to reuse > > the compat_sys_statfs64 code. > > > > > Another option would be to set "#define __statfs_word __u64" and use > > > the 64-bit statfs call, instead of compat_sys_statfs64, but that in turn > > > requires special-casing statfs in libc. > > > > I wouldn't go this route as we kind of agreed that ILP32 should look > > like any other 32-bit ABI. > > It's really tricky then: in order to support EABI binaries from a libc > that uses the kernel headers with the OABI compatible definition, we > must not copy the 88 byte structure to user space, because that would > overwrite user space stack data, and that in turn means we have to set > ARCH_PACK_COMPAT_STATFS64, but that in turn prevents us from using the > generic 32-bit syscall ABI for the arm64/ilp32 fstatfs64 call. > > It seems that today, put_compat_statfs64() doesn't actually use > the size argument, and it just copies the individual fields, which > is fine either way. This means we could turn around the logic > in the arm32 wrapper, remove ARCH_PACK_COMPAT_STATFS64, and make > the ilp32 code call directly into compat_sys_fstatfs64(), but it > would be a bit fragile, as we rely on put_compat_statfs64() not > actually writing the padding fields that the native do_statfs64() > writes. If someone changed them to both use copy_to_user, we'd > silently introduce data corruption on rarely used libc implementations. > > Arnd So. For ilp32, the only wrapper left here, is compat_sys_mmap2_wrapper. But this is workaroud, as comment tells: Note: off_4k (w5) is always in units of 4K. If we can't do the requested offset because it is not page-aligned, we return -EINVAL. Not sure we should pull it to ILP32. If so, we can call sys_mmap_pgoff() directly. And we don't need this patch at all therefore. Any throughts? Yury.
On Wednesday 30 December 2015 20:29:05 Yury Norov wrote: > > So. For ilp32, the only wrapper left here, is compat_sys_mmap2_wrapper. > But this is workaroud, as comment tells: > Note: off_4k (w5) is always in units of 4K. If we can't do the > requested offset because it is not page-aligned, we return -EINVAL. > > Not sure we should pull it to ILP32. If so, we can call sys_mmap_pgoff() > directly. And we don't need this patch at all therefore. Any throughts? > > I think providing the 64-bit version of sys_mmap() would be the simplest API, as that avoids any possible confusion about the shift amount (hardcoded 12 bits vs PAGE_BITS). It fits in with the other syscalls that pass an loff_t value here. Arnd
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 8787347..837d730 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -25,6 +25,7 @@ arm64-obj-$(CONFIG_AARCH32_EL0) += sys32.o kuser32.o signal32.o \ ../../arm/kernel/opcodes.o arm64-obj-$(CONFIG_FUNCTION_TRACER) += ftrace.o entry-ftrace.o arm64-obj-$(CONFIG_ARM64_ILP32) += sys_ilp32.o +arm64-obj-$(CONFIG_COMPAT) += entry32-common.o arm64-obj-$(CONFIG_MODULES) += arm64ksyms.o module.o arm64-obj-$(CONFIG_PERF_EVENTS) += perf_regs.o perf_callchain.o arm64-obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o diff --git a/arch/arm64/kernel/entry32-common.S b/arch/arm64/kernel/entry32-common.S new file mode 100644 index 0000000..2ad5912 --- /dev/null +++ b/arch/arm64/kernel/entry32-common.S @@ -0,0 +1,37 @@ +#include <linux/linkage.h> +#include <linux/const.h> + +#include <asm/assembler.h> +#include <asm/asm-offsets.h> +#include <asm/errno.h> +#include <asm/page.h> + +ENTRY(compat_sys_statfs64_wrapper) + mov w3, #84 + cmp w1, #88 + csel w1, w3, w1, eq + b compat_sys_statfs64 +ENDPROC(compat_sys_statfs64_wrapper) + +ENTRY(compat_sys_fstatfs64_wrapper) + mov w3, #84 + cmp w1, #88 + csel w1, w3, w1, eq + b compat_sys_fstatfs64 +ENDPROC(compat_sys_fstatfs64_wrapper) + +/* + * Note: off_4k (w5) is always in units of 4K. If we can't do the + * requested offset because it is not page-aligned, we return -EINVAL. + */ +ENTRY(compat_sys_mmap2_wrapper) +#if PAGE_SHIFT > 12 + tst w5, #~PAGE_MASK >> 12 + b.ne 1f + lsr w5, w5, #PAGE_SHIFT - 12 +#endif + b sys_mmap_pgoff +1: mov x0, #-EINVAL + ret +ENDPROC(compat_sys_mmap2_wrapper) + diff --git a/arch/arm64/kernel/entry32.S b/arch/arm64/kernel/entry32.S index f332d5d..8026129 100644 --- a/arch/arm64/kernel/entry32.S +++ b/arch/arm64/kernel/entry32.S @@ -40,35 +40,6 @@ ENTRY(compat_sys_rt_sigreturn_wrapper) b compat_sys_rt_sigreturn ENDPROC(compat_sys_rt_sigreturn_wrapper) -ENTRY(compat_sys_statfs64_wrapper) - mov w3, #84 - cmp w1, #88 - csel w1, w3, w1, eq - b compat_sys_statfs64 -ENDPROC(compat_sys_statfs64_wrapper) - -ENTRY(compat_sys_fstatfs64_wrapper) - mov w3, #84 - cmp w1, #88 - csel w1, w3, w1, eq - b compat_sys_fstatfs64 -ENDPROC(compat_sys_fstatfs64_wrapper) - -/* - * Note: off_4k (w5) is always in units of 4K. If we can't do the - * requested offset because it is not page-aligned, we return -EINVAL. - */ -ENTRY(compat_sys_mmap2_wrapper) -#if PAGE_SHIFT > 12 - tst w5, #~PAGE_MASK >> 12 - b.ne 1f - lsr w5, w5, #PAGE_SHIFT - 12 -#endif - b sys_mmap_pgoff -1: mov x0, #-EINVAL - ret -ENDPROC(compat_sys_mmap2_wrapper) - /* * Wrappers for AArch32 syscalls that either take 64-bit parameters * in registers or that take 32-bit parameters which require sign diff --git a/arch/arm64/kernel/sys_ilp32.c b/arch/arm64/kernel/sys_ilp32.c index 8ce79db..c282fa2 100644 --- a/arch/arm64/kernel/sys_ilp32.c +++ b/arch/arm64/kernel/sys_ilp32.c @@ -45,6 +45,15 @@ #define compat_sys_open_by_handle_at sys_open_by_handle_at #define compat_sys_openat sys_openat +asmlinkage long compat_sys_mmap2_wrapper(void); +#define sys_mmap2 compat_sys_mmap2_wrapper + +asmlinkage long compat_sys_fstatfs64_wrapper(void); +#define compat_sys_fstatfs64 compat_sys_fstatfs64_wrapper + +asmlinkage long compat_sys_statfs64_wrapper(void); +#define compat_sys_statfs64 compat_sys_statfs64_wrapper + #include <asm/syscall.h> #undef __SYSCALL