Message ID | 20220921065605.1051927-14-rmclure@linux.ibm.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | powerpc: Syscall wrapper and register clearing | expand |
Rohan McLure <rmclure@linux.ibm.com> writes: > Syscall handlers should not be invoked internally by their symbol names, > as these symbols defined by the architecture-defined SYSCALL_DEFINE > macro. Move the compatibility syscall definition for mmap2 to > syscalls.c, so that all mmap implementations can share a helper function. > > Remove 'inline' on static mmap helper. > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > --- > V2: Move mmap2 compat implementation to asm/kernel/syscalls.c. > V4: Move to be applied before syscall wrapper introduced. > V5: Remove 'inline' in helper. > --- > arch/powerpc/kernel/sys_ppc32.c | 9 --------- > arch/powerpc/kernel/syscalls.c | 17 ++++++++++++++--- > 2 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c > index d961634976d8..776ae7565fc5 100644 > --- a/arch/powerpc/kernel/sys_ppc32.c > +++ b/arch/powerpc/kernel/sys_ppc32.c > @@ -25,7 +25,6 @@ > #include <linux/poll.h> > #include <linux/personality.h> > #include <linux/stat.h> > -#include <linux/mman.h> > #include <linux/in.h> > #include <linux/syscalls.h> > #include <linux/unistd.h> > @@ -48,14 +47,6 @@ > #include <asm/syscalls.h> > #include <asm/switch_to.h> > > -unsigned long compat_sys_mmap2(unsigned long addr, size_t len, > - unsigned long prot, unsigned long flags, > - unsigned long fd, unsigned long pgoff) > -{ > - /* This should remain 12 even if PAGE_SIZE changes */ > - return sys_mmap(addr, len, prot, flags, fd, pgoff << 12); > -} > - > compat_ssize_t compat_sys_pread64(unsigned int fd, char __user *ubuf, compat_size_t count, > u32 reg6, u32 pos1, u32 pos2) > { > diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c > index a04c97faa21a..9830957498b0 100644 > --- a/arch/powerpc/kernel/syscalls.c > +++ b/arch/powerpc/kernel/syscalls.c > @@ -36,9 +36,9 @@ > #include <asm/time.h> > #include <asm/unistd.h> > > -static inline long do_mmap2(unsigned long addr, size_t len, > - unsigned long prot, unsigned long flags, > - unsigned long fd, unsigned long off, int shift) > +static long do_mmap2(unsigned long addr, size_t len, > + unsigned long prot, unsigned long flags, > + unsigned long fd, unsigned long off, int shift) > { > if (!arch_validate_prot(prot, addr)) > return -EINVAL; > @@ -56,6 +56,17 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len, > return do_mmap2(addr, len, prot, flags, fd, pgoff, PAGE_SHIFT-12); > } > > +#ifdef CONFIG_COMPAT > +COMPAT_SYSCALL_DEFINE6(mmap2, > + unsigned long, addr, size_t, len, > + unsigned long, prot, unsigned long, flags, > + unsigned long, fd, unsigned long, pgoff) > +{ > + /* This should remain 12 even if PAGE_SIZE changes */ > + return do_mmap2(addr, len, prot, flags, fd, pgoff << 12, PAGE_SHIFT-12); This isn't quite right. The comment about it remaining 12 is kind of misleading, it was true when compat_sys_mmap2() called sys_mmap(), but it's wrong now that we're calling do_mmap2(). The incoming "pgoff" here is in units of 4K. do_mmap2() takes "off" in whatever units, but also takes "shift", which has to tell us how to shift "off" into PAGE_SIZE units. If we pass off = pgoff << 12, that's in bytes, so we need to page shift = PAGE_SHIFT. But I think it makes more sense to do the same as mmap2() and pass the 4K offset through, and pass shift = PAGE_SHIFT - 12. I also borrowed the "off_4k" name from arm64. End result: #ifdef CONFIG_COMPAT COMPAT_SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len, unsigned long, prot, unsigned long, flags, unsigned long, fd, unsigned long, off_4k) { return do_mmap2(addr, len, prot, flags, fd, off_4k, PAGE_SHIFT-12); } #endif With that my G5 boots again :) cheers
On Wed, Sep 28, 2022, at 2:15 PM, Michael Ellerman wrote: > But I think it makes more sense to do the same as mmap2() and pass the > 4K offset through, and pass shift = PAGE_SHIFT - 12. I also borrowed the > "off_4k" name from arm64. End result: > > #ifdef CONFIG_COMPAT > COMPAT_SYSCALL_DEFINE6(mmap2, > unsigned long, addr, size_t, len, > unsigned long, prot, unsigned long, flags, > unsigned long, fd, unsigned long, off_4k) > { > return do_mmap2(addr, len, prot, flags, fd, off_4k, PAGE_SHIFT-12); > } > #endif > > With that my G5 boots again :) Any chance we can instead add a working compat_sys_mmap2/sys_mmap2 in mm/mmap.c alongside the sys_mmap_pgoff implementation? While sys_mmap_pgoff() was meant to replace the various sys_mmap2() implementations, I think it was ultimately a mistake, and we later converged on the sys_mmap2() calling conventions with 12 bits offset for almost all 32-bit architectures. Arnd
"Arnd Bergmann" <arnd@arndb.de> writes: > On Wed, Sep 28, 2022, at 2:15 PM, Michael Ellerman wrote: > >> But I think it makes more sense to do the same as mmap2() and pass the >> 4K offset through, and pass shift = PAGE_SHIFT - 12. I also borrowed the >> "off_4k" name from arm64. End result: >> >> #ifdef CONFIG_COMPAT >> COMPAT_SYSCALL_DEFINE6(mmap2, >> unsigned long, addr, size_t, len, >> unsigned long, prot, unsigned long, flags, >> unsigned long, fd, unsigned long, off_4k) >> { >> return do_mmap2(addr, len, prot, flags, fd, off_4k, PAGE_SHIFT-12); >> } >> #endif >> >> With that my G5 boots again :) > > Any chance we can instead add a working compat_sys_mmap2/sys_mmap2 > in mm/mmap.c alongside the sys_mmap_pgoff implementation? I've merged this, but happy to clean things up in a subsequent patch :) > While sys_mmap_pgoff() was meant to replace the various sys_mmap2() > implementations, I think it was ultimately a mistake, and we later > converged on the sys_mmap2() calling conventions with 12 bits > offset for almost all 32-bit architectures. I only see 3 compat mmap2s: $ gg "COMPAT_SYSCALL.*mmap2" arch/arm64/kernel/sys32.c:COMPAT_SYSCALL_DEFINE6(aarch32_mmap2, unsigned long, addr, unsigned long, len, arch/powerpc/kernel/syscalls.c:COMPAT_SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len, arch/s390/kernel/compat_linux.c:COMPAT_SYSCALL_DEFINE1(s390_mmap2, struct mmap_arg_struct_emu31 __user *, arg) s390 is weird. The arm64 one and ours are similar, but we have the additional call to arch_validate_prot(prot, addr). arm64 does implement arch_validate_prot(). Similar with mmap2, we call arch_validate_prot() but no one else does (why not?). cheers
On Fri, Sep 30, 2022, at 3:19 PM, Michael Ellerman wrote: > "Arnd Bergmann" <arnd@arndb.de> writes: >> >> While sys_mmap_pgoff() was meant to replace the various sys_mmap2() >> implementations, I think it was ultimately a mistake, and we later >> converged on the sys_mmap2() calling conventions with 12 bits >> offset for almost all 32-bit architectures. > > I only see 3 compat mmap2s: > > $ gg "COMPAT_SYSCALL.*mmap2" > arch/arm64/kernel/sys32.c:COMPAT_SYSCALL_DEFINE6(aarch32_mmap2, > unsigned long, addr, unsigned long, len, > arch/powerpc/kernel/syscalls.c:COMPAT_SYSCALL_DEFINE6(mmap2, unsigned > long, addr, size_t, len, > arch/s390/kernel/compat_linux.c:COMPAT_SYSCALL_DEFINE1(s390_mmap2, > struct mmap_arg_struct_emu31 __user *, arg) They are all inconsistently named, and some are shared with the 64-bit implementation on architectures that provide mmap2 for both 32-bit and 64-bit mode, rather than only for 32-bit. arch/mips/kernel/syscall.c:SYSCALL_DEFINE6(mips_mmap2, unsigned long, addr, unsigned long, len, arch/sparc/kernel/sys32.S: .globl sys32_mmap2 arch/parisc/kernel/sys_parisc.c:asmlinkage unsigned long sys_mmap2(unsigned long addr, unsigned long len, > s390 is weird. Right, they used to be limited to 5 register arguments > The arm64 one and ours are similar, but we have the additional call to > arch_validate_prot(prot, addr). arm64 does implement arch_validate_prot(). > > Similar with mmap2, we call arch_validate_prot() but no one else does > (why not?). This looks like it was added in ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support"), which is powerpc specific. It looks like this should correspond to a custom arch_calc_vm_prot_bits() implementation, which exists on arm64, powerpc, sparc and x86. I suppose it should be there for those four. Arnd
diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c index d961634976d8..776ae7565fc5 100644 --- a/arch/powerpc/kernel/sys_ppc32.c +++ b/arch/powerpc/kernel/sys_ppc32.c @@ -25,7 +25,6 @@ #include <linux/poll.h> #include <linux/personality.h> #include <linux/stat.h> -#include <linux/mman.h> #include <linux/in.h> #include <linux/syscalls.h> #include <linux/unistd.h> @@ -48,14 +47,6 @@ #include <asm/syscalls.h> #include <asm/switch_to.h> -unsigned long compat_sys_mmap2(unsigned long addr, size_t len, - unsigned long prot, unsigned long flags, - unsigned long fd, unsigned long pgoff) -{ - /* This should remain 12 even if PAGE_SIZE changes */ - return sys_mmap(addr, len, prot, flags, fd, pgoff << 12); -} - compat_ssize_t compat_sys_pread64(unsigned int fd, char __user *ubuf, compat_size_t count, u32 reg6, u32 pos1, u32 pos2) { diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c index a04c97faa21a..9830957498b0 100644 --- a/arch/powerpc/kernel/syscalls.c +++ b/arch/powerpc/kernel/syscalls.c @@ -36,9 +36,9 @@ #include <asm/time.h> #include <asm/unistd.h> -static inline long do_mmap2(unsigned long addr, size_t len, - unsigned long prot, unsigned long flags, - unsigned long fd, unsigned long off, int shift) +static long do_mmap2(unsigned long addr, size_t len, + unsigned long prot, unsigned long flags, + unsigned long fd, unsigned long off, int shift) { if (!arch_validate_prot(prot, addr)) return -EINVAL; @@ -56,6 +56,17 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len, return do_mmap2(addr, len, prot, flags, fd, pgoff, PAGE_SHIFT-12); } +#ifdef CONFIG_COMPAT +COMPAT_SYSCALL_DEFINE6(mmap2, + unsigned long, addr, size_t, len, + unsigned long, prot, unsigned long, flags, + unsigned long, fd, unsigned long, pgoff) +{ + /* This should remain 12 even if PAGE_SIZE changes */ + return do_mmap2(addr, len, prot, flags, fd, pgoff << 12, PAGE_SHIFT-12); +} +#endif + SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len, unsigned long, prot, unsigned long, flags, unsigned long, fd, off_t, offset)