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

[Christophe Leroy]

Le 09/12/2021 à 12:22, Michael Ellerman a écrit :
> Nicholas Piggin <npiggin@xxxxxxxxx> 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@xxxxxx/
>>
>> 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`.
> 

That was the purpose of adding in the change log a reference to commit 
8b8addf891de ("x86/mm/32: Enable full randomization on i386
and X86_32")

All this applies to powerpc as well.

But I can copy paste the changelog of that commit into mine if you think 
it is more explicit.

I agree that old patch was only refering to stack limit, I had no clue 
of everything else at that time.

Christophe