* [PATCH v2 2/2] powerpc/watchdog: provide more data in watchdog messages
From: Nicholas Piggin @ 2018-05-05 7:26 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Balbir Singh
In-Reply-To: <20180505072600.13270-1-npiggin@gmail.com>
Provide timebase and timebase of last heartbeat in watchdog lockup
messages. Also provide a stack trace of when a CPU becomes un-stuck,
which can be useful -- it could be where irqs are re-enabled, so it
may be the end of the critical section which is responsible for the
latency which is useful information.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/watchdog.c | 28 +++++++++++++++++++++++++---
1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/watchdog.c b/arch/powerpc/kernel/watchdog.c
index 0bc701f9ab35..a99951e8199e 100644
--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -111,7 +111,13 @@ static inline void wd_smp_unlock(unsigned long *flags)
static void wd_lockup_ipi(struct pt_regs *regs)
{
- pr_emerg("CPU %d Hard LOCKUP\n", raw_smp_processor_id());
+ int cpu = raw_smp_processor_id();
+ u64 tb = get_tb();
+
+ pr_emerg("CPU %d Hard LOCKUP\n", cpu);
+ pr_emerg("CPU %d TB:%lld, last heartbeat TB:%lld (%lldms ago)\n",
+ cpu, tb, per_cpu(wd_timer_tb, cpu),
+ tb_to_ns(tb - per_cpu(wd_timer_tb, cpu)) / 1000000);
print_modules();
print_irqtrace_events(current);
if (regs)
@@ -154,6 +160,9 @@ static void watchdog_smp_panic(int cpu, u64 tb)
pr_emerg("CPU %d detected hard LOCKUP on other CPUs %*pbl\n",
cpu, cpumask_pr_args(&wd_smp_cpus_pending));
+ pr_emerg("CPU %d TB:%lld, last SMP heartbeat TB:%lld (%lldms ago)\n",
+ cpu, tb, wd_smp_last_reset_tb,
+ tb_to_ns(tb - wd_smp_last_reset_tb) / 1000000);
if (!sysctl_hardlockup_all_cpu_backtrace) {
/*
@@ -194,10 +203,19 @@ static void wd_smp_clear_cpu_pending(int cpu, u64 tb)
{
if (!cpumask_test_cpu(cpu, &wd_smp_cpus_pending)) {
if (unlikely(cpumask_test_cpu(cpu, &wd_smp_cpus_stuck))) {
+ struct pt_regs *regs = get_irq_regs();
unsigned long flags;
- pr_emerg("CPU %d became unstuck\n", cpu);
wd_smp_lock(&flags);
+
+ pr_emerg("CPU %d became unstuck TB:%lld\n",
+ cpu, tb);
+ print_irqtrace_events(current);
+ if (regs)
+ show_regs(regs);
+ else
+ dump_stack();
+
cpumask_clear_cpu(cpu, &wd_smp_cpus_stuck);
wd_smp_unlock(&flags);
}
@@ -252,7 +270,11 @@ void soft_nmi_interrupt(struct pt_regs *regs)
}
set_cpu_stuck(cpu, tb);
- pr_emerg("CPU %d self-detected hard LOCKUP @ %pS\n", cpu, (void *)regs->nip);
+ pr_emerg("CPU %d self-detected hard LOCKUP @ %pS\n",
+ cpu, (void *)regs->nip);
+ pr_emerg("CPU %d TB:%lld, last heartbeat TB:%lld (%lldms ago)\n",
+ cpu, tb, per_cpu(wd_timer_tb, cpu),
+ tb_to_ns(tb - per_cpu(wd_timer_tb, cpu)) / 1000000);
print_modules();
print_irqtrace_events(current);
show_regs(regs);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 1/2] powerpc/watchdog: don't update the watchdog timestamp if a lockup is detected
From: Nicholas Piggin @ 2018-05-05 7:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Balbir Singh
In-Reply-To: <20180505072600.13270-1-npiggin@gmail.com>
The watchdog heartbeat timestamp is updated when the local heartbeat
timer fires (or touch_nmi_watchdog() is called).
This is an interesting data point, so don't overwrite it when the
soft-NMI interrupt detects a hard lockup. That code came from a pre-
merge version to prevent hard lockup messages flood, but that's taken
care of with the stuck CPU logic now, so there is no reason to
update the heartbeat timestamp here.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/watchdog.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/powerpc/kernel/watchdog.c b/arch/powerpc/kernel/watchdog.c
index 6256dc3b0087..0bc701f9ab35 100644
--- a/arch/powerpc/kernel/watchdog.c
+++ b/arch/powerpc/kernel/watchdog.c
@@ -245,8 +245,6 @@ void soft_nmi_interrupt(struct pt_regs *regs)
tb = get_tb();
if (tb - per_cpu(wd_timer_tb, cpu) >= wd_panic_timeout_tb) {
- per_cpu(wd_timer_tb, cpu) = tb;
-
wd_smp_lock(&flags);
if (cpumask_test_cpu(cpu, &wd_smp_cpus_stuck)) {
wd_smp_unlock(&flags);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 0/2] powerpc/watchdog: provide more data in watchdog messages
From: Nicholas Piggin @ 2018-05-05 7:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Balbir Singh
Since v1, Balbir's feedback:
- Split into two patches
- Added human-readable intervals as well as TB timestamps.
Nicholas Piggin (2):
powerpc/watchdog: don't update the watchdog timestamp if a lockup is
detected
powerpc/watchdog: provide more data in watchdog messages
arch/powerpc/kernel/watchdog.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)
--
2.17.0
^ permalink raw reply
* Re: [RFC PATCH 1/4] powerpc/64: Save stack pointer when we hard disable interrupts
From: Nicholas Piggin @ 2018-05-05 6:26 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <20180502130729.24077-1-mpe@ellerman.id.au>
On Wed, 2 May 2018 23:07:26 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> A CPU that gets stuck with interrupts hard disable can be difficult to
> debug, as on some platforms we have no way to interrupt the CPU to
> find out what it's doing.
>
> A stop-gap is to have the CPU save it's stack pointer (r1) in its paca
> when it hard disables interrupts. That way if we can't interrupt it,
> we can at least trace the stack based on where it last disabled
> interrupts.
>
> In some cases that will be total junk, but the stack trace code should
> handle that. In the simple case of a CPU that disable interrupts and
> then gets stuck in a loop, the stack trace should be informative.
>
> We could clear the saved stack pointer when we enable interrupts, but
> that loses information which could be useful if we have nothing else
> to go on.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/include/asm/hw_irq.h | 6 +++++-
> arch/powerpc/include/asm/paca.h | 2 +-
> arch/powerpc/kernel/exceptions-64s.S | 1 +
> arch/powerpc/xmon/xmon.c | 2 ++
> 4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 855e17d158b1..35cb37be61fe 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -237,8 +237,12 @@ static inline bool arch_irqs_disabled(void)
> __hard_irq_disable(); \
> flags = irq_soft_mask_set_return(IRQS_ALL_DISABLED); \
> local_paca->irq_happened |= PACA_IRQ_HARD_DIS; \
> - if (!arch_irqs_disabled_flags(flags)) \
> + if (!arch_irqs_disabled_flags(flags)) { \
> + asm ("stdx %%r1, 0, %1 ;" \
> + : "=m" (local_paca->saved_r1) \
> + : "b" (&local_paca->saved_r1)); \
> trace_hardirqs_off(); \
> + }
This is pretty neat, it would be good to have something that's not so
destructive as the NMI IPI.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v13 3/3] mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
From: Dave Hansen @ 2018-05-05 4:42 UTC (permalink / raw)
To: Ram Pai
Cc: mpe, mingo, akpm, linuxppc-dev, linux-mm, x86, linux-arch,
linux-kernel, benh, paulus, khandual, aneesh.kumar, bsingharora,
hbabu, mhocko, bauerman, ebiederm, corbet, arnd
In-Reply-To: <20180505011243.GB5617@ram.oc3035372033.ibm.com>
On 05/04/2018 06:12 PM, Ram Pai wrote:
>> That new line boils down to:
>>
>> [ilog2(0)] = "",
>>
>> on x86. It wasn't *obvious* to me that it is OK to do that. The other
>> possibly undefined bits (VM_SOFTDIRTY for instance) #ifdef themselves
>> out of this array.
>>
>> I would just be a wee bit worried that this would overwrite the 0 entry
>> ("??") with "".
> Yes it would :-( and could potentially break anything that depends on
> 0th entry being "??"
>
> Is the following fix acceptable?
>
> #if VM_PKEY_BIT4
> [ilog2(VM_PKEY_BIT4)] = "",
> #endif
Yep, I think that works for me.
^ permalink raw reply
* [PATCH] powerpc: cpm_gpio: Remove owner assignment from platform_driver
From: Fabio Estevam @ 2018-05-05 3:01 UTC (permalink / raw)
To: mpe; +Cc: benh, paulus, linuxppc-dev, Fabio Estevam
From: Fabio Estevam <fabio.estevam@nxp.com>
Structure platform_driver does not need to set the owner field, as this
will be populated by the driver core.
Generated by scripts/coccinelle/api/platform_no_drv_owner.cocci.
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
arch/powerpc/sysdev/cpm_gpio.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/sysdev/cpm_gpio.c b/arch/powerpc/sysdev/cpm_gpio.c
index 0badc90..0695d26 100644
--- a/arch/powerpc/sysdev/cpm_gpio.c
+++ b/arch/powerpc/sysdev/cpm_gpio.c
@@ -63,7 +63,6 @@ static struct platform_driver cpm_gpio_driver = {
.probe = cpm_gpio_probe,
.driver = {
.name = "cpm-gpio",
- .owner = THIS_MODULE,
.of_match_table = cpm_gpio_match,
},
};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v13 3/3] mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
From: Ram Pai @ 2018-05-05 1:12 UTC (permalink / raw)
To: Dave Hansen
Cc: mpe, mingo, akpm, linuxppc-dev, linux-mm, x86, linux-arch,
linux-kernel, benh, paulus, khandual, aneesh.kumar, bsingharora,
hbabu, mhocko, bauerman, ebiederm, corbet, arnd
In-Reply-To: <1e37895e-5a18-11c1-58f1-834f96dfd4d5@intel.com>
On Fri, May 04, 2018 at 03:57:33PM -0700, Dave Hansen wrote:
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 0c9e392..3ddddc7 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -679,6 +679,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> > [ilog2(VM_PKEY_BIT1)] = "",
> > [ilog2(VM_PKEY_BIT2)] = "",
> > [ilog2(VM_PKEY_BIT3)] = "",
> > + [ilog2(VM_PKEY_BIT4)] = "",
> > #endif /* CONFIG_ARCH_HAS_PKEYS */
> ...
> > +#if defined(CONFIG_PPC)
> > +# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
> > +#else
> > +# define VM_PKEY_BIT4 0
> > +#endif
> > #endif /* CONFIG_ARCH_HAS_PKEYS */
>
> That new line boils down to:
>
> [ilog2(0)] = "",
>
> on x86. It wasn't *obvious* to me that it is OK to do that. The other
> possibly undefined bits (VM_SOFTDIRTY for instance) #ifdef themselves
> out of this array.
>
> I would just be a wee bit worried that this would overwrite the 0 entry
> ("??") with "".
Yes it would :-( and could potentially break anything that depends on
0th entry being "??"
Is the following fix acceptable?
#if VM_PKEY_BIT4
[ilog2(VM_PKEY_BIT4)] = "",
#endif
--
Ram Pai
^ permalink raw reply
* Re: [PATCH v13 3/3] mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
From: Dave Hansen @ 2018-05-04 22:57 UTC (permalink / raw)
To: Ram Pai, mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-kernel, benh,
paulus, khandual, aneesh.kumar, bsingharora, hbabu, mhocko,
bauerman, ebiederm, corbet, arnd
In-Reply-To: <1525471183-21277-3-git-send-email-linuxram@us.ibm.com>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 0c9e392..3ddddc7 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -679,6 +679,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> [ilog2(VM_PKEY_BIT1)] = "",
> [ilog2(VM_PKEY_BIT2)] = "",
> [ilog2(VM_PKEY_BIT3)] = "",
> + [ilog2(VM_PKEY_BIT4)] = "",
> #endif /* CONFIG_ARCH_HAS_PKEYS */
...
> +#if defined(CONFIG_PPC)
> +# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
> +#else
> +# define VM_PKEY_BIT4 0
> +#endif
> #endif /* CONFIG_ARCH_HAS_PKEYS */
That new line boils down to:
[ilog2(0)] = "",
on x86. It wasn't *obvious* to me that it is OK to do that. The other
possibly undefined bits (VM_SOFTDIRTY for instance) #ifdef themselves
out of this array.
I would just be a wee bit worried that this would overwrite the 0 entry
("??") with "".
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays
From: Benjamin Herrenschmidt @ 2018-05-04 22:29 UTC (permalink / raw)
To: Michael Ellerman, Cédric Le Goater, linuxppc-dev
In-Reply-To: <87d0yb8zye.fsf@concordia.ellerman.id.au>
On Fri, 2018-05-04 at 20:42 +1000, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
>
> > This is not the case for the moment, but future releases of pHyp might
> > need to introduce some synchronisation routines under the hood which
> > would make the XIVE hcalls longer to complete.
> >
> > As this was done for H_INT_RESET, let's wrap the other hcalls in a
> > loop catching the H_LONG_BUSY_* codes.
>
> Are we sure it's safe to msleep() in all these paths?
Probably not. We can have the IRQ descriptor lock. We might need to
mdelay.
There's a Kconfig option (forgot which one) that will add checks for
attempts to sleep inside locks, you should run with that.
Cheers,
Ben.
>
> cheers
>
> > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
> > index 7113f5d87952..97ea0a67a173 100644
> > --- a/arch/powerpc/sysdev/xive/spapr.c
> > +++ b/arch/powerpc/sysdev/xive/spapr.c
> > @@ -165,7 +165,10 @@ static long plpar_int_get_source_info(unsigned long flags,
> > unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
> > long rc;
> >
> > - rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
> > + do {
> > + rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
> > + } while (plpar_busy_delay(rc));
> > +
> > if (rc) {
> > pr_err("H_INT_GET_SOURCE_INFO lisn=%ld failed %ld\n", lisn, rc);
> > return rc;
> > @@ -198,8 +201,11 @@ static long plpar_int_set_source_config(unsigned long flags,
> > flags, lisn, target, prio, sw_irq);
> >
> >
> > - rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
> > - target, prio, sw_irq);
> > + do {
> > + rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
> > + target, prio, sw_irq);
> > + } while (plpar_busy_delay(rc));
> > +
> > if (rc) {
> > pr_err("H_INT_SET_SOURCE_CONFIG lisn=%ld target=%lx prio=%lx failed %ld\n",
> > lisn, target, prio, rc);
> > @@ -218,7 +224,11 @@ static long plpar_int_get_queue_info(unsigned long flags,
> > unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
> > long rc;
> >
> > - rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target, priority);
> > + do {
> > + rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target,
> > + priority);
> > + } while (plpar_busy_delay(rc));
> > +
> > if (rc) {
> > pr_err("H_INT_GET_QUEUE_INFO cpu=%ld prio=%ld failed %ld\n",
> > target, priority, rc);
> > @@ -247,8 +257,11 @@ static long plpar_int_set_queue_config(unsigned long flags,
> > pr_devel("H_INT_SET_QUEUE_CONFIG flags=%lx target=%lx priority=%lx qpage=%lx qsize=%lx\n",
> > flags, target, priority, qpage, qsize);
> >
> > - rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
> > - priority, qpage, qsize);
> > + do {
> > + rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
> > + priority, qpage, qsize);
> > + } while (plpar_busy_delay(rc));
> > +
> > if (rc) {
> > pr_err("H_INT_SET_QUEUE_CONFIG cpu=%ld prio=%ld qpage=%lx returned %ld\n",
> > target, priority, qpage, rc);
> > @@ -262,7 +275,10 @@ static long plpar_int_sync(unsigned long flags, unsigned long lisn)
> > {
> > long rc;
> >
> > - rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
> > + do {
> > + rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
> > + } while (plpar_busy_delay(rc));
> > +
> > if (rc) {
> > pr_err("H_INT_SYNC lisn=%ld returned %ld\n", lisn, rc);
> > return rc;
> > @@ -285,7 +301,11 @@ static long plpar_int_esb(unsigned long flags,
> > pr_devel("H_INT_ESB flags=%lx lisn=%lx offset=%lx in=%lx\n",
> > flags, lisn, offset, in_data);
> >
> > - rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset, in_data);
> > + do {
> > + rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset,
> > + in_data);
> > + } while (plpar_busy_delay(rc));
> > +
> > if (rc) {
> > pr_err("H_INT_ESB lisn=%ld offset=%ld returned %ld\n",
> > lisn, offset, rc);
> > --
> > 2.13.6
^ permalink raw reply
* [PATCH v11 3/3] mm, x86, powerpc: display pkey in smaps only if arch supports pkeys
From: Ram Pai @ 2018-05-04 21:59 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-kernel,
dave.hansen, benh, paulus, khandual, aneesh.kumar, bsingharora,
hbabu, mhocko, bauerman, ebiederm, linuxram, corbet, arnd
In-Reply-To: <1525471183-21277-1-git-send-email-linuxram@us.ibm.com>
Currently the architecture specific code is expected to
display the protection keys in smap for a given vma.
This can lead to redundant code and possibly to divergent
formats in which the key gets displayed.
This patch changes the implementation. It displays the
pkey only if the architecture support pkeys, i.e
arch_pkeys_enabled() returns true. This patch
provides x86 implementation for arch_pkeys_enabled().
x86 arch_show_smap() function is not needed anymore.
Deleting it.
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
(fixed compilation errors for x86 configs)
Acked-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/include/asm/mmu_context.h | 5 -----
arch/x86/include/asm/mmu_context.h | 5 -----
arch/x86/include/asm/pkeys.h | 1 +
arch/x86/kernel/fpu/xstate.c | 5 +++++
arch/x86/kernel/setup.c | 8 --------
fs/proc/task_mmu.c | 11 ++++++-----
include/linux/pkeys.h | 7 ++++++-
7 files changed, 18 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 1835ca1..896efa5 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -250,11 +250,6 @@ static inline bool arch_vma_access_permitted(struct vm_area_struct *vma,
#define thread_pkey_regs_restore(new_thread, old_thread)
#define thread_pkey_regs_init(thread)
-static inline int vma_pkey(struct vm_area_struct *vma)
-{
- return 0;
-}
-
static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
{
return 0x0UL;
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index 57e3785..3d748bd 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -296,11 +296,6 @@ static inline int vma_pkey(struct vm_area_struct *vma)
return (vma->vm_flags & vma_pkey_mask) >> VM_PKEY_SHIFT;
}
-#else
-static inline int vma_pkey(struct vm_area_struct *vma)
-{
- return 0;
-}
#endif
/*
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index a0ba1ff..f6c287b 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -6,6 +6,7 @@
extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
unsigned long init_val);
+extern bool arch_pkeys_enabled(void);
/*
* Try to dedicate one of the protection keys to be used as an
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 87a57b7..4f566e9 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -945,6 +945,11 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return 0;
}
+
+bool arch_pkeys_enabled(void)
+{
+ return boot_cpu_has(X86_FEATURE_OSPKE);
+}
#endif /* ! CONFIG_ARCH_HAS_PKEYS */
/*
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 6285697..960dbab 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1306,11 +1306,3 @@ static int __init register_kernel_offset_dumper(void)
return 0;
}
__initcall(register_kernel_offset_dumper);
-
-void arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
-{
- if (!boot_cpu_has(X86_FEATURE_OSPKE))
- return;
-
- seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
-}
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3ddddc7..9ce0097 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -18,10 +18,12 @@
#include <linux/page_idle.h>
#include <linux/shmem_fs.h>
#include <linux/uaccess.h>
+#include <linux/pkeys.h>
#include <asm/elf.h>
#include <asm/tlb.h>
#include <asm/tlbflush.h>
+#include <asm/mmu_context.h>
#include "internal.h"
#define SEQ_PUT_DEC(str, val) \
@@ -728,12 +730,9 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
}
#endif /* HUGETLB_PAGE */
-void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
-{
-}
-
#define SEQ_PUT_DEC(str, val) \
seq_put_decimal_ull_width(m, str, (val) >> 10, 8)
+
static int show_smap(struct seq_file *m, void *v, int is_pid)
{
struct proc_maps_private *priv = m->private;
@@ -836,9 +835,11 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
seq_puts(m, " kB\n");
}
if (!rollup_mode) {
- arch_show_smap(m, vma);
+ if (arch_pkeys_enabled())
+ seq_printf(m, "ProtectionKey: %8u\n", vma_pkey(vma));
show_smap_vma_flags(m, vma);
}
+
m_cache_vma(m, vma);
return ret;
}
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 0794ca7..49dff15 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -3,7 +3,6 @@
#define _LINUX_PKEYS_H
#include <linux/mm_types.h>
-#include <asm/mmu_context.h>
#ifdef CONFIG_ARCH_HAS_PKEYS
#include <asm/pkeys.h>
@@ -13,6 +12,7 @@
#define arch_override_mprotect_pkey(vma, prot, pkey) (0)
#define PKEY_DEDICATED_EXECUTE_ONLY 0
#define ARCH_VM_PKEY_FLAGS 0
+#define vma_pkey(vma) 0
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
@@ -35,6 +35,11 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return 0;
}
+static inline bool arch_pkeys_enabled(void)
+{
+ return false;
+}
+
static inline void copy_init_pkru_to_fpregs(void)
{
}
--
1.7.1
^ permalink raw reply related
* [PATCH v13 3/3] mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
From: Ram Pai @ 2018-05-04 21:59 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-kernel,
dave.hansen, benh, paulus, khandual, aneesh.kumar, bsingharora,
hbabu, mhocko, bauerman, ebiederm, linuxram, corbet, arnd
In-Reply-To: <1525471183-21277-1-git-send-email-linuxram@us.ibm.com>
Only 4bits are allocated in the vma flags to hold 16 keys. This is
sufficient on x86. PowerPC supports 32 keys, which needs 5bits.
Allocate an additional bit.
cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Balbir Singh <bsingharora@gmail.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
fs/proc/task_mmu.c | 1 +
include/linux/mm.h | 8 +++++++-
2 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 0c9e392..3ddddc7 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -679,6 +679,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
[ilog2(VM_PKEY_BIT1)] = "",
[ilog2(VM_PKEY_BIT2)] = "",
[ilog2(VM_PKEY_BIT3)] = "",
+ [ilog2(VM_PKEY_BIT4)] = "",
#endif /* CONFIG_ARCH_HAS_PKEYS */
};
size_t i;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index c6a6f24..cca67d1 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -230,10 +230,16 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
#ifdef CONFIG_ARCH_HAS_PKEYS
# define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
-# define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 4-bit value */
+/* Protection key is a 4-bit value on x86 and 5-bit value on ppc64 */
+# define VM_PKEY_BIT0 VM_HIGH_ARCH_0
# define VM_PKEY_BIT1 VM_HIGH_ARCH_1
# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
+#if defined(CONFIG_PPC)
+# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
+#else
+# define VM_PKEY_BIT4 0
+#endif
#endif /* CONFIG_ARCH_HAS_PKEYS */
#if defined(CONFIG_X86)
--
1.7.1
^ permalink raw reply related
* [PATCH v13 1/3] mm, powerpc, x86: define VM_PKEY_BITx bits if CONFIG_ARCH_HAS_PKEYS is enabled
From: Ram Pai @ 2018-05-04 21:59 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-kernel,
dave.hansen, benh, paulus, khandual, aneesh.kumar, bsingharora,
hbabu, mhocko, bauerman, ebiederm, linuxram, corbet, arnd
In-Reply-To: <1525471183-21277-1-git-send-email-linuxram@us.ibm.com>
VM_PKEY_BITx are defined only if CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
is enabled. Powerpc also needs these bits. Hence lets define the
VM_PKEY_BITx bits for any architecture that enables
CONFIG_ARCH_HAS_PKEYS.
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Reviewed-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/pkeys.h | 2 ++
fs/proc/task_mmu.c | 4 ++--
include/linux/mm.h | 9 +++++----
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 3a9b82b..425b181 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -26,6 +26,8 @@
# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
+#elif !defined(VM_PKEY_BIT4)
+# define VM_PKEY_BIT4 VM_HIGH_ARCH_4
#endif
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 65ae546..0c9e392 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -673,13 +673,13 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
[ilog2(VM_MERGEABLE)] = "mg",
[ilog2(VM_UFFD_MISSING)]= "um",
[ilog2(VM_UFFD_WP)] = "uw",
-#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
+#ifdef CONFIG_ARCH_HAS_PKEYS
/* These come out via ProtectionKey: */
[ilog2(VM_PKEY_BIT0)] = "",
[ilog2(VM_PKEY_BIT1)] = "",
[ilog2(VM_PKEY_BIT2)] = "",
[ilog2(VM_PKEY_BIT3)] = "",
-#endif
+#endif /* CONFIG_ARCH_HAS_PKEYS */
};
size_t i;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1ac1f06..c6a6f24 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -228,15 +228,16 @@ extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *,
#define VM_HIGH_ARCH_4 BIT(VM_HIGH_ARCH_BIT_4)
#endif /* CONFIG_ARCH_USES_HIGH_VMA_FLAGS */
-#if defined(CONFIG_X86)
-# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
-#if defined (CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS)
+#ifdef CONFIG_ARCH_HAS_PKEYS
# define VM_PKEY_SHIFT VM_HIGH_ARCH_BIT_0
# define VM_PKEY_BIT0 VM_HIGH_ARCH_0 /* A protection key is a 4-bit value */
# define VM_PKEY_BIT1 VM_HIGH_ARCH_1
# define VM_PKEY_BIT2 VM_HIGH_ARCH_2
# define VM_PKEY_BIT3 VM_HIGH_ARCH_3
-#endif
+#endif /* CONFIG_ARCH_HAS_PKEYS */
+
+#if defined(CONFIG_X86)
+# define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */
#elif defined(CONFIG_PPC)
# define VM_SAO VM_ARCH_1 /* Strong Access Ordering (powerpc) */
#elif defined(CONFIG_PARISC)
--
1.7.1
^ permalink raw reply related
* [PATCH v13 0/3] mm, x86, powerpc: Enhancements to Memory Protection Keys.
From: Ram Pai @ 2018-05-04 21:59 UTC (permalink / raw)
To: mpe, mingo, akpm
Cc: linuxppc-dev, linux-mm, x86, linux-arch, linux-kernel,
dave.hansen, benh, paulus, khandual, aneesh.kumar, bsingharora,
hbabu, mhocko, bauerman, ebiederm, linuxram, corbet, arnd
This patch series provides arch-neutral enhancements to
enable memory-keys on new architecutes, and the corresponding
changes in x86 and powerpc specific code to support that.
a) Provides ability to support upto 32 keys. PowerPC
can handle 32 keys and hence needs this.
b) Arch-neutral code; and not the arch-specific code,
determines the format of the string, that displays the key
for each vma in smaps.
History:
-------
version 14:
(1) made VM_PKEY_BIT4 unusable on x86, #defined it to 0
-- comment by Dave Hansen
(2) due to some reason this patch series continue to
break some or the other build. The last series
passed everything but created a merge
conflict followed by build failure for
Michael Ellermen. :(
version v13:
(1) fixed a git bisect error. :(
version v12:
(1) fixed compilation errors seen with various x86
configs.
version v11:
(1) code that displays key in smaps is not any more
defined under CONFIG_ARCH_HAS_PKEYS.
- Comment by Eric W. Biederman and Michal Hocko
(2) merged two patches that implemented (1).
- comment by Michal Hocko
version prior to v11:
(1) used one additional bit from VM_HIGH_ARCH_*
to support 32 keys.
- Suggestion by Dave Hansen.
(2) powerpc specific changes to support memory keys.
Ram Pai (3):
mm, powerpc, x86: define VM_PKEY_BITx bits if CONFIG_ARCH_HAS_PKEYS
is enabled
mm, powerpc, x86: introduce an additional vma bit for powerpc pkey
mm, x86, powerpc: display pkey in smaps only if arch supports pkeys
arch/powerpc/include/asm/mmu_context.h | 5 -----
arch/powerpc/include/asm/pkeys.h | 2 ++
arch/x86/include/asm/mmu_context.h | 5 -----
arch/x86/include/asm/pkeys.h | 1 +
arch/x86/kernel/fpu/xstate.c | 5 +++++
arch/x86/kernel/setup.c | 8 --------
fs/proc/task_mmu.c | 16 +++++++++-------
include/linux/mm.h | 15 +++++++++++----
include/linux/pkeys.h | 7 ++++++-
9 files changed, 34 insertions(+), 30 deletions(-)
^ permalink raw reply
* Re: [PATCH v3] powerpc, pkey: make protection key 0 less special
From: Ram Pai @ 2018-05-04 21:45 UTC (permalink / raw)
To: Dave Hansen
Cc: Michal Suchánek, fweimer, Andrew Morton, mhocko,
Ulrich.Weigand, paulus, aneesh.kumar, bauerman, Thomas Gleixner,
linuxppc-dev, Ingo Molnar
In-Reply-To: <edd3d544-9fa8-5a79-4325-818ee5d7dfb0@intel.com>
On Fri, May 04, 2018 at 02:31:10PM -0700, Dave Hansen wrote:
> On 05/04/2018 02:26 PM, Michal Suchánek wrote:
> > If it is not ok to change permissions of pkey 0 is it ok to free it?
>
> It's pretty much never OK to free it on x86 or ppc. But, we're not
> going to put code in to keep userspace from shooting itself in the foot,
> at least on x86.
and on powerpc aswell.
--
Ram Pai
^ permalink raw reply
* Re: [PATCH v3] powerpc, pkey: make protection key 0 less special
From: Dave Hansen @ 2018-05-04 21:31 UTC (permalink / raw)
To: Michal Suchánek, Ram Pai
Cc: mpe, Ulrich.Weigand, bsingharora, benh, mhocko, Ingo Molnar,
Thomas Gleixner, Andrew Morton, aneesh.kumar, bauerman,
linuxppc-dev, fweimer, paulus, hbabu
In-Reply-To: <20180504232647.3412f563@naga.suse.cz>
On 05/04/2018 02:26 PM, Michal Suchánek wrote:
> If it is not ok to change permissions of pkey 0 is it ok to free it?
It's pretty much never OK to free it on x86 or ppc. But, we're not
going to put code in to keep userspace from shooting itself in the foot,
at least on x86.
^ permalink raw reply
* Re: [PATCH v3] powerpc, pkey: make protection key 0 less special
From: Michal Suchánek @ 2018-05-04 21:26 UTC (permalink / raw)
To: Ram Pai
Cc: mpe, Ulrich.Weigand, bsingharora, Dave Hansen, benh, mhocko,
Ingo Molnar, Thomas Gleixner, Andrew Morton, aneesh.kumar,
bauerman, linuxppc-dev, fweimer, paulus, hbabu
In-Reply-To: <1525461778-26265-1-git-send-email-linuxram@us.ibm.com>
On Fri, 4 May 2018 12:22:58 -0700
"Ram Pai" <linuxram@us.ibm.com> wrote:
> Applications need the ability to associate an address-range with some
> key and latter revert to its initial default key. Pkey-0 comes close
> to providing this function but falls short, because the current
> implementation disallows applications to explicitly associate pkey-0
> to the address range.
>
> Lets make pkey-0 less special and treat it almost like any other key.
> Thus it can be explicitly associated with any address range, and can
> be freed. This gives the application more flexibility and power. The
> ability to free pkey-0 must be used responsibily, since pkey-0 is
> associated with almost all address-range by default.
>
> Even with this change pkey-0 continues to be slightly more special
> from the following point of view.
> (a) it is implicitly allocated.
> (b) it is the default key assigned to any address-range.
> (c) its permissions cannot be modified by userspace.
>
> NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
> with all pages including kernel pages, and pkeys are also active in
> kernel mode. If any permission is denied on pkey-0, the kernel running
> in the context of the application will be unable to operate.
If it is not ok to change permissions of pkey 0 is it ok to free it?
Thanks
Michal
>
> Tested on powerpc.
>
> cc: Thomas Gleixner <tglx@linutronix.de>
> cc: Dave Hansen <dave.hansen@intel.com>
> cc: Michael Ellermen <mpe@ellerman.id.au>
> cc: Ingo Molnar <mingo@kernel.org>
> cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> History:
>
> v3: . Corrected a comment in arch_set_user_pkey_access().
> . Clarified the header, to capture the notion that
> pkey-0 permissions cannot be modified by userspace on
> powerpc. -- comment from Thiago
>
> v2: . mm_pkey_is_allocated() continued to treat pkey-0
> special. fixed it.
>
> arch/powerpc/include/asm/pkeys.h | 22 ++++++++++++++++++----
> arch/powerpc/mm/pkeys.c | 26 +++++++++++++++-----------
> 2 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h
> b/arch/powerpc/include/asm/pkeys.h index 0409c80..31a6976 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
>
> static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int
> pkey) {
> - /* A reserved key is never considered as 'explicitly
> allocated' */
> - return ((pkey < arch_max_pkey()) &&
> - !__mm_pkey_is_reserved(pkey) &&
> - __mm_pkey_is_allocated(mm, pkey));
> + if (pkey < 0 || pkey >= arch_max_pkey())
> + return false;
> +
> + /* Reserved keys are never allocated. */
> + if (__mm_pkey_is_reserved(pkey))
> + return false;
> +
> + return __mm_pkey_is_allocated(mm, pkey);
> }
>
> extern void __arch_activate_pkey(int pkey);
> @@ -200,6 +204,16 @@ static inline int
> arch_set_user_pkey_access(struct task_struct *tsk, int pkey, {
> if (static_branch_likely(&pkey_disabled))
> return -EINVAL;
> +
> + /*
> + * userspace should not change pkey-0 permissions.
> + * pkey-0 is associated with every page in the kernel.
> + * If userspace denies any permission on pkey-0, the
> + * kernel cannot operate.
> + */
> + if (!pkey)
> + return init_val ? -EINVAL : 0;
> +
> return __arch_set_user_pkey_access(tsk, pkey, init_val);
> }
>
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 0eafdf0..d6873b4 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -119,16 +119,21 @@ int pkey_initialize(void)
> #else
> os_reserved = 0;
> #endif
> + /* Bits are in LE format. */
> initial_allocation_mask = ~0x0;
> +
> + /* register mask is in BE format */
> pkey_amr_uamor_mask = ~0x0ul;
> pkey_iamr_mask = ~0x0ul;
> - /*
> - * key 0, 1 are reserved.
> - * key 0 is the default key, which allows read/write/execute.
> - * key 1 is recommended not to be used. PowerISA(3.0) page
> 1015,
> - * programming note.
> - */
> - for (i = 2; i < (pkeys_total - os_reserved); i++) {
> +
> + for (i = 0; i < (pkeys_total - os_reserved); i++) {
> + /*
> + * key 1 is recommended not to be used.
> + * PowerISA(3.0) page 1015,
> + */
> + if (i == 1)
> + continue;
> +
> initial_allocation_mask &= ~(0x1 << i);
> pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
> pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
> @@ -142,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
> {
> if (static_branch_likely(&pkey_disabled))
> return;
> - mm_pkey_allocation_map(mm) = initial_allocation_mask;
> +
> + /* allocate key-0 by default */
> + mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
> /* -1 means unallocated or invalid */
> mm->context.execute_only_pkey = -1;
> }
> @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool
> write, bool execute) int pkey_shift;
> u64 amr;
>
> - if (!pkey)
> - return true;
> -
> if (!is_pkey_enabled(pkey))
> return true;
>
^ permalink raw reply
* Re: [PATCH v3] powerpc, pkey: make protection key 0 less special
From: Ram Pai @ 2018-05-04 20:21 UTC (permalink / raw)
To: Dave Hansen
Cc: mpe, linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, fweimer, msuchanek, Ulrich.Weigand,
Thomas Gleixner, Ingo Molnar, Andrew Morton
In-Reply-To: <373dffe1-4752-5eb0-f97c-a06fe0e0fdb5@intel.com>
On Fri, May 04, 2018 at 12:59:27PM -0700, Dave Hansen wrote:
> On 05/04/2018 12:22 PM, Ram Pai wrote:
> > @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
> > int pkey_shift;
> > u64 amr;
> >
> > - if (!pkey)
> > - return true;
> > -
> > if (!is_pkey_enabled(pkey))
> > return true;
>
> Looks fine to me. Obviously doesn't have any impact on x86 or the
> generic code.
>
> One question, though. Which other check makes up for this removed !pkey
> check?
is_pkey_enabled() does take care of it. we do not enable userspace to
change permissions on pkey-0. This information is tracked in
UAMOR register. is_pkey_enabled() refers to UAMOR to determine
if the given key is modifiable by userspace. since UAMOR has the bit
corresponding to key-0 set to 0, is_pkey_enabled(key-0) will return
false.
The deleted code above, would have done the same job without
referring UAMOR. However having special checks on pkey-0 makes
pkey-0 special. It defeats the purpose of this patch; which is to make
pkey-0 less special :).
--
Ram Pai
^ permalink raw reply
* [PATCH ] powerpc/pkeys: Detach execute_only key on !PROT_EXEC
From: Ram Pai @ 2018-05-04 20:01 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, linuxram, msuchanek, Ulrich.Weigand,
dave.hansen, Shakeel Butt
Disassociate the exec_key from a VMA if the VMA permission is not
PROT_EXEC anymore. Otherwise the exec_only key continues to be
associated with the vma, causing unexpected behavior.
The problem was reported on x86 by Shakeel Butt,
which is also applicable on powerpc.
cc: Shakeel Butt <shakeelb@google.com>
Reported-by: Shakeel Butt <shakeelb@google.com>
Fixes 5586cf6 ("powerpc: introduce execute-only pkey")
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/mm/pkeys.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index e81d59e..fdeb9f5 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -425,9 +425,9 @@ int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot,
{
/*
* If the currently associated pkey is execute-only, but the requested
- * protection requires read or write, move it back to the default pkey.
+ * protection is not execute-only, move it back to the default pkey.
*/
- if (vma_is_pkey_exec_only(vma) && (prot & (PROT_READ | PROT_WRITE)))
+ if (vma_is_pkey_exec_only(vma) && (prot != PROT_EXEC))
return 0;
/*
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v3] powerpc, pkey: make protection key 0 less special
From: Dave Hansen @ 2018-05-04 19:59 UTC (permalink / raw)
To: Ram Pai, mpe
Cc: linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, fweimer, msuchanek, Ulrich.Weigand,
Thomas Gleixner, Ingo Molnar, Andrew Morton
In-Reply-To: <1525461778-26265-1-git-send-email-linuxram@us.ibm.com>
On 05/04/2018 12:22 PM, Ram Pai wrote:
> @@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
> int pkey_shift;
> u64 amr;
>
> - if (!pkey)
> - return true;
> -
> if (!is_pkey_enabled(pkey))
> return true;
Looks fine to me. Obviously doesn't have any impact on x86 or the
generic code.
One question, though. Which other check makes up for this removed !pkey
check?
^ permalink raw reply
* [PATCH v2] powerpc: do not allow userspace to modify execute-only pkey
From: Ram Pai @ 2018-05-04 19:56 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, linuxram, msuchanek, Ulrich.Weigand
When mprotect(....,PROT_EXEC) is called, the kernel allocates a
execute-only pkey and associates the pkey with the given address space.
The permission of this key should not be modifiable from userspace.
However a bug in the current implementation lets the permissions on the
key modifiable from userspace.
Whenever a key is allocated through mm_pkey_alloc(), the kernel programs
the UAMOR register to allow userspace to change permissions on the key.
This is fine for keys explicitly allocated through the
sys_pkey_alloc(). But for execute-only pkey, it must be disallowed.
Restructured the code to fix the bug.
cc: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
History:
v2: Thiago noticed a bug -- __execute_only_pkey() will always fail
since it calls is_pkey_enabled() which always returns false
for execute_only key. is_pkey_enabled() returns false
because UAMOR bit for the execute_only key is and never be set.
Fixed it.
arch/powerpc/include/asm/pkeys.h | 24 ++++------------
arch/powerpc/mm/pkeys.c | 57 ++++++++++++++++++++++++++++++-------
2 files changed, 52 insertions(+), 29 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 31a6976..3a9b82b 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -113,6 +113,8 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
extern void __arch_activate_pkey(int pkey);
extern void __arch_deactivate_pkey(int pkey);
+extern int __mm_pkey_alloc(struct mm_struct *mm);
+
/*
* Returns a positive, 5-bit key on success, or -1 on failure.
* Relies on the mmap_sem to protect against concurrency in mm_pkey_alloc() and
@@ -120,29 +122,14 @@ static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
*/
static inline int mm_pkey_alloc(struct mm_struct *mm)
{
- /*
- * Note: this is the one and only place we make sure that the pkey is
- * valid as far as the hardware is concerned. The rest of the kernel
- * trusts that only good, valid pkeys come out of here.
- */
- u32 all_pkeys_mask = (u32)(~(0x0));
int ret;
if (static_branch_likely(&pkey_disabled))
return -1;
+ ret = __mm_pkey_alloc(mm);
/*
- * Are we out of pkeys? We must handle this specially because ffz()
- * behavior is undefined if there are no zeros.
- */
- if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
- return -1;
-
- ret = ffz((u32)mm_pkey_allocation_map(mm));
- __mm_pkey_allocated(mm, ret);
-
- /*
- * Enable the key in the hardware
+ * Enable userspace to modify the key permissions.
*/
if (ret > 0)
__arch_activate_pkey(ret);
@@ -158,7 +145,8 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
return -EINVAL;
/*
- * Disable the key in the hardware
+ * Reset the key and disable userspace
+ * from modifying the key permissions.
*/
__arch_deactivate_pkey(pkey);
__mm_pkey_free(mm, pkey);
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index d6873b4..e81d59e 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -190,6 +190,9 @@ static inline void write_uamor(u64 value)
mtspr(SPRN_UAMOR, value);
}
+/*
+ * return true if userspace can modify the pkey permissions.
+ */
static bool is_pkey_enabled(int pkey)
{
u64 uamor = read_uamor();
@@ -228,7 +231,10 @@ static void pkey_status_change(int pkey, bool enable)
init_amr(pkey, 0x0);
init_iamr(pkey, 0x0);
- /* Enable/disable key */
+ /*
+ * Enable/disable userspace to/from modifying the permissions
+ * on the key
+ */
old_uamor = read_uamor();
if (enable)
old_uamor |= (0x3ul << pkeyshift(pkey));
@@ -247,19 +253,35 @@ void __arch_deactivate_pkey(int pkey)
pkey_status_change(pkey, false);
}
-/*
- * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
- * specified in @init_val.
- */
-int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+int __mm_pkey_alloc(struct mm_struct *mm)
+{
+ /*
+ * Note: this is the one and only place we make sure that the pkey is
+ * valid as far as the hardware is concerned. The rest of the kernel
+ * trusts that only good, valid pkeys come out of here.
+ */
+ u32 all_pkeys_mask = (u32)(~(0x0));
+ int ret;
+
+ /*
+ * Are we out of pkeys? We must handle this specially because ffz()
+ * behavior is undefined if there are no zeros.
+ */
+ if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
+ return -1;
+
+ ret = ffz((u32)mm_pkey_allocation_map(mm));
+ __mm_pkey_allocated(mm, ret);
+
+ return ret;
+}
+
+static int set_user_pkey_access(struct task_struct *tsk, int pkey,
unsigned long init_val)
{
u64 new_amr_bits = 0x0ul;
u64 new_iamr_bits = 0x0ul;
- if (!is_pkey_enabled(pkey))
- return -EINVAL;
-
if (init_val & PKEY_DISABLE_EXECUTE) {
if (!pkey_execute_disable_supported)
return -EINVAL;
@@ -277,6 +299,19 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
return 0;
}
+/*
+ * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
+ * specified in @init_val.
+ */
+int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+ unsigned long init_val)
+{
+ if (!is_pkey_enabled(pkey))
+ return -EINVAL;
+
+ return set_user_pkey_access(tsk, pkey, init_val);
+}
+
void thread_pkey_regs_save(struct thread_struct *thread)
{
if (static_branch_likely(&pkey_disabled))
@@ -336,7 +371,7 @@ int __execute_only_pkey(struct mm_struct *mm)
/* Do we need to assign a pkey for mm's execute-only maps? */
if (execute_only_pkey == -1) {
/* Go allocate one to use, which might fail */
- execute_only_pkey = mm_pkey_alloc(mm);
+ execute_only_pkey = __mm_pkey_alloc(mm);
if (execute_only_pkey < 0)
return -1;
need_to_set_mm_pkey = true;
@@ -355,7 +390,7 @@ int __execute_only_pkey(struct mm_struct *mm)
* Set up AMR so that it denies access for everything other than
* execution.
*/
- ret = __arch_set_user_pkey_access(current, execute_only_pkey,
+ ret = set_user_pkey_access(current, execute_only_pkey,
PKEY_DISABLE_ACCESS |
PKEY_DISABLE_WRITE);
/*
--
1.7.1
^ permalink raw reply related
* [PATCH v3] powerpc, pkey: make protection key 0 less special
From: Ram Pai @ 2018-05-04 19:22 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, benh, paulus, aneesh.kumar, bsingharora, hbabu,
mhocko, bauerman, linuxram, fweimer, msuchanek, Ulrich.Weigand,
Thomas Gleixner, Dave Hansen, Ingo Molnar, Andrew Morton
Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.
Lets make pkey-0 less special and treat it almost like any other key.
Thus it can be explicitly associated with any address range, and can be
freed. This gives the application more flexibility and power. The
ability to free pkey-0 must be used responsibily, since pkey-0 is
associated with almost all address-range by default.
Even with this change pkey-0 continues to be slightly more special
from the following point of view.
(a) it is implicitly allocated.
(b) it is the default key assigned to any address-range.
(c) its permissions cannot be modified by userspace.
NOTE: (c) is specific to powerpc only. pkey-0 is associated by default
with all pages including kernel pages, and pkeys are also active in
kernel mode. If any permission is denied on pkey-0, the kernel running
in the context of the application will be unable to operate.
Tested on powerpc.
cc: Thomas Gleixner <tglx@linutronix.de>
cc: Dave Hansen <dave.hansen@intel.com>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Ingo Molnar <mingo@kernel.org>
cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
History:
v3: . Corrected a comment in arch_set_user_pkey_access().
. Clarified the header, to capture the notion that
pkey-0 permissions cannot be modified by userspace on powerpc.
-- comment from Thiago
v2: . mm_pkey_is_allocated() continued to treat pkey-0 special.
fixed it.
arch/powerpc/include/asm/pkeys.h | 22 ++++++++++++++++++----
arch/powerpc/mm/pkeys.c | 26 +++++++++++++++-----------
2 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 0409c80..31a6976 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -101,10 +101,14 @@ static inline u16 pte_to_pkey_bits(u64 pteflags)
static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
{
- /* A reserved key is never considered as 'explicitly allocated' */
- return ((pkey < arch_max_pkey()) &&
- !__mm_pkey_is_reserved(pkey) &&
- __mm_pkey_is_allocated(mm, pkey));
+ if (pkey < 0 || pkey >= arch_max_pkey())
+ return false;
+
+ /* Reserved keys are never allocated. */
+ if (__mm_pkey_is_reserved(pkey))
+ return false;
+
+ return __mm_pkey_is_allocated(mm, pkey);
}
extern void __arch_activate_pkey(int pkey);
@@ -200,6 +204,16 @@ static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
{
if (static_branch_likely(&pkey_disabled))
return -EINVAL;
+
+ /*
+ * userspace should not change pkey-0 permissions.
+ * pkey-0 is associated with every page in the kernel.
+ * If userspace denies any permission on pkey-0, the
+ * kernel cannot operate.
+ */
+ if (!pkey)
+ return init_val ? -EINVAL : 0;
+
return __arch_set_user_pkey_access(tsk, pkey, init_val);
}
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 0eafdf0..d6873b4 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -119,16 +119,21 @@ int pkey_initialize(void)
#else
os_reserved = 0;
#endif
+ /* Bits are in LE format. */
initial_allocation_mask = ~0x0;
+
+ /* register mask is in BE format */
pkey_amr_uamor_mask = ~0x0ul;
pkey_iamr_mask = ~0x0ul;
- /*
- * key 0, 1 are reserved.
- * key 0 is the default key, which allows read/write/execute.
- * key 1 is recommended not to be used. PowerISA(3.0) page 1015,
- * programming note.
- */
- for (i = 2; i < (pkeys_total - os_reserved); i++) {
+
+ for (i = 0; i < (pkeys_total - os_reserved); i++) {
+ /*
+ * key 1 is recommended not to be used.
+ * PowerISA(3.0) page 1015,
+ */
+ if (i == 1)
+ continue;
+
initial_allocation_mask &= ~(0x1 << i);
pkey_amr_uamor_mask &= ~(0x3ul << pkeyshift(i));
pkey_iamr_mask &= ~(0x1ul << pkeyshift(i));
@@ -142,7 +147,9 @@ void pkey_mm_init(struct mm_struct *mm)
{
if (static_branch_likely(&pkey_disabled))
return;
- mm_pkey_allocation_map(mm) = initial_allocation_mask;
+
+ /* allocate key-0 by default */
+ mm_pkey_allocation_map(mm) = initial_allocation_mask | 0x1;
/* -1 means unallocated or invalid */
mm->context.execute_only_pkey = -1;
}
@@ -407,9 +414,6 @@ static bool pkey_access_permitted(int pkey, bool write, bool execute)
int pkey_shift;
u64 amr;
- if (!pkey)
- return true;
-
if (!is_pkey_enabled(pkey))
return true;
--
1.7.1
^ permalink raw reply related
* [PATCH 2/2] powerpc/ptrace: Disable array-bounds warning with gcc8
From: Khem Raj @ 2018-05-04 19:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Khem Raj, Benjamin Herrenschmidt
In-Reply-To: <20180504192313.18625-1-raj.khem@gmail.com>
This masks the new gcc8 warning
regset.h:270:4: error: 'memcpy' offset [-527, -529] is out
of the bounds [0, 16] of object 'vrsave' with type 'union <anonymous>'
Signed-off-by: Khem Raj <raj.khem@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/kernel/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 7ac5a68ad6b1..ab159a34704a 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -4,6 +4,7 @@
#
CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"' $(call cc-disable-warning, attribute-alias)
+CFLAGS_ptrace.o += $(call cc-disable-warning, array-bounds)
CFLAGS_syscalls.o += $(call cc-disable-warning, attribute-alias)
subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
--
2.17.0
^ permalink raw reply related
* [PATCH 1/2] powerpc: Disable attribute-alias warnings from gcc8
From: Khem Raj @ 2018-05-04 19:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Khem Raj, Benjamin Herrenschmidt
Fixes
alias between functions of incompatible types warnings
which are new with gcc8
Signed-off-by: Khem Raj <raj.khem@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/kernel/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 2b4c40b255e4..7ac5a68ad6b1 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -3,7 +3,8 @@
# Makefile for the linux kernel.
#
-CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"'
+CFLAGS_ptrace.o += -DUTS_MACHINE='"$(UTS_MACHINE)"' $(call cc-disable-warning, attribute-alias)
+CFLAGS_syscalls.o += $(call cc-disable-warning, attribute-alias)
subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
--
2.17.0
^ permalink raw reply related
* [PATCH 11/11] powerpc/time: account broadcast timer event interrupts separately
From: Nicholas Piggin @ 2018-05-04 17:19 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Benjamin Herrenschmidt
In-Reply-To: <20180504171935.25410-1-npiggin@gmail.com>
These are not local timer interrupts but IPIs. It's good to be able
to see how timer offloading is behaving, so split these out into
their own category.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hardirq.h | 1 +
arch/powerpc/kernel/irq.c | 6 ++++++
arch/powerpc/kernel/time.c | 5 +----
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/hardirq.h b/arch/powerpc/include/asm/hardirq.h
index 5986d473722b..20b01897ea5d 100644
--- a/arch/powerpc/include/asm/hardirq.h
+++ b/arch/powerpc/include/asm/hardirq.h
@@ -8,6 +8,7 @@
typedef struct {
unsigned int __softirq_pending;
unsigned int timer_irqs_event;
+ unsigned int broadcast_irqs_event;
unsigned int timer_irqs_others;
unsigned int pmu_irqs;
unsigned int mce_exceptions;
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 6569b5ffff93..627db34bb79d 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -518,6 +518,11 @@ int arch_show_interrupts(struct seq_file *p, int prec)
seq_printf(p, "%10u ", per_cpu(irq_stat, j).timer_irqs_event);
seq_printf(p, " Local timer interrupts for timer event device\n");
+ seq_printf(p, "%*s: ", prec, "BCT");
+ for_each_online_cpu(j)
+ seq_printf(p, "%10u ", per_cpu(irq_stat, j).broadcast_irqs_event);
+ seq_printf(p, " Broadcast timer interrupts for timer event device\n");
+
seq_printf(p, "%*s: ", prec, "LOC");
for_each_online_cpu(j)
seq_printf(p, "%10u ", per_cpu(irq_stat, j).timer_irqs_others);
@@ -577,6 +582,7 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
{
u64 sum = per_cpu(irq_stat, cpu).timer_irqs_event;
+ sum += per_cpu(irq_stat, cpu).broadcast_irqs_event;
sum += per_cpu(irq_stat, cpu).pmu_irqs;
sum += per_cpu(irq_stat, cpu).mce_exceptions;
sum += per_cpu(irq_stat, cpu).spurious_irqs;
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 23921f7b6e67..ed6b2abdde15 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -647,13 +647,10 @@ EXPORT_SYMBOL(timer_interrupt);
void timer_broadcast_interrupt(void)
{
u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
- struct pt_regs *regs = get_irq_regs();
- trace_timer_interrupt_entry(regs);
*next_tb = ~(u64)0;
tick_receive_broadcast();
- __this_cpu_inc(irq_stat.timer_irqs_event);
- trace_timer_interrupt_exit(regs);
+ __this_cpu_inc(irq_stat.broadcast_irqs_event);
}
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH 10/11] powerpc: move a stray NMI IPI case under NMI_IPI ifdef
From: Nicholas Piggin @ 2018-05-04 17:19 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Benjamin Herrenschmidt
In-Reply-To: <20180504171935.25410-1-npiggin@gmail.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/smp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 914708eeb43f..28ec1638a540 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -193,7 +193,9 @@ const char *smp_ipi_name[] = {
#ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
[PPC_MSG_TICK_BROADCAST] = "ipi tick-broadcast",
#endif
+#ifdef CONFIG_NMI_IPI
[PPC_MSG_NMI_IPI] = "nmi ipi",
+#endif
};
/* optional function to request ipi, for controllers with >= 4 ipis */
--
2.17.0
^ permalink raw reply related
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