* [PATCH v7 6/7] powerpc/mm: implement set_memory_attr()
From: Russell Currey @ 2020-03-31 4:48 UTC (permalink / raw)
To: linuxppc-dev
Cc: ajd, kernel-hardening, npiggin, kbuild test robot, Russell Currey,
dja
In-Reply-To: <20200331044825.591653-1-ruscur@russell.cc>
From: Christophe Leroy <christophe.leroy@c-s.fr>
In addition to the set_memory_xx() functions which allows to change
the memory attributes of not (yet) used memory regions, implement a
set_memory_attr() function to:
- set the final memory protection after init on currently used
kernel regions.
- enable/disable kernel memory regions in the scope of DEBUG_PAGEALLOC.
Unlike the set_memory_xx() which can act in three step as the regions
are unused, this function must modify 'on the fly' as the kernel is
executing from them. At the moment only PPC32 will use it and changing
page attributes on the fly is not an issue.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reported-by: kbuild test robot <lkp@intel.com>
[ruscur: cast "data" to unsigned long instead of int]
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v7: Use apply_to_existing_page_range() and check for negative numpages
arch/powerpc/include/asm/set_memory.h | 2 ++
arch/powerpc/mm/pageattr.c | 33 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
index 64011ea444b4..b040094f7920 100644
--- a/arch/powerpc/include/asm/set_memory.h
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -29,4 +29,6 @@ static inline int set_memory_x(unsigned long addr, int numpages)
return change_memory_attr(addr, numpages, SET_MEMORY_X);
}
+int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot);
+
#endif
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index 2da3fbab6ff7..2fde1b195c85 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -79,3 +79,36 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
return apply_to_existing_page_range(&init_mm, start, sz,
change_page_attr, (void *)action);
}
+
+/*
+ * Set the attributes of a page:
+ *
+ * This function is used by PPC32 at the end of init to set final kernel memory
+ * protection. It includes changing the maping of the page it is executing from
+ * and data pages it is using.
+ */
+static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ pgprot_t prot = __pgprot((unsigned long)data);
+
+ spin_lock(&init_mm.page_table_lock);
+
+ set_pte_at(&init_mm, addr, ptep, pte_modify(*ptep, prot));
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+
+ spin_unlock(&init_mm.page_table_lock);
+
+ return 0;
+}
+
+int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long sz = numpages * PAGE_SIZE;
+
+ if (numpages <= 0)
+ return 0;
+
+ return apply_to_existing_page_range(&init_mm, start, sz, set_page_attr,
+ (void *)pgprot_val(prot));
+}
--
2.26.0
^ permalink raw reply related
* [PATCH v7 5/7] powerpc/configs: Enable STRICT_MODULE_RWX in skiroot_defconfig
From: Russell Currey @ 2020-03-31 4:48 UTC (permalink / raw)
To: linuxppc-dev
Cc: ajd, kernel-hardening, Joel Stanley, npiggin, Russell Currey, dja
In-Reply-To: <20200331044825.591653-1-ruscur@russell.cc>
skiroot_defconfig is the only powerpc defconfig with STRICT_KERNEL_RWX
enabled, and if you want memory protection for kernel text you'd want it
for modules too, so enable STRICT_MODULE_RWX there.
Acked-by: Joel Stanley <joel@joel.id.au>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/configs/skiroot_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/configs/skiroot_defconfig b/arch/powerpc/configs/skiroot_defconfig
index 1b6bdad36b13..66d20dbe67b7 100644
--- a/arch/powerpc/configs/skiroot_defconfig
+++ b/arch/powerpc/configs/skiroot_defconfig
@@ -51,6 +51,7 @@ CONFIG_CMDLINE="console=tty0 console=hvc0 ipr.fast_reboot=1 quiet"
# CONFIG_PPC_MEM_KEYS is not set
CONFIG_JUMP_LABEL=y
CONFIG_STRICT_KERNEL_RWX=y
+CONFIG_STRICT_MODULE_RWX=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_MODULE_SIG_FORCE=y
--
2.26.0
^ permalink raw reply related
* [PATCH v7 4/7] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
From: Russell Currey @ 2020-03-31 4:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, Russell Currey, dja
In-Reply-To: <20200331044825.591653-1-ruscur@russell.cc>
To enable strict module RWX on powerpc, set:
CONFIG_STRICT_MODULE_RWX=y
You should also have CONFIG_STRICT_KERNEL_RWX=y set to have any real
security benefit.
ARCH_HAS_STRICT_MODULE_RWX is set to require ARCH_HAS_STRICT_KERNEL_RWX.
This is due to a quirk in arch/Kconfig and arch/powerpc/Kconfig that
makes STRICT_MODULE_RWX *on by default* in configurations where
STRICT_KERNEL_RWX is *unavailable*.
Since this doesn't make much sense, and module RWX without kernel RWX
doesn't make much sense, having the same dependencies as kernel RWX
works around this problem.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index bd074246e34e..e1fc7fba10bf 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -131,6 +131,7 @@ config PPC
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
+ select ARCH_HAS_STRICT_MODULE_RWX if ARCH_HAS_STRICT_KERNEL_RWX
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
select ARCH_HAS_UACCESS_MCSAFE if PPC64
--
2.26.0
^ permalink raw reply related
* [PATCH v7 3/7] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
From: Russell Currey @ 2020-03-31 4:48 UTC (permalink / raw)
To: linuxppc-dev
Cc: ajd, Kees Cook, kernel-hardening, npiggin, Russell Currey, dja
In-Reply-To: <20200331044825.591653-1-ruscur@russell.cc>
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 0b063830eea8..e37960ef68c6 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -370,7 +370,7 @@ config PPC_PTDUMP
If you are unsure, say N.
config PPC_DEBUG_WX
- bool "Warn on W+X mappings at boot"
+ bool "Warn on W+X mappings at boot & enable manual checks at runtime"
depends on PPC_PTDUMP && STRICT_KERNEL_RWX
help
Generate a warning if any W+X mappings are found at boot.
@@ -384,7 +384,9 @@ config PPC_DEBUG_WX
of other unfixed kernel bugs easier.
There is no runtime or memory usage effect of this option
- once the kernel has booted up - it's a one time check.
+ once the kernel has booted up, it only automatically checks once.
+
+ Enables the "check_wx_pages" debugfs entry for checking at runtime.
If in doubt, say "Y".
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index 206156255247..a15e19a3b14e 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -4,7 +4,7 @@
*
* This traverses the kernel pagetables and dumps the
* information about the used sections of memory to
- * /sys/kernel/debug/kernel_pagetables.
+ * /sys/kernel/debug/kernel_page_tables.
*
* Derived from the arm64 implementation:
* Copyright (c) 2014, The Linux Foundation, Laura Abbott.
@@ -413,6 +413,25 @@ void ptdump_check_wx(void)
else
pr_info("Checked W+X mappings: passed, no W+X pages found\n");
}
+
+static int check_wx_debugfs_set(void *data, u64 val)
+{
+ if (val != 1ULL)
+ return -EINVAL;
+
+ ptdump_check_wx();
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n");
+
+static int ptdump_check_wx_init(void)
+{
+ return debugfs_create_file("check_wx_pages", 0200, NULL,
+ NULL, &check_wx_fops) ? 0 : -ENOMEM;
+}
+device_initcall(ptdump_check_wx_init);
#endif
static int ptdump_init(void)
--
2.26.0
^ permalink raw reply related
* [PATCH v7 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Russell Currey @ 2020-03-31 4:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, Russell Currey, dja
In-Reply-To: <20200331044825.591653-1-ruscur@russell.cc>
With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one
W+X page at boot by default. This can be tested with
CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
kernel log during boot.
powerpc doesn't implement its own alloc() for kprobes like other
architectures do, but we couldn't immediately mark RO anyway since we do
a memcpy to the page we allocate later. After that, nothing should be
allowed to modify the page, and write permissions are removed well
before the kprobe is armed.
The memcpy() would fail if >1 probes were allocated, so use
patch_instruction() instead which is safe for RO.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 2d27ec4feee4..bfab91ded234 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -24,6 +24,8 @@
#include <asm/sstep.h>
#include <asm/sections.h>
#include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/vmalloc.h>
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
return addr;
}
+void *alloc_insn_page(void)
+{
+ void *page = vmalloc_exec(PAGE_SIZE);
+
+ if (page)
+ set_memory_ro((unsigned long)page, 1);
+
+ return page;
+}
+
int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
@@ -124,11 +136,8 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- memcpy(p->ainsn.insn, p->addr,
- MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+ patch_instruction(p->ainsn.insn, *p->addr);
p->opcode = *p->addr;
- flush_icache_range((unsigned long)p->ainsn.insn,
- (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
}
p->ainsn.boostable = 0;
--
2.26.0
^ permalink raw reply related
* [PATCH v7 1/7] powerpc/mm: Implement set_memory() routines
From: Russell Currey @ 2020-03-31 4:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, Russell Currey, dja
In-Reply-To: <20200331044825.591653-1-ruscur@russell.cc>
The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
and are generally useful primitives to have. This implementation is
designed to be completely generic across powerpc's many MMUs.
It's possible that this could be optimised to be faster for specific
MMUs, but the focus is on having a generic and safe implementation for
now.
This implementation does not handle cases where the caller is attempting
to change the mapping of the page it is executing from, or if another
CPU is concurrently using the page being altered. These cases likely
shouldn't happen, but a more complex implementation with MMU-specific code
could safely handle them, so that is left as a TODO for now.
These functions do nothing if STRICT_KERNEL_RWX is not enabled.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v7: Use apply_to_existing_page_range() and check for negative numpages
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 32 +++++++++++
arch/powerpc/mm/Makefile | 2 +-
arch/powerpc/mm/pageattr.c | 81 +++++++++++++++++++++++++++
4 files changed, 115 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 497b7d0b2d7e..bd074246e34e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -129,6 +129,7 @@ config PPC
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_MEMBARRIER_CALLBACKS
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
+ select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
new file mode 100644
index 000000000000..64011ea444b4
--- /dev/null
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_SET_MEMORY_H
+#define _ASM_POWERPC_SET_MEMORY_H
+
+#define SET_MEMORY_RO 0
+#define SET_MEMORY_RW 1
+#define SET_MEMORY_NX 2
+#define SET_MEMORY_X 3
+
+int change_memory_attr(unsigned long addr, int numpages, long action);
+
+static inline int set_memory_ro(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RO);
+}
+
+static inline int set_memory_rw(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RW);
+}
+
+static inline int set_memory_nx(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_NX);
+}
+
+static inline int set_memory_x(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_X);
+}
+
+#endif
diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
index 5e147986400d..a998fdac52f9 100644
--- a/arch/powerpc/mm/Makefile
+++ b/arch/powerpc/mm/Makefile
@@ -5,7 +5,7 @@
ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
-obj-y := fault.o mem.o pgtable.o mmap.o \
+obj-y := fault.o mem.o pgtable.o mmap.o pageattr.o \
init_$(BITS).o pgtable_$(BITS).o \
pgtable-frag.o ioremap.o ioremap_$(BITS).o \
init-common.o mmu_context.o drmem.o
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
new file mode 100644
index 000000000000..2da3fbab6ff7
--- /dev/null
+++ b/arch/powerpc/mm/pageattr.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * MMU-generic set_memory implementation for powerpc
+ *
+ * Copyright 2019, IBM Corporation.
+ */
+
+#include <linux/mm.h>
+#include <linux/set_memory.h>
+
+#include <asm/mmu.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
+
+
+/*
+ * Updates the attributes of a page in three steps:
+ *
+ * 1. invalidate the page table entry
+ * 2. flush the TLB
+ * 3. install the new entry with the updated attributes
+ *
+ * This is unsafe if the caller is attempting to change the mapping of the
+ * page it is executing from, or if another CPU is concurrently using the
+ * page being altered.
+ *
+ * TODO make the implementation resistant to this.
+ *
+ * NOTE: can be dangerous to call without STRICT_KERNEL_RWX
+ */
+static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ long action = (long)data;
+ pte_t pte;
+
+ spin_lock(&init_mm.page_table_lock);
+
+ /* invalidate the PTE so it's safe to modify */
+ pte = ptep_get_and_clear(&init_mm, addr, ptep);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+
+ /* modify the PTE bits as desired, then apply */
+ switch (action) {
+ case SET_MEMORY_RO:
+ pte = pte_wrprotect(pte);
+ break;
+ case SET_MEMORY_RW:
+ pte = pte_mkwrite(pte);
+ break;
+ case SET_MEMORY_NX:
+ pte = pte_exprotect(pte);
+ break;
+ case SET_MEMORY_X:
+ pte = pte_mkexec(pte);
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ break;
+ }
+
+ set_pte_at(&init_mm, addr, ptep, pte);
+ spin_unlock(&init_mm.page_table_lock);
+
+ return 0;
+}
+
+int change_memory_attr(unsigned long addr, int numpages, long action)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long sz = numpages * PAGE_SIZE;
+
+ if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
+ return 0;
+
+ if (numpages <= 0)
+ return 0;
+
+ return apply_to_existing_page_range(&init_mm, start, sz,
+ change_page_attr, (void *)action);
+}
--
2.26.0
^ permalink raw reply related
* [PATCH v2 1/1] vfio-pci/nvlink2: Allow fallback to ibm,mmio-atsd[0]
From: Sam Bobroff @ 2020-03-31 4:12 UTC (permalink / raw)
To: kvm, linuxppc-dev; +Cc: aik
Older versions of skiboot only provide a single value in the device
tree property "ibm,mmio-atsd", even when multiple Address Translation
Shoot Down (ATSD) registers are present. This prevents NVLink2 devices
(other than the first) from being used with vfio-pci because vfio-pci
expects to be able to assign a dedicated ATSD register to each NVLink2
device.
However, ATSD registers can be shared among devices. This change
allows vfio-pci to fall back to sharing the register at index 0 if
necessary.
Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
---
Patch set v2:
Patch 1/1: vfio-pci/nvlink2: Allow fallback to ibm,mmio-atsd[0]
- Removed unnecessary warning.
- Added Fixes tag.
Patch set v1:
Patch 1/1: vfio-pci/nvlink2: Allow fallback to ibm,mmio-atsd[0]
drivers/vfio/pci/vfio_pci_nvlink2.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_nvlink2.c b/drivers/vfio/pci/vfio_pci_nvlink2.c
index f2983f0f84be..ae2af590e501 100644
--- a/drivers/vfio/pci/vfio_pci_nvlink2.c
+++ b/drivers/vfio/pci/vfio_pci_nvlink2.c
@@ -420,8 +420,14 @@ int vfio_pci_ibm_npu2_init(struct vfio_pci_device *vdev)
if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", nvlink_index,
&mmio_atsd)) {
- dev_warn(&vdev->pdev->dev, "No available ATSD found\n");
- mmio_atsd = 0;
+ if (of_property_read_u64_index(hose->dn, "ibm,mmio-atsd", 0,
+ &mmio_atsd)) {
+ dev_warn(&vdev->pdev->dev, "No available ATSD found\n");
+ mmio_atsd = 0;
+ } else {
+ dev_warn(&vdev->pdev->dev,
+ "Using fallback ibm,mmio-atsd[0] for ATSD.\n");
+ }
}
if (of_property_read_u64(npu_node, "ibm,device-tgt-addr", &tgt)) {
--
2.22.0.216.g00a2a96fc9
^ permalink raw reply related
* Re: [RFC PATCH 2/3] powerpc/lib: Initialize a temporary mm for code patching
From: Christopher M Riedl @ 2020-03-31 3:19 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <ecccbeb2-731b-f9a3-1039-f2a662e3a9a5@c-s.fr>
> On March 24, 2020 11:10 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> > When code patching a STRICT_KERNEL_RWX kernel the page containing the
> > address to be patched is temporarily mapped with permissive memory
> > protections. Currently, a per-cpu vmalloc patch area is used for this
> > purpose. While the patch area is per-cpu, the temporary page mapping is
> > inserted into the kernel page tables for the duration of the patching.
> > The mapping is exposed to CPUs other than the patching CPU - this is
> > undesirable from a hardening perspective.
> >
> > Use the `poking_init` init hook to prepare a temporary mm and patching
> > address. Initialize the temporary mm by copying the init mm. Choose a
> > randomized patching address inside the temporary mm userspace address
> > portion. The next patch uses the temporary mm and patching address for
> > code patching.
> >
> > Based on x86 implementation:
> >
> > commit 4fc19708b165
> > ("x86/alternatives: Initialize temporary mm for patching")
> >
> > Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> > ---
> > arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index 3345f039a876..18b88ecfc5a8 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -11,6 +11,8 @@
> > #include <linux/cpuhotplug.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/sched/task.h>
> > +#include <linux/random.h>
> >
> > #include <asm/pgtable.h>
> > #include <asm/tlbflush.h>
> > @@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)
> > }
> >
> > #ifdef CONFIG_STRICT_KERNEL_RWX
> > +
> > +__ro_after_init struct mm_struct *patching_mm;
> > +__ro_after_init unsigned long patching_addr;
>
> Can we make those those static ?
>
Yes, makes sense to me.
> > +
> > +void __init poking_init(void)
> > +{
> > + spinlock_t *ptl; /* for protecting pte table */
> > + pte_t *ptep;
> > +
> > + patching_mm = copy_init_mm();
> > + BUG_ON(!patching_mm);
>
> Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
> WARN_ON ?
>
I'm not sure what failing gracefully means here? The main reason this could
fail is if there is not enough memory to allocate the patching_mm. The
previous implementation had this justification for BUG_ON():
/*
* Run as a late init call. This allows all the boot time patching to be done
* simply by patching the code, and then we're called here prior to
* mark_rodata_ro(), which happens after all init calls are run. Although
* BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
* it as being preferable to a kernel that will crash later when someone tries
* to use patch_instruction().
*/
static int __init setup_text_poke_area(void)
{
BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"powerpc/text_poke:online", text_area_cpu_up,
text_area_cpu_down));
return 0;
}
late_initcall(setup_text_poke_area);
I think the BUG_ON() is appropriate even if only to adhere to the previous
judgement call. I can add a similar comment explaining the reasoning if
that helps.
> > +
> > + /*
> > + * In hash we cannot go above DEFAULT_MAP_WINDOW easily.
> > + * XXX: Do we want additional bits of entropy for radix?
> > + */
> > + patching_addr = (get_random_long() & PAGE_MASK) %
> > + (DEFAULT_MAP_WINDOW - PAGE_SIZE);
> > +
> > + ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
> > + BUG_ON(!ptep);
>
> Same here, can we fail gracefully instead ?
>
Same reasoning as above.
> > + pte_unmap_unlock(ptep, ptl);
> > +}
> > +
> > static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> >
> > static int text_area_cpu_up(unsigned int cpu)
> >
>
> Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/44x: Make AKEBONO depends on NET
From: Yuehaibing @ 2020-03-31 3:05 UTC (permalink / raw)
To: Michael Ellerman, mporter, benh, paulus
Cc: alistair, linuxppc-dev, linux-kernel
In-Reply-To: <87pnctuyq3.fsf@mpe.ellerman.id.au>
On 2020/3/31 8:58, Michael Ellerman wrote:
> YueHaibing <yuehaibing@huawei.com> writes:
>> Fix Kconfig warnings:
>>
>> WARNING: unmet direct dependencies detected for NETDEVICES
>> Depends on [n]: NET [=n]
>> Selected by [y]:
>> - AKEBONO [=y] && PPC_47x [=y]
>>
>> WARNING: unmet direct dependencies detected for ETHERNET
>> Depends on [n]: NETDEVICES [=y] && NET [=n]
>> Selected by [y]:
>> - AKEBONO [=y] && PPC_47x [=y]
>>
>> AKEBONO select NETDEVICES and ETHERNET unconditionally,
>
> It shouldn't do that, that's the job of a defconfig.
>
> It might want to enable NET_VENDOR_IBM iff the config already has NET
> and other dependencies enabled.
>
> So the patch below might work?
Yes, It works for me, Thanks!
Tested-by: YueHaibing <yuehaibing@huawei.com> # build-tested
>
> cheers
>
> diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
> index 25ebe634a661..32aac4f40f1b 100644
> --- a/arch/powerpc/platforms/44x/Kconfig
> +++ b/arch/powerpc/platforms/44x/Kconfig
> @@ -207,9 +207,7 @@ config AKEBONO
> select PPC4xx_HSTA_MSI
> select I2C
> select I2C_IBM_IIC
> - select NETDEVICES
> - select ETHERNET
> - select NET_VENDOR_IBM
> + imply NET_VENDOR_IBM
> select IBM_EMAC_EMAC4 if IBM_EMAC
> select USB if USB_SUPPORT
> select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
>
>
>
>> If NET is not set, build fails. Add this dependcy to fix this.
>>
>> Fixes: 2a2c74b2efcb ("IBM Akebono: Add the Akebono platform")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> arch/powerpc/platforms/44x/Kconfig | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
>> index 25ebe634a661..394f662d7df2 100644
>> --- a/arch/powerpc/platforms/44x/Kconfig
>> +++ b/arch/powerpc/platforms/44x/Kconfig
>> @@ -199,6 +199,7 @@ config FSP2
>> config AKEBONO
>> bool "IBM Akebono (476gtr) Support"
>> depends on PPC_47x
>> + depends on NET
>> select SWIOTLB
>> select 476FPE
>> select PPC4xx_PCI_EXPRESS
>> --
>> 2.17.1
>
> .
>
^ permalink raw reply
* Re: [RFC PATCH 1/3] powerpc/mm: Introduce temporary mm
From: Christopher M Riedl @ 2020-03-31 2:41 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <2057834a-01d3-958a-1674-cb264029505f@c-s.fr>
> On March 24, 2020 11:07 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> > x86 supports the notion of a temporary mm which restricts access to
> > temporary PTEs to a single CPU. A temporary mm is useful for situations
> > where a CPU needs to perform sensitive operations (such as patching a
> > STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
> > said mappings to other CPUs. A side benefit is that other CPU TLBs do
> > not need to be flushed when the temporary mm is torn down.
> >
> > Mappings in the temporary mm can be set in the userspace portion of the
> > address-space.
> >
> > Interrupts must be disabled while the temporary mm is in use. HW
> > breakpoints, which may have been set by userspace as watchpoints on
> > addresses now within the temporary mm, are saved and disabled when
> > loading the temporary mm. The HW breakpoints are restored when unloading
> > the temporary mm. All HW breakpoints are indiscriminately disabled while
> > the temporary mm is in use.
> >
> > Based on x86 implementation:
> >
> > commit cefa929c034e
> > ("x86/mm: Introduce temporary mm structs")
> >
> > Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> > ---
> > arch/powerpc/include/asm/debug.h | 1 +
> > arch/powerpc/include/asm/mmu_context.h | 56 +++++++++++++++++++++++++-
> > arch/powerpc/kernel/process.c | 5 +++
> > 3 files changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/include/asm/debug.h b/arch/powerpc/include/asm/debug.h
> > index 7756026b95ca..b945bc16c932 100644
> > --- a/arch/powerpc/include/asm/debug.h
> > +++ b/arch/powerpc/include/asm/debug.h
> > @@ -45,6 +45,7 @@ static inline int debugger_break_match(struct pt_regs *regs) { return 0; }
> > static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
> > #endif
> >
> > +void __get_breakpoint(struct arch_hw_breakpoint *brk);
> > void __set_breakpoint(struct arch_hw_breakpoint *brk);
> > bool ppc_breakpoint_available(void);
> > #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> > index 360367c579de..3e6381d04c28 100644
> > --- a/arch/powerpc/include/asm/mmu_context.h
> > +++ b/arch/powerpc/include/asm/mmu_context.h
> > @@ -7,9 +7,10 @@
> > #include <linux/mm.h>
> > #include <linux/sched.h>
> > #include <linux/spinlock.h>
> > -#include <asm/mmu.h>
> > +#include <asm/mmu.h>
>
> What's this change ?
> I see you are removing a space at the end of the line, but it shouldn't
> be part of this patch.
>
Overly aggressive "helpful" editor setting apparently.
Removed this change in the next version.
> > #include <asm/cputable.h>
> > #include <asm/cputhreads.h>
> > +#include <asm/hw_breakpoint.h>
> >
> > /*
> > * Most if the context management is out of line
> > @@ -270,5 +271,58 @@ static inline int arch_dup_mmap(struct mm_struct *oldmm,
> > return 0;
> > }
> >
> > +struct temp_mm {
> > + struct mm_struct *temp;
> > + struct mm_struct *prev;
> > + bool is_kernel_thread;
> > + struct arch_hw_breakpoint brk;
> > +};
> > +
> > +static inline void init_temp_mm(struct temp_mm *temp_mm, struct mm_struct *mm)
> > +{
> > + temp_mm->temp = mm;
> > + temp_mm->prev = NULL;
> > + temp_mm->is_kernel_thread = false;
> > + memset(&temp_mm->brk, 0, sizeof(temp_mm->brk));
> > +}
> > +
> > +static inline void use_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > + lockdep_assert_irqs_disabled();
> > +
> > + temp_mm->is_kernel_thread = current->mm == NULL;
> > + if (temp_mm->is_kernel_thread)
> > + temp_mm->prev = current->active_mm;
> > + else
> > + temp_mm->prev = current->mm;
> > +
> > + /*
> > + * Hash requires a non-NULL current->mm to allocate a userspace address
> > + * when handling a page fault. Does not appear to hurt in Radix either.
> > + */
> > + current->mm = temp_mm->temp;
> > + switch_mm_irqs_off(NULL, temp_mm->temp, current);
> > +
> > + if (ppc_breakpoint_available()) {
> > + __get_breakpoint(&temp_mm->brk);
> > + if (temp_mm->brk.type != 0)
> > + hw_breakpoint_disable();
> > + }
> > +}
> > +
> > +static inline void unuse_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > + lockdep_assert_irqs_disabled();
> > +
> > + if (temp_mm->is_kernel_thread)
> > + current->mm = NULL;
> > + else
> > + current->mm = temp_mm->prev;
> > + switch_mm_irqs_off(NULL, temp_mm->prev, current);
> > +
> > + if (ppc_breakpoint_available() && temp_mm->brk.type != 0)
> > + __set_breakpoint(&temp_mm->brk);
> > +}
> > +
> > #endif /* __KERNEL__ */
> > #endif /* __ASM_POWERPC_MMU_CONTEXT_H */
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index fad50db9dcf2..5e5cf33fc358 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -793,6 +793,11 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
> > return 0;
> > }
> >
> > +void __get_breakpoint(struct arch_hw_breakpoint *brk)
> > +{
> > + memcpy(brk, this_cpu_ptr(¤t_brk), sizeof(*brk));
> > +}
> > +
> > void __set_breakpoint(struct arch_hw_breakpoint *brk)
> > {
> > memcpy(this_cpu_ptr(¤t_brk), brk, sizeof(*brk));
> >
>
>
> Christophe
^ permalink raw reply
* Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl, asrc-format
From: Shengjiu Wang @ 2020-03-31 2:28 UTC (permalink / raw)
To: Nicolin Chen
Cc: Mark Rutland, Rob Herring, Linux-ALSA, Timur Tabi, Xiubo Li,
Fabio Estevam, Shengjiu Wang, Takashi Iwai, Liam Girdwood,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Mark Brown, linuxppc-dev, linux-kernel
In-Reply-To: <20200323212038.GA7527@Asurada-Nvidia.nvidia.com>
Hi
On Tue, Mar 24, 2020 at 5:22 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Fri, Mar 20, 2020 at 11:32:13AM -0600, Rob Herring wrote:
> > On Mon, Mar 09, 2020 at 02:19:44PM -0700, Nicolin Chen wrote:
> > > On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> > > > In order to support new EASRC and simplify the code structure,
> > > > We decide to share the common structure between them. This bring
> > > > a problem that EASRC accept format directly from devicetree, but
> > > > ASRC accept width from devicetree.
> > > >
> > > > In order to align with new ESARC, we add new property fsl,asrc-format.
> > > > The fsl,asrc-format can replace the fsl,asrc-width, then driver
> > > > can accept format from devicetree, don't need to convert it to
> > > > format through width.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > > Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +++++
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > > index cb9a25165503..780455cf7f71 100644
> > > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > > @@ -51,6 +51,11 @@ Optional properties:
> > > > will be in use as default. Otherwise, the big endian
> > > > mode will be in use for all the device registers.
> > > >
> > > > + - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> > > > + Ends, which can replace the fsl,asrc-width.
> > > > + The value is SNDRV_PCM_FORMAT_S16_LE, or
> > > > + SNDRV_PCM_FORMAT_S24_LE
> > >
> > > I am still holding the concern at the DT binding of this format,
> > > as it uses values from ASoC header file instead of a dt-binding
> > > header file -- not sure if we can do this. Let's wait for Rob's
> > > comments.
> >
> > I assume those are an ABI as well, so it's okay to copy them unless we
>
> They are defined under include/uapi. So I think we can use them?
>
> > already have some format definitions for DT. But it does need to be copy
> > in a header under include/dt-bindings/.
>
> Shengjiu is actually quoting those integral values, rather than
> those macros, so actually no need copy to include/dt-bindings,
> yet whoever adds this format property to a new DT would need to
> look up the value in a header file under include/uapi. I's just
> wondering if that's okay.
>
> Thanks
Shall I keep this change or drop this change?
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v6 1/7] powerpc/mm: Implement set_memory() routines
From: Russell Currey @ 2020-03-31 1:49 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel
In-Reply-To: <87imjbpgw2.fsf@dja-thinkpad.axtens.net>
On Wed, 2020-03-11 at 17:03 +1100, Daniel Axtens wrote:
> Russell Currey <ruscur@russell.cc> writes:
>
> > The set_memory_{ro/rw/nx/x}() functions are required for
> > STRICT_MODULE_RWX,
> > and are generally useful primitives to have. This implementation
> > is
> > designed to be completely generic across powerpc's many MMUs.
> >
> > It's possible that this could be optimised to be faster for
> > specific
> > MMUs, but the focus is on having a generic and safe implementation
> > for
> > now.
> >
> > This implementation does not handle cases where the caller is
> > attempting
> > to change the mapping of the page it is executing from, or if
> > another
> > CPU is concurrently using the page being altered. These cases
> > likely
> > shouldn't happen, but a more complex implementation with MMU-
> > specific code
> > could safely handle them, so that is left as a TODO for now.
> >
> > These functions do nothing if STRICT_KERNEL_RWX is not enabled.
> >
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > ---
> > v6: Merge patch 8/8 from v5, handling RWX not being enabled.
> > Add note to change_page_attr() in case it's ever made non-
> > static
> > ---
> > arch/powerpc/Kconfig | 1 +
> > arch/powerpc/include/asm/set_memory.h | 32 +++++++++++
> > arch/powerpc/mm/Makefile | 2 +-
> > arch/powerpc/mm/pageattr.c | 79
> > +++++++++++++++++++++++++++
> > 4 files changed, 113 insertions(+), 1 deletion(-)
> > create mode 100644 arch/powerpc/include/asm/set_memory.h
> > create mode 100644 arch/powerpc/mm/pageattr.c
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 497b7d0b2d7e..bd074246e34e 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -129,6 +129,7 @@ config PPC
> > select ARCH_HAS_PTE_SPECIAL
> > select ARCH_HAS_MEMBARRIER_CALLBACKS
> > select ARCH_HAS_SCALED_CPUTIME if
> > VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
> > + select ARCH_HAS_SET_MEMORY
> > select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 ||
> > PPC32) && !HIBERNATION)
> > select ARCH_HAS_TICK_BROADCAST if
> > GENERIC_CLOCKEVENTS_BROADCAST
> > select ARCH_HAS_UACCESS_FLUSHCACHE
> > diff --git a/arch/powerpc/include/asm/set_memory.h
> > b/arch/powerpc/include/asm/set_memory.h
> > new file mode 100644
> > index 000000000000..64011ea444b4
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/set_memory.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_POWERPC_SET_MEMORY_H
> > +#define _ASM_POWERPC_SET_MEMORY_H
> > +
> > +#define SET_MEMORY_RO 0
> > +#define SET_MEMORY_RW 1
> > +#define SET_MEMORY_NX 2
> > +#define SET_MEMORY_X 3
> > +
> > +int change_memory_attr(unsigned long addr, int numpages, long
> > action);
> > +
> > +static inline int set_memory_ro(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_RO);
> > +}
> > +
> > +static inline int set_memory_rw(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_RW);
> > +}
> > +
> > +static inline int set_memory_nx(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_NX);
> > +}
> > +
> > +static inline int set_memory_x(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_X);
> > +}
> > +
> > +#endif
> > diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
> > index 5e147986400d..a998fdac52f9 100644
> > --- a/arch/powerpc/mm/Makefile
> > +++ b/arch/powerpc/mm/Makefile
> > @@ -5,7 +5,7 @@
> >
> > ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
> >
> > -obj-y := fault.o mem.o pgtable.o
> > mmap.o \
> > +obj-y := fault.o mem.o pgtable.o
> > mmap.o pageattr.o \
> > init_$(BITS).o pgtable_$(BITS).o \
> > pgtable-frag.o ioremap.o
> > ioremap_$(BITS).o \
> > init-common.o mmu_context.o drmem.o
> > diff --git a/arch/powerpc/mm/pageattr.c
> > b/arch/powerpc/mm/pageattr.c
> > new file mode 100644
> > index 000000000000..748fa56d9db0
> > --- /dev/null
> > +++ b/arch/powerpc/mm/pageattr.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * MMU-generic set_memory implementation for powerpc
> > + *
> > + * Copyright 2019, IBM Corporation.
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/set_memory.h>
> > +
> > +#include <asm/mmu.h>
> > +#include <asm/page.h>
> > +#include <asm/pgtable.h>
> > +
> > +
> > +/*
> > + * Updates the attributes of a page in three steps:
> > + *
> > + * 1. invalidate the page table entry
> > + * 2. flush the TLB
> > + * 3. install the new entry with the updated attributes
> > + *
> > + * This is unsafe if the caller is attempting to change the
> > mapping of the
> > + * page it is executing from, or if another CPU is concurrently
> > using the
> > + * page being altered.
> > + *
> > + * TODO make the implementation resistant to this.
> > + *
> > + * NOTE: can be dangerous to call without STRICT_KERNEL_RWX
> > + */
> > +static int change_page_attr(pte_t *ptep, unsigned long addr, void
> > *data)
> > +{
> > + long action = (long)data;
> > + pte_t pte;
> > +
> > + spin_lock(&init_mm.page_table_lock);
> > +
> > + /* invalidate the PTE so it's safe to modify */
> > + pte = ptep_get_and_clear(&init_mm, addr, ptep);
> > + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> > +
> > + /* modify the PTE bits as desired, then apply */
> > + switch (action) {
> > + case SET_MEMORY_RO:
> > + pte = pte_wrprotect(pte);
> > + break;
> > + case SET_MEMORY_RW:
> > + pte = pte_mkwrite(pte);
> > + break;
> > + case SET_MEMORY_NX:
> > + pte = pte_exprotect(pte);
> > + break;
> > + case SET_MEMORY_X:
> > + pte = pte_mkexec(pte);
> > + break;
> > + default:
> > + break;
>
> Should this have a WARN_ON_ONCE to let you know you're doing
> something
> that doesn't work? I know it's only ever called by things in this
> file,
> but still... Anyway it's very minor and I'm not fussed either way.
True, might as well.
>
> > + }
> > +
> > + set_pte_at(&init_mm, addr, ptep, pte);
> > + spin_unlock(&init_mm.page_table_lock);
>
> Initially I thought: shouldn't you put the PTL lock/unlock in the
> outer
> function? Then I remembered that apply_to_page_range can potentially
> allocate new page table entries which would deadlock if you held the
> lock.
>
> Speaking of which - apply_to_page_range will create new pte entries
> if
> you apply it over an address range that isn't filled in. That doesn't
> really make sense here - should you use apply_to_existing_page_range
> instead?
>
> You _might_ be able to move the PTL lock if you use
> apply_to_existing_page_range but I'm not completely sure if that's
> safe
> or if the speed boost is worth it. You could check mm/memory.c if you
> wanted.
Seems like I should definitely be using apply_to_existing_page_range()
but I'm not too keen on moving the lock in case it's unsafe - and these
only get called on module load so it's not a particularly hot path.
> > +
> > + return 0;
> > +}
> > +
> > +int change_memory_attr(unsigned long addr, int numpages, long
> > action)
> > +{
> > + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> > + unsigned long sz = numpages * PAGE_SIZE;
> > +
> > + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> > + return 0;
> > +
> > + if (!numpages)
> > + return 0;
>
> What happens if numpages is negative? Doesn't the guard need to check
> for that rather than just for zero?
I don't know why numpages isn't unsigned in the set_memory API, that
sounds like another potential patch.
Anyway, yes.
> With those caveats, and noting that I've been focused only on:
> - lock/unlock paths
> - integer arithmetic
> - stuff about apply_page_range semantics
> this patch is:
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
Thanks for the review, I wasn't aware apply_to_existing_page_range()
existed.
>
> Regards,
> Daniel
>
> > +
> > + return apply_to_page_range(&init_mm, start, sz,
> > change_page_attr, (void *)action);
> > +}
> > --
> > 2.25.1
^ permalink raw reply
* [PATCH kernel] powerpc/pseries/ddw: Extend upper limit for huge DMA window for persistent memory
From: Alexey Kardashevskiy @ 2020-03-31 1:23 UTC (permalink / raw)
To: linuxppc-dev
Cc: Brian J King, Alexey Kardashevskiy, Oliver O'Halloran,
Aneesh Kumar K . V, David Gibson, Wen Xiong
Unlike normal memory ("memory" compatible type in the FDT),
the persistent memory ("ibm,pmemory" in the FDT) can be mapped anywhere
in the guest physical space and it can be used for DMA.
In order to maintain 1:1 mapping via the huge DMA window, we need to
know the maximum physical address at the time of the window setup.
So far we've been looking at "memory" nodes but "ibm,pmemory" does not
have fixed addresses and the persistent memory may be mapped afterwards.
Since the persistent memory is still backed with page structs,
use MAX_PHYSMEM_BITS as the upper limit.
This effectively disables huge DMA window in LPAR under pHyp if
persistent memory is present but this is the best we can do.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/platforms/pseries/iommu.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 2e0a8eab5588..6d47b4a3ce39 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -945,6 +945,15 @@ static phys_addr_t ddw_memory_hotplug_max(void)
phys_addr_t max_addr = memory_hotplug_max();
struct device_node *memory;
+ /*
+ * The "ibm,pmemory" can appear anywhere in the address space.
+ * Assuming it is still backed by page structs, set the upper limit
+ * for the huge DMA window as MAX_PHYSMEM_BITS.
+ */
+ if (of_find_node_by_type(NULL, "ibm,pmemory"))
+ return (sizeof(phys_addr_t) * 8 <= MAX_PHYSMEM_BITS) ?
+ (phys_addr_t) -1 : (1ULL << MAX_PHYSMEM_BITS);
+
for_each_node_by_type(memory, "memory") {
unsigned long start, size;
int n_mem_addr_cells, n_mem_size_cells, len;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] powerpc/44x: Make AKEBONO depends on NET
From: Michael Ellerman @ 2020-03-31 0:58 UTC (permalink / raw)
To: YueHaibing, mporter, benh, paulus
Cc: alistair, YueHaibing, linuxppc-dev, linux-kernel
In-Reply-To: <20200330143153.32800-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> writes:
> Fix Kconfig warnings:
>
> WARNING: unmet direct dependencies detected for NETDEVICES
> Depends on [n]: NET [=n]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> WARNING: unmet direct dependencies detected for ETHERNET
> Depends on [n]: NETDEVICES [=y] && NET [=n]
> Selected by [y]:
> - AKEBONO [=y] && PPC_47x [=y]
>
> AKEBONO select NETDEVICES and ETHERNET unconditionally,
It shouldn't do that, that's the job of a defconfig.
It might want to enable NET_VENDOR_IBM iff the config already has NET
and other dependencies enabled.
So the patch below might work?
cheers
diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
index 25ebe634a661..32aac4f40f1b 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -207,9 +207,7 @@ config AKEBONO
select PPC4xx_HSTA_MSI
select I2C
select I2C_IBM_IIC
- select NETDEVICES
- select ETHERNET
- select NET_VENDOR_IBM
+ imply NET_VENDOR_IBM
select IBM_EMAC_EMAC4 if IBM_EMAC
select USB if USB_SUPPORT
select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
> If NET is not set, build fails. Add this dependcy to fix this.
>
> Fixes: 2a2c74b2efcb ("IBM Akebono: Add the Akebono platform")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> arch/powerpc/platforms/44x/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
> index 25ebe634a661..394f662d7df2 100644
> --- a/arch/powerpc/platforms/44x/Kconfig
> +++ b/arch/powerpc/platforms/44x/Kconfig
> @@ -199,6 +199,7 @@ config FSP2
> config AKEBONO
> bool "IBM Akebono (476gtr) Support"
> depends on PPC_47x
> + depends on NET
> select SWIOTLB
> select 476FPE
> select PPC4xx_PCI_EXPRESS
> --
> 2.17.1
^ permalink raw reply related
* Re: [PATCH 0/2] mm/thp: Rename pmd_mknotpresent() as pmd_mknotvalid()
From: Andrew Morton @ 2020-03-30 22:51 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Catalin Marinas, Peter Zijlstra, Dave Hansen, linux-kernel,
linux-mm, Paul Mackerras, H. Peter Anvin, Will Deacon, x86,
Russell King, Ingo Molnar, nouveau, linux-snps-arc,
Steven Rostedt, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
linux-arm-kernel, Thomas Bogendoerfer, Vineet Gupta, linux-mips,
linuxppc-dev
In-Reply-To: <2e67f1b8-d196-89e4-aee1-f552db1433a0@arm.com>
On Sun, 29 Mar 2020 19:12:35 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>
>
> On 03/20/2020 10:24 AM, Anshuman Khandual wrote:
> > This series renames pmd_mknotpresent() as pmd_mknotvalid(). Before that it
> > drops an existing pmd_mknotpresent() definition from powerpc platform which
> > was never required as it defines it's pmdp_invalidate() through subscribing
> > __HAVE_ARCH_PMDP_INVALIDATE. This does not create any functional change.
> >
> > This rename was suggested by Catalin during a previous discussion while we
> > were trying to change the THP helpers on arm64 platform for migration.
> >
> > https://patchwork.kernel.org/patch/11019637/
> >
> > This series is based on v5.6-rc6.
> >
> > Boot tested on arm64 and x86 platforms.
> > Built tested on many other platforms including the ones changed here.
>
> Gentle ping, any updates regarding this ?
We're in the merge window so I have parked this for consideration after
-rc1.
^ permalink raw reply
* Re: [PATCH v7 3/6] perf/tools: Refactoring metricgroup__add_metric function
From: Jiri Olsa @ 2020-03-30 19:36 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200327102528.4267-4-kjain@linux.ibm.com>
On Fri, Mar 27, 2020 at 03:55:25PM +0530, Kajol Jain wrote:
SNIP
> + eg->ids = ids;
> + eg->idnum = idnum;
> + eg->metric_name = pe->metric_name;
> + eg->metric_expr = pe->metric_expr;
> + eg->metric_unit = pe->unit;
> + list_add_tail(&eg->nd, group_list);
> +
> + return 0;
> +}
> +
> static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> struct list_head *group_list)
> {
> @@ -504,35 +538,12 @@ static int metricgroup__add_metric(const char *metric, struct strbuf *events,
> continue;
> if (match_metric(pe->metric_group, metric) ||
> match_metric(pe->metric_name, metric)) {
> - const char **ids;
> - int idnum;
> - struct egroup *eg;
>
> pr_debug("metric expr %s for %s\n", pe->metric_expr, pe->metric_name);
>
> - if (expr__find_other(pe->metric_expr,
> - NULL, &ids, &idnum) < 0)
> + ret = __metricgroup__add_metric(events, group_list, pe);
> + if (ret == -EINVAL || !ret)
> continue;
what's the point of continue in here? it's end of the loop..
jirka
^ permalink raw reply
* Re: [PATCH v7 3/6] perf/tools: Refactoring metricgroup__add_metric function
From: Jiri Olsa @ 2020-03-30 19:36 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200327102528.4267-4-kjain@linux.ibm.com>
On Fri, Mar 27, 2020 at 03:55:25PM +0530, Kajol Jain wrote:
> This patch refactor metricgroup__add_metric function where
> some part of it move to function metricgroup__add_metric_param.
> No logic change.
>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
> tools/perf/util/metricgroup.c | 61 +++++++++++++++++++++--------------
> 1 file changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 926449a7cdbf..b905eaa907a7 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -485,6 +485,40 @@ static bool metricgroup__has_constraint(struct pmu_event *pe)
> return false;
> }
>
> +static int __metricgroup__add_metric(struct strbuf *events,
> + struct list_head *group_list, struct pmu_event *pe)
> +{
> +
> + const char **ids;
> + int idnum;
> + struct egroup *eg;
> + int ret = -EINVAL;
> +
> + if (expr__find_other(pe->metric_expr, NULL, &ids, &idnum) < 0)
> + return ret;
no need for ret variable, return -EINVAL directly
jirka
^ permalink raw reply
* Re: [PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: Jiri Olsa @ 2020-03-30 19:35 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200327102528.4267-5-kjain@linux.ibm.com>
On Fri, Mar 27, 2020 at 03:55:26PM +0530, Kajol Jain wrote:
SNIP
>
> %%
> struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> @@ -93,7 +106,7 @@ if { return IF; }
> else { return ELSE; }
> #smt_on { return SMT_ON; }
> {number} { return value(yyscanner, 10); }
> -{symbol} { return str(yyscanner, ID); }
> +{symbol} { return str(yyscanner, ID, sctx->runtime_param); }
> "|" { return '|'; }
> "^" { return '^'; }
> "&" { return '&'; }
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index b905eaa907a7..66695b4a358d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -90,6 +90,7 @@ struct egroup {
> const char *metric_name;
> const char *metric_expr;
> const char *metric_unit;
> + int param;
could you please use runtime_param everywhere? ... s/param/runtime_param/
or maybe just 'runtime' to keep it short..? it's param in any case ;-)
other than that it looks ok
jirka
^ permalink raw reply
* Re: [PATCH v7 4/6] perf/tools: Enhance JSON/metric infrastructure to handle "?"
From: Jiri Olsa @ 2020-03-30 19:35 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200327102528.4267-5-kjain@linux.ibm.com>
On Fri, Mar 27, 2020 at 03:55:26PM +0530, Kajol Jain wrote:
SNIP
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index ea10fc4412c4..516504cf0ea5 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -10,7 +10,7 @@ static int test(struct expr_parse_ctx *ctx, const char *e, double val2)
> {
> double val;
>
> - if (expr__parse(&val, ctx, e))
> + if (expr__parse(&val, ctx, e, 1))
> TEST_ASSERT_VAL("parse test failed", 0);
> TEST_ASSERT_VAL("unexpected value", val == val2);
> return 0;
> @@ -44,15 +44,15 @@ int test__expr(struct test *t __maybe_unused, int subtest __maybe_unused)
> return ret;
>
> p = "FOO/0";
> - ret = expr__parse(&val, &ctx, p);
> + ret = expr__parse(&val, &ctx, p, 1);
> TEST_ASSERT_VAL("division by zero", ret == -1);
>
> p = "BAR/";
> - ret = expr__parse(&val, &ctx, p);
> + ret = expr__parse(&val, &ctx, p, 1);
> TEST_ASSERT_VAL("missing operand", ret == -1);
>
> TEST_ASSERT_VAL("find other",
> - expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other) == 0);
> + expr__find_other("FOO + BAR + BAZ + BOZO", "FOO", &other, &num_other, 1) == 0);
> TEST_ASSERT_VAL("find other", num_other == 3);
> TEST_ASSERT_VAL("find other", !strcmp(other[0], "BAR"));
> TEST_ASSERT_VAL("find other", !strcmp(other[1], "BAZ"));
could we add test case for runtime param > 1 in here?
expr_parse should return value that would depend on this value..
jirka
^ permalink raw reply
* Re: [PATCH 10/12] powerpc/entry32: Blacklist exception entry points for kprobe.
From: Christophe Leroy @ 2020-03-30 18:33 UTC (permalink / raw)
To: Naveen N. Rao, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1585588031.jvow7mwq4x.naveen@linux.ibm.com>
Le 30/03/2020 à 19:08, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> kprobe does not handle events happening in real mode.
>>
>> As exception entry points are running with MMU disabled,
>> blacklist them.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/kernel/entry_32.S | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/entry_32.S
>> b/arch/powerpc/kernel/entry_32.S
>> index 94f78c03cb79..9a1a45d6038a 100644
>> --- a/arch/powerpc/kernel/entry_32.S
>> +++ b/arch/powerpc/kernel/entry_32.S
>> @@ -51,6 +51,7 @@ mcheck_transfer_to_handler:
>> mfspr r0,SPRN_DSRR1
>> stw r0,_DSRR1(r11)
>> /* fall through */
>> +_ASM_NOKPROBE_SYMBOL(mcheck_transfer_to_handler)
>>
>> .globl debug_transfer_to_handler
>> debug_transfer_to_handler:
>> @@ -59,6 +60,7 @@ debug_transfer_to_handler:
>> mfspr r0,SPRN_CSRR1
>> stw r0,_CSRR1(r11)
>> /* fall through */
>> +_ASM_NOKPROBE_SYMBOL(debug_transfer_to_handler)
>>
>> .globl crit_transfer_to_handler
>> crit_transfer_to_handler:
>> @@ -94,6 +96,7 @@ crit_transfer_to_handler:
>> rlwinm r0,r1,0,0,(31 - THREAD_SHIFT)
>> stw r0,KSP_LIMIT(r8)
>> /* fall through */
>> +_ASM_NOKPROBE_SYMBOL(crit_transfer_to_handler)
>> #endif
>>
>> #ifdef CONFIG_40x
>> @@ -115,6 +118,7 @@ crit_transfer_to_handler:
>> rlwinm r0,r1,0,0,(31 - THREAD_SHIFT)
>> stw r0,KSP_LIMIT(r8)
>> /* fall through */
>> +_ASM_NOKPROBE_SYMBOL(crit_transfer_to_handler)
>> #endif
>>
>> /*
>> @@ -127,6 +131,7 @@ crit_transfer_to_handler:
>> .globl transfer_to_handler_full
>> transfer_to_handler_full:
>> SAVE_NVGPRS(r11)
>> +_ASM_NOKPROBE_SYMBOL(transfer_to_handler_full)
>> /* fall through */
>>
>> .globl transfer_to_handler
>> @@ -286,6 +291,8 @@ reenable_mmu:
>> lwz r2, GPR2(r11)
>> b fast_exception_return
>> #endif
>> +_ASM_NOKPROBE_SYMBOL(transfer_to_handler)
>> +_ASM_NOKPROBE_SYMBOL(transfer_to_handler_cont)
>
> These are added after 'reenable_mmu', which is itself not blacklisted.
> Is that intentional?
Yes I put it as the complete end of the entry part, ie just before
stack_ovf which is a function by itself.
Note that reenable_mmu is inside an #ifdef CONFIG_TRACE_IRQFLAGS.
I'm not completely sure where to put the _ASM_NOKPROBE_SYMBOL()s, that's
the reason why I put it close to the symbol itself in my first series.
Could you have a look at the code and tell me what looks the most
appropriate as a location to you ?
https://elixir.bootlin.com/linux/v5.6/source/arch/powerpc/kernel/entry_32.S#L230
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH v3 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Michal Hocko @ 2020-03-30 18:23 UTC (permalink / raw)
To: Mike Rapoport
Cc: mmorana, Catalin Marinas, Heiko Carstens,
open list:MEMORY MANAGEMENT, Paul Mackerras, H. Peter Anvin,
sparclinux, Alexander Duyck, linux-s390, x86,
Christian Borntraeger, Ingo Molnar, Hoan Tran, Pavel Tatashin,
lho, Vasily Gorbik, Vlastimil Babka, Will Deacon, Borislav Petkov,
Thomas Gleixner, linux-arm-kernel, Oscar Salvador, linux-kernel,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200330175100.GD30942@linux.ibm.com>
On Mon 30-03-20 20:51:00, Mike Rapoport wrote:
> On Mon, Mar 30, 2020 at 09:42:46AM +0200, Michal Hocko wrote:
> > On Sat 28-03-20 11:31:17, Hoan Tran wrote:
> > > In NUMA layout which nodes have memory ranges that span across other nodes,
> > > the mm driver can detect the memory node id incorrectly.
> > >
> > > For example, with layout below
> > > Node 0 address: 0000 xxxx 0000 xxxx
> > > Node 1 address: xxxx 1111 xxxx 1111
> > >
> > > Note:
> > > - Memory from low to high
> > > - 0/1: Node id
> > > - x: Invalid memory of a node
> > >
> > > When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
> > > config, mm only checks the memory validity but not the node id.
> > > Because of that, Node 1 also detects the memory from node 0 as below
> > > when it scans from the start address to the end address of node 1.
> > >
> > > Node 0 address: 0000 xxxx xxxx xxxx
> > > Node 1 address: xxxx 1111 1111 1111
> > >
> > > This layout could occur on any architecture. Most of them enables
> > > this config by default with CONFIG_NUMA. This patch, by default, enables
> > > CONFIG_NODES_SPAN_OTHER_NODES or uses early_pfn_in_nid() for NUMA.
> >
> > I am not opposed to this at all. It reduces the config space and that is
> > a good thing on its own. The history has shown that meory layout might
> > be really wild wrt NUMA. The config is only used for early_pfn_in_nid
> > which is clearly an overkill.
> >
> > Your description doesn't really explain why this is safe though. The
> > history of this config is somehow messy, though. Mike has tried
> > to remove it a94b3ab7eab4 ("[PATCH] mm: remove arch independent
> > NODES_SPAN_OTHER_NODES") just to be reintroduced by 7516795739bd
> > ("[PATCH] Reintroduce NODES_SPAN_OTHER_NODES for powerpc") without any
> > reasoning what so ever. This doesn't make it really easy see whether
> > reasons for reintroduction are still there. Maybe there are some subtle
> > dependencies. I do not see any TBH but that might be burried deep in an
> > arch specific code.
>
> I've looked at this a bit more and it seems that the check for
> early_pfn_in_nid() in memmap_init_zone() can be simply removed.
>
> The commits you've mentioned were way before the addition of
> HAVE_MEMBLOCK_NODE_MAP and the whole infrastructure that calculates zone
> sizes and boundaries based on the memblock node map.
> So, the memmap_init_zone() is called when zone boundaries are already
> within a node.
But zones from different nodes might overlap in the pfn range. And this
check is there to skip over those overlapping areas. The only way to
skip over this check I can see is to do a different pfn walk and go
through memblock ranges which are guaranteed to belong to a single node.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 0/5] mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
From: Mike Rapoport @ 2020-03-30 17:51 UTC (permalink / raw)
To: Michal Hocko
Cc: mmorana, Catalin Marinas, Heiko Carstens,
open list:MEMORY MANAGEMENT, Paul Mackerras, H. Peter Anvin,
sparclinux, Alexander Duyck, linux-s390, x86,
Christian Borntraeger, Ingo Molnar, Hoan Tran, Pavel Tatashin,
lho, Vasily Gorbik, Vlastimil Babka, Will Deacon, Borislav Petkov,
Thomas Gleixner, linux-arm-kernel, Oscar Salvador, linux-kernel,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200330074246.GA14243@dhcp22.suse.cz>
On Mon, Mar 30, 2020 at 09:42:46AM +0200, Michal Hocko wrote:
> On Sat 28-03-20 11:31:17, Hoan Tran wrote:
> > In NUMA layout which nodes have memory ranges that span across other nodes,
> > the mm driver can detect the memory node id incorrectly.
> >
> > For example, with layout below
> > Node 0 address: 0000 xxxx 0000 xxxx
> > Node 1 address: xxxx 1111 xxxx 1111
> >
> > Note:
> > - Memory from low to high
> > - 0/1: Node id
> > - x: Invalid memory of a node
> >
> > When mm probes the memory map, without CONFIG_NODES_SPAN_OTHER_NODES
> > config, mm only checks the memory validity but not the node id.
> > Because of that, Node 1 also detects the memory from node 0 as below
> > when it scans from the start address to the end address of node 1.
> >
> > Node 0 address: 0000 xxxx xxxx xxxx
> > Node 1 address: xxxx 1111 1111 1111
> >
> > This layout could occur on any architecture. Most of them enables
> > this config by default with CONFIG_NUMA. This patch, by default, enables
> > CONFIG_NODES_SPAN_OTHER_NODES or uses early_pfn_in_nid() for NUMA.
>
> I am not opposed to this at all. It reduces the config space and that is
> a good thing on its own. The history has shown that meory layout might
> be really wild wrt NUMA. The config is only used for early_pfn_in_nid
> which is clearly an overkill.
>
> Your description doesn't really explain why this is safe though. The
> history of this config is somehow messy, though. Mike has tried
> to remove it a94b3ab7eab4 ("[PATCH] mm: remove arch independent
> NODES_SPAN_OTHER_NODES") just to be reintroduced by 7516795739bd
> ("[PATCH] Reintroduce NODES_SPAN_OTHER_NODES for powerpc") without any
> reasoning what so ever. This doesn't make it really easy see whether
> reasons for reintroduction are still there. Maybe there are some subtle
> dependencies. I do not see any TBH but that might be burried deep in an
> arch specific code.
I've looked at this a bit more and it seems that the check for
early_pfn_in_nid() in memmap_init_zone() can be simply removed.
The commits you've mentioned were way before the addition of
HAVE_MEMBLOCK_NODE_MAP and the whole infrastructure that calculates zone
sizes and boundaries based on the memblock node map.
So, the memmap_init_zone() is called when zone boundaries are already
within a node.
I don't have access to machines with memory layout that required this check
at the first place, so if anybody who does could test the change below on
such machine it would be great.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3c4eb750a199..6d3eb0901864 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5908,10 +5908,6 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
pfn = next_pfn(pfn);
continue;
}
- if (!early_pfn_in_nid(pfn, nid)) {
- pfn++;
- continue;
- }
if (overlap_memmap_init(zone, &pfn))
continue;
if (defer_init(nid, pfn, end_pfn))
> > v3:
> > * Revise the patch description
> >
> > V2:
> > * Revise the patch description
> >
> > Hoan Tran (5):
> > mm: Enable CONFIG_NODES_SPAN_OTHER_NODES by default for NUMA
> > powerpc: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
> > x86: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
> > sparc: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
> > s390: Kconfig: Remove CONFIG_NODES_SPAN_OTHER_NODES
> >
> > arch/powerpc/Kconfig | 9 ---------
> > arch/s390/Kconfig | 8 --------
> > arch/sparc/Kconfig | 9 ---------
> > arch/x86/Kconfig | 9 ---------
> > mm/page_alloc.c | 2 +-
> > 5 files changed, 1 insertion(+), 36 deletions(-)
> >
> > --
> > 1.8.3.1
> >
>
> --
> Michal Hocko
> SUSE Labs
--
Sincerely yours,
Mike.
^ permalink raw reply related
* Re: [PATCH 06/12] powerpc/32s: Make local symbols non visible in hash_low.
From: Naveen N. Rao @ 2020-03-30 17:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <6bb184ed-b42f-f334-9445-e4fde107e8c9@c-s.fr>
Christophe Leroy wrote:
>
>
> Le 30/03/2020 à 19:06, Naveen N. Rao a écrit :
>> Christophe Leroy wrote:
>>> In hash_low.S, a lot of named local symbols are used instead of
>>> numbers to ease code lisibility. However, they don't need to be
>> ^^^^^^^^^^
>> Nit.. visibility
>
>
> Lol, no.
>
> I mean't "lisibilité" in French, which means "readability"
Touche :D
- Naveen
^ permalink raw reply
* Re: [PATCH 06/12] powerpc/32s: Make local symbols non visible in hash_low.
From: Christophe Leroy @ 2020-03-30 17:22 UTC (permalink / raw)
To: Naveen N. Rao, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1585587984.mmaeo0dvju.naveen@linux.ibm.com>
Le 30/03/2020 à 19:06, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> In hash_low.S, a lot of named local symbols are used instead of
>> numbers to ease code lisibility. However, they don't need to be
> ^^^^^^^^^^
> Nit.. visibility
Lol, no.
I mean't "lisibilité" in French, which means "readability"
^ permalink raw reply
* Re: hardcoded SIGSEGV in __die() ?
From: Joakim Tjernlund @ 2020-03-30 17:16 UTC (permalink / raw)
To: christophe.leroy@c-s.fr, mpe@ellerman.id.au,
linuxppc-dev@ozlabs.org
In-Reply-To: <87lfnovu11.fsf@mpe.ellerman.id.au>
On Thu, 2020-03-26 at 11:28 +1100, Michael Ellerman wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Joakim Tjernlund <Joakim.Tjernlund@infinera.com> writes:
> > On Mon, 2020-03-23 at 15:45 +0100, Christophe Leroy wrote:
> > > Le 23/03/2020 à 15:43, Christophe Leroy a écrit :
> > > > Le 23/03/2020 à 15:17, Joakim Tjernlund a écrit :
> > > > > In __die(), see below, there is this call to notify_send() with
> > > > > SIGSEGV hardcoded, this seems odd
> > > > > to me as the variable "err" holds the true signal(in my case SIGBUS)
> > > > > Should not SIGSEGV be replaced with the true signal no.?
> > > >
> > > > As far as I can see, comes from
> > > > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3D66fcb1059&data=02%7C01%7CJoakim.Tjernlund%40infinera.com%7Caa316058f9e34dd758c808d7d11ca391%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637207793252449714&sdata=LBzRMxHWJzNEztnnG0UzJb7PHvaDGVswQD%2B8WpY9YX8%3D&reserved=0
> > > >
> > >
> > > And
> > > https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2F%3Fid%3Dae87221d3ce49d9de1e43756da834fd0bf05a2ad&data=02%7C01%7CJoakim.Tjernlund%40infinera.com%7Caa316058f9e34dd758c808d7d11ca391%7C285643de5f5b4b03a1530ae2dc8aaf77%7C1%7C0%7C637207793252449714&sdata=Dh%2BUTRgG85oVSgC3SCR1B7izQH4HofT4ppOMiy9xvDA%3D&reserved=0
> > > shows it is (was?) similar on x86.
> > >
> >
> > I tried to follow that chain thinking it would end up sending a signal to user space but I cannot see
> > that happens. Seems to be related to debugging.
> >
> > In short, I cannot see any signal being delivered to user space. If so that would explain why
> > our user space process never dies.
> > Is there a signal hidden in machine_check handler for SIGBUS I cannot see?
>
> It's platform specific. What platform are you on?
>
> See the ppc_md & cur_cpu_spec calls here:
>
> void machine_check_exception(struct pt_regs *regs)
> {
> int recover = 0;
> bool nested = in_nmi();
> if (!nested)
> nmi_enter();
>
> __this_cpu_inc(irq_stat.mce_exceptions);
>
> add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
>
> /* See if any machine dependent calls. In theory, we would want
> * to call the CPU first, and call the ppc_md. one if the CPU
> * one returns a positive number. However there is existing code
> * that assumes the board gets a first chance, so let's keep it
> * that way for now and fix things later. --BenH.
> */
> if (ppc_md.machine_check_exception)
> recover = ppc_md.machine_check_exception(regs);
> else if (cur_cpu_spec->machine_check)
> recover = cur_cpu_spec->machine_check(regs);
>
> if (recover > 0)
> goto bail;
>
>
> Either the ppc_md or cpu_spec handlers can send a signal, but after a
> bit of grepping I think only the pseries and powernv ones do.
>
> If you get into die() then it's an oops, which is not the same as a
> normal signal.
I had a look at opal_machine_check and friends and came up with:
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0381242920d9..12715d24141c 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -621,6 +621,11 @@ int machine_check_e500mc(struct pt_regs *regs)
reason & MCSR_MEA ? "Effective" : "Physical", addr);
}
+ if ((user_mode(regs))) {
+ _exception(SIGBUS, regs, reason, regs->nip);
+ recoverable = 1;
+ }
+
silent_out:
mtspr(SPRN_MCSR, mcsr);
return mfspr(SPRN_MCSR) == 0 && recoverable;
@@ -665,6 +670,10 @@ int machine_check_e500(struct pt_regs *regs)
if (reason & MCSR_BUS_RPERR)
printk("Bus - Read Parity Error\n");
+ if ((user_mode(regs))) {
+ _exception(SIGBUS, regs, reason, regs->nip);
+ return 1;
+ }
return 0;
}
@@ -695,6 +704,10 @@ int machine_check_e200(struct pt_regs *regs)
if (reason & MCSR_BUS_WRERR)
printk("Bus - Write Bus Error on buffered store or cache line push\n");
+ if ((user_mode(regs))) {
+ _exception(SIGBUS, regs, reason, regs->nip);
+ return 1;
+ }
return 0;
}
#elif defined(CONFIG_PPC32)
@@ -731,6 +744,10 @@ int machine_check_generic(struct pt_regs *regs)
default:
printk("Unknown values in msr\n");
}
+ if ((user_mode(regs))) {
+ _exception(SIGBUS, regs, reason, regs->nip);
+ return 1;
+ }
return 0;
}
#endif /* everything else */
I don't really know what I am doing, does the above make sense to you?
Jocke
^ 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