2018-09-28 04:44:39

by Andy Lutomirski

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

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: [email protected]
Cc: Martin Schwidefsky <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Finn Thain <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
---

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



2018-09-28 06:22:55

by Thomas Gleixner

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

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
>
>

2018-09-28 06:25:12

by Thomas Gleixner

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

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

2018-09-28 07:13:41

by Geert Uytterhoeven

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

Hi Thomas,

On Fri, Sep 28, 2018 at 8:21 AM Thomas Gleixner <[email protected]> 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 -- [email protected]

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

2018-09-28 08:32:16

by Thomas Gleixner

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

On Fri, 28 Sep 2018, Geert Uytterhoeven wrote:
>
> On Fri, Sep 28, 2018 at 8:21 AM Thomas Gleixner <[email protected]> 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

2018-09-28 08:39:02

by Martin Schwidefsky

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

On Fri, 28 Sep 2018 09:12:10 +0200
Geert Uytterhoeven <[email protected]> wrote:

> Hi Thomas,
>
> On Fri, Sep 28, 2018 at 8:21 AM Thomas Gleixner <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc: Finn Thain <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
Signed-off-by: Thomas Gleixner <[email protected]>


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.


2018-09-28 08:43:06

by Thomas Gleixner

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

On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> On Fri, 28 Sep 2018 09:12:10 +0200
> Geert Uytterhoeven <[email protected]> wrote:
> > I don't know if that has happened, and whether it would work on s390 now.
>
> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> Author: Heiko Carstens <[email protected]>
> 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 <[email protected]>
> Cc: Finn Thain <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> Signed-off-by: Thomas Gleixner <[email protected]>
>
>
> 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

2018-09-28 14:12:10

by Andy Lutomirski

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



> On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <[email protected]> wrote:
>
>> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
>> On Fri, 28 Sep 2018 09:12:10 +0200
>> Geert Uytterhoeven <[email protected]> wrote:
>>> I don't know if that has happened, and whether it would work on s390 now.
>>
>> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
>> Author: Heiko Carstens <[email protected]>
>> 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 <[email protected]>
>> Cc: Finn Thain <[email protected]>
>> Cc: Geert Uytterhoeven <[email protected]>
>> Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
>> Signed-off-by: Thomas Gleixner <[email protected]>
>>
>>
>> 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?

2018-09-28 14:41:37

by Thomas Gleixner

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

On Fri, 28 Sep 2018, Andy Lutomirski wrote:
> > On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <[email protected]> wrote:
> >
> >> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> >> On Fri, 28 Sep 2018 09:12:10 +0200
> >> Geert Uytterhoeven <[email protected]> wrote:
> >>> I don't know if that has happened, and whether it would work on s390 now.
> >>
> >> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> >> Author: Heiko Carstens <[email protected]>
> >> 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 <[email protected]>
> >> Cc: Finn Thain <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> >> Signed-off-by: Thomas Gleixner <[email protected]>
> >>
> >>
> >> 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

2018-09-28 14:54:27

by Martin Schwidefsky

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

On Fri, 28 Sep 2018 07:11:44 -0700
Andy Lutomirski <[email protected]> wrote:

> > On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <[email protected]> wrote:
> >
> >> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> >> On Fri, 28 Sep 2018 09:12:10 +0200
> >> Geert Uytterhoeven <[email protected]> wrote:
> >>> I don't know if that has happened, and whether it would work on s390 now.
> >>
> >> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> >> Author: Heiko Carstens <[email protected]>
> >> 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 <[email protected]>
> >> Cc: Finn Thain <[email protected]>
> >> Cc: Geert Uytterhoeven <[email protected]>
> >> Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> >> Signed-off-by: Thomas Gleixner <[email protected]>
> >>
> >>
> >> 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.


2018-09-28 18:03:13

by Andy Lutomirski

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

On Fri, Sep 28, 2018 at 7:53 AM Martin Schwidefsky
<[email protected]> wrote:
>
> On Fri, 28 Sep 2018 07:11:44 -0700
> Andy Lutomirski <[email protected]> wrote:
>
> > > On Sep 28, 2018, at 1:42 AM, Thomas Gleixner <[email protected]> wrote:
> > >
> > >> On Fri, 28 Sep 2018, Martin Schwidefsky wrote:
> > >> On Fri, 28 Sep 2018 09:12:10 +0200
> > >> Geert Uytterhoeven <[email protected]> wrote:
> > >>> I don't know if that has happened, and whether it would work on s390 now.
> > >>
> > >> commit 03b8c7b623c80af264c4c8d6111e5c6289933666
> > >> Author: Heiko Carstens <[email protected]>
> > >> 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 <[email protected]>
> > >> Cc: Finn Thain <[email protected]>
> > >> Cc: Geert Uytterhoeven <[email protected]>
> > >> Link: http://lkml.kernel.org/r/20140302120947.GA3641@osiris
> > >> Signed-off-by: Thomas Gleixner <[email protected]>
> > >>
> > >>
> > >> 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.

2018-09-28 19:39:14

by Thomas Gleixner

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

On Fri, 28 Sep 2018, Andy Lutomirski wrote:
> On Fri, Sep 28, 2018 at 7:53 AM Martin Schwidefsky <[email protected]> wrote:
> > On Fri, 28 Sep 2018 07:11:44 -0700 Andy Lutomirski <[email protected]> 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

2018-09-28 20:20:21

by Max Filippov

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

On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <[email protected]> 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

2018-09-28 20:28:28

by Thomas Gleixner

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

On Fri, 28 Sep 2018, Max Filippov wrote:

> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <[email protected]> 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

2018-09-28 21:08:54

by Andy Lutomirski

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



> On Sep 28, 2018, at 1:26 PM, Thomas Gleixner <[email protected]> wrote:
>
>> On Fri, 28 Sep 2018, Max Filippov wrote:
>>
>>> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <[email protected]> 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.

2018-09-28 21:37:33

by Max Filippov

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

On Fri, Sep 28, 2018 at 2:08 PM, Andy Lutomirski <[email protected]> wrote:
>
>
>> On Sep 28, 2018, at 1:26 PM, Thomas Gleixner <[email protected]> wrote:
>>
>>> On Fri, 28 Sep 2018, Max Filippov wrote:
>>>
>>>> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <[email protected]> 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

2018-09-29 06:33:22

by Thomas Gleixner

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

On Fri, 28 Sep 2018, Max Filippov wrote:
> On Fri, Sep 28, 2018 at 2:08 PM, Andy Lutomirski <[email protected]> wrote:
> >> On Sep 28, 2018, at 1:26 PM, Thomas Gleixner <[email protected]> wrote:
> >>
> >>> On Fri, 28 Sep 2018, Max Filippov wrote:
> >>>
> >>>> On Fri, Sep 28, 2018 at 11:02 AM, Andy Lutomirski <[email protected]> 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