lkml.org 
[lkml]   [2017]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHv4 2/5] x86/mm: introduce mmap{,_legacy}_base
From
Date
On 02/11/2017 05:13 PM, Thomas Gleixner wrote:
> On Mon, 30 Jan 2017, Dmitry Safonov wrote:
>
>> In the following patch they will be used to compute:
>> - mmap{,_legacy}_base for 64-bit mmap()
>> - mmap_compat{,_legacy}_base for 32-bit mmap()
>>
>> This patch makes it possible to calculate mmap bases for any specified
>> task_size, which is needed to correctly choose the base address for mmap
>> in 32-bit syscalls and 64-bit syscalls.
>
> Please rework your changelogs so they follow the requirements in
> Documentation.... Look for 'this patch' there.

Sure, I'll rewrite the changelogs.

>
> Also order it in a way which makes it clear what the problem is and how
> it's solved.
>
>> +#define STACK_RND_MASK_MODE(native) (0x7ff)
>> #define STACK_RND_MASK (0x7ff)
>>
>> #define ARCH_DLINFO ARCH_DLINFO_IA32
>> @@ -295,7 +296,8 @@ do { \
>> #else /* CONFIG_X86_32 */
>>
>> /* 1GB for 64bit, 8MB for 32bit */
>> -#define STACK_RND_MASK (test_thread_flag(TIF_ADDR32) ? 0x7ff : 0x3fffff)
>> +#define STACK_RND_MASK_MODE(native) ((native) ? 0x3fffff : 0x7ff)
>> +#define STACK_RND_MASK STACK_RND_MASK_MODE(!test_thread_flag(TIF_ADDR32))
>
> I had to look twice what MODE means. The macro name suggests that it
> returns the mode, while it actually returns the mask. That's confusing at
> best. And 'native' is not intuitive either.
>
> #define __STACK_RND_MASK(is64bit) ((is64bit) ? 0x3fffff : 0x7ff)
> #define STACK_RND_MASK __STACK_RND_MASK(!mmap_is_ia32())
>
> That is pretty much clear and it uses mmap_is_ia32() as we do in other
> places. As a consequemce you can use the same macros for 64 and 32 bit.

Thanks, that's more readable. I was tempting to name it more
self-descriptive, but two underscores looks enough.
I'll resend with `is32bit' parameter - this way I'll also get rid of
negaion in a call.
I'll also need to move it down in header to omit forward-declaration of
mmap_is_ia32().

>
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 1be64da0384e..52086e65b422 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -862,7 +862,8 @@ extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
>> * This decides where the kernel will search for a free chunk of vm
>> * space during mmap's.
>> */
>> -#define TASK_UNMAPPED_BASE (PAGE_ALIGN(TASK_SIZE / 3))
>> +#define _TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3))
>> +#define TASK_UNMAPPED_BASE _TASK_UNMAPPED_BASE(TASK_SIZE)
>
> Please use two underscores and align the macros in tabular fashion.
>
> #define __TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3))
> #define TASK_UNMAPPED_BASE __TASK_UNMAPPED_BASE(TASK_SIZE)
>
> That way stuff becomes easy to read.

Ok

>
>> #define KSTK_EIP(task) (task_pt_regs(task)->ip)
>>
>> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
>> index 42063e787717..98be520fd270 100644
>> --- a/arch/x86/mm/mmap.c
>> +++ b/arch/x86/mm/mmap.c
>> @@ -35,12 +35,14 @@ struct va_alignment __read_mostly va_align = {
>> .flags = -1,
>> };
>>
>> -static unsigned long stack_maxrandom_size(void)
>> +static unsigned long stack_maxrandom_size(unsigned long task_size)
>> {
>> unsigned long max = 0;
>> if ((current->flags & PF_RANDOMIZE) &&
>> !(current->personality & ADDR_NO_RANDOMIZE)) {
>> - max = ((-1UL) & STACK_RND_MASK) << PAGE_SHIFT;
>> + max = (-1UL);
>> + max &= STACK_RND_MASK_MODE(task_size == TASK_SIZE_MAX);
>
> That just makes me barf, really. I have to go and lookup how TASK_SIZE_MAX
> is defined in order to read that code. TASK_SIZE_MAX as is does not give a
> hint at all that it means TASK_SIZE_MAX of 64 bit tasks.
>
> You just explained me that you want stuff proper for clarity reasons. So
> what's so wrong with adding a helper inline tasksize_64bit() or such?

I thought about that, but I'll need to redefine it under ifdefs :-/
I mean, for 32-bit native code.
Hmm, I think, if I use is32bit parameter for __STACK_RND_MASK(),
will it be more readable if I just compare to IA32_PAGE_OFFSET here?
Or does it makes sence to introduce tasksize_32bit()?

>
>> + max <<= PAGE_SHIFT;
>> }
>>
>> return max;
>> @@ -51,8 +53,8 @@ static unsigned long stack_maxrandom_size(void)
>> *
>> * Leave an at least ~128 MB hole with possible stack randomization.
>> */
>> -#define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
>> -#define MAX_GAP (TASK_SIZE/6*5)
>> +#define MIN_GAP(task_size) (128*1024*1024UL + stack_maxrandom_size(task_size))
>> +#define MAX_GAP(task_size) (task_size/6*5)
>
> Eew. Yes it's existing macro mess, but there is not point in proliferating
> that. That macro crap is only used in mmap_base() and there is no
> justification for these unreadable macros at all. It just makes stuff
> obfuscated for no reason. Just blindly making it more obfuscated does not
> make it any better.
>
>> static int mmap_is_legacy(void)
>> {
>> @@ -88,16 +90,22 @@ unsigned long arch_mmap_rnd(void)
>> return arch_native_rnd();
>> }
>
> #define SIZE_128M (128 * 1024 * 1024UL)
>
>> -static unsigned long mmap_base(unsigned long rnd)
>> +static unsigned long mmap_base(unsigned long rnd, unsigned long task_size)
>> {
>> unsigned long gap = rlimit(RLIMIT_STACK);
> unsigned long gap_min, gap_max;
>
> /* Add comment what this means */
> gap_min = SIZE_128M + stack_maxrandom_size(task_size);
> /* Explain that ' /6 * 5' magic */
> gap_max = (task_size / 6) * 5;
>
> and use gap_min/gap_max below. That would be too intuitive and readable,
> right?

Haha, will do, thanks again.

>
>> - if (gap < MIN_GAP)
>> - gap = MIN_GAP;
>> - else if (gap > MAX_GAP)
>> - gap = MAX_GAP;
>> + if (gap < MIN_GAP(task_size))
>> + gap = MIN_GAP(task_size);
>> + else if (gap > MAX_GAP(task_size))
>> + gap = MAX_GAP(task_size);
>>
>> - return PAGE_ALIGN(TASK_SIZE - gap - rnd);
>> + return PAGE_ALIGN(task_size - gap - rnd);
>> +}
>> +
>> +static unsigned long mmap_legacy_base(unsigned long rnd,
>> + unsigned long task_size)
>
> Please align the argument in the second line with the argument in the first
> one
>
> static unsigned long mmap_legacy_base(unsigned long rnd,
> unsigned long task_size)

Ok

>
> Thanks,
>
> tglx
>


--
Dmitry

\
 
 \ /
  Last update: 2017-02-13 14:08    [W:0.165 / U:1.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site