[lkml]   [2021]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v4 09/10] powerpc/mm: Convert to default topdown mmap layout

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

 \ /
  Last update: 2021-12-09 12:32    [W:0.057 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site