Re: [PATCH 8/8] x86/mm: Allow to have userspace mappings above 47-bits

[Dmitry Safonov <dsafonov@virtuozzo.com>]

On 04/06/2017 09:43 PM, Dmitry Safonov wrote:
> Hi Kirill,
>
> On 04/06/2017 05:01 PM, Kirill A. Shutemov wrote:
>> On x86, 5-level paging enables 56-bit userspace virtual address space.
>> Not all user space is ready to handle wide addresses. It's known that
>> at least some JIT compilers use higher bits in pointers to encode their
>> information. It collides with valid pointers with 5-level paging and
>> leads to crashes.
>>
>> To mitigate this, we are not going to allocate virtual address space
>> above 47-bit by default.
>>
>> But userspace can ask for allocation from full address space by
>> specifying hint address (with or without MAP_FIXED) above 47-bits.
>>
>> If hint address set above 47-bit, but MAP_FIXED is not specified, we try
>> to look for unmapped area by specified address. If it's already
>> occupied, we look for unmapped area in *full* address space, rather than
>> from 47-bit window.
>
> Do you wish after the first over-47-bit mapping the following mmap()
> calls return also over-47-bits if there is free space?
> It so, you could simplify all this code by changing only mm->mmap_base
> on the first over-47-bit mmap() call.
> This will do simple trick.
>
>>
>> This approach helps to easily make application's memory allocator aware
>> about large address space without manually tracking allocated virtual
>> address space.
>>
>> One important case we need to handle here is interaction with MPX.
>> MPX (without MAWA( extension cannot handle addresses above 47-bit, so we
>> need to make sure that MPX cannot be enabled we already have VMA above
>> the boundary and forbid creating such VMAs once MPX is enabled.
>>
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Dmitry Safonov <dsafonov@virtuozzo.com>
>> ---
>>  arch/x86/include/asm/elf.h       |  2 +-
>>  arch/x86/include/asm/mpx.h       |  9 +++++++++
>>  arch/x86/include/asm/processor.h |  9 ++++++---
>>  arch/x86/kernel/sys_x86_64.c     | 28 +++++++++++++++++++++++++++-
>>  arch/x86/mm/hugetlbpage.c        | 27 ++++++++++++++++++++++++---
>>  arch/x86/mm/mmap.c               |  2 +-
>>  arch/x86/mm/mpx.c                | 33 ++++++++++++++++++++++++++++++++-
>>  7 files changed, 100 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
>> index d4d3ed456cb7..67260dbe1688 100644
>> --- a/arch/x86/include/asm/elf.h
>> +++ b/arch/x86/include/asm/elf.h
>> @@ -250,7 +250,7 @@ extern int force_personality32;
>>     the loader.  We need to make sure that it is out of the way of the
>> program
>>     that it will "exec", and that there is sufficient room for the
>> brk.  */
>>
>> -#define ELF_ET_DYN_BASE        (TASK_SIZE / 3 * 2)
>> +#define ELF_ET_DYN_BASE        (DEFAULT_MAP_WINDOW / 3 * 2)
>
> This will kill 32-bit userspace:
> As DEFAULT_MAP_WINDOW is defined as what previously was TASK_SIZE_MAX,
> not TASK_SIZE, for ia32/x32 ELF_ET_DYN_BASE will be over 4Gb.
>
> Here is the test:
> [root@localhost test]# cat hello-world.c
> #include <stdio.h>
>
> int main(int argc, char **argv)
> {
>     printf("Maybe this world is another planet's hell.\n");
>         return 0;
> }
> [root@localhost test]# gcc -m32 hello-world.c -o hello-world
> [root@localhost test]# ./hello-world
> [   35.306726] hello-world[1948]: segfault at ffa5288c ip
> 00000000f77b5a82 sp 00000000ffa52890 error 6 in ld-2.23.so[f77b5000+23000]
> Segmentation fault (core dumped)
>
> So, dynamic base should differ between 32/64-bits as it was with TASK_SIZE.

I just tried to define it like this:
-#define DEFAULT_MAP_WINDOW     ((1UL << 47) - PAGE_SIZE)
+#define DEFAULT_MAP_WINDOW     (test_thread_flag(TIF_ADDR32) ?         \
+                               IA32_PAGE_OFFSET : ((1UL << 47) - 
PAGE_SIZE))

And it looks working better.

>
>
>>
>>  /* This yields a mask that user programs can use to figure out what
>>     instruction set this CPU supports.  This could be done in user space,
>> diff --git a/arch/x86/include/asm/mpx.h b/arch/x86/include/asm/mpx.h
>> index a0d662be4c5b..7d7404756bb4 100644
>> --- a/arch/x86/include/asm/mpx.h
>> +++ b/arch/x86/include/asm/mpx.h
>> @@ -73,6 +73,9 @@ static inline void mpx_mm_init(struct mm_struct *mm)
>>  }
>>  void mpx_notify_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
>>                unsigned long start, unsigned long end);
>> +
>> +unsigned long mpx_unmapped_area_check(unsigned long addr, unsigned
>> long len,
>> +        unsigned long flags);
>>  #else
>>  static inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
>>  {
>> @@ -94,6 +97,12 @@ static inline void mpx_notify_unmap(struct
>> mm_struct *mm,
>>                      unsigned long start, unsigned long end)
>>  {
>>  }
>> +
>> +static inline unsigned long mpx_unmapped_area_check(unsigned long addr,
>> +        unsigned long len, unsigned long flags)
>> +{
>> +    return addr;
>> +}
>>  #endif /* CONFIG_X86_INTEL_MPX */
>>
>>  #endif /* _ASM_X86_MPX_H */
>> diff --git a/arch/x86/include/asm/processor.h
>> b/arch/x86/include/asm/processor.h
>> index 3cada998a402..9f437aea7f57 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -795,6 +795,7 @@ static inline void spin_lock_prefetch(const void *x)
>>  #define IA32_PAGE_OFFSET    PAGE_OFFSET
>>  #define TASK_SIZE        PAGE_OFFSET
>>  #define TASK_SIZE_MAX        TASK_SIZE
>> +#define DEFAULT_MAP_WINDOW    TASK_SIZE
>>  #define STACK_TOP        TASK_SIZE
>>  #define STACK_TOP_MAX        STACK_TOP
>>
>> @@ -834,7 +835,9 @@ static inline void spin_lock_prefetch(const void *x)
>>   * particular problem by preventing anything from being mapped
>>   * at the maximum canonical address.
>>   */
>> -#define TASK_SIZE_MAX    ((1UL << 47) - PAGE_SIZE)
>> +#define TASK_SIZE_MAX    ((1UL << __VIRTUAL_MASK_SHIFT) - PAGE_SIZE)
>> +
>> +#define DEFAULT_MAP_WINDOW    ((1UL << 47) - PAGE_SIZE)
>>
>>  /* This decides where the kernel will search for a free chunk of vm
>>   * space during mmap's.
>> @@ -847,7 +850,7 @@ static inline void spin_lock_prefetch(const void *x)
>>  #define TASK_SIZE_OF(child)    ((test_tsk_thread_flag(child,
>> TIF_ADDR32)) ? \
>>                      IA32_PAGE_OFFSET : TASK_SIZE_MAX)
>>
>> -#define STACK_TOP        TASK_SIZE
>> +#define STACK_TOP        DEFAULT_MAP_WINDOW
>>  #define STACK_TOP_MAX        TASK_SIZE_MAX
>>
>>  #define INIT_THREAD  {                        \
>> @@ -870,7 +873,7 @@ extern void start_thread(struct pt_regs *regs,
>> unsigned long new_ip,
>>   * space during mmap's.
>>   */
>>  #define __TASK_UNMAPPED_BASE(task_size)    (PAGE_ALIGN(task_size / 3))
>> -#define TASK_UNMAPPED_BASE        __TASK_UNMAPPED_BASE(TASK_SIZE)
>> +#define TASK_UNMAPPED_BASE
>> __TASK_UNMAPPED_BASE(DEFAULT_MAP_WINDOW)
>
> ditto
>
>>
>>  #define KSTK_EIP(task)        (task_pt_regs(task)->ip)
>>
>> diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
>> index 207b8f2582c7..593a31e93812 100644
>> --- a/arch/x86/kernel/sys_x86_64.c
>> +++ b/arch/x86/kernel/sys_x86_64.c
>> @@ -21,6 +21,7 @@
>>  #include <asm/compat.h>
>>  #include <asm/ia32.h>
>>  #include <asm/syscalls.h>
>> +#include <asm/mpx.h>
>>
>>  /*
>>   * Align a virtual address to avoid aliasing in the I$ on AMD F15h.
>> @@ -132,6 +133,10 @@ arch_get_unmapped_area(struct file *filp,
>> unsigned long addr,
>>      struct vm_unmapped_area_info info;
>>      unsigned long begin, end;
>>
>> +    addr = mpx_unmapped_area_check(addr, len, flags);
>> +    if (IS_ERR_VALUE(addr))
>> +        return addr;
>> +
>>      if (flags & MAP_FIXED)
>>          return addr;
>>
>> @@ -151,7 +156,16 @@ arch_get_unmapped_area(struct file *filp,
>> unsigned long addr,
>>      info.flags = 0;
>>      info.length = len;
>>      info.low_limit = begin;
>> -    info.high_limit = end;
>> +
>> +    /*
>> +     * If hint address is above DEFAULT_MAP_WINDOW, look for unmapped
>> area
>> +     * in the full address space.
>> +     */
>> +    if (addr > DEFAULT_MAP_WINDOW)
>> +        info.high_limit = min(end, TASK_SIZE);
>> +    else
>> +        info.high_limit = min(end, DEFAULT_MAP_WINDOW);
>> +
>>      info.align_mask = 0;
>>      info.align_offset = pgoff << PAGE_SHIFT;
>>      if (filp) {
>> @@ -171,6 +185,10 @@ arch_get_unmapped_area_topdown(struct file *filp,
>> const unsigned long addr0,
>>      unsigned long addr = addr0;
>>      struct vm_unmapped_area_info info;
>>
>> +    addr = mpx_unmapped_area_check(addr, len, flags);
>> +    if (IS_ERR_VALUE(addr))
>> +        return addr;
>> +
>>      /* requested length too big for entire address space */
>>      if (len > TASK_SIZE)
>>          return -ENOMEM;
>> @@ -195,6 +213,14 @@ arch_get_unmapped_area_topdown(struct file *filp,
>> const unsigned long addr0,
>>      info.length = len;
>>      info.low_limit = PAGE_SIZE;
>>      info.high_limit = get_mmap_base(0);
>> +
>> +    /*
>> +     * If hint address is above DEFAULT_MAP_WINDOW, look for unmapped
>> area
>> +     * in the full address space.
>> +     */
>> +    if (addr > DEFAULT_MAP_WINDOW && !in_compat_syscall())
>> +        info.high_limit += TASK_SIZE - DEFAULT_MAP_WINDOW;
>
> Hmm, TASK_SIZE depends now on TIF_ADDR32, which is set during exec().
> That means for ia32/x32 ELF which has TASK_SIZE < 4Gb as TIF_ADDR32
> is set, which can do 64-bit syscalls - the subtraction will be
> a negative..
>
>
>> +
>>      info.align_mask = 0;
>>      info.align_offset = pgoff << PAGE_SHIFT;
>>      if (filp) {
>> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
>> index 302f43fd9c28..9a0b89252c52 100644
>> --- a/arch/x86/mm/hugetlbpage.c
>> +++ b/arch/x86/mm/hugetlbpage.c
>> @@ -18,6 +18,7 @@
>>  #include <asm/tlbflush.h>
>>  #include <asm/pgalloc.h>
>>  #include <asm/elf.h>
>> +#include <asm/mpx.h>
>>
>>  #if 0    /* This is just for testing */
>>  struct page *
>> @@ -87,23 +88,38 @@ static unsigned long
>> hugetlb_get_unmapped_area_bottomup(struct file *file,
>>      info.low_limit = get_mmap_base(1);
>>      info.high_limit = in_compat_syscall() ?
>>          tasksize_32bit() : tasksize_64bit();
>> +
>> +    /*
>> +     * If hint address is above DEFAULT_MAP_WINDOW, look for unmapped
>> area
>> +     * in the full address space.
>> +     */
>> +    if (addr > DEFAULT_MAP_WINDOW)
>> +        info.high_limit = TASK_SIZE;
>> +
>>      info.align_mask = PAGE_MASK & ~huge_page_mask(h);
>>      info.align_offset = 0;
>>      return vm_unmapped_area(&info);
>>  }
>>
>>  static unsigned long hugetlb_get_unmapped_area_topdown(struct file
>> *file,
>> -        unsigned long addr0, unsigned long len,
>> +        unsigned long addr, unsigned long len,
>>          unsigned long pgoff, unsigned long flags)
>>  {
>>      struct hstate *h = hstate_file(file);
>>      struct vm_unmapped_area_info info;
>> -    unsigned long addr;
>>
>>      info.flags = VM_UNMAPPED_AREA_TOPDOWN;
>>      info.length = len;
>>      info.low_limit = PAGE_SIZE;
>>      info.high_limit = get_mmap_base(0);
>> +
>> +    /*
>> +     * If hint address is above DEFAULT_MAP_WINDOW, look for unmapped
>> area
>> +     * in the full address space.
>> +     */
>> +    if (addr > DEFAULT_MAP_WINDOW && !in_compat_syscall())
>> +        info.high_limit += TASK_SIZE - DEFAULT_MAP_WINDOW;
>
> ditto
>
>> +
>>      info.align_mask = PAGE_MASK & ~huge_page_mask(h);
>>      info.align_offset = 0;
>>      addr = vm_unmapped_area(&info);
>> @@ -118,7 +134,7 @@ static unsigned long
>> hugetlb_get_unmapped_area_topdown(struct file *file,
>>          VM_BUG_ON(addr != -ENOMEM);
>>          info.flags = 0;
>>          info.low_limit = TASK_UNMAPPED_BASE;
>> -        info.high_limit = TASK_SIZE;
>> +        info.high_limit = DEFAULT_MAP_WINDOW;
>
> ditto about 32-bits
>
>>          addr = vm_unmapped_area(&info);
>>      }
>>
>> @@ -135,6 +151,11 @@ hugetlb_get_unmapped_area(struct file *file,
>> unsigned long addr,
>>
>>      if (len & ~huge_page_mask(h))
>>          return -EINVAL;
>> +
>> +    addr = mpx_unmapped_area_check(addr, len, flags);
>> +    if (IS_ERR_VALUE(addr))
>> +        return addr;
>> +
>>      if (len > TASK_SIZE)
>>          return -ENOMEM;
>>
>> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
>> index 19ad095b41df..d63232a31945 100644
>> --- a/arch/x86/mm/mmap.c
>> +++ b/arch/x86/mm/mmap.c
>> @@ -44,7 +44,7 @@ unsigned long tasksize_32bit(void)
>>
>>  unsigned long tasksize_64bit(void)
>>  {
>> -    return TASK_SIZE_MAX;
>> +    return DEFAULT_MAP_WINDOW;
>>  }
>>
>>  static unsigned long stack_maxrandom_size(unsigned long task_size)
>> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
>> index cd44ae727df7..a26a1b373fd0 100644
>> --- a/arch/x86/mm/mpx.c
>> +++ b/arch/x86/mm/mpx.c
>> @@ -355,10 +355,19 @@ int mpx_enable_management(void)
>>       */
>>      bd_base = mpx_get_bounds_dir();
>>      down_write(&mm->mmap_sem);
>> +
>> +    /* MPX doesn't support addresses above 47-bits yet. */
>> +    if (find_vma(mm, DEFAULT_MAP_WINDOW)) {
>> +        pr_warn_once("%s (%d): MPX cannot handle addresses "
>> +                "above 47-bits. Disabling.",
>> +                current->comm, current->pid);
>> +        ret = -ENXIO;
>> +        goto out;
>> +    }
>>      mm->context.bd_addr = bd_base;
>>      if (mm->context.bd_addr == MPX_INVALID_BOUNDS_DIR)
>>          ret = -ENXIO;
>> -
>> +out:
>>      up_write(&mm->mmap_sem);
>>      return ret;
>>  }
>> @@ -1038,3 +1047,25 @@ void mpx_notify_unmap(struct mm_struct *mm,
>> struct vm_area_struct *vma,
>>      if (ret)
>>          force_sig(SIGSEGV, current);
>>  }
>> +
>> +/* MPX cannot handle addresses above 47-bits yet. */
>> +unsigned long mpx_unmapped_area_check(unsigned long addr, unsigned
>> long len,
>> +        unsigned long flags)
>> +{
>> +    if (!kernel_managing_mpx_tables(current->mm))
>> +        return addr;
>> +    if (addr + len <= DEFAULT_MAP_WINDOW)
>> +        return addr;
>> +    if (flags & MAP_FIXED)
>> +        return -ENOMEM;
>> +
>> +    /*
>> +     * Requested len is larger than whole area we're allowed to map in.
>> +     * Resetting hinting address wouldn't do much good -- fail early.
>> +     */
>> +    if (len > DEFAULT_MAP_WINDOW)
>> +        return -ENOMEM;
>> +
>> +    /* Look for unmap area within DEFAULT_MAP_WINDOW */
>> +    return 0;
>> +}
>>
>
>


-- 
              Dmitry