linux-kernel - Re: [patches] Re: [PATCH 16/17] RISC-V: User-facing API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <mhng-ec8dc13e-34c8-4235-8fc1-f5a6f32f8995@palmer-si-x1c4>
Date:   Tue, 11 Jul 2017 10:28:14 -0700 (PDT)
From:   Palmer Dabbelt <palmer@...belt.com>
To:     james.hogan@...tec.com
CC:     hch@...radead.org, yamada.masahiro@...ionext.com, mmarek@...e.com,
        will.deacon@....com, peterz@...radead.org, boqun.feng@...il.com,
        mingo@...hat.com, daniel.lezcano@...aro.org, tglx@...utronix.de,
        jason@...edaemon.net, marc.zyngier@....com,
        gregkh@...uxfoundation.org, jslaby@...e.com, davem@...emloft.net,
        mchehab@...nel.org, sfr@...b.auug.org.au, fweisbec@...il.com,
        viro@...iv.linux.org.uk, mcgrof@...nel.org, dledford@...hat.com,
        bart.vanassche@...disk.com, sstabellini@...nel.org,
        daniel.vetter@...ll.ch, mpe@...erman.id.au, msalter@...hat.com,
        nicolas.dichtel@...nd.com, paul.gortmaker@...driver.com,
        linux@...ck-us.net, heiko.carstens@...ibm.com,
        schwidefsky@...ibm.com, linux-kernel@...r.kernel.org,
        patches@...ups.riscv.org, akpm@...ux-foundation.org,
        albert@...ive.com
Subject:     Re: [patches] Re: [PATCH 16/17] RISC-V: User-facing API

On Tue, 11 Jul 2017 07:01:32 PDT (-0700), james.hogan@...tec.com wrote:
> Hi Christoph,
>
> On Tue, Jul 11, 2017 at 06:39:48AM -0700, Christoph Hellwig wrote:
>> > +#ifdef CONFIG_64BIT
>> > +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
>> > +	unsigned long, prot, unsigned long, flags,
>> > +	unsigned long, fd, off_t, offset)
>> > +{
>> > +	if (unlikely(offset & (~PAGE_MASK)))
>> > +		return -EINVAL;
>> > +	return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
>> > +}
>> > +#else
>> > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>> > +	unsigned long, prot, unsigned long, flags,
>> > +	unsigned long, fd, off_t, offset)
>> > +{
>> > +	/*
>> > +	 * Note that the shift for mmap2 is constant (12),
>> > +	 * regardless of PAGE_SIZE
>> > +	 */
>> > +	if (unlikely(offset & (~PAGE_MASK >> 12)))
>> > +		return -EINVAL;
>> > +	return sys_mmap_pgoff(addr, len, prot, flags, fd,
>> > +		offset >> (PAGE_SHIFT - 12));
>> > +}
>> > +#endif /* !CONFIG_64BIT */
>>
>> Most modern ports seem to expose sys_mmap_pgoff as the
>> syscall directly.  Any reason you're doing this differently?
>
> I think Palmer's patch is probably correct here. Exposing sys_mmap_pgoff
> is only really correct on 32-bit arches where the only page size is 4k.
> If other page sizes are supported then this is the correct way to handle
> it as the page offset from 32-bit userland is supposed to be in 4k
> units.
>
> 64-bit doesn't need to worry about squeezing big file offsets into the
> off_t offset so don't need to do the shift at all.
>
> See the mmap2 man page. It says "the final argument specifies the offset
> into the file in 4096-byte units", and it points out ia64 as an
> exception where it depends on the page size of the system.
>
>>
>> But even the code for the older ones should probably be consolidated..
>
> Quite probably, yes.

This looks like what arm64 does, though I'm OK either way.  Here's my attempt
at consolidating the code, even though there isn't a lot to help with:

  diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
  index e18fc0ebdd91..4351be7d0533 100644
  --- a/arch/riscv/kernel/sys_riscv.c
  +++ b/arch/riscv/kernel/sys_riscv.c
  @@ -17,14 +17,23 @@
   #include <asm/cmpxchg.h>
   #include <asm/unistd.h>

  +static long riscv_sys_mmap(unsigned long addr, unsigned long len,
  +                          unsigned long prot, unsigned long flags,
  +                          unsigned long fd, off_t offset,
  +                          unsigned long page_shift_offset)
  +{
  +       if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
  +               return -EINVAL;
  +       return sys_mmap_pgoff(addr, len, prot, flags, fd,
  +                             offset >> (PAGE_SHIFT - page_shift_offset));
  +}
  +
   #ifdef CONFIG_64BIT
   SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
          unsigned long, prot, unsigned long, flags,
          unsigned long, fd, off_t, offset)
   {
  -       if (unlikely(offset & (~PAGE_MASK)))
  -               return -EINVAL;
  -       return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
  +       return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 0);
   }
   #else
   SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
  @@ -35,9 +44,6 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
           * Note that the shift for mmap2 is constant (12),
           * regardless of PAGE_SIZE
           */
  -       if (unlikely(offset & (~PAGE_MASK >> 12)))
  -               return -EINVAL;
  -       return sys_mmap_pgoff(addr, len, prot, flags, fd,
  -               offset >> (PAGE_SHIFT - 12));
  +       return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 12);
   }
   #endif /* !CONFIG_64BIT */

I'll submit this as part of our v6, which will hopefully be coming out soon.

Thanks!