Nicholas Piggin [off-list ref] writes:

Excerpts from Christophe Leroy's message of December 9, 2021 8:22 pm:

quoted

Le 09/12/2021 à 11:15, Nicholas Piggin a écrit :

quoted

Excerpts from Christophe Leroy's message of December 9, 2021 3:18 am:

quoted

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.

quoted

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