[PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Andy Lutomirski]

futex_detect_cmpxchg() checks whether cmpxchg is available by trying
it on the NULL pointer and seeing what the error code is (EFAULT vs
ENOSYS).  This happens with KERNEL_DS set, which is impolite: while
the NULL *user* pointer is definitely invalid when there is no user
program running, the NULL *kernel* pointer seems more like a
programming error than a safe place to do an intentionally-failing
access.  An upcoming hardening series I'm working on causes the
existing code to OOPS, because it considers any failed uaccess with
KERNEL_DS to be a sign of a bug.

Explicitly set USER_DS to avoid this problem.

Cc: linux-s390@vger.kernel.org
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Finn Thain <fthain@telegraphics.com.au>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
---

I have a couple questions here:

 - Is this actually okay on all architectures?  That is, are there
   cases where we'll screw up if we fail a USER_DS access this early?
   s390 stands out as the obvious special case (where USER_DS is not
   than just a subset of KERNEL_DS), but s390 opts out.

 - Why doesn't x86 set HAVE_FUTEX_CMPXCHG?  Or do we still support
   some 32-bit configurations that don't have cmpxchg and don't know
   about it at compile time?

 kernel/futex.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/futex.c b/kernel/futex.c
index 11fc3bb456d6..16bd3e72602a 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -3593,6 +3593,7 @@ static void __init futex_detect_cmpxchg(void)
 {
 #ifndef CONFIG_HAVE_FUTEX_CMPXCHG
 	u32 curval;
+	mm_segment_t old_seg;
 
 	/*
 	 * This will fail and we want it. Some arch implementations do
@@ -3604,8 +3605,11 @@ static void __init futex_detect_cmpxchg(void)
 	 * implementation, the non-functional ones will return
 	 * -ENOSYS.
 	 */
+	old_seg = get_fs();
+	set_fs(USER_DS);
 	if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
 		futex_cmpxchg_enabled = 1;
+	set_fs(old_seg);
 #endif
 }
 
-- 
2.17.1

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

On Thu, 27 Sep 2018, Andy Lutomirski wrote:
> I have a couple questions here:
> 
>  - Is this actually okay on all architectures?  That is, are there
>    cases where we'll screw up if we fail a USER_DS access this early?
>    s390 stands out as the obvious special case (where USER_DS is not
>    than just a subset of KERNEL_DS), but s390 opts out.
>
>  - Why doesn't x86 set HAVE_FUTEX_CMPXCHG?  Or do we still support
>    some 32-bit configurations that don't have cmpxchg and don't know
>    about it at compile time?

I'm not entirely sure. Have to dig into the details. I assume S390 just can
set it though.

Thanks,

	tglx

>  kernel/futex.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 11fc3bb456d6..16bd3e72602a 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -3593,6 +3593,7 @@ static void __init futex_detect_cmpxchg(void)
>  {
>  #ifndef CONFIG_HAVE_FUTEX_CMPXCHG
>  	u32 curval;
> +	mm_segment_t old_seg;
>  
>  	/*
>  	 * This will fail and we want it. Some arch implementations do
> @@ -3604,8 +3605,11 @@ static void __init futex_detect_cmpxchg(void)
>  	 * implementation, the non-functional ones will return
>  	 * -ENOSYS.
>  	 */
> +	old_seg = get_fs();
> +	set_fs(USER_DS);
>  	if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
>  		futex_cmpxchg_enabled = 1;
> +	set_fs(old_seg);
>  #endif
>  }
>  
> -- 
> 2.17.1
> 
> 

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

On Fri, 28 Sep 2018, Thomas Gleixner wrote:

> On Thu, 27 Sep 2018, Andy Lutomirski wrote:
> > I have a couple questions here:
> > 
> >  - Is this actually okay on all architectures?  That is, are there
> >    cases where we'll screw up if we fail a USER_DS access this early?
> >    s390 stands out as the obvious special case (where USER_DS is not
> >    than just a subset of KERNEL_DS), but s390 opts out.
> >
> >  - Why doesn't x86 set HAVE_FUTEX_CMPXCHG?  Or do we still support
> >    some 32-bit configurations that don't have cmpxchg and don't know
> >    about it at compile time?
> 
> I'm not entirely sure. Have to dig into the details. I assume S390 just can
> set it though.

x86 as well. It's supported from 486 onwards and we ripped out 386 years ago.

Thanks,

	tglx

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Geert Uytterhoeven]

Hi Thomas,

On Fri, Sep 28, 2018 at 8:21 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 27 Sep 2018, Andy Lutomirski wrote:
> > I have a couple questions here:
> >
> >  - Is this actually okay on all architectures?  That is, are there
> >    cases where we'll screw up if we fail a USER_DS access this early?
> >    s390 stands out as the obvious special case (where USER_DS is not
> >    than just a subset of KERNEL_DS), but s390 opts out.
> >
> >  - Why doesn't x86 set HAVE_FUTEX_CMPXCHG?  Or do we still support
> >    some 32-bit configurations that don't have cmpxchg and don't know
> >    about it at compile time?
>
> I'm not entirely sure. Have to dig into the details. I assume S390 just can
> set it though.

Not sure. My "[PATCH] futex: Switch to USER_DS for futex test"
(https://www.spinics.net/lists/stable/msg28846.html), which is
basically the same
as this patch, broke s390, so it was never merged.

See "[BUG -next] "futex: switch to USER_DS for futex test" breaks s390"
(https://www.spinics.net/lists/linux-next/msg27902.html)

Heiko said:
| Martin and I discussed this today and we will change the s390 code so that
| it will also survive very early USER_DS accesses (without valid current->mm)
| since we also discovered a couple of other oddities in our code.

I don't know if that has happened, and whether it would work on s390 now.

> >  kernel/futex.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index 11fc3bb456d6..16bd3e72602a 100644
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -3593,6 +3593,7 @@ static void __init futex_detect_cmpxchg(void)
> >  {
> >  #ifndef CONFIG_HAVE_FUTEX_CMPXCHG
> >       u32 curval;
> > +     mm_segment_t old_seg;
> >
> >       /*
> >        * This will fail and we want it. Some arch implementations do
> > @@ -3604,8 +3605,11 @@ static void __init futex_detect_cmpxchg(void)
> >        * implementation, the non-functional ones will return
> >        * -ENOSYS.
> >        */
> > +     old_seg = get_fs();
> > +     set_fs(USER_DS);
> >       if (cmpxchg_futex_value_locked(&curval, NULL, 0, 0) == -EFAULT)
> >               futex_cmpxchg_enabled = 1;
> > +     set_fs(old_seg);
> >  #endif
> >  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

On Fri, 28 Sep 2018, Geert Uytterhoeven wrote:
> 
> On Fri, Sep 28, 2018 at 8:21 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 27 Sep 2018, Andy Lutomirski wrote:
> > > I have a couple questions here:
> > >
> > >  - Is this actually okay on all architectures?  That is, are there
> > >    cases where we'll screw up if we fail a USER_DS access this early?
> > >    s390 stands out as the obvious special case (where USER_DS is not
> > >    than just a subset of KERNEL_DS), but s390 opts out.
> > >
> > >  - Why doesn't x86 set HAVE_FUTEX_CMPXCHG?  Or do we still support
> > >    some 32-bit configurations that don't have cmpxchg and don't know
> > >    about it at compile time?
> >
> > I'm not entirely sure. Have to dig into the details. I assume S390 just can
> > set it though.
> 
> Not sure. My "[PATCH] futex: Switch to USER_DS for futex test"
> (https://www.spinics.net/lists/stable/msg28846.html), which is
> basically the same
> as this patch, broke s390, so it was never merged.
> 
> See "[BUG -next] "futex: switch to USER_DS for futex test" breaks s390"
> (https://www.spinics.net/lists/linux-next/msg27902.html)
> 
> Heiko said:
> | Martin and I discussed this today and we will change the s390 code so that
> | it will also survive very early USER_DS accesses (without valid current->mm)
> | since we also discovered a couple of other oddities in our code.
> 
> I don't know if that has happened, and whether it would work on s390 now.

Duh yes, forgot about that one. But as S390 always has cmpxchg it simply
can set HAVE_FUTEX_CMPXCHG which avoids the check completely.

Surely they want to fix the other oddities or have done so already :)

Thanks,

	tglx

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Martin Schwidefsky]

On Fri, 28 Sep 2018 09:12:10 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Thomas,
> 
> On Fri, Sep 28, 2018 at 8:21 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 27 Sep 2018, Andy Lutomirski wrote:  
> > > I have a couple questions here:
> > >
> > >  - Is this actually okay on all architectures?  That is, are there
> > >    cases where we'll screw up if we fail a USER_DS access this early?
> > >    s390 stands out as the obvious special case (where USER_DS is not
> > >    than just a subset of KERNEL_DS), but s390 opts out.
> > >
> > >  - Why doesn't x86 set HAVE_FUTEX_CMPXCHG?  Or do we still support
> > >    some 32-bit configurations that don't have cmpxchg and don't know
> > >    about it at compile time?  
> >
> > I'm not entirely sure. Have to dig into the details. I assume S390 just can
> > set it though.  
> 
> Not sure. My "[PATCH] futex: Switch to USER_DS for futex test"
> (https://www.spinics.net/lists/stable/msg28846.html), which is
> basically the same
> as this patch, broke s390, so it was never merged.
> 
> See "[BUG -next] "futex: switch to USER_DS for futex test" breaks s390"
> (https://www.spinics.net/lists/linux-next/msg27902.html)
> 
> Heiko said:
> | Martin and I discussed this today and we will change the s390 code so that
> | it will also survive very early USER_DS accesses (without valid current->mm)
> | since we also discovered a couple of other oddities in our code.
> 
> I don't know if that has happened, and whether it would work on s390 now.

commit 03b8c7b623c80af264c4c8d6111e5c6289933666
Author: Heiko Carstens <heiko.carstens@de.ibm.com>
Date:   Sun Mar 2 13:09:47 2014 +0100

    futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test
    
    If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
    is no runtime check necessary, allow to skip the test within futex_init().
    
    This allows to get rid of some code which would always give the same result,
    and also allows the compiler to optimize a couple of if statements away.
    
    Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
    Cc: Finn Thain <fthain@telegraphics.com.au>
    Cc: Geert Uytterhoeven <geert@linux-m68k.org>
    Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>


Heiko created the CONFIG_HAVE_FUTEX_CMPXCHG to get around this issue.
We just skip the runtime check as well as arc, m68k and sh. Not sure
about xtensa, the set it config option only for !MMU.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> On Fri, 28 Sep 2018 09:12:10 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > I don't know if that has happened, and whether it would work on s390 now.
> 
> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> Date:   Sun Mar 2 13:09:47 2014 +0100
> 
>     futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test
>     
>     If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
>     is no runtime check necessary, allow to skip the test within futex_init().
>     
>     This allows to get rid of some code which would always give the same result,
>     and also allows the compiler to optimize a couple of if statements away.
>     
>     Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>     Cc: Finn Thain <fthain@telegraphics.com.au>
>     Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>     Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> 
> Heiko created the CONFIG_HAVE_FUTEX_CMPXCHG to get around this issue.
> We just skip the runtime check as well as arc, m68k and sh. Not sure
> about xtensa, the set it config option only for !MMU.

Duh. grep would have told me. -ENOTENOUGHCOFFEE

Thanks,

	tglx

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Andy Lutomirski]

> On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
>> On Fri, 28 Sep 2018 09:12:10 +0200
>> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> I don't know if that has happened, and whether it would work on s390 now.
>> 
>> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
>> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Date:   Sun Mar 2 13:09:47 2014 +0100
>> 
>>    futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test
>> 
>>    If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
>>    is no runtime check necessary, allow to skip the test within futex_init().
>> 
>>    This allows to get rid of some code which would always give the same result,
>>    and also allows the compiler to optimize a couple of if statements away.
>> 
>>    Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>>    Cc: Finn Thain <fthain@telegraphics.com.au>
>>    Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>    Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
>>    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> 
>> 
>> Heiko created the CONFIG_HAVE_FUTEX_CMPXCHG to get around this issue.
>> We just skip the runtime check as well as arc, m68k and sh. Not sure
>> about xtensa, the set it config option only for !MMU.
> 
> Duh. grep would have told me. -ENOTENOUGHCOFFEE
> 
> 

There’s another way to skin this cat: keep KERNEL_DS but pass a valid pointer. I don’t suppose you remember why you didn’t do that?

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

On Fri, 28 Sep 2018, Andy Lutomirski wrote:
> > On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> >> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> >> On Fri, 28 Sep 2018 09:12:10 +0200
> >> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> I don't know if that has happened, and whether it would work on s390 now.
> >> 
> >> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> >> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> >> Date:   Sun Mar 2 13:09:47 2014 +0100
> >> 
> >>    futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test
> >> 
> >>    If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
> >>    is no runtime check necessary, allow to skip the test within futex_init().
> >> 
> >>    This allows to get rid of some code which would always give the same result,
> >>    and also allows the compiler to optimize a couple of if statements away.
> >> 
> >>    Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> >>    Cc: Finn Thain <fthain@telegraphics.com.au>
> >>    Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> >>    Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> >>    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> 
> >> 
> >> Heiko created the CONFIG_HAVE_FUTEX_CMPXCHG to get around this issue.
> >> We just skip the runtime check as well as arc, m68k and sh. Not sure
> >> about xtensa, the set it config option only for !MMU.
> > 
> > Duh. grep would have told me. -ENOTENOUGHCOFFEE
> > 
> > 
> There’s another way to skin this cat: keep KERNEL_DS but pass a valid
> pointer. I don’t suppose you remember why you didn’t do that?

IIRC, there was an issue with extra checks in some architectures when you
handed in a kernel address spitting warnings or such. That's probably gone
by now, but I can't tell for sure. At least the requirement to do runtime
detection for x86 is gone. Don't know if any other architecture still has
it.

Thanks,

	tglx

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Martin Schwidefsky]

On Fri, 28 Sep 2018 07:11:44 -0700
Andy Lutomirski <luto@amacapital.net> wrote:

> > On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >   
> >> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> >> On Fri, 28 Sep 2018 09:12:10 +0200
> >> Geert Uytterhoeven <geert@linux-m68k.org> wrote:  
> >>> I don't know if that has happened, and whether it would work on s390 now.  
> >> 
> >> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> >> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> >> Date:   Sun Mar 2 13:09:47 2014 +0100
> >> 
> >>    futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test
> >> 
> >>    If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
> >>    is no runtime check necessary, allow to skip the test within futex_init().
> >> 
> >>    This allows to get rid of some code which would always give the same result,
> >>    and also allows the compiler to optimize a couple of if statements away.
> >> 
> >>    Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> >>    Cc: Finn Thain <fthain@telegraphics.com.au>
> >>    Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> >>    Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> >>    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> 
> >> 
> >> Heiko created the CONFIG_HAVE_FUTEX_CMPXCHG to get around this issue.
> >> We just skip the runtime check as well as arc, m68k and sh. Not sure
> >> about xtensa, the set it config option only for !MMU.  
> > 
> > Duh. grep would have told me. -ENOTENOUGHCOFFEE
> > 
> >   
> 
> There’s another way to skin this cat: keep KERNEL_DS but pass a valid pointer.
> I don’t suppose you remember why you didn’t do that?

No, I don't remember. To use a valid kernel pointer with KERNEL_DS and
then test for == 0 (vs -ENOSYS) imho should work.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Andy Lutomirski]

On Fri, Sep 28, 2018 at 7:53 AM Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
>
> On Fri, 28 Sep 2018 07:11:44 -0700
> Andy Lutomirski <luto@amacapital.net> wrote:
>
> > > On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > >> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> > >> On Fri, 28 Sep 2018 09:12:10 +0200
> > >> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >>> I don't know if that has happened, and whether it would work on s390 now.
> > >>
> > >> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> > >> Author: Heiko Carstens <heiko.carstens@de.ibm.com>
> > >> Date:   Sun Mar 2 13:09:47 2014 +0100
> > >>
> > >>    futex: Allow architectures to skip futex_atomic_cmpxchg_inatomic() test
> > >>
> > >>    If an architecture has futex_atomic_cmpxchg_inatomic() implemented and there
> > >>    is no runtime check necessary, allow to skip the test within futex_init().
> > >>
> > >>    This allows to get rid of some code which would always give the same result,
> > >>    and also allows the compiler to optimize a couple of if statements away.
> > >>
> > >>    Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > >>    Cc: Finn Thain <fthain@telegraphics.com.au>
> > >>    Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > >>    Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> > >>    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > >>
> > >>
> > >> Heiko created the CONFIG_HAVE_FUTEX_CMPXCHG to get around this issue.
> > >> We just skip the runtime check as well as arc, m68k and sh. Not sure
> > >> about xtensa, the set it config option only for !MMU.
> > >
> > > Duh. grep would have told me. -ENOTENOUGHCOFFEE
> > >
> > >
> >
> > There’s another way to skin this cat: keep KERNEL_DS but pass a valid pointer.
> > I don’t suppose you remember why you didn’t do that?
>
> No, I don't remember. To use a valid kernel pointer with KERNEL_DS and
> then test for == 0 (vs -ENOSYS) imho should work.
>

There may be a much nicer solution.  Unless I missed something, only
mips and xtensa even have the possibility of cmpxchg being missing.
We could just make those arches supply a futex-detecting helper.

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On Fri, 28 Sep 2018, Andy Lutomirski wrote:
> On Fri, Sep 28, 2018 at 7:53 AM Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
> > On Fri, 28 Sep 2018 07:11:44 -0700 Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > > There’s another way to skin this cat: keep KERNEL_DS but pass a valid pointer.
> > > I don’t suppose you remember why you didn’t do that?
> >
> > No, I don't remember. To use a valid kernel pointer with KERNEL_DS and
> > then test for == 0 (vs -ENOSYS) imho should work.
> >
> 
> There may be a much nicer solution.  Unless I missed something, only
> mips and xtensa even have the possibility of cmpxchg being missing.
> We could just make those arches supply a futex-detecting helper.

That makes sense.

Thanks,

	tglx

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Max Filippov]

On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> There may be a much nicer solution.  Unless I missed something, only
> mips and xtensa even have the possibility of cmpxchg being missing.
> We could just make those arches supply a futex-detecting helper.

In case of xtensa availability of cmpxchg is known at build time.

-- 
Thanks.
-- Max

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

On Fri, 28 Sep 2018, Max Filippov wrote:

> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> > There may be a much nicer solution.  Unless I missed something, only
> > mips and xtensa even have the possibility of cmpxchg being missing.
> > We could just make those arches supply a futex-detecting helper.
> 
> In case of xtensa availability of cmpxchg is known at build time.

That makes it even simpler. Could you provide a patch which selects
CONFIG_HAVE_FUTEX_CMPXCHG for the right set of CPUs please?

Thanks,

	tglx

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Andy Lutomirski]

> On Sep 28, 2018, at 1:26 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> On Fri, 28 Sep 2018, Max Filippov wrote:
>> 
>>> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> There may be a much nicer solution.  Unless I missed something, only
>>> mips and xtensa even have the possibility of cmpxchg being missing.
>>> We could just make those arches supply a futex-detecting helper.
>> 
>> In case of xtensa availability of cmpxchg is known at build time.
> 
> That makes it even simpler. Could you provide a patch which selects
> CONFIG_HAVE_FUTEX_CMPXCHG for the right set of CPUs please?
> 
> 

I think that’s the wrong approach, since it won’t cover mips. How about adding this to mips and xtensa only:

static inline void arch_have_futex_cmpxchg(void) {...};
#define arch_have_futex_cmpxchg arch_have_futex_cmpxchg

And getting rid of the config option.

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Max Filippov]

On Fri, Sep 28, 2018 at 2:08 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>> On Sep 28, 2018, at 1:26 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>>> On Fri, 28 Sep 2018, Max Filippov wrote:
>>>
>>>> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> There may be a much nicer solution.  Unless I missed something, only
>>>> mips and xtensa even have the possibility of cmpxchg being missing.
>>>> We could just make those arches supply a futex-detecting helper.
>>>
>>> In case of xtensa availability of cmpxchg is known at build time.
>>
>> That makes it even simpler. Could you provide a patch which selects
>> CONFIG_HAVE_FUTEX_CMPXCHG for the right set of CPUs please?
>>
>>
>
> I think that’s the wrong approach, since it won’t cover mips. How about adding this to mips and xtensa only:
>
> static inline void arch_have_futex_cmpxchg(void) {...};
> #define arch_have_futex_cmpxchg arch_have_futex_cmpxchg
>
> And getting rid of the config option.

I'd rather do that, given that defining Kconfig entries that describe
parts of xtensa configuration is somewhat awkward and redundant.

-- 
Thanks.
-- Max

Re: [PATCH] futex: Set USER_DS for the futex_detect_cmpxchg() test

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Fri, 28 Sep 2018, Max Filippov wrote:
> On Fri, Sep 28, 2018 at 2:08 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >> On Sep 28, 2018, at 1:26 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >>> On Fri, 28 Sep 2018, Max Filippov wrote:
> >>>
> >>>> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> >>>> There may be a much nicer solution.  Unless I missed something, only
> >>>> mips and xtensa even have the possibility of cmpxchg being missing.
> >>>> We could just make those arches supply a futex-detecting helper.
> >>>
> >>> In case of xtensa availability of cmpxchg is known at build time.
> >>
> >> That makes it even simpler. Could you provide a patch which selects
> >> CONFIG_HAVE_FUTEX_CMPXCHG for the right set of CPUs please?
> >>
> >>
> >
> > I think that’s the wrong approach, since it won’t cover mips. How about adding this to mips and xtensa only:
> >
> > static inline void arch_have_futex_cmpxchg(void) {...};
> > #define arch_have_futex_cmpxchg arch_have_futex_cmpxchg
> >
> > And getting rid of the config option.
> 
> I'd rather do that, given that defining Kconfig entries that describe
> parts of xtensa configuration is somewhat awkward and redundant.

Fair enough.

Thanks,

	tglx