* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Michal Suchánek @ 2018-10-31 22:41 UTC (permalink / raw)
To: Tulio Magno Quites Machado Filho
Cc: Florian Weimer, Lynn A. Boger, linuxppc-dev, linux-mm
In-Reply-To: <87bm79n57l.fsf@linux.ibm.com>
On Wed, 31 Oct 2018 19:04:14 -0300
Tulio Magno Quites Machado Filho <tuliom@ascii.art.br> wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
> > * Tulio Magno Quites Machado Filho:
> >
> >> I wonder if this is restricted to linker that Golang uses.
> >> Were you able to reproduce the same problem with Binutils'
> >> linker?
> >
> > The example is carefully constructed to use the external linker. It
> > invokes gcc, which then invokes the BFD linker in my case.
>
> Indeed. That question was unnecessary. :-D
>
> > Based on the relocations, I assume there is only so much the linker
> > can do here. I'm amazed that it produces an executable at all, let
> > alone one that runs correctly on some kernel versions!
>
> Agreed. That isn't expected to work. Both the compiler and the
> linker have to generate PIE for it to work.
>
> > I assume that the Go toolchain simply lacks PIE support on
> > ppc64le.
>
> Maybe the support is there, but it doesn't generate PIC by default?
>
golang has -fPIC IIRC. It does not benefit from the GNU toolchian
synergy of always calling the linker with the correct flags
corresponding to the generated code, though. So when gcc flips the
switch default value golang happily produces incompatible objects.
Also I suspect some pieces of stdlib are not compiled with the flags
you pass in for the build so there are always some objects somewhere
that are not compatible.
Thanks
Michal
^ permalink raw reply
* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Benjamin Herrenschmidt @ 2018-10-31 22:24 UTC (permalink / raw)
To: Florian Weimer, Michal Suchánek
Cc: linux-mm, Anton Blanchard, linuxppc-dev, Nick Piggin
In-Reply-To: <877ehyf1cj.fsf@oldenburg.str.redhat.com>
On Wed, 2018-10-31 at 18:54 +0100, Florian Weimer wrote:
>
> It would matter to C code which returns the address of a global variable
> in the main program through and (implicit) int return value.
>
> The old behavior hid some pointer truncation issues.
Hiding bugs like that is never a good idea..
> > Maybe it would be good idea to generate 64bit relocations on 64bit
> > targets?
>
> Yes, the Go toolchain definitely needs fixing for PIE. I don't dispute
> that.
There was never any ABI guarantee that programs would be loaded below
4G... it just *happened*, so that's not per-se an ABI change.
That said, I'm surprised of the choice of address.. I would have rather
moved to above 1TB to benefit from 1T segments...
Nick, Anton, do you know anything about that change ?
Ben.
^ permalink raw reply
* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Tulio Magno Quites Machado Filho @ 2018-10-31 22:04 UTC (permalink / raw)
To: Florian Weimer
Cc: Lynn A. Boger, linux-mm, Michal Suchánek, linuxppc-dev
In-Reply-To: <87in1hlsa7.fsf@oldenburg.str.redhat.com>
Florian Weimer <fweimer@redhat.com> writes:
> * Tulio Magno Quites Machado Filho:
>
>> I wonder if this is restricted to linker that Golang uses.
>> Were you able to reproduce the same problem with Binutils' linker?
>
> The example is carefully constructed to use the external linker. It
> invokes gcc, which then invokes the BFD linker in my case.
Indeed. That question was unnecessary. :-D
> Based on the relocations, I assume there is only so much the linker can
> do here. I'm amazed that it produces an executable at all, let alone
> one that runs correctly on some kernel versions!
Agreed. That isn't expected to work. Both the compiler and the linker have
to generate PIE for it to work.
> I assume that the Go toolchain simply lacks PIE support on ppc64le.
Maybe the support is there, but it doesn't generate PIC by default?
--
Tulio Magno
^ permalink raw reply
* Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed
From: Guenter Roeck @ 2018-10-31 22:02 UTC (permalink / raw)
To: Paul Burton
Cc: linux-mips@linux-mips.org, Arnd Bergmann, James Hogan,
linux-kernel@vger.kernel.org, Ralf Baechle, Paul Mackerras,
Andrew Morton, linuxppc-dev@lists.ozlabs.org, Trond Myklebust
In-Reply-To: <20181031213240.zhh7dfcm47ucuyfl@pburton-laptop>
On Wed, Oct 31, 2018 at 09:32:43PM +0000, Paul Burton wrote:
> Hi Guenter,
>
> On Wed, Oct 31, 2018 at 12:52:18PM -0700, Guenter Roeck wrote:
> > +/*
> > + * Generic version of __cmpxchg_u64, to be used for cmpxchg64().
> > + * Takes u64 parameters.
> > + */
> > +u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new)
> > +{
> > + raw_spinlock_t *lock = lock_addr(ptr);
> > + unsigned long flags;
> > + u64 prev;
> > +
> > + raw_spin_lock_irqsave(lock, flags);
> > + prev = READ_ONCE(*ptr);
> > + if (prev == old)
> > + *ptr = new;
> > + raw_spin_unlock_irqrestore(lock, flags);
> > +
> > + return prev;
> > +}
> > +EXPORT_SYMBOL(__cmpxchg_u64);
>
> This is only going to work if we know that memory modified using
> __cmpxchg_u64() is *always* modified using __cmpxchg_u64(). Without that
> guarantee there's nothing to stop some other CPU writing to *ptr after
> the READ_ONCE() above but before we write new to it.
>
> As far as I'm aware this is not a guarantee we currently provide, so it
> would mean making that a requirement for cmpxchg64() users & auditing
> them all. That would also leave cmpxchg64() with semantics that differ
> from plain cmpxchg(), and semantics that may surprise people. In my view
> that's probably not worth it, and it would be better to avoid using
> cmpxchg64() on systems that can't properly support it.
>
Good point. Unfortunately this is also true for the architectures with
similar implementations, ie at least sparc32 (and possibly parisc).
The alternatives I can see are
- Do not use cmpxchg64() outside architecture code (ie drop its use from
the offending driver, and keep doing the same whenever the problem comes
up again).
or
- Introduce something like ARCH_HAS_CMPXCHG64 and use it to determine
if cmpxchg64 is supported or not.
Any preference ?
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH v2 2/2] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()
From: Doug Anderson @ 2018-10-31 21:41 UTC (permalink / raw)
To: Daniel Thompson
Cc: Kate Stewart, linux-mips, dalias, linux-sh, Peter Zijlstra,
Catalin Marinas, Will Deacon, LKML, paulus, Yoshinori Sato,
linux-hexagon, Russell King - ARM Linux, kgdb-bugreport, jhogan,
linux-snps-arc, Philippe Ombredanne, Thomas Gleixner, Linux ARM,
Vineet Gupta, Greg Kroah-Hartman, Ralf Baechle, rkuo, paul.burton,
Jason Wessel, linuxppc-dev
In-Reply-To: <20181031184050.sd5opni3mznaapkv@holly.lan>
Hi,
On Wed, Oct 31, 2018 at 11:40 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Tue, Oct 30, 2018 at 03:18:43PM -0700, Douglas Anderson wrote:
> > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> > index f3cadda45f07..9a3f952de6ed 100644
> > --- a/kernel/debug/debug_core.c
> > +++ b/kernel/debug/debug_core.c
> > @@ -55,6 +55,7 @@
> > #include <linux/mm.h>
> > #include <linux/vmacache.h>
> > #include <linux/rcupdate.h>
> > +#include <linux/irq.h>
> >
> > #include <asm/cacheflush.h>
> > #include <asm/byteorder.h>
> > @@ -220,6 +221,39 @@ int __weak kgdb_skipexception(int exception, struct pt_regs *regs)
> > return 0;
> > }
> >
> > +/*
> > + * Default (weak) implementation for kgdb_roundup_cpus
> > + */
> > +
> > +static DEFINE_PER_CPU(call_single_data_t, kgdb_roundup_csd);
> > +
> > +void __weak kgdb_call_nmi_hook(void *ignored)
> > +{
> > + kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
> > +}
> > +
> > +void __weak kgdb_roundup_cpus(void)
> > +{
> > + call_single_data_t *csd;
> > + int cpu;
> > +
> > + for_each_cpu(cpu, cpu_online_mask) {
> > + csd = &per_cpu(kgdb_roundup_csd, cpu);
> > + smp_call_function_single_async(cpu, csd);
> > + }
>
> smp_call_function() automatically skips the calling CPU but this code does
> not. It isn't a hard bug since kgdb_nmicallback() does a re-entrancy
> check but I'd still prefer to skip the calling CPU.
I'll incorporate this into the next version.
> As mentioned in another part of the thread we can also add robustness
> by skipping a cpu where csd->flags != 0 (and adding an appropriately
> large comment regarding why). Doing the check directly is abusing
> internal knowledge that smp.c normally keeps to itself so an accessor
> of some kind would be needed.
Sure. I could add smp_async_func_finished() that just looked like:
int smp_async_func_finished(call_single_data_t *csd)
{
return !(csd->flags & CSD_FLAG_LOCK);
}
My understanding of all the mutual exclusion / memory barrier concepts
employed by smp.c is pretty weak, though. I'm hoping that it's safe
to just access the structure and check the bit directly.
...but do you think adding a generic accessor like this is better than
just keeping track of this in kgdb directly? I could avoid the
accessor by adding a "rounding_up" member to "struct
debuggerinfo_struct" and doing something like this in roundup:
/* If it didn't round up last time, don't try again */
if (kgdb_info[cpu].rounding_up)
continue
kgdb_info[cpu].rounding_up = true
smp_call_function_single_async(cpu, csd);
...and then in kgdb_nmicallback() I could just add:
kgdb_info[cpu].rounding_up = false
In that case we're not adding a generic accessor to smp.c that most
people should never use.
I'll wait to hear back from you if you think the accessor is OK. It
seems like it might be nice not to have to add something to smp.c just
for this one use case.
> > +}
> > +
> > +static void kgdb_generic_roundup_init(void)
> > +{
> > + call_single_data_t *csd;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu) {
> > + csd = &per_cpu(kgdb_roundup_csd, cpu);
> > + csd->func = kgdb_call_nmi_hook;
> > + }
> > +}
>
> I can't help noticing this code is very similar to kgdb_roundup_cpus. Do
> we really gain much from ahead-of-time initializing csd->func?
Oh! Right... At first I thought about just trying to put the "csd"
on the stack in kgdb_roundup_cpus() but then I realized that it needed
to persist past the end of kgdb_roundup_cpus(). ...and once I gave up
on the idea of putting it on the stack I decided I needed the init.
...but you're right that I don't really. The only thing I'm initting
is the function pointer and it totally wouldn't hurt to just init that
over and over again every time kgdb_roundup_cpus() is called.
-Doug
^ permalink raw reply
* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Tulio Magno Quites Machado Filho @ 2018-10-31 21:23 UTC (permalink / raw)
To: Florian Weimer, Michal Suchánek
Cc: Lynn A. Boger, linux-mm, linuxppc-dev
In-Reply-To: <877ehyf1cj.fsf@oldenburg.str.redhat.com>
Florian Weimer <fweimer@redhat.com> writes:
> * Michal Suchánek:
>
>> On Wed, 31 Oct 2018 18:20:56 +0100
>> Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> And it needs to be built with:
>>>
>>> go build -ldflags=-extldflags=-pie extld.go
>>>
>>> I'm not entirely sure what to make of this, but I'm worried that this
>>> could be a regression that matters to userspace.
>>
>> I encountered the same when trying to build go on ppc64le. I am not
>> familiar with the internals so I just let it be.
>>
>> It does not seem to matter to any other userspace.
>
> It would matter to C code which returns the address of a global variable
> in the main program through and (implicit) int return value.
I wonder if this is restricted to linker that Golang uses.
Were you able to reproduce the same problem with Binutils' linker?
--
Tulio Magno
^ permalink raw reply
* Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed
From: Paul Burton @ 2018-10-31 21:32 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-mips@linux-mips.org, Arnd Bergmann, James Hogan,
linux-kernel@vger.kernel.org, Ralf Baechle, Paul Mackerras,
Andrew Morton, linuxppc-dev@lists.ozlabs.org, Trond Myklebust
In-Reply-To: <1541015538-11382-1-git-send-email-linux@roeck-us.net>
Hi Guenter,
On Wed, Oct 31, 2018 at 12:52:18PM -0700, Guenter Roeck wrote:
> +/*
> + * Generic version of __cmpxchg_u64, to be used for cmpxchg64().
> + * Takes u64 parameters.
> + */
> +u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new)
> +{
> + raw_spinlock_t *lock = lock_addr(ptr);
> + unsigned long flags;
> + u64 prev;
> +
> + raw_spin_lock_irqsave(lock, flags);
> + prev = READ_ONCE(*ptr);
> + if (prev == old)
> + *ptr = new;
> + raw_spin_unlock_irqrestore(lock, flags);
> +
> + return prev;
> +}
> +EXPORT_SYMBOL(__cmpxchg_u64);
This is only going to work if we know that memory modified using
__cmpxchg_u64() is *always* modified using __cmpxchg_u64(). Without that
guarantee there's nothing to stop some other CPU writing to *ptr after
the READ_ONCE() above but before we write new to it.
As far as I'm aware this is not a guarantee we currently provide, so it
would mean making that a requirement for cmpxchg64() users & auditing
them all. That would also leave cmpxchg64() with semantics that differ
from plain cmpxchg(), and semantics that may surprise people. In my view
that's probably not worth it, and it would be better to avoid using
cmpxchg64() on systems that can't properly support it.
For MIPS the problem will go away with the nanoMIPS ISA which includes &
requires LLWP/SCWP (W = word, P = paired) instructions that we can use
to implement cmpxchg64() properly, but of course that won't help older
systems.
Thanks,
Paul
^ permalink raw reply
* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Florian Weimer @ 2018-10-31 21:28 UTC (permalink / raw)
To: Tulio Magno Quites Machado Filho
Cc: Lynn A. Boger, linux-mm, Michal Suchánek, linuxppc-dev
In-Reply-To: <87efc5n73a.fsf@linux.ibm.com>
* Tulio Magno Quites Machado Filho:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> * Michal Suchánek:
>>
>>> On Wed, 31 Oct 2018 18:20:56 +0100
>>> Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>>> And it needs to be built with:
>>>>
>>>> go build -ldflags=-extldflags=-pie extld.go
>>>>
>>>> I'm not entirely sure what to make of this, but I'm worried that this
>>>> could be a regression that matters to userspace.
>>>
>>> I encountered the same when trying to build go on ppc64le. I am not
>>> familiar with the internals so I just let it be.
>>>
>>> It does not seem to matter to any other userspace.
>>
>> It would matter to C code which returns the address of a global variable
>> in the main program through and (implicit) int return value.
>
> I wonder if this is restricted to linker that Golang uses.
> Were you able to reproduce the same problem with Binutils' linker?
The example is carefully constructed to use the external linker. It
invokes gcc, which then invokes the BFD linker in my case.
Based on the relocations, I assume there is only so much the linker can
do here. I'm amazed that it produces an executable at all, let alone
one that runs correctly on some kernel versions! I assume that the Go
toolchain simply lacks PIE support on ppc64le.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] raid6/ppc: Fix build for clang
From: Nick Desaulniers @ 2018-10-31 19:54 UTC (permalink / raw)
To: joel; +Cc: Kees Cook, Arnd Bergmann, jji, LKML, linuxppc-dev
In-Reply-To: <20181031032829.18131-1-joel@jms.id.au>
On Tue, Oct 30, 2018 at 8:28 PM Joel Stanley <joel@jms.id.au> wrote:
>
> We cannot build these files with clang as it does not allow altivec
> instructions in assembly when -msoft-float is passed.
>
> Jinsong Ji <jji@us.ibm.com> wrote:
> > We currently disable Altivec/VSX support when enabling soft-float. So
> > any usage of vector builtins will break.
> >
> > Enable Altivec/VSX with soft-float may need quite some clean up work, so
> > I guess this is currently a limitation.
> >
> > Removing -msoft-float will make it work (and we are lucky that no
> > floating point instructions will be generated as well).
>
> This is a workaround until the issue is resolved in clang.
>
> Link: https://bugs.llvm.org/show_bug.cgi?id=31177
> Link: https://github.com/ClangBuiltLinux/linux/issues/239
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Hopefully we can have this fixed up soon. I'll try to see who works
on Clang+PPC here and see if they can help move that bug along. Thanks
for the patch!
> ---
> lib/raid6/Makefile | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/lib/raid6/Makefile b/lib/raid6/Makefile
> index 2f8b61dfd9b0..3a844e6fd01c 100644
> --- a/lib/raid6/Makefile
> +++ b/lib/raid6/Makefile
> @@ -18,6 +18,21 @@ quiet_cmd_unroll = UNROLL $@
>
> ifeq ($(CONFIG_ALTIVEC),y)
> altivec_flags := -maltivec $(call cc-option,-mabi=altivec)
> +
> +ifdef CONFIG_CC_IS_CLANG
Note that the top level makefile detects clang via:
ifeq ($(cc-name),clang)
Does that not work here? I'd prefer to keep compiler detection in
Makefiles consistent, if it works?
> +# clang ppc port does not yet support -maltifec when -msoft-float is
> +# enabled. A future release of clang will resolve this
> +# https://bugs.llvm.org/show_bug.cgi?id=31177
> +CFLAGS_REMOVE_altivec1.o += -msoft-float
> +CFLAGS_REMOVE_altivec2.o += -msoft-float
> +CFLAGS_REMOVE_altivec4.o += -msoft-float
> +CFLAGS_REMOVE_altivec8.o += -msoft-float
> +CFLAGS_REMOVE_altivec8.o += -msoft-float
> +CFLAGS_REMOVE_vpermxor1.o += -msoft-float
> +CFLAGS_REMOVE_vpermxor2.o += -msoft-float
> +CFLAGS_REMOVE_vpermxor4.o += -msoft-float
> +CFLAGS_REMOVE_vpermxor8.o += -msoft-float
> +endif
> endif
>
> # The GCC option -ffreestanding is required in order to compile code containing
> --
> 2.19.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed
From: Guenter Roeck @ 2018-10-31 19:52 UTC (permalink / raw)
To: Ralf Baechle, Paul Burton, James Hogan, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman
Cc: linux-mips, Arnd Bergmann, linux-kernel, Andrew Morton,
linuxppc-dev, Trond Myklebust, Guenter Roeck
Some architectures do not or not always support cmpxchg64(). This results
in on/off problems when the function is used in common code. The latest
example is
net/sunrpc/auth_gss/gss_krb5_seal.c: In function 'gss_seq_send64_fetch_and_inc':
net/sunrpc/auth_gss/gss_krb5_seal.c:145:14: error:
implicit declaration of function 'cmpxchg64'
which is seen with some powerpc and mips builds.
Introduce a generic version of __cmpxchg_u64() and use it for affected
architectures.
Fixes: 21924765862a
("SUNRPC: use cmpxchg64() in gss_seq_send64_fetch_and_inc()")
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Couple of questions:
- Is this the right (or an acceptable) approach to fix the problem ?
- Should I split the patch into three, one to introduce __cmpxchg_u64()
and one per architecture ?
- Who should take the patch (series) ?
arch/mips/Kconfig | 1 +
arch/mips/include/asm/cmpxchg.h | 3 ++
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/cmpxchg.h | 3 ++
lib/Kconfig | 3 ++
lib/Makefile | 2 ++
lib/cmpxchg64.c | 60 ++++++++++++++++++++++++++++++++++++++
7 files changed, 73 insertions(+)
create mode 100644 lib/cmpxchg64.c
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 80778b40f8fa..7392a5f4e517 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -18,6 +18,7 @@ config MIPS
select CPU_PM if CPU_IDLE
select DMA_DIRECT_OPS
select GENERIC_ATOMIC64 if !64BIT
+ select GENERIC_CMPXCHG64 if !64BIT && SMP
select GENERIC_CLOCKEVENTS
select GENERIC_CMOS_UPDATE
select GENERIC_CPU_AUTOPROBE
diff --git a/arch/mips/include/asm/cmpxchg.h b/arch/mips/include/asm/cmpxchg.h
index 89e9fb7976fe..ca837b05bf3d 100644
--- a/arch/mips/include/asm/cmpxchg.h
+++ b/arch/mips/include/asm/cmpxchg.h
@@ -206,6 +206,9 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
#define cmpxchg64_local(ptr, o, n) __cmpxchg64_local_generic((ptr), (o), (n))
#ifndef CONFIG_SMP
#define cmpxchg64(ptr, o, n) cmpxchg64_local((ptr), (o), (n))
+#else
+extern u64 __cmpxchg_u64(u64 *p, u64 old, u64 new);
+#define cmpxchg64(ptr, o, n) __cmpxchg_u64((ptr), (o), (n))
#endif
#endif
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e84943d24e5c..bd1d99c664c4 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -161,6 +161,7 @@ config PPC
select EDAC_ATOMIC_SCRUB
select EDAC_SUPPORT
select GENERIC_ATOMIC64 if PPC32
+ select GENERIC_CMPXCHG64 if PPC32
select GENERIC_CLOCKEVENTS
select GENERIC_CLOCKEVENTS_BROADCAST if SMP
select GENERIC_CMOS_UPDATE
diff --git a/arch/powerpc/include/asm/cmpxchg.h b/arch/powerpc/include/asm/cmpxchg.h
index 27183871eb3b..da8be4189731 100644
--- a/arch/powerpc/include/asm/cmpxchg.h
+++ b/arch/powerpc/include/asm/cmpxchg.h
@@ -534,8 +534,11 @@ __cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
cmpxchg_acquire((ptr), (o), (n)); \
})
#else
+extern u64 __cmpxchg_u64(u64 *p, u64 old, u64 new);
+
#include <asm-generic/cmpxchg-local.h>
#define cmpxchg64_local(ptr, o, n) __cmpxchg64_local_generic((ptr), (o), (n))
+#define cmpxchg64(ptr, o, n) __cmpxchg_u64((ptr), (o), (n))
#endif
#endif /* __KERNEL__ */
diff --git a/lib/Kconfig b/lib/Kconfig
index d1573a16aa92..2b581a70ded2 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -500,6 +500,9 @@ config NLATTR
config GENERIC_ATOMIC64
bool
+config GENERIC_CMPXCHG64
+ bool
+
config LRU_CACHE
tristate
diff --git a/lib/Makefile b/lib/Makefile
index 988949c4fd3a..4646a06ed418 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -172,6 +172,8 @@ obj-$(CONFIG_GENERIC_CSUM) += checksum.o
obj-$(CONFIG_GENERIC_ATOMIC64) += atomic64.o
+obj-$(CONFIG_GENERIC_CMPXCHG64) += cmpxchg64.o
+
obj-$(CONFIG_ATOMIC64_SELFTEST) += atomic64_test.o
obj-$(CONFIG_CPU_RMAP) += cpu_rmap.o
diff --git a/lib/cmpxchg64.c b/lib/cmpxchg64.c
new file mode 100644
index 000000000000..239c43d05d00
--- /dev/null
+++ b/lib/cmpxchg64.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic implementation of cmpxchg64().
+ * Derived from implementation in arch/sparc/lib/atomic32.c
+ * and from locking code implemented in lib/atomic32.c.
+ */
+
+#include <linux/cache.h>
+#include <linux/export.h>
+#include <linux/irqflags.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+/*
+ * We use a hashed array of spinlocks to provide exclusive access
+ * to each variable. Since this is expected to used on systems
+ * with small numbers of CPUs (<= 4 or so), we use a relatively
+ * small array of 16 spinlocks to avoid wasting too much memory
+ * on the spinlock array.
+ */
+#define NR_LOCKS 16
+
+/* Ensure that each lock is in a separate cacheline */
+static union {
+ raw_spinlock_t lock;
+ char pad[L1_CACHE_BYTES];
+} cmpxchg_lock[NR_LOCKS] __cacheline_aligned_in_smp = {
+ [0 ... (NR_LOCKS - 1)] = {
+ .lock = __RAW_SPIN_LOCK_UNLOCKED(cmpxchg_lock.lock),
+ },
+};
+
+static inline raw_spinlock_t *lock_addr(const u64 *v)
+{
+ unsigned long addr = (unsigned long) v;
+
+ addr >>= L1_CACHE_SHIFT;
+ addr ^= (addr >> 8) ^ (addr >> 16);
+ return &cmpxchg_lock[addr & (NR_LOCKS - 1)].lock;
+}
+
+/*
+ * Generic version of __cmpxchg_u64, to be used for cmpxchg64().
+ * Takes u64 parameters.
+ */
+u64 __cmpxchg_u64(u64 *ptr, u64 old, u64 new)
+{
+ raw_spinlock_t *lock = lock_addr(ptr);
+ unsigned long flags;
+ u64 prev;
+
+ raw_spin_lock_irqsave(lock, flags);
+ prev = READ_ONCE(*ptr);
+ if (prev == old)
+ *ptr = new;
+ raw_spin_unlock_irqrestore(lock, flags);
+
+ return prev;
+}
+EXPORT_SYMBOL(__cmpxchg_u64);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] selftests/powerpc: Fix compilation issue due to asm label
From: Breno Leitao @ 2018-10-31 19:05 UTC (permalink / raw)
To: Naveen N. Rao, Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <1541006651.phri6idrd6.naveen@linux.ibm.com>
hi Naveen,
On 10/31/18 2:24 PM, Naveen N. Rao wrote:
> Naveen N. Rao wrote:
>> We are using 'dscr_insn' as a label in inline asm to identify if a
>> SIGILL was generated by the mtspr instruction at that point. However,
>> with inline assembly, the compiler is still free to duplicate the asm
>> statement for optimization purposes, which results in the label being
>> defined twice with the error:
>> /tmp/ccerQCql.s:874: Error: symbol `dscr_insn' is already defined
>>
>> With different compiler versions, we may also see:
>> /tmp/ccJzLDlN.o:(.toc+0x0): undefined reference to `dscr_insn'
>>
>> Remove the use of the label in the inline assembly. Instead, just look
>> for the offending instruction in the signal handler.
Cool, I've tested this patch on my mainline tree and selftests are being
able to build again. I also tested the rfi_flush_test selftest as other
tests (ptrace/ and tm/) and everything seems to be normal again.
Thank you!
>> Reported-by: Breno Leitao <leitao@debian.org>
>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Tested-by: Breno Leitao <leitao@debian.org>
^ permalink raw reply
* Re: [PATCH v2 2/2] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()
From: Daniel Thompson @ 2018-10-31 18:40 UTC (permalink / raw)
To: Douglas Anderson
Cc: Kate Stewart, linux-mips, Rich Felker, linux-sh, Peter Zijlstra,
Catalin Marinas, Will Deacon, linux-kernel, Paul Mackerras,
Yoshinori Sato, linux-hexagon, Russell King, kgdb-bugreport,
James Hogan, linux-snps-arc, Philippe Ombredanne, Thomas Gleixner,
linux-arm-kernel, Vineet Gupta, Greg Kroah-Hartman, Ralf Baechle,
Richard Kuo, Paul Burton, Jason Wessel, linuxppc-dev
In-Reply-To: <20181030221843.121254-3-dianders@chromium.org>
On Tue, Oct 30, 2018 at 03:18:43PM -0700, Douglas Anderson wrote:
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index f3cadda45f07..9a3f952de6ed 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -55,6 +55,7 @@
> #include <linux/mm.h>
> #include <linux/vmacache.h>
> #include <linux/rcupdate.h>
> +#include <linux/irq.h>
>
> #include <asm/cacheflush.h>
> #include <asm/byteorder.h>
> @@ -220,6 +221,39 @@ int __weak kgdb_skipexception(int exception, struct pt_regs *regs)
> return 0;
> }
>
> +/*
> + * Default (weak) implementation for kgdb_roundup_cpus
> + */
> +
> +static DEFINE_PER_CPU(call_single_data_t, kgdb_roundup_csd);
> +
> +void __weak kgdb_call_nmi_hook(void *ignored)
> +{
> + kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
> +}
> +
> +void __weak kgdb_roundup_cpus(void)
> +{
> + call_single_data_t *csd;
> + int cpu;
> +
> + for_each_cpu(cpu, cpu_online_mask) {
> + csd = &per_cpu(kgdb_roundup_csd, cpu);
> + smp_call_function_single_async(cpu, csd);
> + }
smp_call_function() automatically skips the calling CPU but this code does
not. It isn't a hard bug since kgdb_nmicallback() does a re-entrancy
check but I'd still prefer to skip the calling CPU.
As mentioned in another part of the thread we can also add robustness
by skipping a cpu where csd->flags != 0 (and adding an appropriately
large comment regarding why). Doing the check directly is abusing
internal knowledge that smp.c normally keeps to itself so an accessor
of some kind would be needed.
> +}
> +
> +static void kgdb_generic_roundup_init(void)
> +{
> + call_single_data_t *csd;
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + csd = &per_cpu(kgdb_roundup_csd, cpu);
> + csd->func = kgdb_call_nmi_hook;
> + }
> +}
I can't help noticing this code is very similar to kgdb_roundup_cpus. Do
we really gain much from ahead-of-time initializing csd->func?
Daniel.
^ permalink raw reply
* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Florian Weimer @ 2018-10-31 17:54 UTC (permalink / raw)
To: Michal Suchánek; +Cc: linux-mm, linuxppc-dev
In-Reply-To: <20181031185032.679e170a@naga.suse.cz>
* Michal Suchánek:
> On Wed, 31 Oct 2018 18:20:56 +0100
> Florian Weimer <fweimer@redhat.com> wrote:
>
>> We tried to use Go to build PIE binaries, and while the Go toolchain
>> is definitely not ready (it produces text relocations and problematic
>> relocations in general), it exposed what could be an accidental
>> userspace ABI change.
>>
>> With our 4.10-derived kernel, PIE binaries are mapped below 4 GiB, so
>> relocations like R_PPC64_ADDR16_HA work:
>>
> ...
>
>> There are fewer mappings because the loader detects a relocation
>> overflow and aborts (“error while loading shared libraries:
>> R_PPC64_ADDR16_HA reloc at 0x0000000120f0983c for symbol `' out of
>> range”), so I had to recover the mappings externally. Disabling ASLR
>> does not help.
>>
> ...
>>
>> And it needs to be built with:
>>
>> go build -ldflags=-extldflags=-pie extld.go
>>
>> I'm not entirely sure what to make of this, but I'm worried that this
>> could be a regression that matters to userspace.
>
> I encountered the same when trying to build go on ppc64le. I am not
> familiar with the internals so I just let it be.
>
> It does not seem to matter to any other userspace.
It would matter to C code which returns the address of a global variable
in the main program through and (implicit) int return value.
The old behavior hid some pointer truncation issues.
> Maybe it would be good idea to generate 64bit relocations on 64bit
> targets?
Yes, the Go toolchain definitely needs fixing for PIE. I don't dispute
that.
Thanks,
Florian
^ permalink raw reply
* Re: PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Michal Suchánek @ 2018-10-31 17:50 UTC (permalink / raw)
To: Florian Weimer; +Cc: linux-mm, linuxppc-dev
In-Reply-To: <87k1lyf2x3.fsf@oldenburg.str.redhat.com>
On Wed, 31 Oct 2018 18:20:56 +0100
Florian Weimer <fweimer@redhat.com> wrote:
> We tried to use Go to build PIE binaries, and while the Go toolchain
> is definitely not ready (it produces text relocations and problematic
> relocations in general), it exposed what could be an accidental
> userspace ABI change.
>
> With our 4.10-derived kernel, PIE binaries are mapped below 4 GiB, so
> relocations like R_PPC64_ADDR16_HA work:
>
...
> There are fewer mappings because the loader detects a relocation
> overflow and aborts (“error while loading shared libraries:
> R_PPC64_ADDR16_HA reloc at 0x0000000120f0983c for symbol `' out of
> range”), so I had to recover the mappings externally. Disabling ASLR
> does not help.
>
...
>
> And it needs to be built with:
>
> go build -ldflags=-extldflags=-pie extld.go
>
> I'm not entirely sure what to make of this, but I'm worried that this
> could be a regression that matters to userspace.
I encountered the same when trying to build go on ppc64le. I am not
familiar with the internals so I just let it be.
It does not seem to matter to any other userspace. Maybe it would be
good idea to generate 64bit relocations on 64bit targets?
Thanks
Michal
^ permalink raw reply
* Re: [PATCH] selftests/powerpc: Fix compilation issue due to asm label
From: Naveen N. Rao @ 2018-10-31 17:24 UTC (permalink / raw)
To: Breno Leitao, Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <20181031171813.8970-1-naveen.n.rao@linux.vnet.ibm.com>
Naveen N. Rao wrote:
> We are using 'dscr_insn' as a label in inline asm to identify if a
> SIGILL was generated by the mtspr instruction at that point. However,
> with inline assembly, the compiler is still free to duplicate the asm
> statement for optimization purposes, which results in the label being
> defined twice with the error:
> /tmp/ccerQCql.s:874: Error: symbol `dscr_insn' is already defined
>
> With different compiler versions, we may also see:
> /tmp/ccJzLDlN.o:(.toc+0x0): undefined reference to `dscr_insn'
>
> Remove the use of the label in the inline assembly. Instead, just look
> for the offending instruction in the signal handler.
>
> Reported-by: Breno Leitao <leitao@debian.org>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
Missed adding:
Fixes: d2bf793237b3aa9c ("selftests/powerpc: Add test to verify rfi flush across a system call")
- Naveen
^ permalink raw reply
* PIE binaries are no longer mapped below 4 GiB on ppc64le
From: Florian Weimer @ 2018-10-31 17:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linux-mm
We tried to use Go to build PIE binaries, and while the Go toolchain is
definitely not ready (it produces text relocations and problematic
relocations in general), it exposed what could be an accidental
userspace ABI change.
With our 4.10-derived kernel, PIE binaries are mapped below 4 GiB, so
relocations like R_PPC64_ADDR16_HA work:
21f00000-220d0000 r-xp 00000000 fd:00 36593493 /root/extld
220d0000-220e0000 r--p 001c0000 fd:00 36593493 /root/extld
220e0000-22100000 rw-p 001d0000 fd:00 36593493 /root/extld
22100000-22120000 rw-p 00000000 00:00 0
264b0000-264e0000 rw-p 00000000 00:00 0 [heap]
c000000000-c000010000 rw-p 00000000 00:00 0
c41ffe0000-c420300000 rw-p 00000000 00:00 0
3fff8c000000-3fff8c030000 rw-p 00000000 00:00 0
3fff8c030000-3fff90000000 ---p 00000000 00:00 0
3fff90000000-3fff90030000 rw-p 00000000 00:00 0
3fff90030000-3fff94000000 ---p 00000000 00:00 0
3fff94000000-3fff94030000 rw-p 00000000 00:00 0
3fff94030000-3fff98000000 ---p 00000000 00:00 0
3fff98000000-3fff98030000 rw-p 00000000 00:00 0
3fff98030000-3fff9c000000 ---p 00000000 00:00 0
3fff9c000000-3fff9c030000 rw-p 00000000 00:00 0
3fff9c030000-3fffa0000000 ---p 00000000 00:00 0
3fffa2290000-3fffa22d0000 rw-p 00000000 00:00 0
3fffa22d0000-3fffa22e0000 ---p 00000000 00:00 0
3fffa22e0000-3fffa2ae0000 rw-p 00000000 00:00 0
3fffa2ae0000-3fffa2af0000 ---p 00000000 00:00 0
3fffa2af0000-3fffa32f0000 rw-p 00000000 00:00 0
3fffa32f0000-3fffa3300000 ---p 00000000 00:00 0
3fffa3300000-3fffa3b00000 rw-p 00000000 00:00 0
3fffa3b00000-3fffa3b10000 ---p 00000000 00:00 0
3fffa3b10000-3fffa4310000 rw-p 00000000 00:00 0
3fffa4310000-3fffa4320000 ---p 00000000 00:00 0
3fffa4320000-3fffa4bb0000 rw-p 00000000 00:00 0
3fffa4bb0000-3fffa4da0000 r-xp 00000000 fd:00 34316081 /usr/lib64/power9/libc-2.28.so
3fffa4da0000-3fffa4db0000 r--p 001e0000 fd:00 34316081 /usr/lib64/power9/libc-2.28.so
3fffa4db0000-3fffa4dc0000 rw-p 001f0000 fd:00 34316081 /usr/lib64/power9/libc-2.28.so
3fffa4dc0000-3fffa4df0000 r-xp 00000000 fd:00 34316085 /usr/lib64/power9/libpthread-2.28.so
3fffa4df0000-3fffa4e00000 r--p 00020000 fd:00 34316085 /usr/lib64/power9/libpthread-2.28.so
3fffa4e00000-3fffa4e10000 rw-p 00030000 fd:00 34316085 /usr/lib64/power9/libpthread-2.28.so
3fffa4e10000-3fffa4e20000 rw-p 00000000 00:00 0
3fffa4e20000-3fffa4e40000 r-xp 00000000 00:00 0 [vdso]
3fffa4e40000-3fffa4e70000 r-xp 00000000 fd:00 874114 /usr/lib64/ld-2.28.so
3fffa4e70000-3fffa4e80000 r--p 00020000 fd:00 874114 /usr/lib64/ld-2.28.so
3fffa4e80000-3fffa4e90000 rw-p 00030000 fd:00 874114 /usr/lib64/ld-2.28.so
3ffff3000000-3ffff3030000 rw-p 00000000 00:00 0
[stack]
With a 4.18-derived kernel (with the hashed mm), we get this instead:
120e60000-121030000 rw-p 00000000 fd:00 102447141 /root/extld
121030000-121060000 rw-p 001c0000 fd:00 102447141 /root/extld
121060000-121080000 rw-p 00000000 00:00 0
7fffb5b00000-7fffb5cf0000 r-xp 00000000 fd:00 67169871 /usr/lib64/power9/libc-2.28.so
7fffb5cf0000-7fffb5d00000 r--p 001e0000 fd:00 67169871 /usr/lib64/power9/libc-2.28.so
7fffb5d00000-7fffb5d10000 rw-p 001f0000 fd:00 67169871 /usr/lib64/power9/libc-2.28.so
7fffb5d10000-7fffb5d40000 r-xp 00000000 fd:00 67169875 /usr/lib64/power9/libpthread-2.28.so
7fffb5d40000-7fffb5d50000 r--p 00020000 fd:00 67169875 /usr/lib64/power9/libpthread-2.28.so
7fffb5d50000-7fffb5d60000 rw-p 00030000 fd:00 67169875 /usr/lib64/power9/libpthread-2.28.so
7fffb5d60000-7fffb5d70000 r--p 00000000 fd:00 67780267 /etc/ld.so.cache
7fffb5d70000-7fffb5d90000 r-xp 00000000 00:00 0 [vdso]
7fffb5d90000-7fffb5dc0000 r-xp 00000000 fd:00 1477 /usr/lib64/ld-2.28.so
7fffb5dc0000-7fffb5de0000 rw-p 00020000 fd:00 1477 /usr/lib64/ld-2.28.so
7fffff6c0000-7fffff6f0000 rw-p 00000000 00:00 0 [stack]
There are fewer mappings because the loader detects a relocation
overflow and aborts (“error while loading shared libraries:
R_PPC64_ADDR16_HA reloc at 0x0000000120f0983c for symbol `' out of
range”), so I had to recover the mappings externally. Disabling ASLR
does not help.
The Go program looks like this:
package main
import (
"fmt"
"io/ioutil"
"os"
)
// #include <gnu/libc-version.h>
import "C"
func main() {
// Force external linking against glibc.
fmt.Printf("%#v\n", C.GoString(C.gnu_get_libc_version()))
maps, err := os.Open("/proc/self/maps")
if err != nil {
panic(err)
}
defer maps.Close()
contents, err := ioutil.ReadAll(maps)
if err != nil {
panic(err)
}
_, err = os.Stdout.Write(contents)
if err != nil {
panic(err)
}
}
And it needs to be built with:
go build -ldflags=-extldflags=-pie extld.go
I'm not entirely sure what to make of this, but I'm worried that this
could be a regression that matters to userspace.
Thanks,
Florian
^ permalink raw reply
* [PATCH] selftests/powerpc: Fix compilation issue due to asm label
From: Naveen N. Rao @ 2018-10-31 17:18 UTC (permalink / raw)
To: Michael Ellerman, Breno Leitao; +Cc: linuxppc-dev
We are using 'dscr_insn' as a label in inline asm to identify if a
SIGILL was generated by the mtspr instruction at that point. However,
with inline assembly, the compiler is still free to duplicate the asm
statement for optimization purposes, which results in the label being
defined twice with the error:
/tmp/ccerQCql.s:874: Error: symbol `dscr_insn' is already defined
With different compiler versions, we may also see:
/tmp/ccJzLDlN.o:(.toc+0x0): undefined reference to `dscr_insn'
Remove the use of the label in the inline assembly. Instead, just look
for the offending instruction in the signal handler.
Reported-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
tools/testing/selftests/powerpc/utils.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/powerpc/utils.c b/tools/testing/selftests/powerpc/utils.c
index 43c342845be0..ed62f4153d3e 100644
--- a/tools/testing/selftests/powerpc/utils.c
+++ b/tools/testing/selftests/powerpc/utils.c
@@ -25,7 +25,6 @@
#include "utils.h"
static char auxv[4096];
-extern unsigned int dscr_insn[];
int read_auxv(char *buf, ssize_t buf_size)
{
@@ -247,7 +246,8 @@ static void sigill_handler(int signr, siginfo_t *info, void *unused)
ucontext_t *ctx = (ucontext_t *)unused;
unsigned long *pc = &UCONTEXT_NIA(ctx);
- if (*pc == (unsigned long)&dscr_insn) {
+ /* mtspr 3,RS to check for move to DSCR below */
+ if ((*((unsigned int *)*pc) & 0xfc1fffff) == 0x7c0303a6) {
if (!warned++)
printf("WARNING: Skipping over dscr setup. Consider running 'ppc64_cpu --dscr=1' manually.\n");
*pc += 4;
@@ -271,5 +271,5 @@ void set_dscr(unsigned long val)
init = 1;
}
- asm volatile("dscr_insn: mtspr %1,%0" : : "r" (val), "i" (SPRN_DSCR));
+ asm volatile("mtspr %1,%0" : : "r" (val), "i" (SPRN_DSCR));
}
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap
From: LEROY Christophe @ 2018-10-31 17:06 UTC (permalink / raw)
To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin
In-Reply-To: <7b3e80077b4d9f22da891c143c78bb743cc24136.camel@russell.cc>
Russell Currey <ruscur@russell.cc> a écrit :
> On Fri, 2018-10-26 at 18:35 +0200, LEROY Christophe wrote:
>> Why not call our new functionnality SMAP instead of calling it GUAP ?
>
> mpe wasn't a fan of using the same terminology as other architectures.
I don't like too much the word 'guarded' because it means something
different for powerpc MMUs.
What about something like 'user space access protection' ?
Christophe
> Having a separate term does avoid some assumptions about how things
> work or are implemented, but sharing compatibility with an existing
> parameter is nice.
>
> Personally I don't really care too much about the name.
>
> - Russell
>
>>
>> Christophe
>>
>> Russell Currey <ruscur@russell.cc> a écrit :
>>
>> > Signed-off-by: Russell Currey <ruscur@russell.cc>
>> > ---
>> > Documentation/admin-guide/kernel-parameters.txt | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
>> > b/Documentation/admin-guide/kernel-parameters.txt
>> > index a5ad67d5cb16..8f78e75965f0 100644
>> > --- a/Documentation/admin-guide/kernel-parameters.txt
>> > +++ b/Documentation/admin-guide/kernel-parameters.txt
>> > @@ -2764,7 +2764,7 @@
>> > noexec=on: enable non-executable mappings
>> > (default)
>> > noexec=off: disable non-executable mappings
>> >
>> > - nosmap [X86]
>> > + nosmap [X86,PPC]
>> > Disable SMAP (Supervisor Mode Access
>> > Prevention)
>> > even if it is supported by processor.
>> >
>> > --
>> > 2.19.1
>>
>>
^ permalink raw reply
* Re: [PATCH v2 2/2] kgdb: Fix kgdb_roundup_cpus() for arches who used smp_call_function()
From: Daniel Thompson @ 2018-10-31 17:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Kate Stewart, linux-mips, Rich Felker, linux-sh, Catalin Marinas,
Will Deacon, linux-kernel, Paul Mackerras, Yoshinori Sato,
linux-hexagon, Russell King, kgdb-bugreport, James Hogan,
linux-snps-arc, Jason Wessel, Greg Kroah-Hartman, Thomas Gleixner,
linux-arm-kernel, Vineet Gupta, Douglas Anderson, Ralf Baechle,
Richard Kuo, Paul Burton, Philippe Ombredanne, linuxppc-dev
In-Reply-To: <20181031134926.GB13237@hirez.programming.kicks-ass.net>
On Wed, Oct 31, 2018 at 02:49:26PM +0100, Peter Zijlstra wrote:
> On Tue, Oct 30, 2018 at 03:18:43PM -0700, Douglas Anderson wrote:
> > Looking closely at it, it seems like a really bad idea to be calling
> > local_irq_enable() in kgdb_roundup_cpus(). If nothing else that seems
> > like it could violate spinlock semantics and cause a deadlock.
> >
> > Instead, let's use a private csd alongside
> > smp_call_function_single_async() to round up the other CPUs. Using
> > smp_call_function_single_async() doesn't require interrupts to be
> > enabled so we can remove the offending bit of code.
>
> You might want to mention that the only reason this isn't a deadlock
> itself is because there is a timeout on waiting for the slaves to
> check-in.
dbg_master_lock must be owned to call kgdb_roundup_cpus() so
the calls to smp_call_function_single_async() should never deadlock the
calling CPU unless there has been a previous failure to round up (e.g.
cores that cannot react to the round up signal).
When there is a failure to round up when we resume, there is a window (before
whatever locks that prevented the IPI being serviced are released) during which
the system will deadlock if the debugger is re entered.
I don't think there is any point trying to round up a CPU that did not
previously respond... it should still have an IPI pending. The deadlock
can be eliminated by getting the round up code to avoid CPUs whose csd->flags
are non-zero either by checking the flag in the kgdb code or adding something
like smp_trycall_function_single_async().
Daniel.
^ permalink raw reply
* Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix
From: LEROY Christophe @ 2018-10-31 16:58 UTC (permalink / raw)
To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin
In-Reply-To: <0e66ee72293811f580bd02ab4ed7967a4b703d70.camel@russell.cc>
Russell Currey <ruscur@russell.cc> a écrit :
> On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote:
>> Russell Currey <ruscur@russell.cc> a écrit :
>>
>> > Guarded Userspace Access Prevention is a security mechanism that
>> > prevents
>> > the kernel from being able to read and write userspace addresses
>> > outside of
>> > the allowed paths, most commonly copy_{to/from}_user().
>> >
>> > At present, the only CPU that supports this is POWER9, and only
>> > while using
>> > the Radix MMU. Privileged reads and writes cannot access user data
>> > when
>> > key 0 of the AMR is set. This is described in the "Radix Tree
>> > Translation
>> > Storage Protection" section of the POWER ISA as of version 3.0.
>>
>> It is not right that only power9 can support that.
>
> It's true that not only P9 can support it, but there are more
> considerations under hash than radix, implementing this for radix is a
> first step.
I don't know much about hash, but I was talking about the 8xx which is
a nohash ppc32. I'll see next week if I can do something with it on
top of your serie.
>
>>
>> The 8xx has mmu access protection registers which can serve the
>> same
>> purpose. Today on the 8xx kernel space is group 0 and user space is
>> group 1. Group 0 is set to "page defined access permission" in
>> MD_AP
>> and MI_AP registers, and group 1 is set to "all accesses done with
>> supervisor rights". By setting group 1 to "user and supervisor
>> interpretation swapped" we can forbid kernel access to user space
>> while still allowing user access to it. Then by simply changing
>> group
>> 1 mode at dedicated places we can lock/unlock kernel access to user
>> space.
>>
>> Could you implement something as generic as possible having that in
>> mind for a future patch ?
>
> I don't think anything in this series is particularly problematic in
> relation to future work for hash. I am interested in doing a hash
> implementation in future too.
I think we have to look at that carrefuly to avoid uggly ifdefs
Christophe
>
> - Russell
>
>>
>> Christophe
>>
>> > GUAP code sets key 0 of the AMR (thus disabling accesses of user
>> > data)
>> > early during boot, and only ever "unlocks" access prior to certain
>> > operations, like copy_{to/from}_user(), futex ops, etc. Setting
>> > this does
>> > not prevent unprivileged access, so userspace can operate fine
>> > while access
>> > is locked.
>> >
>> > There is a performance impact, although I don't consider it
>> > heavy. Running
>> > a worst-case benchmark of a 1GB copy 1 byte at a time (and thus
>> > constant
>> > read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower
>> > than
>> > when disabled. In most cases, the difference is negligible. The
>> > main
>> > performance impact is the mtspr instruction, which is quite slow.
>> >
>> > There are a few caveats with this series that could be improved
>> > upon in
>> > future. Right now there is no saving and restoring of the AMR
>> > value -
>> > there is no userspace exploitation of the AMR on Radix in POWER9,
>> > but if
>> > this were to change in future, saving and restoring the value would
>> > be
>> > necessary.
>> >
>> > No attempt to optimise cases of repeated calls - for example, if
>> > some
>> > code was repeatedly calling copy_to_user() for small sizes very
>> > frequently,
>> > it would be slower than the equivalent of wrapping that code in an
>> > unlock
>> > and lock and only having to modify the AMR once.
>> >
>> > There are some interesting cases that I've attempted to handle,
>> > such as if
>> > the AMR is unlocked (i.e. because a copy_{to_from}_user is in
>> > progress)...
>> >
>> > - and an exception is taken, the kernel would then be running
>> > with the
>> > AMR unlocked and freely able to access userspace again. I am
>> > working
>> > around this by storing a flag in the PACA to indicate if the
>> > AMR is
>> > unlocked (to save a costly SPR read), and if so, locking the
>> > AMR in
>> > the exception entry path and unlocking it on the way out.
>> >
>> > - and gets context switched out, goes into a path that locks
>> > the AMR,
>> > then context switches back, access will be disabled and will
>> > fault.
>> > As a result, I context switch the AMR between tasks as if it
>> > was used
>> > by userspace like hash (which already implements this).
>> >
>> > Another consideration is use of the isync instruction. Without an
>> > isync
>> > following the mtspr instruction, there is no guarantee that the
>> > change
>> > takes effect. The issue is that isync is very slow, and so I tried
>> > to
>> > avoid them wherever necessary. In this series, the only place an
>> > isync
>> > gets used is after *unlocking* the AMR, because if an access takes
>> > place
>> > and access is still prevented, the kernel will fault.
>> >
>> > On the flipside, a slight delay in unlocking caused by skipping an
>> > isync
>> > potentially allows a small window of vulnerability. It is my
>> > opinion
>> > that this window is practically impossible to exploit, but if
>> > someone
>> > thinks otherwise, please do share.
>> >
>> > This series is my first attempt at POWER assembly so all feedback
>> > is very
>> > welcome.
>> >
>> > The official theme song of this series can be found here:
>> > https://www.youtube.com/watch?v=QjTrnKAcYjE
>> >
>> > Russell Currey (5):
>> > powerpc/64s: Guarded Userspace Access Prevention
>> > powerpc/futex: GUAP support for futex ops
>> > powerpc/lib: checksum GUAP support
>> > powerpc/64s: Disable GUAP with nosmap option
>> > powerpc/64s: Document that PPC supports nosmap
>> >
>> > .../admin-guide/kernel-parameters.txt | 2 +-
>> > arch/powerpc/include/asm/exception-64e.h | 3 +
>> > arch/powerpc/include/asm/exception-64s.h | 19 ++++++-
>> > arch/powerpc/include/asm/futex.h | 6 ++
>> > arch/powerpc/include/asm/mmu.h | 7 +++
>> > arch/powerpc/include/asm/paca.h | 3 +
>> > arch/powerpc/include/asm/reg.h | 1 +
>> > arch/powerpc/include/asm/uaccess.h | 57
>> > ++++++++++++++++---
>> > arch/powerpc/kernel/asm-offsets.c | 1 +
>> > arch/powerpc/kernel/dt_cpu_ftrs.c | 4 ++
>> > arch/powerpc/kernel/entry_64.S | 17 +++++-
>> > arch/powerpc/lib/checksum_wrappers.c | 6 +-
>> > arch/powerpc/mm/fault.c | 9 +++
>> > arch/powerpc/mm/init_64.c | 15 +++++
>> > arch/powerpc/mm/pgtable-radix.c | 2 +
>> > arch/powerpc/mm/pkeys.c | 7 ++-
>> > arch/powerpc/platforms/Kconfig.cputype | 15 +++++
>> > 17 files changed, 158 insertions(+), 16 deletions(-)
>> >
>> > --
>> > 2.19.1
>>
>>
^ permalink raw reply
* Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention
From: LEROY Christophe @ 2018-10-31 16:54 UTC (permalink / raw)
To: Russell Currey; +Cc: mikey, linuxppc-dev, npiggin
In-Reply-To: <eaf851ae401556d25d2da5decf6337296ef20a35.camel@russell.cc>
Russell Currey <ruscur@russell.cc> a écrit :
> On Sun, 2018-10-28 at 18:57 +0100, LEROY Christophe wrote:
>> Russell Currey <ruscur@russell.cc> a écrit :
>>
>> > Guarded Userspace Access Prevention (GUAP) utilises a feature of
>> > the Radix MMU which disallows read and write access to userspace
>> > addresses. By utilising this, the kernel is prevented from
>> > accessing
>> > user data from outside of trusted paths that perform proper safety
>> > checks,
>> > such as copy_{to/from}_user() and friends.
>> >
>> > Userspace access is disabled from early boot and is only enabled
>> > when:
>> >
>> > - exiting the kernel and entering userspace
>> > - performing an operation like copy_{to/from}_user()
>> > - context switching to a process that has access enabled
>> >
>> > and similarly, access is disabled again when exiting userspace and
>> > entering
>> > the kernel.
>> >
>> > This feature has a slight performance impact which I roughly
>> > measured to be
>> > 3% slower in the worst case (performing 1GB of 1 byte
>> > read()/write()
>> > syscalls), and is gated behind the CONFIG_PPC_RADIX_GUAP option for
>> > performance-critical builds.
>> >
>> > This feature can be tested by using the lkdtm driver
>> > (CONFIG_LKDTM=y) and
>> > performing the following:
>> >
>> > echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT
>> >
>> > if enabled, this should send SIGSEGV to the thread.
>> >
>> > Signed-off-by: Russell Currey <ruscur@russell.cc>
>>
>> I think this patch should be split in at least two parts:
>> First part for implementing the generic part, including the changes
>> to
>> futex and csum, and a second part implementing the radix part.
>
> I'll see how I go making generic handlers - I am concerned about the
> implementation becoming more complex than it needs to be just to
> accommodate potential future changes that could end up having different
> requirements anyway, rather than something simple that works today.
I think we can do something quite simple. I'll draft something next
week and send it to get your feedback.
>
>> > ---
>> > Since the previous version of this patchset (named KHRAP) there
>> > have been
>> > several changes, some of which include:
>> >
>> > - macro naming, suggested by Nick
>> > - builds should be fixed outside of 64s
>> > - no longer unlock heading out to userspace
>> > - removal of unnecessary isyncs
>> > - more config option testing
>> > - removal of save/restore
>> > - use pr_crit() and reword message on fault
>> >
>> > arch/powerpc/include/asm/exception-64e.h | 3 ++
>> > arch/powerpc/include/asm/exception-64s.h | 19 +++++++-
>> > arch/powerpc/include/asm/mmu.h | 7 +++
>> > arch/powerpc/include/asm/paca.h | 3 ++
>> > arch/powerpc/include/asm/reg.h | 1 +
>> > arch/powerpc/include/asm/uaccess.h | 57
>> > ++++++++++++++++++++----
>> > arch/powerpc/kernel/asm-offsets.c | 1 +
>> > arch/powerpc/kernel/dt_cpu_ftrs.c | 4 ++
>> > arch/powerpc/kernel/entry_64.S | 17 ++++++-
>> > arch/powerpc/mm/fault.c | 12 +++++
>> > arch/powerpc/mm/pgtable-radix.c | 2 +
>> > arch/powerpc/mm/pkeys.c | 7 ++-
>> > arch/powerpc/platforms/Kconfig.cputype | 15 +++++++
>> > 13 files changed, 135 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/exception-64e.h
>> > b/arch/powerpc/include/asm/exception-64e.h
>> > index 555e22d5e07f..bf25015834ee 100644
>> > --- a/arch/powerpc/include/asm/exception-64e.h
>> > +++ b/arch/powerpc/include/asm/exception-64e.h
>> > @@ -215,5 +215,8 @@ exc_##label##_book3e:
>> > #define RFI_TO_USER
>> > \
>> > rfi
>> >
>> > +#define UNLOCK_USER_ACCESS(reg)
>> > +#define LOCK_USER_ACCESS(reg)
>> > +
>> > #endif /* _ASM_POWERPC_EXCEPTION_64E_H */
>> >
>> > diff --git a/arch/powerpc/include/asm/exception-64s.h
>> > b/arch/powerpc/include/asm/exception-64s.h
>> > index 3b4767ed3ec5..0cac5bd380ca 100644
>> > --- a/arch/powerpc/include/asm/exception-64s.h
>> > +++ b/arch/powerpc/include/asm/exception-64s.h
>> > @@ -264,6 +264,19 @@ BEGIN_FTR_SECTION_NESTED(943)
>> > \
>> > std ra,offset(r13); \
>> > END_FTR_SECTION_NESTED(ftr,ftr,943)
>> >
>> > +#define LOCK_USER_ACCESS(reg)
>> > \
>> > +BEGIN_MMU_FTR_SECTION_NESTED(944)
>> > \
>> > + LOAD_REG_IMMEDIATE(reg,AMR_LOCKED); \
>> > + mtspr SPRN_AMR,reg;
>> > \
>> > +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,9
>> > 44)
>> > +
>> > +#define UNLOCK_USER_ACCESS(reg)
>> > \
>> > +BEGIN_MMU_FTR_SECTION_NESTED(945)
>> > \
>> > + li reg,0; \
>> > + mtspr SPRN_AMR,reg;
>> > \
>> > + isync
>> > \
>> > +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,9
>> > 45)
>> > +
>> > #define EXCEPTION_PROLOG_0(area)
>> > \
>> > GET_PACA(r13);
>> > \
>> > std r9,area+EX_R9(r13); /* save r9 */ \
>> > @@ -500,7 +513,11 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
>> > beq 4f; /* if from kernel mode */
>> > \
>> > ACCOUNT_CPU_USER_ENTRY(r13, r9, r10);
>> > \
>> > SAVE_PPR(area, r9);
>> > \
>> > -4: EXCEPTION_PROLOG_COMMON_2(area)
>> > \
>> > +4: lbz r9,PACA_USER_ACCESS_ALLOWED(r13);
>> > \
>> > + cmpwi cr1,r9,0;
>> > \
>> > + beq 5f;
>> > \
>> > + LOCK_USER_ACCESS(r9);
>> > \
>> > +5: EXCEPTION_PROLOG_COMMON_2(area)
>> > \
>> > EXCEPTION_PROLOG_COMMON_3(n)
>> > \
>> > ACCOUNT_STOLEN_TIME
>> >
>> > diff --git a/arch/powerpc/include/asm/mmu.h
>> > b/arch/powerpc/include/asm/mmu.h
>> > index eb20eb3b8fb0..3b31ed702785 100644
>> > --- a/arch/powerpc/include/asm/mmu.h
>> > +++ b/arch/powerpc/include/asm/mmu.h
>> > @@ -107,6 +107,10 @@
>> > */
>> > #define MMU_FTR_1T_SEGMENT ASM_CONST(0x40000000)
>> >
>> > +/* Supports GUAP (key 0 controlling userspace addresses) on radix
>> > + */
>> > +#define MMU_FTR_RADIX_GUAP ASM_CONST(0x80000000)
>> > +
>> > /* MMU feature bit sets for various CPUs */
>> > #define MMU_FTRS_DEFAULT_HPTE_ARCH_V2 \
>> > MMU_FTR_HPTE_TABLE | MMU_FTR_PPCAS_ARCH_V2
>> > @@ -143,6 +147,9 @@ enum {
>> > MMU_FTR_KERNEL_RO | MMU_FTR_68_BIT_VA |
>> > #ifdef CONFIG_PPC_RADIX_MMU
>> > MMU_FTR_TYPE_RADIX |
>> > +#endif
>> > +#ifdef CONFIG_PPC_RADIX_GUAP
>> > + MMU_FTR_RADIX_GUAP |
>>
>> Can this exist without MMT_FTR_TYPE_RADIX ?
>
> No, no it can't.
>
>>
>> > #endif
>> > 0,
>> > };
>> > diff --git a/arch/powerpc/include/asm/paca.h
>> > b/arch/powerpc/include/asm/paca.h
>> > index e843bc5d1a0f..e905f09b2d38 100644
>> > --- a/arch/powerpc/include/asm/paca.h
>> > +++ b/arch/powerpc/include/asm/paca.h
>> > @@ -169,6 +169,9 @@ struct paca_struct {
>> > u64 saved_r1; /* r1 save for RTAS calls
>> > or PM or EE=0 */
>> > u64 saved_msr; /* MSR saved here by
>> > enter_rtas */
>> > u16 trap_save; /* Used when bad stack is
>> > encountered */
>> > +#ifdef CONFIG_PPC_RADIX_GUAP
>> > + u8 user_access_allowed; /* set when AMR allows user
>> > accesses */
>> > +#endif
>> > u8 irq_soft_mask; /* mask for irq soft masking */
>> > u8 irq_happened; /* irq happened while soft-disabled
>> > */
>> > u8 io_sync; /* writel() needs spin_unlock sync
>> > */
>> > diff --git a/arch/powerpc/include/asm/reg.h
>> > b/arch/powerpc/include/asm/reg.h
>> > index 640a4d818772..b994099a906b 100644
>> > --- a/arch/powerpc/include/asm/reg.h
>> > +++ b/arch/powerpc/include/asm/reg.h
>> > @@ -246,6 +246,7 @@
>> > #define SPRN_DSCR 0x11
>> > #define SPRN_CFAR 0x1c /* Come From Address Register */
>> > #define SPRN_AMR 0x1d /* Authority Mask Register */
>> > +#define AMR_LOCKED 0xC000000000000000ULL /* Read & Write
>> > disabled */
>>
>> Why ULL ? mtspr() takes unsigned long arg.
>>
>> > #define SPRN_UAMOR 0x9d /* User Authority Mask Override
>> > Register */
>> > #define SPRN_AMOR 0x15d /* Authority Mask Override Register
>> > */
>> > #define SPRN_ACOP 0x1F /* Available Coprocessor Register
>> > */
>> > diff --git a/arch/powerpc/include/asm/uaccess.h
>> > b/arch/powerpc/include/asm/uaccess.h
>> > index 15bea9a0f260..209bfc47c340 100644
>> > --- a/arch/powerpc/include/asm/uaccess.h
>> > +++ b/arch/powerpc/include/asm/uaccess.h
>> > @@ -62,6 +62,27 @@ static inline int __access_ok(unsigned long
>> > addr,
>> > unsigned long size,
>> >
>> > #endif
>> >
>> > +static inline void unlock_user_access(void)
>> > +{
>> > +#ifdef CONFIG_PPC_RADIX_GUAP
>> > + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) {
>>
>> You need to include the .h which provides mmu_has_feature()
>>
>> I think uaccess.h should only include the empty function for when
>> CONFIG_PPC_GUAP is not defined. Radix guap function should go in a
>> radix header file.
>>
>> > + mtspr(SPRN_AMR, 0);
>> > + isync();
>> > + get_paca()->user_access_allowed = 1;
>> > + }
>> > +#endif
>> > +}
>> > +
>> > +static inline void lock_user_access(void)
>> > +{
>> > +#ifdef CONFIG_PPC_RADIX_GUAP
>> > + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) {
>> > + mtspr(SPRN_AMR, AMR_LOCKED);
>> > + get_paca()->user_access_allowed = 0;
>> > + }
>> > +#endif
>> > +}
>> > +
>> > #define access_ok(type, addr, size) \
>> > (__chk_user_ptr(addr), \
>> > __access_ok((__force unsigned long)(addr), (size), get_fs()))
>> > @@ -141,6 +162,7 @@ extern long __put_user_bad(void);
>> > #define __put_user_size(x, ptr, size, retval)
>> > \
>> > do {
>> > \
>> > retval = 0; \
>> > + unlock_user_access(); \
>> > switch (size) { \
>> > case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
>> > case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
>> > @@ -148,6 +170,7 @@ do {
>> > \
>> > case 8: __put_user_asm2(x, ptr, retval); break; \
>> > default: __put_user_bad(); \
>> > } \
>> > + lock_user_access(); \
>> > } while (0)
>> >
>> > #define __put_user_nocheck(x, ptr, size) \
>> > @@ -240,6 +263,7 @@ do {
>> > \
>> > __chk_user_ptr(ptr); \
>> > if (size > sizeof(x)) \
>> > (x) = __get_user_bad(); \
>> > + unlock_user_access(); \
>> > switch (size) { \
>> > case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
>> > case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
>> > @@ -247,6 +271,7 @@ do {
>> > \
>> > case 8: __get_user_asm2(x, ptr, retval); break; \
>> > default: (x) = __get_user_bad(); \
>> > } \
>> > + lock_user_access(); \
>> > } while (0)
>> >
>> > /*
>> > @@ -306,15 +331,20 @@ extern unsigned long
>> > __copy_tofrom_user(void
>> > __user *to,
>> > static inline unsigned long
>> > raw_copy_in_user(void __user *to, const void __user *from,
>> > unsigned long n)
>> > {
>> > - return __copy_tofrom_user(to, from, n);
>> > + unsigned long ret;
>> > + unlock_user_access(); \
>> > + ret = __copy_tofrom_user(to, from, n); \
>> > + lock_user_access(); \
>> > + return ret; \
>> > }
>> > #endif /* __powerpc64__ */
>> >
>> > static inline unsigned long raw_copy_from_user(void *to,
>> > const void __user *from, unsigned long n)
>> > {
>> > + unsigned long ret;
>> > if (__builtin_constant_p(n) && (n <= 8)) {
>> > - unsigned long ret = 1;
>> > + ret = 1;
>> >
>> > switch (n) {
>> > case 1:
>> > @@ -339,14 +369,18 @@ static inline unsigned long
>> > raw_copy_from_user(void *to,
>> > }
>> >
>> > barrier_nospec();
>> > - return __copy_tofrom_user((__force void __user *)to, from, n);
>> > + unlock_user_access();
>> > + ret = __copy_tofrom_user((__force void __user *)to, from, n);
>> > + lock_user_access();
>> > + return ret;
>> > }
>> >
>> > static inline unsigned long raw_copy_to_user(void __user *to,
>> > const void *from, unsigned long n)
>> > {
>> > + unsigned long ret;
>> > if (__builtin_constant_p(n) && (n <= 8)) {
>> > - unsigned long ret = 1;
>> > + ret = 1;
>> >
>> > switch (n) {
>> > case 1:
>> > @@ -366,17 +400,24 @@ static inline unsigned long
>> > raw_copy_to_user(void __user *to,
>> > return 0;
>> > }
>> >
>> > - return __copy_tofrom_user(to, (__force const void __user
>> > *)from, n);
>> > + unlock_user_access();
>> > + ret = __copy_tofrom_user(to, (__force const void __user *)from,
>> > n);
>> > + lock_user_access();
>> > + return ret;
>> > }
>> >
>> > extern unsigned long __clear_user(void __user *addr, unsigned long
>> > size);
>> >
>> > static inline unsigned long clear_user(void __user *addr,
>> > unsigned
>> > long size)
>> > {
>> > + unsigned long ret = size;
>> > might_fault();
>> > - if (likely(access_ok(VERIFY_WRITE, addr, size)))
>> > - return __clear_user(addr, size);
>> > - return size;
>> > + if (likely(access_ok(VERIFY_WRITE, addr, size))) {
>> > + unlock_user_access();
>> > + ret = __clear_user(addr, size);
>> > + lock_user_access();
>> > + }
>> > + return ret;
>> > }
>> >
>> > extern long strncpy_from_user(char *dst, const char __user *src,
>> > long count);
>> > diff --git a/arch/powerpc/kernel/asm-offsets.c
>> > b/arch/powerpc/kernel/asm-offsets.c
>> > index 10ef2e4db2fd..5050f15ad2f5 100644
>> > --- a/arch/powerpc/kernel/asm-offsets.c
>> > +++ b/arch/powerpc/kernel/asm-offsets.c
>> > @@ -260,6 +260,7 @@ int main(void)
>> > OFFSET(ACCOUNT_STARTTIME_USER, paca_struct,
>> > accounting.starttime_user);
>> > OFFSET(ACCOUNT_USER_TIME, paca_struct, accounting.utime);
>> > OFFSET(ACCOUNT_SYSTEM_TIME, paca_struct, accounting.stime);
>> > + OFFSET(PACA_USER_ACCESS_ALLOWED, paca_struct,
>> > user_access_allowed);
>> > OFFSET(PACA_TRAP_SAVE, paca_struct, trap_save);
>> > OFFSET(PACA_NAPSTATELOST, paca_struct, nap_state_lost);
>> > OFFSET(PACA_SPRG_VDSO, paca_struct, sprg_vdso);
>> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > index f432054234a4..df4716624840 100644
>> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > @@ -337,6 +337,10 @@ static int __init
>> > feat_enable_mmu_radix(struct
>> > dt_cpu_feature *f)
>> > cur_cpu_spec->mmu_features |= MMU_FTRS_HASH_BASE;
>> > cur_cpu_spec->cpu_user_features |= PPC_FEATURE_HAS_MMU;
>> >
>> > +#ifdef CONFIG_PPC_RADIX_GUAP
>> > + cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_GUAP;
>> > +#endif
>> > +
>> > return 1;
>> > #endif
>> > return 0;
>> > diff --git a/arch/powerpc/kernel/entry_64.S
>> > b/arch/powerpc/kernel/entry_64.S
>> > index 7b1693adff2a..23f0944185d3 100644
>> > --- a/arch/powerpc/kernel/entry_64.S
>> > +++ b/arch/powerpc/kernel/entry_64.S
>> > @@ -297,7 +297,12 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> > b . /* prevent speculative execution */
>> >
>> > /* exit to kernel */
>> > -1: ld r2,GPR2(r1)
>> > +1: /* if the AMR was unlocked before, unlock it again */
>> > + lbz r2,PACA_USER_ACCESS_ALLOWED(r13)
>> > + cmpwi cr1,0
>> > + bne 2f
>> > + UNLOCK_USER_ACCESS(r2)
>> > +2: ld r2,GPR2(r1)
>> > ld r1,GPR1(r1)
>> > mtlr r4
>> > mtcr r5
>> > @@ -965,6 +970,7 @@ BEGIN_FTR_SECTION
>> > ld r2,_PPR(r1)
>> > mtspr SPRN_PPR,r2
>> > END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> > +
>> > ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
>> > REST_GPR(13, r1)
>> >
>> > @@ -983,7 +989,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> > RFI_TO_USER
>> > b . /* prevent speculative execution */
>> >
>> > -1: mtspr SPRN_SRR1,r3
>> > +1: /* exit to kernel */
>> > + /* if the AMR was unlocked before, unlock it again */
>> > + lbz r2,PACA_USER_ACCESS_ALLOWED(r13)
>> > + cmpwi cr1,0
>> > + bne 2f
>> > + UNLOCK_USER_ACCESS(r2)
>> > +
>> > +2: mtspr SPRN_SRR1,r3
>> >
>> > ld r2,_CCR(r1)
>> > mtcrf 0xFF,r2
>> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> > index d51cf5f4e45e..17fd8c6b055b 100644
>> > --- a/arch/powerpc/mm/fault.c
>> > +++ b/arch/powerpc/mm/fault.c
>> > @@ -462,6 +462,18 @@ static int __do_page_fault(struct pt_regs
>> > *regs, unsigned long address,
>> > return bad_key_fault_exception(regs, address,
>> > get_mm_addr_key(mm,
>> > address));
>> >
>> > +#ifdef CONFIG_PPC_RADIX_SMAP
>>
>> SMAP ?
>
> Whoops, leftover. Good catch.
>
>>
>> > + if (mmu_has_feature(MMU_FTR_RADIX_GUAP)) {
>> > + if (unlikely(!is_user &&
>> > + (error_code & DSISR_PROTFAULT) &&
>> > + (mfspr(SPRN_AMR) & AMR_LOCKED))) {
>>
>> Do you mean that in case of fault in user copy, we leave the
>> protection open for handling the exception ? What is the purpose of
>> the new paca flag then ?
>
> No. The protection doesn't get left open for handling the exception -
> in fact the opposite, the protection gets enforced again on entry. The
> PACA flag is to make sure that on exception exit we unlock the AMR on
> the way back out during usercopy, without it there is no way of knowing
> whether we're supposed to go back to the kernel locked or unlocked.
But then the above test checking SPRN_AMR will always be true.
>
>>
>> > + pr_crit("Kernel attempted to access user data"
>> > + " unsafely, possible exploit
>> > attempt\n");
>> > + return bad_area_nosemaphore(regs, address);
>> > + }
>> > + }
>>
>> Are we sure it is an access to user data ?
>
> No, this condition could in theory be hit by another kind of PROTFAULT
> hit in the kernel. I haven't seen that happen, but I should try
> checking the address or something.
What about checking search_exception_table() ?
Christophe
>
>>
>> > +#endif
>> > +
>> > /*
>> > * We want to do this outside mmap_sem, because reading code
>> > around nip
>> > * can result in fault, which will cause a deadlock when called
>> > with
>> > diff --git a/arch/powerpc/mm/pgtable-radix.c
>> > b/arch/powerpc/mm/pgtable-radix.c
>> > index c879979faa73..9e5b98887a05 100644
>> > --- a/arch/powerpc/mm/pgtable-radix.c
>> > +++ b/arch/powerpc/mm/pgtable-radix.c
>> > @@ -29,6 +29,7 @@
>> > #include <asm/powernv.h>
>> > #include <asm/sections.h>
>> > #include <asm/trace.h>
>> > +#include <asm/uaccess.h>
>> >
>> > #include <trace/events/thp.h>
>> >
>> > @@ -608,6 +609,7 @@ void __init radix__early_init_mmu(void)
>> > mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>> > radix_init_partition_table();
>> > radix_init_amor();
>> > + lock_user_access();
>> > } else {
>> > radix_init_pseries();
>> > }
>> > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
>> > index b271b283c785..0b9bc320138c 100644
>> > --- a/arch/powerpc/mm/pkeys.c
>> > +++ b/arch/powerpc/mm/pkeys.c
>> > @@ -7,6 +7,7 @@
>> >
>> > #include <asm/mman.h>
>> > #include <asm/setup.h>
>> > +#include <asm/uaccess.h>
>> > #include <linux/pkeys.h>
>> > #include <linux/of_device.h>
>> >
>> > @@ -266,7 +267,8 @@ int __arch_set_user_pkey_access(struct
>> > task_struct *tsk, int pkey,
>> >
>> > void thread_pkey_regs_save(struct thread_struct *thread)
>> > {
>> > - if (static_branch_likely(&pkey_disabled))
>> > + if (static_branch_likely(&pkey_disabled) &&
>> > + !mmu_has_feature(MMU_FTR_RADIX_GUAP))
>> > return;
>> >
>> > /*
>> > @@ -280,7 +282,8 @@ void thread_pkey_regs_save(struct thread_struct
>> > *thread)
>> > void thread_pkey_regs_restore(struct thread_struct *new_thread,
>> > struct thread_struct *old_thread)
>> > {
>> > - if (static_branch_likely(&pkey_disabled))
>> > + if (static_branch_likely(&pkey_disabled) &&
>> > + !mmu_has_feature(MMU_FTR_RADIX_GUAP))
>> > return;
>> >
>> > if (old_thread->amr != new_thread->amr)
>> > diff --git a/arch/powerpc/platforms/Kconfig.cputype
>> > b/arch/powerpc/platforms/Kconfig.cputype
>> > index f4e2c5729374..6617d3e415a7 100644
>> > --- a/arch/powerpc/platforms/Kconfig.cputype
>> > +++ b/arch/powerpc/platforms/Kconfig.cputype
>> > @@ -351,6 +351,21 @@ config PPC_RADIX_MMU_DEFAULT
>> >
>> > If you're unsure, say Y.
>> >
>> > +config PPC_RADIX_GUAP
>> > + bool "Guarded Userspace Access Prevention on Radix"
>> > + depends on PPC_RADIX_MMU
>> > + default y
>> > + help
>> > + Enable support for Guarded Userspace Access Prevention (GUAP)
>> > + when using the Radix MMU. GUAP is a security feature
>> > + preventing the kernel from directly accessing userspace data
>> > + without going through the proper checks.
>> > +
>> > + GUAP has a minor performance impact on context switching and
>> > can be
>> > + disabled at boot time using the "nosmap" kernel command line
>> > option.
>> > +
>> > + If you're unsure, say Y.
>> > +
>>
>> I think this should be a named in a generic way without the radix
>> thing.
>> Then one day it will be reused by the 8xx
>
> I agree in theory, will have to play with making it more generic.
>
> Thanks for the review.
>
> - Russell
>
>>
>> Christophe
>>
>> > config ARCH_ENABLE_HUGEPAGE_MIGRATION
>> > def_bool y
>> > depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
>> > --
>> > 2.19.1
>>
>>
^ permalink raw reply
* Re: [PATCH 6/9] PCI: consolidate PCI config entry in drivers/pci
From: Masahiro Yamada @ 2018-10-31 16:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, linux-scsi, Linux Kbuild mailing list, linux-pci,
Linux Kernel Mailing List, Dominik Brodowski, Russell King,
Alex Bounine, linuxppc-dev, linux-arm-kernel
In-Reply-To: <CAK7LNATZ8LagrTrTae_QCQbSuTW7_y6sDrdTHfTyONPrsYDs4A@mail.gmail.com>
Hi Christoph,
On Fri, Oct 19, 2018 at 9:58 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Fri, Oct 19, 2018 at 9:23 PM Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
>
> > > index a68b34183107..b185794549be 100644
> > > --- a/arch/arm/mach-pxa/Kconfig
> > > +++ b/arch/arm/mach-pxa/Kconfig
> > > @@ -125,7 +125,7 @@ config MACH_ARMCORE
> > > bool "CompuLab CM-X255/CM-X270 modules"
> > > select ARCH_HAS_DMA_SET_COHERENT_MASK if PCI
> > > select IWMMXT
> > > - select MIGHT_HAVE_PCI
> > > + select HAVE_PCI
> > > select NEED_MACH_IO_H if PCI
> > > select PXA25x
> > > select PXA27x
> >
> > This is wrong. "MIGHT_HAVE_PCI" is _not_ the same as "HAVE_PCI" - we
> > have a bunch of platforms that mandatorily have PCI and these select
> > PCI directly. "MIGHT_HAVE_PCI" controls the _visibility_ of the PCI
> > menu option, but does not prevent it being selected. Your patch will
> > cause Kconfig to complain for those which mandatorily have PCI but
> > do not set HAVE_PCI.
>
>
> Good catch!
> But, adding a bunch of 'select HAVE_PCI' along with 'select PCI' is ugly.
>
> Do you have any suggestion?
>
> How about letting CONFIG_ARM to select HAVE_PCI ?
>
I applied 1/9, 3/9, 4/9, 5/9.
(I think 2/9 should be squashed to 9/9)
As Russell pointed out, we need to avoid
the unmet dependency.
Are you planning to send
the updated version for 6/9 through - 9/9 ?
If so, could you please rebase 6/9
so that it is cleanly applicable ?
Thanks.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2 3/3] selftests/powerpc: Skip test instead of failing
From: Thiago Jung Bauermann @ 2018-10-31 15:45 UTC (permalink / raw)
To: Breno Leitao; +Cc: linuxppc-dev, Tyrel Datwyler
In-Reply-To: <1540996702-27161-3-git-send-email-leitao@debian.org>
Breno Leitao <leitao@debian.org> writes:
> Current core-pkey selftest fails if the test runs without privileges to
> write into the core pattern file (/proc/sys/kernel/core_pattern). This
> causes the test to fail and give the impression that the subsystem being
> tested is broken, when, in fact, the test is being executed without the
> proper privileges. This is the current error:
>
> test: core_pkey
> tags: git_version:v4.19-3-g9e3363be9bce-dirty
> Error writing to core_pattern file: Permission denied
> failure: core_pkey
>
> This patch simply skips this test if it runs without the proper privileges,
> avoiding this undesired failure.
>
> CC: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> CC: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
> tools/testing/selftests/powerpc/ptrace/core-pkey.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/powerpc/ptrace/core-pkey.c b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> index e23e2e199eb4..d5c64fee032d 100644
> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
> @@ -352,10 +352,7 @@ static int write_core_pattern(const char *core_pattern)
> FILE *f;
>
> f = fopen(core_pattern_file, "w");
> - if (!f) {
> - perror("Error writing to core_pattern file");
> - return TEST_FAIL;
> - }
> + SKIP_IF_MSG(!f, "Try with root privileges");
>
> ret = fwrite(core_pattern, 1, len, f);
> fclose(f);
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v2 2/3] selftests/powerpc: Create a new SKIP_IF macro
From: Thiago Jung Bauermann @ 2018-10-31 15:44 UTC (permalink / raw)
To: Breno Leitao; +Cc: linuxppc-dev
In-Reply-To: <1540996702-27161-2-git-send-email-leitao@debian.org>
Breno Leitao <leitao@debian.org> writes:
> This patch creates a new macro that skips a test and prints a message to
> stderr. This is useful to give an idea why the tests is being skipped,
> other than just skipping the test blindly.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> ---
> tools/testing/selftests/powerpc/include/utils.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/tools/testing/selftests/powerpc/include/utils.h b/tools/testing/selftests/powerpc/include/utils.h
> index da1b963cdb32..486c1782c23e 100644
> --- a/tools/testing/selftests/powerpc/include/utils.h
> +++ b/tools/testing/selftests/powerpc/include/utils.h
> @@ -72,6 +72,16 @@ do { \
> } \
> } while (0)
>
> +#define SKIP_IF_MSG(x, msg) \
> +do { \
> + if ((x)) { \
> + fprintf(stderr, \
> + "[SKIP] Test skipped on line %d: %s\n", \
> + __LINE__, msg); \
> + return MAGIC_SKIP_RETURN_VALUE; \
> + } \
> +} while (0)
> +
> #define _str(s) #s
> #define str(s) _str(s)
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] selftests: powerpc: Fix warning for security subdir
From: Naveen N. Rao @ 2018-10-31 15:38 UTC (permalink / raw)
To: Joel Stanley, linuxppc-dev
In-Reply-To: <20181022120926.12797-1-joel@jms.id.au>
Joel Stanley wrote:
> typing 'make' inside tools/testing/selftests/powerpc gave a build
> warning:
>
> BUILD_TARGET=tools/testing/selftests/powerpc/security; mkdir -p $BUILD_TARGET; make OUTPUT=$BUILD_TARGET -k -C security all
> make[1]: Entering directory 'tools/testing/selftests/powerpc/security'
> ../../lib.mk:20: ../../../../scripts/subarch.include: No such file or directory
> make[1]: *** No rule to make target '../../../../scripts/subarch.include'.
> make[1]: Failed to remake makefile '../../../../scripts/subarch.include'.
>
> The build is one level deeper than lib.mk thinks it is. Set top_srcdir
> to set things straight.
>
> Note that the test program is still built.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Thanks.
Tested-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox