Re: [PATCH v4 09/10] powerpc/mm: Convert to default topdown mmap layout

[Michael Ellerman <mpe@ellerman.id.au>]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm:
>> 
>> 
>> Le 09/12/2021 à 11:15, Nicholas Piggin a écrit :
>>> Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am:
>>>> Select CONFIG_ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT and
>>>> remove arch/powerpc/mm/mmap.c
>>>>
>>>> This change provides standard randomisation of mmaps.
>>>>
>>>> See commit 8b8addf891de ("x86/mm/32: Enable full randomization on i386
>>>> and X86_32") for all the benefits of mmap randomisation.
>>> 
>>> The justification seems pretty reasonable.
>>> 
>>>>
>>>> Comparison between powerpc implementation and the generic one:
>>>> - mmap_is_legacy() is identical.
>>>> - arch_mmap_rnd() does exactly the same allthough it's written
>>>> slightly differently.
>>>> - MIN_GAP and MAX_GAP are identical.
>>>> - mmap_base() does the same but uses STACK_RND_MASK which provides
>>>> the same values as stack_maxrandom_size().
>>>> - arch_pick_mmap_layout() is almost identical. The only difference
>>>> is that it also adds the random factor to mm->mmap_base in legacy mode.
>>>>
>>>> That last point is what provides the standard randomisation of mmaps.
>>> 
>>> Thanks for describing it. Could you add random_factor to mmap_base for
>>> the legacy path for powerpc as a 2-line change that adds the legacy
>>> randomisation. And then this bigger patch would be closer to a no-op.
>>> 
>> 
>> You mean you would like to see the following patch before doing the 
>> convert ?
>> 
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7dabf1cbde67a346a187881d4f0bd17347e0334a.1533732583.git.christophe.leroy@c-s.fr/
>
> Yes.

My comment at the time was:

  Basically mmap_is_legacy() tells you if any of these is true:
  
   - process has the ADDR_COMPAT_LAYOUT personality
   - global legacy_va_layout sysctl is enabled
   - stack is unlimited

  And we only want to change the behaviour for the stack. Or at least the
  change log of your patch only talks about the stack limit, not the
  others.
  
  Possibly we should just enable randomisation for all three of those
  cases, but if so we must spell it out in the patch.
  
  It'd also be good to see the output of /proc/x/maps for some processes
  before and after, to show what actually changes.


From: https://github.com/linuxppc/issues/issues/59#issuecomment-502066947


So I think at least the change log on that patch still needs updating to
be clear that it's changing behaviour for all mmap_is_legacy() cases,
not just the stack unlimited case.

There's also a risk changing the mmap legacy behaviour breaks something.
But we are at least matching the behaviour of other architectures, and
there is also an escape hatch in the form of `setarch -R`.

cheers