[PATCHv4,1/5] x86/mm: split arch_mmap_rnd() on compat/native versions
diff mbox series

Message ID 20170130120432.6716-2-dsafonov@virtuozzo.com
State New, archived
Headers show
Series
  • Fix compatible mmap() return pointer over 4Gb
Related show

Commit Message

Dmitry Safonov Jan. 30, 2017, 12:04 p.m. UTC
I need those arch_{native,compat}_rnd() to compute separately
random factor for mmap() in compat syscalls for 64-bit binaries
and vice-versa for native syscall in 32-bit compat binaries.
They will be used in the following patches.

Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
 arch/x86/mm/mmap.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

Boris Petkov Feb. 9, 2017, 1:55 p.m. UTC | #1
On Mon, Jan 30, 2017 at 03:04:28PM +0300, Dmitry Safonov wrote:
> I need those arch_{native,compat}_rnd() to compute separately
> random factor for mmap() in compat syscalls for 64-bit binaries
> and vice-versa for native syscall in 32-bit compat binaries.
> They will be used in the following patches.
> 
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> ---
>  arch/x86/mm/mmap.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
> index d2dc0438d654..42063e787717 100644
> --- a/arch/x86/mm/mmap.c
> +++ b/arch/x86/mm/mmap.c
> @@ -65,20 +65,27 @@ static int mmap_is_legacy(void)
>  	return sysctl_legacy_va_layout;
>  }
>  
> -unsigned long arch_mmap_rnd(void)
> +#ifdef CONFIG_COMPAT
> +static unsigned long arch_compat_rnd(void)
>  {
> -	unsigned long rnd;
> +	return (get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1))
> +		<< PAGE_SHIFT;
> +}
> +#endif
>  
> -	if (mmap_is_ia32())
> +static unsigned long arch_native_rnd(void)
> +{
> +	return (get_random_long() & ((1UL << mmap_rnd_bits) - 1)) << PAGE_SHIFT;
> +}
> +
> +unsigned long arch_mmap_rnd(void)
> +{
>  #ifdef CONFIG_COMPAT
> -		rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> -#else
> -		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> +	if (mmap_is_ia32())
> +		return arch_compat_rnd();
>  #endif

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though -
that's generic and arch-specific code intertwined muck. Sad face.
Andy Lutomirski Feb. 9, 2017, 11:06 p.m. UTC | #2
On Thu, Feb 9, 2017 at 5:55 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Jan 30, 2017 at 03:04:28PM +0300, Dmitry Safonov wrote:
>> I need those arch_{native,compat}_rnd() to compute separately
>> random factor for mmap() in compat syscalls for 64-bit binaries
>> and vice-versa for native syscall in 32-bit compat binaries.
>> They will be used in the following patches.
>>
>> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
>> ---
>>  arch/x86/mm/mmap.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
>> index d2dc0438d654..42063e787717 100644
>> --- a/arch/x86/mm/mmap.c
>> +++ b/arch/x86/mm/mmap.c
>> @@ -65,20 +65,27 @@ static int mmap_is_legacy(void)
>>       return sysctl_legacy_va_layout;
>>  }
>>
>> -unsigned long arch_mmap_rnd(void)
>> +#ifdef CONFIG_COMPAT
>> +static unsigned long arch_compat_rnd(void)
>>  {
>> -     unsigned long rnd;
>> +     return (get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1))
>> +             << PAGE_SHIFT;
>> +}
>> +#endif
>>
>> -     if (mmap_is_ia32())
>> +static unsigned long arch_native_rnd(void)
>> +{
>> +     return (get_random_long() & ((1UL << mmap_rnd_bits) - 1)) << PAGE_SHIFT;
>> +}
>> +
>> +unsigned long arch_mmap_rnd(void)
>> +{
>>  #ifdef CONFIG_COMPAT
>> -             rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
>> -#else
>> -             rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
>> +     if (mmap_is_ia32())
>> +             return arch_compat_rnd();
>>  #endif
>
> I can't say that I'm thrilled about the ifdeffery this is adding.
>
> But I can't think of a cleaner approach at a quick glance, though -
> that's generic and arch-specific code intertwined muck. Sad face.

I can, but it could be considerably more churn: get rid of the
compat/native split and do a 32-bit/64-bit split instead.

>
> --
> Regards/Gruss,
>     Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
Thomas Gleixner Feb. 10, 2017, 8:10 p.m. UTC | #3
On Thu, 9 Feb 2017, Borislav Petkov wrote:
> I can't say that I'm thrilled about the ifdeffery this is adding.
> 
> But I can't think of a cleaner approach at a quick glance, though -
> that's generic and arch-specific code intertwined muck. Sad face.

It's trivial enough to do ....

Thanks,

	tglx

---
 arch/x86/mm/mmap.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 
+#ifndef CONFIG_COMPAT
+# define mmap_rnd_compat_bits	mmap_rnd_bits
+#endif
+
 static int mmap_is_legacy(void)
 {
 	if (current->personality & ADDR_COMPAT_LAYOUT)
@@ -66,20 +70,14 @@ static int mmap_is_legacy(void)
 	return sysctl_legacy_va_layout;
 }
 
-unsigned long arch_mmap_rnd(void)
+static unsigned long arch_rnd(unsigned int rndbits)
 {
-	unsigned long rnd;
-
-	if (mmap_is_ia32())
-#ifdef CONFIG_COMPAT
-		rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
-#else
-		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
-#endif
-	else
-		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
+	return (get_random_long() & ((1UL << rndbits) - 1)) << PAGE_SHIFT;
+}
 
-	return rnd << PAGE_SHIFT;
+unsigned long arch_mmap_rnd(void)
+{
+	return arch_rnd(mmap_is_ia32() ? mmap_rnd_compat_bits : mmap_rnd_bits);
 }
 
 static unsigned long mmap_base(unsigned long rnd)
Borislav Petkov Feb. 10, 2017, 8:25 p.m. UTC | #4
On Fri, Feb 10, 2017 at 09:10:30PM +0100, Thomas Gleixner wrote:
> On Thu, 9 Feb 2017, Borislav Petkov wrote:
> > I can't say that I'm thrilled about the ifdeffery this is adding.
> > 
> > But I can't think of a cleaner approach at a quick glance, though -
> > that's generic and arch-specific code intertwined muck. Sad face.
> 
> It's trivial enough to do ....
> 
> Thanks,
> 
> 	tglx
> 
> ---
>  arch/x86/mm/mmap.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> --- a/arch/x86/mm/mmap.c
> +++ b/arch/x86/mm/mmap.c
> @@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
>  #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
>  #define MAX_GAP (TASK_SIZE/6*5)
>  
> +#ifndef CONFIG_COMPAT
> +# define mmap_rnd_compat_bits	mmap_rnd_bits
> +#endif
> +
>  static int mmap_is_legacy(void)
>  {
>  	if (current->personality & ADDR_COMPAT_LAYOUT)
> @@ -66,20 +70,14 @@ static int mmap_is_legacy(void)
>  	return sysctl_legacy_va_layout;
>  }
>  
> -unsigned long arch_mmap_rnd(void)
> +static unsigned long arch_rnd(unsigned int rndbits)
>  {
> -	unsigned long rnd;
> -
> -	if (mmap_is_ia32())
> -#ifdef CONFIG_COMPAT
> -		rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> -#else
> -		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> -#endif
> -	else
> -		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> +	return (get_random_long() & ((1UL << rndbits) - 1)) << PAGE_SHIFT;
> +}
>  
> -	return rnd << PAGE_SHIFT;
> +unsigned long arch_mmap_rnd(void)
> +{
> +	return arch_rnd(mmap_is_ia32() ? mmap_rnd_compat_bits : mmap_rnd_bits);
>  }

Ha! Nice. :-)
Dmitry Safonov Feb. 10, 2017, 9:28 p.m. UTC | #5
2017-02-10 23:10 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
> On Thu, 9 Feb 2017, Borislav Petkov wrote:
>> I can't say that I'm thrilled about the ifdeffery this is adding.
>>
>> But I can't think of a cleaner approach at a quick glance, though -
>> that's generic and arch-specific code intertwined muck. Sad face.
>
> It's trivial enough to do ....
>
> Thanks,
>
>         tglx
>
> ---
>  arch/x86/mm/mmap.c |   22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> --- a/arch/x86/mm/mmap.c
> +++ b/arch/x86/mm/mmap.c
> @@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
>  #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
>  #define MAX_GAP (TASK_SIZE/6*5)
>
> +#ifndef CONFIG_COMPAT
> +# define mmap_rnd_compat_bits  mmap_rnd_bits
> +#endif
> +

>From my POV, I can't say that it's clearer to shadow mmap_compat_bits
like that then to have two functions with native/compat names.
But if you insist, I'll resend patches set with your version.

>  static int mmap_is_legacy(void)
>  {
>         if (current->personality & ADDR_COMPAT_LAYOUT)
> @@ -66,20 +70,14 @@ static int mmap_is_legacy(void)
>         return sysctl_legacy_va_layout;
>  }
>
> -unsigned long arch_mmap_rnd(void)
> +static unsigned long arch_rnd(unsigned int rndbits)
>  {
> -       unsigned long rnd;
> -
> -       if (mmap_is_ia32())
> -#ifdef CONFIG_COMPAT
> -               rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
> -#else
> -               rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> -#endif
> -       else
> -               rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
> +       return (get_random_long() & ((1UL << rndbits) - 1)) << PAGE_SHIFT;
> +}
>
> -       return rnd << PAGE_SHIFT;
> +unsigned long arch_mmap_rnd(void)
> +{
> +       return arch_rnd(mmap_is_ia32() ? mmap_rnd_compat_bits : mmap_rnd_bits);
>  }
>
>  static unsigned long mmap_base(unsigned long rnd)
Thomas Gleixner Feb. 11, 2017, 8:23 a.m. UTC | #6
On Sat, 11 Feb 2017, Dmitry Safonov wrote:

> 2017-02-10 23:10 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
> > On Thu, 9 Feb 2017, Borislav Petkov wrote:
> >> I can't say that I'm thrilled about the ifdeffery this is adding.
> >>
> >> But I can't think of a cleaner approach at a quick glance, though -
> >> that's generic and arch-specific code intertwined muck. Sad face.
> >
> > It's trivial enough to do ....
> >
> > Thanks,
> >
> >         tglx
> >
> > ---
> >  arch/x86/mm/mmap.c |   22 ++++++++++------------
> >  1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > --- a/arch/x86/mm/mmap.c
> > +++ b/arch/x86/mm/mmap.c
> > @@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
> >  #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
> >  #define MAX_GAP (TASK_SIZE/6*5)
> >
> > +#ifndef CONFIG_COMPAT
> > +# define mmap_rnd_compat_bits  mmap_rnd_bits
> > +#endif
> > +
> 
> >From my POV, I can't say that it's clearer to shadow mmap_compat_bits
> like that then to have two functions with native/compat names.
> But if you insist, I'll resend patches set with your version.

You can make that

#ifdef CONFIG_64BIT
# define mmap32_rnd_bits  mmap_compat_rnd_bits
# define mmap64_rnd_bits  mmap_rnd_bits
#else
# define mmap32_rnd_bits  mmap_rnd_bits
# define mmap64_rnd_bits  mmap_rnd_bits
#endif

and use that. That's still way more readable than the unholy ifdef mess.

Thanks,

	tglx
Dmitry Safonov Feb. 13, 2017, 11:12 a.m. UTC | #7
On 02/11/2017 11:23 AM, Thomas Gleixner wrote:
> On Sat, 11 Feb 2017, Dmitry Safonov wrote:
>
>> 2017-02-10 23:10 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
>>> On Thu, 9 Feb 2017, Borislav Petkov wrote:
>>>> I can't say that I'm thrilled about the ifdeffery this is adding.
>>>>
>>>> But I can't think of a cleaner approach at a quick glance, though -
>>>> that's generic and arch-specific code intertwined muck. Sad face.
>>>
>>> It's trivial enough to do ....
>>>
>>> Thanks,
>>>
>>>         tglx
>>>
>>> ---
>>>  arch/x86/mm/mmap.c |   22 ++++++++++------------
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> --- a/arch/x86/mm/mmap.c
>>> +++ b/arch/x86/mm/mmap.c
>>> @@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
>>>  #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
>>>  #define MAX_GAP (TASK_SIZE/6*5)
>>>
>>> +#ifndef CONFIG_COMPAT
>>> +# define mmap_rnd_compat_bits  mmap_rnd_bits
>>> +#endif
>>> +
>>
>> >From my POV, I can't say that it's clearer to shadow mmap_compat_bits
>> like that then to have two functions with native/compat names.
>> But if you insist, I'll resend patches set with your version.
>
> You can make that
>
> #ifdef CONFIG_64BIT
> # define mmap32_rnd_bits  mmap_compat_rnd_bits
> # define mmap64_rnd_bits  mmap_rnd_bits
> #else
> # define mmap32_rnd_bits  mmap_rnd_bits
> # define mmap64_rnd_bits  mmap_rnd_bits
> #endif
>
> and use that. That's still way more readable than the unholy ifdef mess.

Ok, will send this version in v5.
Ping me if you mind me using your SOB for this patch.

>
> Thanks,
>
> 	tglx
>
Thomas Gleixner Feb. 13, 2017, 11:22 a.m. UTC | #8
On Mon, 13 Feb 2017, Dmitry Safonov wrote:
> On 02/11/2017 11:23 AM, Thomas Gleixner wrote:
> > On Sat, 11 Feb 2017, Dmitry Safonov wrote:
> > 
> > > 2017-02-10 23:10 GMT+03:00 Thomas Gleixner <tglx@linutronix.de>:
> > > > On Thu, 9 Feb 2017, Borislav Petkov wrote:
> > > > > I can't say that I'm thrilled about the ifdeffery this is adding.
> > > > > 
> > > > > But I can't think of a cleaner approach at a quick glance, though -
> > > > > that's generic and arch-specific code intertwined muck. Sad face.
> > > > 
> > > > It's trivial enough to do ....
> > > > 
> > > > Thanks,
> > > > 
> > > >         tglx
> > > > 
> > > > ---
> > > >  arch/x86/mm/mmap.c |   22 ++++++++++------------
> > > >  1 file changed, 10 insertions(+), 12 deletions(-)
> > > > 
> > > > --- a/arch/x86/mm/mmap.c
> > > > +++ b/arch/x86/mm/mmap.c
> > > > @@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
> > > >  #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
> > > >  #define MAX_GAP (TASK_SIZE/6*5)
> > > > 
> > > > +#ifndef CONFIG_COMPAT
> > > > +# define mmap_rnd_compat_bits  mmap_rnd_bits
> > > > +#endif
> > > > +
> > > 
> > > >From my POV, I can't say that it's clearer to shadow mmap_compat_bits
> > > like that then to have two functions with native/compat names.
> > > But if you insist, I'll resend patches set with your version.
> > 
> > You can make that
> > 
> > #ifdef CONFIG_64BIT
> > # define mmap32_rnd_bits  mmap_compat_rnd_bits
> > # define mmap64_rnd_bits  mmap_rnd_bits
> > #else
> > # define mmap32_rnd_bits  mmap_rnd_bits
> > # define mmap64_rnd_bits  mmap_rnd_bits
> > #endif
> > 
> > and use that. That's still way more readable than the unholy ifdef mess.
> 
> Ok, will send this version in v5.
> Ping me if you mind me using your SOB for this patch.

Suggested-by is enough

Patch
diff mbox series

diff --git a/arch/x86/mm/mmap.c b/arch/x86/mm/mmap.c
index d2dc0438d654..42063e787717 100644
--- a/arch/x86/mm/mmap.c
+++ b/arch/x86/mm/mmap.c
@@ -65,20 +65,27 @@  static int mmap_is_legacy(void)
 	return sysctl_legacy_va_layout;
 }
 
-unsigned long arch_mmap_rnd(void)
+#ifdef CONFIG_COMPAT
+static unsigned long arch_compat_rnd(void)
 {
-	unsigned long rnd;
+	return (get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1))
+		<< PAGE_SHIFT;
+}
+#endif
 
-	if (mmap_is_ia32())
+static unsigned long arch_native_rnd(void)
+{
+	return (get_random_long() & ((1UL << mmap_rnd_bits) - 1)) << PAGE_SHIFT;
+}
+
+unsigned long arch_mmap_rnd(void)
+{
 #ifdef CONFIG_COMPAT
-		rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
-#else
-		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
+	if (mmap_is_ia32())
+		return arch_compat_rnd();
 #endif
-	else
-		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
 
-	return rnd << PAGE_SHIFT;
+	return arch_native_rnd();
 }
 
 static unsigned long mmap_base(unsigned long rnd)