* Re: [PATCH] powerpc/powernv/nvram: opal_nvram_write handle unknown OPAL errors
From: Stewart Smith @ 2018-03-29 4:27 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin
In-Reply-To: <20180326150233.23089-1-npiggin@gmail.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> opal_nvram_write currently just assumes success if it encounters an
> error other than OPAL_BUSY or OPAL_BUSY_EVENT. Have it return -EIO
> on other errors instead.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/platforms/powernv/opal-nvram.c | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Stewart Smith <stewart@linux.ibm.com>
--
Stewart Smith
OPAL Architect, IBM.
^ permalink raw reply
* [PATCH v2 4/4] powerpc/fadump: Do not allow hot-remove memory from fadump reserved area.
From: Mahesh J Salgaonkar @ 2018-03-29 3:58 UTC (permalink / raw)
To: linuxppc-dev
Cc: Srikar Dronamraju, kernelfans, Aneesh Kumar K.V, Ananth Narayan,
Hari Bathini, Nathan Fontenot, Anshuman Khandual
In-Reply-To: <152229586004.24399.4092811524123468871.stgit@jupiter.in.ibm.com>
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
For fadump to work successfully there should not be any holes in reserved
memory ranges where kernel has asked firmware to move the content of old
kernel memory in event of crash. But this memory area is currently not
protected from hot-remove operations. Hence, fadump service can fail to
re-register after the hot-remove operation, if hot-removed memory belongs
to fadump reserved region. To avoid this make sure that memory from fadump
reserved area is not hot-removable if fadump is registered.
However, if user still wants to remove that memory, he can do so by
manually stopping fadump service before hot-remove operation.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/fadump.h | 2 +-
arch/powerpc/kernel/fadump.c | 10 ++++++++--
arch/powerpc/platforms/pseries/hotplug-memory.c | 7 +++++--
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
index 776cba0baec4..bd84b496d2b8 100644
--- a/arch/powerpc/include/asm/fadump.h
+++ b/arch/powerpc/include/asm/fadump.h
@@ -206,7 +206,7 @@ struct fad_crash_memory_ranges {
unsigned long long size;
};
-extern int is_fadump_boot_memory_area(u64 addr, ulong size);
+extern int is_fadump_memory_area(u64 addr, ulong size);
extern int early_init_dt_scan_fw_dump(unsigned long node,
const char *uname, int depth, void *data);
extern int fadump_reserve_mem(void);
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 0268a32b632e..331066eefaee 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -162,13 +162,19 @@ int __init early_init_dt_scan_fw_dump(unsigned long node,
/*
* If fadump is registered, check if the memory provided
- * falls within boot memory area.
+ * falls within boot memory area and reserved memory area.
*/
-int is_fadump_boot_memory_area(u64 addr, ulong size)
+int is_fadump_memory_area(u64 addr, ulong size)
{
+ u64 d_start = fw_dump.reserve_dump_area_start;
+ u64 d_end = d_start + fw_dump.reserve_dump_area_size;
+
if (!fw_dump.dump_registered)
return 0;
+ if (((addr + size) > d_start) && (addr <= d_end))
+ return 1;
+
return (addr + size) > RMA_START && addr <= fw_dump.boot_memory_size;
}
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index c1578f54c626..e4c658cda3a7 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -389,8 +389,11 @@ static bool lmb_is_removable(struct drmem_lmb *lmb)
phys_addr = lmb->base_addr;
#ifdef CONFIG_FA_DUMP
- /* Don't hot-remove memory that falls in fadump boot memory area */
- if (is_fadump_boot_memory_area(phys_addr, block_sz))
+ /*
+ * Don't hot-remove memory that falls in fadump boot memory area
+ * and memory that is reserved for capturing old kernel memory.
+ */
+ if (is_fadump_memory_area(phys_addr, block_sz))
return false;
#endif
^ permalink raw reply related
* [PATCH v2 3/4] powerpc/fadump: throw proper error message on fadump registration failure.
From: Mahesh J Salgaonkar @ 2018-03-29 3:58 UTC (permalink / raw)
To: linuxppc-dev
Cc: Srikar Dronamraju, kernelfans, Aneesh Kumar K.V, Ananth Narayan,
Hari Bathini, Nathan Fontenot, Anshuman Khandual
In-Reply-To: <152229586004.24399.4092811524123468871.stgit@jupiter.in.ibm.com>
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
fadump fails to register when there are holes in reserved memory area.
This can happen if user has hot-removed a memory that falls in the fadump
reserved memory area. Throw a meaningful error message to the user in
such case.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
arch/powerpc/kernel/fadump.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 9d75619cac28..0268a32b632e 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -216,6 +216,36 @@ static int is_boot_memory_area_contiguous(void)
return ret;
}
+/*
+ * Returns 1, if there are no holes in reserved memory area,
+ * 0 otherwise.
+ */
+static int is_reserved_memory_area_contiguous(void)
+{
+ struct memblock_region *reg;
+ unsigned long start, end;
+ unsigned long d_start = fw_dump.reserve_dump_area_start;
+ unsigned long d_end = d_start + fw_dump.reserve_dump_area_size;
+ int ret = 0;
+
+ for_each_memblock(memory, reg) {
+ start = max(d_start, (unsigned long)reg->base);
+ end = min(d_end, (unsigned long)(reg->base + reg->size));
+ if (d_start < end) {
+ /* Memory hole from d_start to start */
+ if (start > d_start)
+ break;
+
+ if (end == d_end) {
+ ret = 1;
+ break;
+ }
+ d_start = end + 1;
+ }
+ }
+ return 0;
+}
+
/* Print firmware assisted dump configurations for debugging purpose. */
static void fadump_show_config(void)
{
@@ -605,6 +635,9 @@ static int register_fw_dump(struct fadump_mem_struct *fdm)
if (!is_boot_memory_area_contiguous())
pr_err("Can't have holes in boot memory area while "
"registering fadump\n");
+ else if (!is_reserved_memory_area_contiguous())
+ pr_err("Can't have holes in reserved memory area while"
+ " registering fadump\n");
printk(KERN_ERR "Failed to register firmware-assisted kernel"
" dump. Parameter Error(%d).\n", rc);
^ permalink raw reply related
* [PATCH v2 2/4] powerpc/fadump: exclude memory holes while reserving memory in second kernel.
From: Mahesh J Salgaonkar @ 2018-03-29 3:58 UTC (permalink / raw)
To: linuxppc-dev
Cc: Srikar Dronamraju, kernelfans, Aneesh Kumar K.V, Ananth Narayan,
Hari Bathini, Nathan Fontenot, Anshuman Khandual
In-Reply-To: <152229586004.24399.4092811524123468871.stgit@jupiter.in.ibm.com>
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
The second kernel, during early boot after the crash, reserves rest of
the memory above boot memory size to make sure it does not touch any of the
dump memory area. It uses memblock_reserve() that reserves the specified
memory region irrespective of memory holes present within that region.
There are chances where previous kernel would have hot removed some of
its memory leaving memory holes behind. In such cases fadump kernel reports
incorrect number of reserved pages through arch_reserved_kernel_pages() hook
causing kernel to hang or panic.
Fix this by excluding memory holes while reserving rest of the memory
above boot memory size during second kernel boot after crash.
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
arch/powerpc/kernel/fadump.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 2098583c935f..9d75619cac28 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -434,6 +434,23 @@ static inline unsigned long get_fadump_metadata_base(
be64_to_cpu(fdm_active->rmr_region.source_len));
}
+static void fadump_memblock_reserve(unsigned long base, unsigned long size)
+{
+ struct memblock_region *reg;
+ unsigned long start, end;
+
+ for_each_memblock(memory, reg) {
+ start = (unsigned long)reg->base;
+ end = start + (unsigned long)reg->size;
+
+ if ((start >= base) && (start < (base + size))) {
+ if (end > (base + size))
+ end = base + size;
+ memblock_reserve(start, end - start);
+ }
+ }
+}
+
int __init fadump_reserve_mem(void)
{
unsigned long base, size, memory_boundary;
@@ -488,7 +505,7 @@ int __init fadump_reserve_mem(void)
*/
base = fw_dump.boot_memory_size;
size = memory_boundary - base;
- memblock_reserve(base, size);
+ fadump_memblock_reserve(base, size);
printk(KERN_INFO "Reserved %ldMB of memory at %ldMB "
"for saving crash dump\n",
(unsigned long)(size >> 20),
^ permalink raw reply related
* [PATCH v2 1/4] powerpc/fadump: Reservationless firmware assisted dump
From: Mahesh J Salgaonkar @ 2018-03-29 3:57 UTC (permalink / raw)
To: linuxppc-dev
Cc: Srikar Dronamraju, kernelfans, Ananth N Mavinakayanahalli,
Aneesh Kumar K.V, Ananth Narayan, Hari Bathini, Nathan Fontenot,
Anshuman Khandual
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
One of the primary issues with Firmware Assisted Dump (fadump) on Power
is that it needs a large amount of memory to be reserved. On large
systems with TeraBytes of memory, this reservation can be quite
significant.
In some cases, fadump fails if the memory reserved is insufficient, or
if the reserved memory was DLPAR hot-removed.
In the normal case, post reboot, the preserved memory is filtered to
extract only relevant areas of interest using the makedumpfile tool.
While the tool provides flexibility to determine what needs to be part
of the dump and what memory to filter out, all supported distributions
default this to "Capture only kernel data and nothing else".
We take advantage of this default and the Linux kernel's Contiguous
Memory Allocator (CMA) to fundamentally change the memory reservation
model for fadump. Fadump can now only capture kernel memory.
Instead of setting aside a significant chunk of memory nobody can use,
this patch uses CMA instead, to reserve a significant chunk of memory
that the kernel is prevented from using (due to MIGRATE_CMA), but
applications are free to use it.
Essentially, on a P9 LPAR with 2 cores, 8GB RAM and current upstream:
[root@zzxx-yy10 ~]# free -m
total used free shared buff/cache available
Mem: 7557 193 6822 12 541 6725
Swap: 4095 0 4095
With this patch:
[root@zzxx-yy10 ~]# free -m
total used free shared buff/cache available
Mem: 8133 194 7464 12 475 7338
Swap: 4095 0 4095
Changes made here are completely transparent to how fadump has
traditionally worked.
Thanks to Aneesh Kumar and Anshuman Khandual for helping us understand
CMA and its usage.
TODO:
- Handle case where CMA reservation spans nodes.
Signed-off-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/fadump.h | 3 +
arch/powerpc/kernel/fadump.c | 179 +++++++++++++++++++++++++++++++------
2 files changed, 154 insertions(+), 28 deletions(-)
diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
index 5a23010af600..776cba0baec4 100644
--- a/arch/powerpc/include/asm/fadump.h
+++ b/arch/powerpc/include/asm/fadump.h
@@ -119,6 +119,7 @@ struct fadump_mem_struct {
struct fadump_section cpu_state_data;
struct fadump_section hpte_region;
struct fadump_section rmr_region;
+ struct fadump_section metadata_region;
};
/* Firmware-assisted dump configuration details. */
@@ -141,6 +142,8 @@ struct fw_dump {
unsigned long fadump_supported:1;
unsigned long dump_active:1;
unsigned long dump_registered:1;
+ /* flag to indicate fadump metadata area is cma allocated */
+ unsigned long cma_alloc:1;
};
/*
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 3c2c2688918f..2098583c935f 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -34,6 +34,7 @@
#include <linux/crash_dump.h>
#include <linux/kobject.h>
#include <linux/sysfs.h>
+#include <linux/cma.h>
#include <asm/debugfs.h>
#include <asm/page.h>
@@ -45,11 +46,57 @@
static struct fw_dump fw_dump;
static struct fadump_mem_struct fdm;
static const struct fadump_mem_struct *fdm_active;
+static struct cma *fadump_cma;
static DEFINE_MUTEX(fadump_mutex);
struct fad_crash_memory_ranges crash_memory_ranges[INIT_CRASHMEM_RANGES];
int crash_mem_ranges;
+/*
+ * fadump_cma_reserve() - reserve area for fadump memory reservation
+ *
+ * This function reserves memory from early allocator. It should be
+ * called by arch specific code once the memblock allocator
+ * has been activated.
+ */
+int __init fadump_cma_reserve(void)
+{
+ unsigned long long base, size;
+ int rc;
+
+ if (!fw_dump.fadump_enabled)
+ return 0;
+
+ base = fw_dump.reserve_dump_area_start;
+ size = fw_dump.reserve_dump_area_size;
+ pr_debug("Original reserve area base %ld, size %ld\n",
+ (unsigned long)base >> 20,
+ (unsigned long)size >> 20);
+ if (!size)
+ return 0;
+
+ rc = cma_declare_contiguous(base, size, 0, 0, 0, false,
+ "fadump_cma", &fadump_cma);
+ if (rc) {
+ printk(KERN_ERR "fadump: Failed to reserve cma area for "
+ "firmware-assisted dump, %d\n", rc);
+ fw_dump.reserve_dump_area_size = 0;
+ return 0;
+ }
+ /*
+ * So we now have cma area reserved for fadump. base may be different
+ * from what we requested.
+ */
+ fw_dump.reserve_dump_area_start = cma_get_base(fadump_cma);
+ fw_dump.reserve_dump_area_size = cma_get_size(fadump_cma);
+ printk("Reserved %ldMB cma area at %ldMB for firmware-assisted dump "
+ "(System RAM: %ldMB)\n",
+ cma_get_size(fadump_cma) >> 20,
+ (unsigned long)cma_get_base(fadump_cma) >> 20,
+ (unsigned long)(memblock_phys_mem_size() >> 20));
+ return 1;
+}
+
/* Scan the Firmware Assisted dump configuration details. */
int __init early_init_dt_scan_fw_dump(unsigned long node,
const char *uname, int depth, void *data)
@@ -188,17 +235,42 @@ static void fadump_show_config(void)
pr_debug("Boot memory size : %lx\n", fw_dump.boot_memory_size);
}
+static unsigned long get_fadump_metadata_size(void)
+{
+ unsigned long size = 0;
+
+ size += sizeof(struct fadump_crash_info_header);
+ size += sizeof(struct elfhdr); /* ELF core header.*/
+ size += sizeof(struct elf_phdr); /* place holder for cpu notes */
+ /* Program headers for crash memory regions. */
+ size += sizeof(struct elf_phdr) * (memblock_num_regions(memory) + 2);
+
+ size = PAGE_ALIGN(size);
+ pr_debug("fadump Metadata size is %ld\n", size);
+ return size;
+}
+
static unsigned long init_fadump_mem_struct(struct fadump_mem_struct *fdm,
unsigned long addr)
{
+ uint16_t num_sections = 0;
+ unsigned long metadata_base = 0;
+ unsigned long metadata_size = 0;
+
if (!fdm)
return 0;
+ if (fw_dump.cma_alloc) {
+ /* Skip the fadump metadata area. */
+ metadata_base = addr;
+ metadata_size = get_fadump_metadata_size();
+ addr += metadata_size;
+ }
+
memset(fdm, 0, sizeof(struct fadump_mem_struct));
addr = addr & PAGE_MASK;
fdm->header.dump_format_version = cpu_to_be32(0x00000001);
- fdm->header.dump_num_sections = cpu_to_be16(3);
fdm->header.dump_status_flag = 0;
fdm->header.offset_first_dump_section =
cpu_to_be32((u32)offsetof(struct fadump_mem_struct, cpu_state_data));
@@ -222,6 +294,7 @@ static unsigned long init_fadump_mem_struct(struct fadump_mem_struct *fdm,
fdm->cpu_state_data.source_address = 0;
fdm->cpu_state_data.source_len = cpu_to_be64(fw_dump.cpu_state_data_size);
fdm->cpu_state_data.destination_address = cpu_to_be64(addr);
+ num_sections++;
addr += fw_dump.cpu_state_data_size;
/* hpte region section */
@@ -230,6 +303,7 @@ static unsigned long init_fadump_mem_struct(struct fadump_mem_struct *fdm,
fdm->hpte_region.source_address = 0;
fdm->hpte_region.source_len = cpu_to_be64(fw_dump.hpte_region_size);
fdm->hpte_region.destination_address = cpu_to_be64(addr);
+ num_sections++;
addr += fw_dump.hpte_region_size;
/* RMA region section */
@@ -238,8 +312,28 @@ static unsigned long init_fadump_mem_struct(struct fadump_mem_struct *fdm,
fdm->rmr_region.source_address = cpu_to_be64(RMA_START);
fdm->rmr_region.source_len = cpu_to_be64(fw_dump.boot_memory_size);
fdm->rmr_region.destination_address = cpu_to_be64(addr);
+ num_sections++;
addr += fw_dump.boot_memory_size;
+ if (!fw_dump.cma_alloc)
+ goto out;
+
+ /*
+ * fadump metadata section.
+ * Add an entry with source len a zero. We are only intereseted in
+ * source address which will help us to detect the location of
+ * metadata area where faump header and elf core header is placed.
+ */
+ fdm->metadata_region.request_flag = cpu_to_be32(FADUMP_REQUEST_FLAG);
+ fdm->metadata_region.source_data_type =
+ cpu_to_be16(FADUMP_REAL_MODE_REGION);
+ fdm->metadata_region.source_address = cpu_to_be64(metadata_base);
+ fdm->metadata_region.source_len = 0;
+ fdm->metadata_region.destination_address = cpu_to_be64(addr);
+ num_sections++;
+
+out:
+ fdm->header.dump_num_sections = cpu_to_be16(num_sections);
return addr;
}
@@ -325,16 +419,21 @@ static unsigned long get_fadump_area_size(void)
size += fw_dump.cpu_state_data_size;
size += fw_dump.hpte_region_size;
size += fw_dump.boot_memory_size;
- size += sizeof(struct fadump_crash_info_header);
- size += sizeof(struct elfhdr); /* ELF core header.*/
- size += sizeof(struct elf_phdr); /* place holder for cpu notes */
- /* Program headers for crash memory regions. */
- size += sizeof(struct elf_phdr) * (memblock_num_regions(memory) + 2);
-
+ size += get_fadump_metadata_size();
size = PAGE_ALIGN(size);
return size;
}
+static inline unsigned long get_fadump_metadata_base(
+ const struct fadump_mem_struct *fdm_active)
+{
+ if (be16_to_cpu(fdm_active->header.dump_num_sections) == 4)
+ return be64_to_cpu(fdm_active->metadata_region.source_address);
+
+ return (be64_to_cpu(fdm_active->rmr_region.destination_address) +
+ be64_to_cpu(fdm_active->rmr_region.source_len));
+}
+
int __init fadump_reserve_mem(void)
{
unsigned long base, size, memory_boundary;
@@ -395,11 +494,12 @@ int __init fadump_reserve_mem(void)
(unsigned long)(size >> 20),
(unsigned long)(base >> 20));
- fw_dump.fadumphdr_addr =
- be64_to_cpu(fdm_active->rmr_region.destination_address) +
- be64_to_cpu(fdm_active->rmr_region.source_len);
- pr_debug("fadumphdr_addr = %p\n",
- (void *) fw_dump.fadumphdr_addr);
+ pr_info("Number of kernel Dump sections: %d\n",
+ be16_to_cpu(fdm_active->header.dump_num_sections));
+ fw_dump.fadumphdr_addr = get_fadump_metadata_base(fdm_active);
+ pr_debug("fadumphdr_addr = %pa\n", &fw_dump.fadumphdr_addr);
+ fw_dump.reserve_dump_area_start = base;
+ fw_dump.reserve_dump_area_size = size;
} else {
size = get_fadump_area_size();
@@ -416,21 +516,10 @@ int __init fadump_reserve_mem(void)
!memblock_is_region_reserved(base, size))
break;
}
- if ((base > (memory_boundary - size)) ||
- memblock_reserve(base, size)) {
- pr_err("Failed to reserve memory\n");
- return 0;
- }
-
- pr_info("Reserved %ldMB of memory at %ldMB for firmware-"
- "assisted dump (System RAM: %ldMB)\n",
- (unsigned long)(size >> 20),
- (unsigned long)(base >> 20),
- (unsigned long)(memblock_phys_mem_size() >> 20));
+ fw_dump.reserve_dump_area_start = base;
+ fw_dump.reserve_dump_area_size = size;
+ return fadump_cma_reserve();
}
-
- fw_dump.reserve_dump_area_start = base;
- fw_dump.reserve_dump_area_size = size;
return 1;
}
@@ -1068,6 +1157,30 @@ static unsigned long init_fadump_header(unsigned long addr)
return addr;
}
+static unsigned long allocate_metadata_area(void)
+{
+ int nr_pages;
+ unsigned long size;
+ struct page *page = NULL;
+
+ /* If fadump_cma->count == 0 means cma activation has failed. */
+ if (!fadump_cma || !cma_get_size(fadump_cma))
+ return 0;
+
+ size = get_fadump_metadata_size();
+ nr_pages = ALIGN(size, PAGE_SIZE) >> PAGE_SHIFT;
+ printk("Fadump metadata size = %ld (nr_pages = %d)\n", size, nr_pages);
+
+ page = cma_alloc(fadump_cma, nr_pages, 0, GFP_KERNEL);
+ if (page) {
+ pr_debug("Allocated fadump metadata area at %ldMB (cma)\n",
+ (unsigned long)page_to_phys(page) >> 20);
+ fw_dump.cma_alloc = 1;
+ return page_to_phys(page);
+ }
+ return 0;
+}
+
static int register_fadump(void)
{
unsigned long addr;
@@ -1082,7 +1195,12 @@ static int register_fadump(void)
fadump_setup_crash_memory_ranges();
- addr = be64_to_cpu(fdm.rmr_region.destination_address) + be64_to_cpu(fdm.rmr_region.source_len);
+ if (fw_dump.cma_alloc)
+ addr = fw_dump.reserve_dump_area_start;
+ else
+ addr = be64_to_cpu(fdm.rmr_region.destination_address)
+ + be64_to_cpu(fdm.rmr_region.source_len);
+
/* Initialize fadump crash info header. */
addr = init_fadump_header(addr);
vaddr = __va(addr);
@@ -1490,8 +1608,13 @@ int __init setup_fadump(void)
fadump_invalidate_release_mem();
}
/* Initialize the kernel dump memory structure for FAD registration. */
- else if (fw_dump.reserve_dump_area_size)
+ else if (fw_dump.reserve_dump_area_size) {
+ /* By this time cma area has been activated. Allocate memory
+ * for metadata from cma region.
+ */
+ allocate_metadata_area();
init_fadump_mem_struct(&fdm, fw_dump.reserve_dump_area_start);
+ }
fadump_init_files();
return 1;
^ permalink raw reply related
* Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation
From: Oliver @ 2018-03-29 3:10 UTC (permalink / raw)
To: Rob Herring
Cc: linux-nvdimm@lists.01.org, Device Tree, linuxppc-dev,
Dan Williams
In-Reply-To: <CAL_Jsq+jC2mAXbKh9c8dbLXZmWbD-sFemrk+8p-oGMkXqSMfnQ@mail.gmail.com>
On Thu, Mar 29, 2018 at 4:06 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Mar 27, 2018 at 9:53 AM, Oliver <oohall@gmail.com> wrote:
>> On Tue, Mar 27, 2018 at 9:24 AM, Rob Herring <robh@kernel.org> wrote:
>>> On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
>>>> Add device-tree binding documentation for the nvdimm region driver.
>>>>
>>>> Cc: devicetree@vger.kernel.org
>>>> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
>>>> ---
>>>> .../devicetree/bindings/nvdimm/nvdimm-region.txt | 45 ++++++++++++++++++++++
>>>> 1 file changed, 45 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>> new file mode 100644
>>>> index 000000000000..02091117ff16
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>> @@ -0,0 +1,45 @@
>>>> +Device-tree bindings for NVDIMM memory regions
>>>> +-----------------------------------------------------
>>>> +
>>>> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
>>>
>>> Are DIMMs always going to be the only form factor for NV memory?
>>>
>>> And if you have multiple DIMMs, does each DT node correspond to a DIMM?
>>
>> A nvdimm-region might correspond to a single NVDIMM, a set of
>> interleaved NVDIMMs, or it might just be a chunk of normal memory that
>> you want treated as a NVDIMM for some reason. The last case is useful
>> for provisioning install media on servers since it allows you do
>> download a DVD image, turn it into an nvdimm-region, and kexec into
>> the installer which can use it as a root disk. That may seem a little
>> esoteric, but it's handy and we're using a full linux environment for
>> our boot loader so it's easy to make use of.
>
> I'm really just asking if we should drop the "dimm" name because it is
> not always a DIMM. Maybe pmem instead? I don't know, naming is
> hard(TM).
pmem is probably a better name. I'll fix that up.
>>> If not, then what if we want/need to provide power control to a DIMM?
>>
>> That would require a DIMM (and probably memory controller) specific
>> driver. I've deliberately left out how regions are mapped back to
>> DIMMs from the binding since it's not really clear to me how that
>> should work. A phandle array pointing to each DIMM device (which could
>> be anything) would do the trick, but I've found that a bit awkward to
>> plumb into the model that libnvdimm expects.
>>
>>>> +that retains its contents across power cycles. In more practical terms, they
>>>> +are kind of storage device where the contents can be accessed by the CPU
>>>> +directly, rather than indirectly via a storage controller or similar. The an
>>>> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
>>>> +device.
>>>> +
>>>> +Bindings for the region nodes:
>>>> +-----------------------------
>>>> +
>>>> +Required properties:
>>>> + - compatible = "nvdimm-region"
>>>> +
>>>> + - reg = <base, size>;
>>>> + The system physical address range of this nvdimm region.
>>>> +
>>>> +Optional properties:
>>>> + - Any relevant NUMA assocativity properties for the target platform.
>>>> + - A "volatile" property indicating that this region is actually in
>>>> + normal DRAM and does not require cache flushes after each write.
>>>> +
>>>> +A complete example:
>>>> +--------------------
>>>> +
>>>> +/ {
>>>> + #size-cells = <2>;
>>>> + #address-cells = <2>;
>>>> +
>>>> + platform {
>>>
>>> Perhaps we need a more well defined node here. Like we have 'memory' for
>>> memory nodes.
>>
>> I think treating it as a platform device is fine. Memory nodes are
>
> Platform device is a Linux term...
>
>> special since the OS needs to know where it can allocate early in boot
>> and I don't see non-volatile memory as being similarly significant.
>> Fundamentally an NVDIMM is just a memory mapped storage device so we
>> should be able to defer looking at them until later in boot.
>
> It's not clear if 'platform' is just an example or random name or what
> the node is required to be called. In the latter case, we should be
> much more specific because 'platform' could be anything. In the former
> case, then we have no way to find or validate the node because the
> name could be anything and there's no compatible property either.
Sorry, the platform node is just there as an example. I'll remove it.
> "region" is pretty generic too.
It is, but I didn't see a compelling reason to call it something else.
>> That said you might have problems with XIP kernels and what not. I
>> think that problem is better solved through other means though.
>>
>>>> + region@5000 {
>>>> + compatible = "nvdimm-region;
>>>> + reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>>>> +
>>>> + };
>>>> +
>>>> + region@6000 {
>>>> + compatible = "nvdimm-region";
>>>> + reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>
> Thinking about this some more, the 2 levels of nodes is pointless.
> Just follow memory nodes structure.
>
> nv-memory@100000000 {
> compatible = "nvdimm-region";
> reg = <0x00000001 0x00000000 0x00000000 0x40000000>;
> };
>
> nv-memory@200000000 {
> compatible = "nvdimm-region";
> reg = <0x00000002 0x00000000 0x00000000 0x40000000>;
> };
>
> or:
>
> nv-memory@100000000 {
> compatible = "nvdimm-region";
> reg = <0x00000001 0x00000000 0x00000000 0x40000000>
> <0x00000002 0x00000000 0x00000000 0x40000000>;
> };
>
> Both forms should be allowed.
In the example you need two separate nodes since one has the
"volatile" property to indicate it's backed by normal memory while the
other doesn't. That detail is important since the OS can skip doing
cache flushes when writing to a region that it knows is volatile.
Anyway, the usefulness of having multiple ranges in the reg is a bit
dubious since you should never see dis-contiguous ranges of memory
backed by the same devices. Keep in mind that this binding here is
deliberately skeletal and leaves out the parts required to map the
region to the backing devices, once that is added there's not going to
be a whole lot of room for coalescing nodes. That said, I'll add
support for it anyway since it might be nice to have for hand-written
DTs (ours are mostly generated by FW).
Thanks,
Oliver
^ permalink raw reply
* Re: [PATCH v8 22/24] mm: Speculative page fault handler return VMA
From: Ganesh Mahendran @ 2018-03-29 3:06 UTC (permalink / raw)
To: Laurent Dufour
Cc: Paul McKenney, Peter Zijlstra, Andrew Morton, kirill, ak,
Michal Hocko, dave, jack, Matthew Wilcox, benh, mpe, paulus,
Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, Sergey Senozhatsky, Daniel Jordan, linux-kernel,
Linux-MM, haren, khandual, npiggin, Balbir Singh, Tim Chen,
linuxppc-dev, x86
In-Reply-To: <CADAEsF9RLY7Cf=xhW2zcM_a94OgpGFNEqZnkp1T-poP-wBT4Nw@mail.gmail.com>
2018-03-29 10:26 GMT+08:00 Ganesh Mahendran <opensource.ganesh@gmail.com>:
> Hi, Laurent
>
> 2018-02-16 23:25 GMT+08:00 Laurent Dufour <ldufour@linux.vnet.ibm.com>:
>> When the speculative page fault handler is returning VM_RETRY, there is a
>> chance that VMA fetched without grabbing the mmap_sem can be reused by the
>> legacy page fault handler. By reusing it, we avoid calling find_vma()
>> again. To achieve, that we must ensure that the VMA structure will not be
>> freed in our back. This is done by getting the reference on it (get_vma())
>> and by assuming that the caller will call the new service
>> can_reuse_spf_vma() once it has grabbed the mmap_sem.
>>
>> can_reuse_spf_vma() is first checking that the VMA is still in the RB tree
>> , and then that the VMA's boundaries matched the passed address and release
>> the reference on the VMA so that it can be freed if needed.
>>
>> In the case the VMA is freed, can_reuse_spf_vma() will have returned false
>> as the VMA is no more in the RB tree.
>
> when I applied this patch to arm64, I got a crash:
>
> [ 6.088296] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> [ 6.088307] pgd = ffffff9d67735000
> [ 6.088313] [00000000] *pgd=00000001795e3003,
> *pud=00000001795e3003, *pmd=0000000000000000
> [ 6.088372] ------------[ cut here ]------------
> [ 6.088377] Kernel BUG at ffffff9d64f65960 [verbose debug info unavailable]
> [ 6.088384] Internal error: Oops - BUG: 96000045 [#1] PREEMPT SMP
> [ 6.088389] BUG: Bad rss-counter state mm:ffffffe8f3861040 idx:0 val:90
> [ 6.088393] BUG: Bad rss-counter state mm:ffffffe8f3861040 idx:1 val:58
> [ 6.088398] Modules linked in:
> [ 6.088408] CPU: 1 PID: 621 Comm: qseecomd Not tainted 4.4.78-perf+ #88
> [ 6.088413] Hardware name: Qualcomm Technologies, Inc. SDM 636
> PM660 + PM660L MTP E7S (DT)
> [ 6.088419] task: ffffffe8f6208000 ti: ffffffe872a8c000 task.ti:
> ffffffe872a8c000
> [ 6.088432] PC is at __rb_erase_color+0x108/0x240
> [ 6.088441] LR is at vma_interval_tree_remove+0x244/0x24c
> [ 6.088447] pc : [<ffffff9d64f65960>] lr : [<ffffff9d64d9c2d8>]
> pstate: 604001c5
> [ 6.088451] sp : ffffffe872a8fa50
> [ 6.088455] x29: ffffffe872a8fa50 x28: 0000000000000008
> [ 6.088462] x27: 0000000000000009 x26: 0000000000000000
> [ 6.088470] x25: ffffffe8f458fb80 x24: 000000768ff87000
> [ 6.088477] x23: 0000000000000000 x22: 0000000000000000
> [ 6.088484] x21: ffffff9d64d9be7c x20: ffffffe8f3ff0680
> [ 6.088492] x19: ffffffe8f212e9b0 x18: 0000000000000074
> [ 6.088499] x17: 0000000000000007 x16: 000000000000000e
> [ 6.088507] x15: ffffff9d65c88000 x14: 0000000000000001
> [ 6.088514] x13: 0000000000192d76 x12: 0000000000989680
> [ 6.088521] x11: 00000000001fffff x10: ffffff9d661ded1b
> [ 6.088528] x9 : 0000007691759000 x8 : 0000000007691759
> [ 6.088535] x7 : 0000000000000000 x6 : ffffffe871ebada8
> [ 6.088541] x5 : 00000000000000e1 x4 : ffffffe8f212e958
> [ 6.088548] x3 : 00000000000000e9 x2 : 0000000000000000
> [ 6.088555] x1 : ffffffe8f212f110 x0 : ffffffe8f212e9b1
> [ 6.088564]
> [ 6.088564] PC: 0xffffff9d64f65920:
> [ 6.088568] 5920 f9000002 aa0103e0 aa1603e1 d63f02a0 aa1603e1
> f9400822 f9000662 f9000833
> [ 6.088590] 5940 1400003b f9400a61 f9400020 370002c0 f9400436
> b2400260 f9000a76 f9000433
> [ 6.088610] 5960 f90002c0 f9400260 f9000020 f9000261 f27ef400
> 54000100 f9400802 eb13005f
> [ 6.088630] 5980 54000061 f9000801 14000004 f9000401 14000002
> f9000281 aa1303e0 d63f02a0
> [ 6.088652]
> [ 6.088652] LR: 0xffffff9d64d9c298:
> [ 6.088656] c298 f9403083 b4000083 f9400c63 eb03005f 9a832042
> f9403883 eb02007f 540000a0
> [ 6.088676] c2b8 f9003882 f9402c82 927ef442 b5fffd22 b4000080
> f0ffffe2 9139f042 94072561
> [ 6.088695] c2d8 a8c17bfd d65f03c0 a9bf7bfd 910003fd f9400003
> d2800000 b40000e3 f9400c65
> [ 6.088715] c2f8 d1016063 eb0100bf 54000063 aa0303e0 97fffef2
> a8c17bfd d65f03c0 a9bf7bfd
> [ 6.088735]
> [ 6.088735] SP: 0xffffffe872a8fa10:
> [ 6.088740] fa10 64d9c2d8 ffffff9d 72a8fa50 ffffffe8 64f65960
> ffffff9d 604001c5 00000000
> [ 6.088759] fa30 71d67d70 ffffffe8 71c281e8 ffffffe8 00000000
> 00000080 64daa90c ffffff9d
> [ 6.088779] fa50 72a8fa90 ffffffe8 64d9c2d8 ffffff9d 71ebada8
> ffffffe8 f3ff0678 ffffffe8
> [ 6.088799] fa70 72a8fb80 ffffffe8 00000000 00000000 00000000
> 00000000 00000001 00000000
> [ 6.088818]
> [ 6.088823] Process qseecomd (pid: 621, stack limit = 0xffffffe872a8c028)
> [ 6.088828] Call trace:
> [ 6.088834] Exception stack(0xffffffe872a8f860 to 0xffffffe872a8f990)
> [ 6.088841] f860: ffffffe8f212e9b0 0000008000000000
> 0000000082b37000 ffffff9d64f65960
> [ 6.088848] f880: 00000000604001c5 ffffff9d672c8680
> ffffff9d672c9c00 ffffff9d672d3ab7
> [ 6.088855] f8a0: ffffffe872a8f8f0 ffffff9d64db9bfc
> 0000000000000000 ffffffe8f9402c00
> [ 6.088861] f8c0: ffffffe872a8c000 0000000000000000
> ffffffe872a8f920 ffffff9d64db9bfc
> [ 6.088867] f8e0: 0000000000000000 ffffffe8f9402b00
> ffffffe872a8fa10 ffffff9d64dba568
> [ 6.088874] f900: ffffffbe61c759c0 ffffffe871d67d70
> ffffffe8f9402c00 1de56fb006cba396
> [ 6.088881] f920: ffffffe8f212e9b1 ffffffe8f212f110
> 0000000000000000 00000000000000e9
> [ 6.088888] f940: ffffffe8f212e958 00000000000000e1
> ffffffe871ebada8 0000000000000000
> [ 6.088895] f960: 0000000007691759 0000007691759000
> ffffff9d661ded1b 00000000001fffff
> [ 6.088901] f980: 0000000000989680 0000000000192d76
> [ 6.088908] [<ffffff9d64f65960>] __rb_erase_color+0x108/0x240
> [ 6.088915] [<ffffff9d64d9c2d8>] vma_interval_tree_remove+0x244/0x24c
> [ 6.088924] [<ffffff9d64da4b5c>] __remove_shared_vm_struct+0x74/0x88
> [ 6.088930] [<ffffff9d64da52b8>] unlink_file_vma+0x40/0x54
> [ 6.088937] [<ffffff9d64d9f928>] free_pgtables+0xb8/0xfc
> [ 6.088945] [<ffffff9d64da6b84>] exit_mmap+0x78/0x13c
> [ 6.088953] [<ffffff9d64c9f5f4>] mmput+0x40/0xe8
> [ 6.088961] [<ffffff9d64ca5af0>] do_exit+0x3ac/0x8d8
> [ 6.088966] [<ffffff9d64ca6090>] do_group_exit+0x44/0x9c
> [ 6.088974] [<ffffff9d64cb10d0>] get_signal+0x4e8/0x524
> [ 6.088981] [<ffffff9d64c87ea0>] do_signal+0xac/0x93c
> [ 6.088989] [<ffffff9d64c88a0c>] do_notify_resume+0x18/0x58
> [ 6.088995] [<ffffff9d64c83038>] work_pending+0x10/0x14
> [ 6.089003] Code: f9400436 b2400260 f9000a76 f9000433 (f90002c0)
> [ 6.089009] ---[ end trace 224ce5f97841b6a5 ]---
> [ 6.110819] Kernel panic - not syncing: Fatal exception
>
> Thanks.
Fixed by below patch:
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index f6838c0..9c61b0e 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -240,18 +240,18 @@
static int __do_page_fault(struct mm_struct *mm, unsigned long addr,
unsigned int mm_flags, unsigned long vm_flags,
- struct task_struct *tsk, struct vm_area_struct *spf_vma)
+ struct task_struct *tsk, struct vm_area_struct **spf_vma)
{
struct vm_area_struct *vma;
int fault;
#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
- if (spf_vma) {
- if (can_reuse_spf_vma(spf_vma, addr))
- vma = spf_vma;
+ if (*spf_vma) {
+ if (can_reuse_spf_vma(*spf_vma, addr))
+ vma = *spf_vma;
else
vma = find_vma(mm, addr);
- spf_vma = NULL;
+ *spf_vma = NULL;
} else
#endif
vma = find_vma(mm, addr);
@@ -393,7 +389,7 @@
#endif
}
- fault = __do_page_fault(mm, addr, mm_flags, vm_flags, tsk, spf_vma);
+ fault = __do_page_fault(mm, addr, mm_flags, vm_flags, tsk, &spf_vma);
/*
* If we need to retry but a fatal signal is pending, handle the
@@ -480,6 +476,11 @@
return 0;
no_context:
+ if (spf_vma) {
+ put_vma(spf_vma);
+ spf_vma = NULL;
+ }
+
__do_kernel_fault(mm, addr, esr, regs);
return 0;
}
>
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>> include/linux/mm.h | 5 +-
>> mm/memory.c | 136 +++++++++++++++++++++++++++++++++--------------------
>> 2 files changed, 88 insertions(+), 53 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index c383a4e2ceb3..0cd31a37bb3d 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1355,7 +1355,10 @@ extern int handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>> unsigned int flags);
>> #ifdef CONFIG_SPECULATIVE_PAGE_FAULT
>> extern int handle_speculative_fault(struct mm_struct *mm,
>> - unsigned long address, unsigned int flags);
>> + unsigned long address, unsigned int flags,
>> + struct vm_area_struct **vma);
>> +extern bool can_reuse_spf_vma(struct vm_area_struct *vma,
>> + unsigned long address);
>> #endif /* CONFIG_SPECULATIVE_PAGE_FAULT */
>> extern int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
>> unsigned long address, unsigned int fault_flags,
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 2ef686405154..1f5ce5ff79af 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4307,13 +4307,22 @@ static int __handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>> /* This is required by vm_normal_page() */
>> #error "Speculative page fault handler requires __HAVE_ARCH_PTE_SPECIAL"
>> #endif
>> -
>> /*
>> * vm_normal_page() adds some processing which should be done while
>> * hodling the mmap_sem.
>> */
>> +
>> +/*
>> + * Tries to handle the page fault in a speculative way, without grabbing the
>> + * mmap_sem.
>> + * When VM_FAULT_RETRY is returned, the vma pointer is valid and this vma must
>> + * be checked later when the mmap_sem has been grabbed by calling
>> + * can_reuse_spf_vma().
>> + * This is needed as the returned vma is kept in memory until the call to
>> + * can_reuse_spf_vma() is made.
>> + */
>> int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> - unsigned int flags)
>> + unsigned int flags, struct vm_area_struct **vma)
>> {
>> struct vm_fault vmf = {
>> .address = address,
>> @@ -4322,7 +4331,6 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> p4d_t *p4d, p4dval;
>> pud_t pudval;
>> int seq, ret = VM_FAULT_RETRY;
>> - struct vm_area_struct *vma;
>> #ifdef CONFIG_NUMA
>> struct mempolicy *pol;
>> #endif
>> @@ -4331,14 +4339,16 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> flags &= ~(FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_KILLABLE);
>> flags |= FAULT_FLAG_SPECULATIVE;
>>
>> - vma = get_vma(mm, address);
>> - if (!vma)
>> + *vma = get_vma(mm, address);
>> + if (!*vma)
>> return ret;
>> + vmf.vma = *vma;
>>
>> - seq = raw_read_seqcount(&vma->vm_sequence); /* rmb <-> seqlock,vma_rb_erase() */
>> + /* rmb <-> seqlock,vma_rb_erase() */
>> + seq = raw_read_seqcount(&vmf.vma->vm_sequence);
>> if (seq & 1) {
>> - trace_spf_vma_changed(_RET_IP_, vma, address);
>> - goto out_put;
>> + trace_spf_vma_changed(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> /*
>> @@ -4346,9 +4356,9 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> * with the VMA.
>> * This include huge page from hugetlbfs.
>> */
>> - if (vma->vm_ops) {
>> - trace_spf_vma_notsup(_RET_IP_, vma, address);
>> - goto out_put;
>> + if (vmf.vma->vm_ops) {
>> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> /*
>> @@ -4356,18 +4366,18 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> * because vm_next and vm_prev must be safe. This can't be guaranteed
>> * in the speculative path.
>> */
>> - if (unlikely(!vma->anon_vma)) {
>> - trace_spf_vma_notsup(_RET_IP_, vma, address);
>> - goto out_put;
>> + if (unlikely(!vmf.vma->anon_vma)) {
>> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> - vmf.vma_flags = READ_ONCE(vma->vm_flags);
>> - vmf.vma_page_prot = READ_ONCE(vma->vm_page_prot);
>> + vmf.vma_flags = READ_ONCE(vmf.vma->vm_flags);
>> + vmf.vma_page_prot = READ_ONCE(vmf.vma->vm_page_prot);
>>
>> /* Can't call userland page fault handler in the speculative path */
>> if (unlikely(vmf.vma_flags & VM_UFFD_MISSING)) {
>> - trace_spf_vma_notsup(_RET_IP_, vma, address);
>> - goto out_put;
>> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> if (vmf.vma_flags & VM_GROWSDOWN || vmf.vma_flags & VM_GROWSUP) {
>> @@ -4376,48 +4386,39 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> * boundaries but we want to trace it as not supported instead
>> * of changed.
>> */
>> - trace_spf_vma_notsup(_RET_IP_, vma, address);
>> - goto out_put;
>> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> - if (address < READ_ONCE(vma->vm_start)
>> - || READ_ONCE(vma->vm_end) <= address) {
>> - trace_spf_vma_changed(_RET_IP_, vma, address);
>> - goto out_put;
>> + if (address < READ_ONCE(vmf.vma->vm_start)
>> + || READ_ONCE(vmf.vma->vm_end) <= address) {
>> + trace_spf_vma_changed(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> - if (!arch_vma_access_permitted(vma, flags & FAULT_FLAG_WRITE,
>> + if (!arch_vma_access_permitted(vmf.vma, flags & FAULT_FLAG_WRITE,
>> flags & FAULT_FLAG_INSTRUCTION,
>> - flags & FAULT_FLAG_REMOTE)) {
>> - trace_spf_vma_access(_RET_IP_, vma, address);
>> - ret = VM_FAULT_SIGSEGV;
>> - goto out_put;
>> - }
>> + flags & FAULT_FLAG_REMOTE))
>> + goto out_segv;
>>
>> /* This is one is required to check that the VMA has write access set */
>> if (flags & FAULT_FLAG_WRITE) {
>> - if (unlikely(!(vmf.vma_flags & VM_WRITE))) {
>> - trace_spf_vma_access(_RET_IP_, vma, address);
>> - ret = VM_FAULT_SIGSEGV;
>> - goto out_put;
>> - }
>> - } else if (unlikely(!(vmf.vma_flags & (VM_READ|VM_EXEC|VM_WRITE)))) {
>> - trace_spf_vma_access(_RET_IP_, vma, address);
>> - ret = VM_FAULT_SIGSEGV;
>> - goto out_put;
>> - }
>> + if (unlikely(!(vmf.vma_flags & VM_WRITE)))
>> + goto out_segv;
>> + } else if (unlikely(!(vmf.vma_flags & (VM_READ|VM_EXEC|VM_WRITE))))
>> + goto out_segv;
>>
>> #ifdef CONFIG_NUMA
>> /*
>> * MPOL_INTERLEAVE implies additional check in mpol_misplaced() which
>> * are not compatible with the speculative page fault processing.
>> */
>> - pol = __get_vma_policy(vma, address);
>> + pol = __get_vma_policy(vmf.vma, address);
>> if (!pol)
>> pol = get_task_policy(current);
>> if (pol && pol->mode == MPOL_INTERLEAVE) {
>> - trace_spf_vma_notsup(_RET_IP_, vma, address);
>> - goto out_put;
>> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>> #endif
>>
>> @@ -4479,9 +4480,8 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> vmf.pte = NULL;
>> }
>>
>> - vmf.vma = vma;
>> - vmf.pgoff = linear_page_index(vma, address);
>> - vmf.gfp_mask = __get_fault_gfp_mask(vma);
>> + vmf.pgoff = linear_page_index(vmf.vma, address);
>> + vmf.gfp_mask = __get_fault_gfp_mask(vmf.vma);
>> vmf.sequence = seq;
>> vmf.flags = flags;
>>
>> @@ -4491,16 +4491,22 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> * We need to re-validate the VMA after checking the bounds, otherwise
>> * we might have a false positive on the bounds.
>> */
>> - if (read_seqcount_retry(&vma->vm_sequence, seq)) {
>> - trace_spf_vma_changed(_RET_IP_, vma, address);
>> - goto out_put;
>> + if (read_seqcount_retry(&vmf.vma->vm_sequence, seq)) {
>> + trace_spf_vma_changed(_RET_IP_, vmf.vma, address);
>> + return ret;
>> }
>>
>> mem_cgroup_oom_enable();
>> ret = handle_pte_fault(&vmf);
>> mem_cgroup_oom_disable();
>>
>> - put_vma(vma);
>> + /*
>> + * If there is no need to retry, don't return the vma to the caller.
>> + */
>> + if (!(ret & VM_FAULT_RETRY)) {
>> + put_vma(vmf.vma);
>> + *vma = NULL;
>> + }
>>
>> /*
>> * The task may have entered a memcg OOM situation but
>> @@ -4513,9 +4519,35 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
>> return ret;
>>
>> out_walk:
>> - trace_spf_vma_notsup(_RET_IP_, vma, address);
>> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
>> local_irq_enable();
>> -out_put:
>> + return ret;
>> +
>> +out_segv:
>> + trace_spf_vma_access(_RET_IP_, vmf.vma, address);
>> + /*
>> + * We don't return VM_FAULT_RETRY so the caller is not expected to
>> + * retrieve the fetched VMA.
>> + */
>> + put_vma(vmf.vma);
>> + *vma = NULL;
>> + return VM_FAULT_SIGSEGV;
>> +}
>> +
>> +/*
>> + * This is used to know if the vma fetch in the speculative page fault handler
>> + * is still valid when trying the regular fault path while holding the
>> + * mmap_sem.
>> + * The call to put_vma(vma) must be made after checking the vma's fields, as
>> + * the vma may be freed by put_vma(). In such a case it is expected that false
>> + * is returned.
>> + */
>> +bool can_reuse_spf_vma(struct vm_area_struct *vma, unsigned long address)
>> +{
>> + bool ret;
>> +
>> + ret = !RB_EMPTY_NODE(&vma->vm_rb) &&
>> + vma->vm_start <= address && address < vma->vm_end;
>> put_vma(vma);
>> return ret;
>> }
>> --
>> 2.7.4
>>
^ permalink raw reply related
* Re: [PATCH v8 22/24] mm: Speculative page fault handler return VMA
From: Ganesh Mahendran @ 2018-03-29 2:26 UTC (permalink / raw)
To: Laurent Dufour
Cc: Paul McKenney, Peter Zijlstra, Andrew Morton, kirill, ak,
Michal Hocko, dave, jack, Matthew Wilcox, benh, mpe, paulus,
Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, Sergey Senozhatsky, Daniel Jordan, linux-kernel,
Linux-MM, haren, khandual, npiggin, Balbir Singh, Tim Chen,
linuxppc-dev, x86
In-Reply-To: <1518794738-4186-23-git-send-email-ldufour@linux.vnet.ibm.com>
Hi, Laurent
2018-02-16 23:25 GMT+08:00 Laurent Dufour <ldufour@linux.vnet.ibm.com>:
> When the speculative page fault handler is returning VM_RETRY, there is a
> chance that VMA fetched without grabbing the mmap_sem can be reused by the
> legacy page fault handler. By reusing it, we avoid calling find_vma()
> again. To achieve, that we must ensure that the VMA structure will not be
> freed in our back. This is done by getting the reference on it (get_vma())
> and by assuming that the caller will call the new service
> can_reuse_spf_vma() once it has grabbed the mmap_sem.
>
> can_reuse_spf_vma() is first checking that the VMA is still in the RB tree
> , and then that the VMA's boundaries matched the passed address and release
> the reference on the VMA so that it can be freed if needed.
>
> In the case the VMA is freed, can_reuse_spf_vma() will have returned false
> as the VMA is no more in the RB tree.
when I applied this patch to arm64, I got a crash:
[ 6.088296] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[ 6.088307] pgd = ffffff9d67735000
[ 6.088313] [00000000] *pgd=00000001795e3003,
*pud=00000001795e3003, *pmd=0000000000000000
[ 6.088372] ------------[ cut here ]------------
[ 6.088377] Kernel BUG at ffffff9d64f65960 [verbose debug info unavailable]
[ 6.088384] Internal error: Oops - BUG: 96000045 [#1] PREEMPT SMP
[ 6.088389] BUG: Bad rss-counter state mm:ffffffe8f3861040 idx:0 val:90
[ 6.088393] BUG: Bad rss-counter state mm:ffffffe8f3861040 idx:1 val:58
[ 6.088398] Modules linked in:
[ 6.088408] CPU: 1 PID: 621 Comm: qseecomd Not tainted 4.4.78-perf+ #88
[ 6.088413] Hardware name: Qualcomm Technologies, Inc. SDM 636
PM660 + PM660L MTP E7S (DT)
[ 6.088419] task: ffffffe8f6208000 ti: ffffffe872a8c000 task.ti:
ffffffe872a8c000
[ 6.088432] PC is at __rb_erase_color+0x108/0x240
[ 6.088441] LR is at vma_interval_tree_remove+0x244/0x24c
[ 6.088447] pc : [<ffffff9d64f65960>] lr : [<ffffff9d64d9c2d8>]
pstate: 604001c5
[ 6.088451] sp : ffffffe872a8fa50
[ 6.088455] x29: ffffffe872a8fa50 x28: 0000000000000008
[ 6.088462] x27: 0000000000000009 x26: 0000000000000000
[ 6.088470] x25: ffffffe8f458fb80 x24: 000000768ff87000
[ 6.088477] x23: 0000000000000000 x22: 0000000000000000
[ 6.088484] x21: ffffff9d64d9be7c x20: ffffffe8f3ff0680
[ 6.088492] x19: ffffffe8f212e9b0 x18: 0000000000000074
[ 6.088499] x17: 0000000000000007 x16: 000000000000000e
[ 6.088507] x15: ffffff9d65c88000 x14: 0000000000000001
[ 6.088514] x13: 0000000000192d76 x12: 0000000000989680
[ 6.088521] x11: 00000000001fffff x10: ffffff9d661ded1b
[ 6.088528] x9 : 0000007691759000 x8 : 0000000007691759
[ 6.088535] x7 : 0000000000000000 x6 : ffffffe871ebada8
[ 6.088541] x5 : 00000000000000e1 x4 : ffffffe8f212e958
[ 6.088548] x3 : 00000000000000e9 x2 : 0000000000000000
[ 6.088555] x1 : ffffffe8f212f110 x0 : ffffffe8f212e9b1
[ 6.088564]
[ 6.088564] PC: 0xffffff9d64f65920:
[ 6.088568] 5920 f9000002 aa0103e0 aa1603e1 d63f02a0 aa1603e1
f9400822 f9000662 f9000833
[ 6.088590] 5940 1400003b f9400a61 f9400020 370002c0 f9400436
b2400260 f9000a76 f9000433
[ 6.088610] 5960 f90002c0 f9400260 f9000020 f9000261 f27ef400
54000100 f9400802 eb13005f
[ 6.088630] 5980 54000061 f9000801 14000004 f9000401 14000002
f9000281 aa1303e0 d63f02a0
[ 6.088652]
[ 6.088652] LR: 0xffffff9d64d9c298:
[ 6.088656] c298 f9403083 b4000083 f9400c63 eb03005f 9a832042
f9403883 eb02007f 540000a0
[ 6.088676] c2b8 f9003882 f9402c82 927ef442 b5fffd22 b4000080
f0ffffe2 9139f042 94072561
[ 6.088695] c2d8 a8c17bfd d65f03c0 a9bf7bfd 910003fd f9400003
d2800000 b40000e3 f9400c65
[ 6.088715] c2f8 d1016063 eb0100bf 54000063 aa0303e0 97fffef2
a8c17bfd d65f03c0 a9bf7bfd
[ 6.088735]
[ 6.088735] SP: 0xffffffe872a8fa10:
[ 6.088740] fa10 64d9c2d8 ffffff9d 72a8fa50 ffffffe8 64f65960
ffffff9d 604001c5 00000000
[ 6.088759] fa30 71d67d70 ffffffe8 71c281e8 ffffffe8 00000000
00000080 64daa90c ffffff9d
[ 6.088779] fa50 72a8fa90 ffffffe8 64d9c2d8 ffffff9d 71ebada8
ffffffe8 f3ff0678 ffffffe8
[ 6.088799] fa70 72a8fb80 ffffffe8 00000000 00000000 00000000
00000000 00000001 00000000
[ 6.088818]
[ 6.088823] Process qseecomd (pid: 621, stack limit = 0xffffffe872a8c028)
[ 6.088828] Call trace:
[ 6.088834] Exception stack(0xffffffe872a8f860 to 0xffffffe872a8f990)
[ 6.088841] f860: ffffffe8f212e9b0 0000008000000000
0000000082b37000 ffffff9d64f65960
[ 6.088848] f880: 00000000604001c5 ffffff9d672c8680
ffffff9d672c9c00 ffffff9d672d3ab7
[ 6.088855] f8a0: ffffffe872a8f8f0 ffffff9d64db9bfc
0000000000000000 ffffffe8f9402c00
[ 6.088861] f8c0: ffffffe872a8c000 0000000000000000
ffffffe872a8f920 ffffff9d64db9bfc
[ 6.088867] f8e0: 0000000000000000 ffffffe8f9402b00
ffffffe872a8fa10 ffffff9d64dba568
[ 6.088874] f900: ffffffbe61c759c0 ffffffe871d67d70
ffffffe8f9402c00 1de56fb006cba396
[ 6.088881] f920: ffffffe8f212e9b1 ffffffe8f212f110
0000000000000000 00000000000000e9
[ 6.088888] f940: ffffffe8f212e958 00000000000000e1
ffffffe871ebada8 0000000000000000
[ 6.088895] f960: 0000000007691759 0000007691759000
ffffff9d661ded1b 00000000001fffff
[ 6.088901] f980: 0000000000989680 0000000000192d76
[ 6.088908] [<ffffff9d64f65960>] __rb_erase_color+0x108/0x240
[ 6.088915] [<ffffff9d64d9c2d8>] vma_interval_tree_remove+0x244/0x24c
[ 6.088924] [<ffffff9d64da4b5c>] __remove_shared_vm_struct+0x74/0x88
[ 6.088930] [<ffffff9d64da52b8>] unlink_file_vma+0x40/0x54
[ 6.088937] [<ffffff9d64d9f928>] free_pgtables+0xb8/0xfc
[ 6.088945] [<ffffff9d64da6b84>] exit_mmap+0x78/0x13c
[ 6.088953] [<ffffff9d64c9f5f4>] mmput+0x40/0xe8
[ 6.088961] [<ffffff9d64ca5af0>] do_exit+0x3ac/0x8d8
[ 6.088966] [<ffffff9d64ca6090>] do_group_exit+0x44/0x9c
[ 6.088974] [<ffffff9d64cb10d0>] get_signal+0x4e8/0x524
[ 6.088981] [<ffffff9d64c87ea0>] do_signal+0xac/0x93c
[ 6.088989] [<ffffff9d64c88a0c>] do_notify_resume+0x18/0x58
[ 6.088995] [<ffffff9d64c83038>] work_pending+0x10/0x14
[ 6.089003] Code: f9400436 b2400260 f9000a76 f9000433 (f90002c0)
[ 6.089009] ---[ end trace 224ce5f97841b6a5 ]---
[ 6.110819] Kernel panic - not syncing: Fatal exception
Thanks.
>
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> ---
> include/linux/mm.h | 5 +-
> mm/memory.c | 136 +++++++++++++++++++++++++++++++++--------------------
> 2 files changed, 88 insertions(+), 53 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c383a4e2ceb3..0cd31a37bb3d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1355,7 +1355,10 @@ extern int handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> unsigned int flags);
> #ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> extern int handle_speculative_fault(struct mm_struct *mm,
> - unsigned long address, unsigned int flags);
> + unsigned long address, unsigned int flags,
> + struct vm_area_struct **vma);
> +extern bool can_reuse_spf_vma(struct vm_area_struct *vma,
> + unsigned long address);
> #endif /* CONFIG_SPECULATIVE_PAGE_FAULT */
> extern int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
> unsigned long address, unsigned int fault_flags,
> diff --git a/mm/memory.c b/mm/memory.c
> index 2ef686405154..1f5ce5ff79af 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4307,13 +4307,22 @@ static int __handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
> /* This is required by vm_normal_page() */
> #error "Speculative page fault handler requires __HAVE_ARCH_PTE_SPECIAL"
> #endif
> -
> /*
> * vm_normal_page() adds some processing which should be done while
> * hodling the mmap_sem.
> */
> +
> +/*
> + * Tries to handle the page fault in a speculative way, without grabbing the
> + * mmap_sem.
> + * When VM_FAULT_RETRY is returned, the vma pointer is valid and this vma must
> + * be checked later when the mmap_sem has been grabbed by calling
> + * can_reuse_spf_vma().
> + * This is needed as the returned vma is kept in memory until the call to
> + * can_reuse_spf_vma() is made.
> + */
> int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> - unsigned int flags)
> + unsigned int flags, struct vm_area_struct **vma)
> {
> struct vm_fault vmf = {
> .address = address,
> @@ -4322,7 +4331,6 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> p4d_t *p4d, p4dval;
> pud_t pudval;
> int seq, ret = VM_FAULT_RETRY;
> - struct vm_area_struct *vma;
> #ifdef CONFIG_NUMA
> struct mempolicy *pol;
> #endif
> @@ -4331,14 +4339,16 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> flags &= ~(FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_KILLABLE);
> flags |= FAULT_FLAG_SPECULATIVE;
>
> - vma = get_vma(mm, address);
> - if (!vma)
> + *vma = get_vma(mm, address);
> + if (!*vma)
> return ret;
> + vmf.vma = *vma;
>
> - seq = raw_read_seqcount(&vma->vm_sequence); /* rmb <-> seqlock,vma_rb_erase() */
> + /* rmb <-> seqlock,vma_rb_erase() */
> + seq = raw_read_seqcount(&vmf.vma->vm_sequence);
> if (seq & 1) {
> - trace_spf_vma_changed(_RET_IP_, vma, address);
> - goto out_put;
> + trace_spf_vma_changed(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> /*
> @@ -4346,9 +4356,9 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> * with the VMA.
> * This include huge page from hugetlbfs.
> */
> - if (vma->vm_ops) {
> - trace_spf_vma_notsup(_RET_IP_, vma, address);
> - goto out_put;
> + if (vmf.vma->vm_ops) {
> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> /*
> @@ -4356,18 +4366,18 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> * because vm_next and vm_prev must be safe. This can't be guaranteed
> * in the speculative path.
> */
> - if (unlikely(!vma->anon_vma)) {
> - trace_spf_vma_notsup(_RET_IP_, vma, address);
> - goto out_put;
> + if (unlikely(!vmf.vma->anon_vma)) {
> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> - vmf.vma_flags = READ_ONCE(vma->vm_flags);
> - vmf.vma_page_prot = READ_ONCE(vma->vm_page_prot);
> + vmf.vma_flags = READ_ONCE(vmf.vma->vm_flags);
> + vmf.vma_page_prot = READ_ONCE(vmf.vma->vm_page_prot);
>
> /* Can't call userland page fault handler in the speculative path */
> if (unlikely(vmf.vma_flags & VM_UFFD_MISSING)) {
> - trace_spf_vma_notsup(_RET_IP_, vma, address);
> - goto out_put;
> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> if (vmf.vma_flags & VM_GROWSDOWN || vmf.vma_flags & VM_GROWSUP) {
> @@ -4376,48 +4386,39 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> * boundaries but we want to trace it as not supported instead
> * of changed.
> */
> - trace_spf_vma_notsup(_RET_IP_, vma, address);
> - goto out_put;
> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> - if (address < READ_ONCE(vma->vm_start)
> - || READ_ONCE(vma->vm_end) <= address) {
> - trace_spf_vma_changed(_RET_IP_, vma, address);
> - goto out_put;
> + if (address < READ_ONCE(vmf.vma->vm_start)
> + || READ_ONCE(vmf.vma->vm_end) <= address) {
> + trace_spf_vma_changed(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> - if (!arch_vma_access_permitted(vma, flags & FAULT_FLAG_WRITE,
> + if (!arch_vma_access_permitted(vmf.vma, flags & FAULT_FLAG_WRITE,
> flags & FAULT_FLAG_INSTRUCTION,
> - flags & FAULT_FLAG_REMOTE)) {
> - trace_spf_vma_access(_RET_IP_, vma, address);
> - ret = VM_FAULT_SIGSEGV;
> - goto out_put;
> - }
> + flags & FAULT_FLAG_REMOTE))
> + goto out_segv;
>
> /* This is one is required to check that the VMA has write access set */
> if (flags & FAULT_FLAG_WRITE) {
> - if (unlikely(!(vmf.vma_flags & VM_WRITE))) {
> - trace_spf_vma_access(_RET_IP_, vma, address);
> - ret = VM_FAULT_SIGSEGV;
> - goto out_put;
> - }
> - } else if (unlikely(!(vmf.vma_flags & (VM_READ|VM_EXEC|VM_WRITE)))) {
> - trace_spf_vma_access(_RET_IP_, vma, address);
> - ret = VM_FAULT_SIGSEGV;
> - goto out_put;
> - }
> + if (unlikely(!(vmf.vma_flags & VM_WRITE)))
> + goto out_segv;
> + } else if (unlikely(!(vmf.vma_flags & (VM_READ|VM_EXEC|VM_WRITE))))
> + goto out_segv;
>
> #ifdef CONFIG_NUMA
> /*
> * MPOL_INTERLEAVE implies additional check in mpol_misplaced() which
> * are not compatible with the speculative page fault processing.
> */
> - pol = __get_vma_policy(vma, address);
> + pol = __get_vma_policy(vmf.vma, address);
> if (!pol)
> pol = get_task_policy(current);
> if (pol && pol->mode == MPOL_INTERLEAVE) {
> - trace_spf_vma_notsup(_RET_IP_, vma, address);
> - goto out_put;
> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
> + return ret;
> }
> #endif
>
> @@ -4479,9 +4480,8 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> vmf.pte = NULL;
> }
>
> - vmf.vma = vma;
> - vmf.pgoff = linear_page_index(vma, address);
> - vmf.gfp_mask = __get_fault_gfp_mask(vma);
> + vmf.pgoff = linear_page_index(vmf.vma, address);
> + vmf.gfp_mask = __get_fault_gfp_mask(vmf.vma);
> vmf.sequence = seq;
> vmf.flags = flags;
>
> @@ -4491,16 +4491,22 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> * We need to re-validate the VMA after checking the bounds, otherwise
> * we might have a false positive on the bounds.
> */
> - if (read_seqcount_retry(&vma->vm_sequence, seq)) {
> - trace_spf_vma_changed(_RET_IP_, vma, address);
> - goto out_put;
> + if (read_seqcount_retry(&vmf.vma->vm_sequence, seq)) {
> + trace_spf_vma_changed(_RET_IP_, vmf.vma, address);
> + return ret;
> }
>
> mem_cgroup_oom_enable();
> ret = handle_pte_fault(&vmf);
> mem_cgroup_oom_disable();
>
> - put_vma(vma);
> + /*
> + * If there is no need to retry, don't return the vma to the caller.
> + */
> + if (!(ret & VM_FAULT_RETRY)) {
> + put_vma(vmf.vma);
> + *vma = NULL;
> + }
>
> /*
> * The task may have entered a memcg OOM situation but
> @@ -4513,9 +4519,35 @@ int handle_speculative_fault(struct mm_struct *mm, unsigned long address,
> return ret;
>
> out_walk:
> - trace_spf_vma_notsup(_RET_IP_, vma, address);
> + trace_spf_vma_notsup(_RET_IP_, vmf.vma, address);
> local_irq_enable();
> -out_put:
> + return ret;
> +
> +out_segv:
> + trace_spf_vma_access(_RET_IP_, vmf.vma, address);
> + /*
> + * We don't return VM_FAULT_RETRY so the caller is not expected to
> + * retrieve the fetched VMA.
> + */
> + put_vma(vmf.vma);
> + *vma = NULL;
> + return VM_FAULT_SIGSEGV;
> +}
> +
> +/*
> + * This is used to know if the vma fetch in the speculative page fault handler
> + * is still valid when trying the regular fault path while holding the
> + * mmap_sem.
> + * The call to put_vma(vma) must be made after checking the vma's fields, as
> + * the vma may be freed by put_vma(). In such a case it is expected that false
> + * is returned.
> + */
> +bool can_reuse_spf_vma(struct vm_area_struct *vma, unsigned long address)
> +{
> + bool ret;
> +
> + ret = !RB_EMPTY_NODE(&vma->vm_rb) &&
> + vma->vm_start <= address && address < vma->vm_end;
> put_vma(vma);
> return ret;
> }
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH] Extract initrd free logic from arch-specific code.
From: Wei Yang @ 2018-03-29 1:12 UTC (permalink / raw)
To: Kees Cook
Cc: Shea Levy, linux-riscv, LKML, Christoph Hellwig,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Vineet Gupta,
Russell King, Catalin Marinas, Will Deacon, Mark Salter,
Aurelien Jacquiot, Mikael Starvik, Jesper Nilsson, Yoshinori Sato,
Richard Kuo, Tony Luck, Fenghua Yu, Geert Uytterhoeven,
James Hogan, Michal Simek, Ralf Baechle, David Howells,
Ley Foon Tan, Jonas Bonn, Stefan Kristiansson, Stafford Horne,
James E.J. Bottomley, Helge Deller, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Palmer Dabbelt, Albert Ou,
Martin Schwidefsky, Heiko Carstens, Chen Liqin, Lennox Wu,
Rich Felker, David S. Miller, Jeff Dike, Richard Weinberger,
Guan Xuetao, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML,
Chris Zankel, Max Filippov, Kate Stewart, Greg Kroah-Hartman,
Philippe Ombredanne, Eugeniy Paltsev, Al Viro, Vladimir Murzin,
Linus Walleij, Michal Hocko, Andrew Morton, Sudip Mukherjee,
Marc Zyngier, Rob Herring, Vlastimil Babka, Balbir Singh,
Christophe Leroy, Joe Perches, Oliver O'Halloran,
Dan Williams, Wei Yang, Christian König, Arnd Bergmann,
Deepa Dinamani, Daniel Thompson, Rob Landley, Florian Fainelli,
linux-alpha, linux-snps-arc, linux-arm-kernel,
adi-buildroot-devel, linux-c6x-dev, linux-cris-kernel,
uclinux-h8-devel, linux-hexagon, linux-ia64, linux-m68k,
linux-metag, Linux MIPS Mailing List, linux-am33-list, nios2-dev,
openrisc, linux-parisc, PowerPC, linux-s390, linux-sh, sparclinux,
user-mode-linux-devel, user-mode-linux-user, linux-xtensa
In-Reply-To: <CAGXu5jLioqnOQniXuuNS=PjeAgJr1=BL78Cqu0cwu_Y5e-NDZA@mail.gmail.com>
On Wed, Mar 28, 2018 at 09:55:07AM -0700, Kees Cook wrote:
>On Wed, Mar 28, 2018 at 8:26 AM, Shea Levy <shea@shealevy.com> wrote:
>> Now only those architectures that have custom initrd free requirements
>> need to define free_initrd_mem.
>>
>> Signed-off-by: Shea Levy <shea@shealevy.com>
>
>Yay consolidation! :)
>
>> --- a/usr/Kconfig
>> +++ b/usr/Kconfig
>> @@ -233,3 +233,7 @@ config INITRAMFS_COMPRESSION
>> default ".lzma" if RD_LZMA
>> default ".bz2" if RD_BZIP2
>> default ""
>> +
>> +config HAVE_ARCH_FREE_INITRD_MEM
>> + bool
>> + default n
>
>If you keep the Kconfig, you can leave off "default n", and I'd
>suggest adding a help section just to describe what the per-arch
>responsibilities are when select-ing the config. (See
>HAVE_ARCH_SECCOMP_FILTER for an example.)
>
One question about this change.
The original code would "select" HAVE_ARCH_FREE_INITRD_MEM on those arch.
After this change, we need to manually "select" this?
>-Kees
>
>--
>Kees Cook
>Pixel Security
--
Wei Yang
Help you, Help me
^ permalink raw reply
* [PATCH] macintosh/adb: Use C99 initializers for struct adb_driver instances
From: Finn Thain @ 2018-03-29 0:36 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Benjamin Herrenschmidt, linuxppc-dev, linux-kernel
No change to object files.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
drivers/macintosh/adb-iop.c | 14 +++++++-------
drivers/macintosh/macio-adb.c | 15 +++++++--------
drivers/macintosh/via-macii.c | 14 +++++++-------
drivers/macintosh/via-pmu.c | 14 +++++++-------
drivers/macintosh/via-pmu68k.c | 14 +++++++-------
5 files changed, 35 insertions(+), 36 deletions(-)
diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index 15db69d8ba69..ca623e6446e4 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -53,13 +53,13 @@ static void adb_iop_poll(void);
static int adb_iop_reset_bus(void);
struct adb_driver adb_iop_driver = {
- "ISM IOP",
- adb_iop_probe,
- adb_iop_init,
- adb_iop_send_request,
- adb_iop_autopoll,
- adb_iop_poll,
- adb_iop_reset_bus
+ .name = "ISM IOP",
+ .probe = adb_iop_probe,
+ .init = adb_iop_init,
+ .send_request = adb_iop_send_request,
+ .autopoll = adb_iop_autopoll,
+ .poll = adb_iop_poll,
+ .reset_bus = adb_iop_reset_bus
};
static void adb_iop_end_req(struct adb_request *req, int state)
diff --git a/drivers/macintosh/macio-adb.c b/drivers/macintosh/macio-adb.c
index 9a6223add30e..eb3adfb7f88d 100644
--- a/drivers/macintosh/macio-adb.c
+++ b/drivers/macintosh/macio-adb.c
@@ -70,14 +70,13 @@ static void macio_adb_poll(void);
static int macio_adb_reset_bus(void);
struct adb_driver macio_adb_driver = {
- "MACIO",
- macio_probe,
- macio_init,
- macio_send_request,
- /*macio_write,*/
- macio_adb_autopoll,
- macio_adb_poll,
- macio_adb_reset_bus
+ .name = "MACIO",
+ .probe = macio_probe,
+ .init = macio_init,
+ .send_request = macio_send_request,
+ .autopoll = macio_adb_autopoll,
+ .poll = macio_adb_poll,
+ .reset_bus = macio_adb_reset_bus,
};
int macio_probe(void)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 4ba06a1695ea..cf6f7d52d6be 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -91,13 +91,13 @@ static void macii_poll(void);
static int macii_reset_bus(void);
struct adb_driver via_macii_driver = {
- "Mac II",
- macii_probe,
- macii_init,
- macii_send_request,
- macii_autopoll,
- macii_poll,
- macii_reset_bus
+ .name = "Mac II",
+ .probe = macii_probe,
+ .init = macii_init,
+ .send_request = macii_send_request,
+ .autopoll = macii_autopoll,
+ .poll = macii_poll,
+ .reset_bus = macii_reset_bus,
};
static enum macii_state {
diff --git a/drivers/macintosh/via-pmu.c b/drivers/macintosh/via-pmu.c
index c4c2b3b85ebc..8da474899d79 100644
--- a/drivers/macintosh/via-pmu.c
+++ b/drivers/macintosh/via-pmu.c
@@ -199,13 +199,13 @@ static const struct file_operations pmu_options_proc_fops;
#ifdef CONFIG_ADB
struct adb_driver via_pmu_driver = {
- "PMU",
- pmu_probe,
- pmu_init,
- pmu_send_request,
- pmu_adb_autopoll,
- pmu_poll_adb,
- pmu_adb_reset_bus
+ .name = "PMU",
+ .probe = pmu_probe,
+ .init = pmu_init,
+ .send_request = pmu_send_request,
+ .autopoll = pmu_adb_autopoll,
+ .poll = pmu_poll_adb,
+ .reset_bus = pmu_adb_reset_bus,
};
#endif /* CONFIG_ADB */
diff --git a/drivers/macintosh/via-pmu68k.c b/drivers/macintosh/via-pmu68k.c
index 7d9c4baf8c11..d545ed45e482 100644
--- a/drivers/macintosh/via-pmu68k.c
+++ b/drivers/macintosh/via-pmu68k.c
@@ -120,13 +120,13 @@ static void pmu_enable_backlight(int on);
static void pmu_set_brightness(int level);
struct adb_driver via_pmu_driver = {
- "68K PMU",
- pmu_probe,
- pmu_init,
- pmu_send_request,
- pmu_autopoll,
- pmu_poll,
- pmu_reset_bus
+ .name = "68K PMU",
+ .probe = pmu_probe,
+ .init = pmu_init,
+ .send_request = pmu_send_request,
+ .autopoll = pmu_autopoll,
+ .poll = pmu_poll,
+ .reset_bus = pmu_reset_bus,
};
/*
^ permalink raw reply related
* Re: [PATCH] Extract initrd free logic from arch-specific code.
From: Nicholas Piggin @ 2018-03-29 0:23 UTC (permalink / raw)
To: Oliver
Cc: Russell King - ARM Linux, Rob Landley, Shea Levy, linux-riscv,
linux-kernel, Christoph Hellwig, Richard Henderson,
Ivan Kokshaysky, Matt Turner, Vineet Gupta, Catalin Marinas,
Will Deacon, Mark Salter, Aurelien Jacquiot, Mikael Starvik,
Jesper Nilsson, Yoshinori Sato, Richard Kuo, Tony Luck,
Fenghua Yu, Geert Uytterhoeven, James Hogan, Michal Simek,
Ralf Baechle, David Howells, Ley Foon Tan, Jonas Bonn,
Stefan Kristiansson, Stafford Horne, James E.J. Bottomley,
Helge Deller, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Palmer Dabbelt, Albert Ou, Martin Schwidefsky,
Heiko Carstens, Chen Liqin, Lennox Wu, Rich Felker,
David S. Miller, Jeff Dike, Richard Weinberger, Guan Xuetao,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Chris Zankel,
Max Filippov, Kate Stewart, Greg Kroah-Hartman,
Philippe Ombredanne, Eugeniy Paltsev, Al Viro, Vladimir Murzin,
Linus Walleij, Michal Hocko, Andrew Morton, Sudip Mukherjee,
Marc Zyngier, Rob Herring, Kees Cook, Vlastimil Babka,
Balbir Singh, Christophe Leroy, Joe Perches, Dan Williams,
Wei Yang, Christian König, Arnd Bergmann, Deepa Dinamani,
Daniel Thompson, Florian Fainelli, linux-alpha, linux-snps-arc,
linux-arm-kernel, adi-buildroot-devel, linux-c6x-dev,
linux-cris-kernel, uclinux-h8-devel, linux-hexagon, linux-ia64,
linux-m68k, linux-metag, linux-mips, linux-am33-list, nios2-dev,
openrisc, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, user-mode-linux-devel, user-mode-linux-user,
linux-xtensa
In-Reply-To: <CAOSf1CG8gQjoL5rDMRMcZp=D8jBEQ9JBSG68=CiXnitC+4Kjvg@mail.gmail.com>
On Thu, 29 Mar 2018 09:37:52 +1100
Oliver <oohall@gmail.com> wrote:
> On Thu, Mar 29, 2018 at 9:14 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Wed, Mar 28, 2018 at 02:04:22PM -0500, Rob Landley wrote:
> >>
> >>
> >> On 03/28/2018 11:48 AM, Russell King - ARM Linux wrote:
> >> > On Wed, Mar 28, 2018 at 10:58:51AM -0500, Rob Landley wrote:
> >> >> On 03/28/2018 10:26 AM, Shea Levy wrote:
> >> >>> Now only those architectures that have custom initrd free requirements
> >> >>> need to define free_initrd_mem.
> >> >> ...
> >> >>> --- a/arch/arc/mm/init.c
> >> >>> +++ b/arch/arc/mm/init.c
> >> >>> @@ -229,10 +229,3 @@ void __ref free_initmem(void)
> >> >>> {
> >> >>> free_initmem_default(-1);
> >> >>> }
> >> >>> -
> >> >>> -#ifdef CONFIG_BLK_DEV_INITRD
> >> >>> -void __init free_initrd_mem(unsigned long start, unsigned long end)
> >> >>> -{
> >> >>> - free_reserved_area((void *)start, (void *)end, -1, "initrd");
> >> >>> -}
> >> >>> -#endif
> >> >>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >> >>> index 3f972e83909b..19d1c5594e2d 100644
> >> >>> --- a/arch/arm/Kconfig
> >> >>> +++ b/arch/arm/Kconfig
> >> >>> @@ -47,6 +47,7 @@ config ARM
> >> >>> select HARDIRQS_SW_RESEND
> >> >>> select HAVE_ARCH_AUDITSYSCALL if (AEABI && !OABI_COMPAT)
> >> >>> select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6
> >> >>> + select HAVE_ARCH_FREE_INITRD_MEM
> >> >>> select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
> >> >>> select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
> >> >>> select HAVE_ARCH_MMAP_RND_BITS if MMU
> >> >>
> >> >> Isn't this why weak symbols were invented?
> >> >
> >> > Weak symbols means that we end up with both the weakly-referenced code
> >> > and the arch code in the kernel image. That's fine if the weak code
> >> > is small.
> >>
> >> The kernel's been able to build with link time garbage collection since 2016:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b67067f1176d
> >>
> >> Wouldn't that remove the unused one?
> >
> > Probably, if anyone bothered to use that, which they don't.
> >
> > LD_DEAD_CODE_DATA_ELIMINATION is a symbol without a prompt, and from
> > what I can see, nothing selects it. Therefore, the symbol is always
> > disabled, and so the feature never gets used in mainline kernels.
> >
> > Brings up the obvious question - why is it there if it's completely
> > unused? (Maybe to cause confusion, and allowing a justification
> > for __weak ?)
Well weak symbols have been used long before it was added.
> IIRC Nick had some patches to do the arch enablement for powerpc, but
> I'm not sure what happened to them though. I suspect it just fell down
> Nick's ever growing TODO list.
Yeah I had started some patches for powerpc and x86 that have ended up
on the back burner. There's been some MIPS people playing with it too.
For the kernel, LD_DEAD_CODE_DATA_ELIMINATION is not great. It can save
a little, but you get issues like any exception table entry or bug table
entry in a function will create a reference back to the function, so the
linker can't trim it away even if nothing else references it.
I'll try to take another look at it within the next few months and
remove it if I can't make progress.
Nicolas Pitre has been doing some much better work on dead code using
real LTO.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()
From: Thiago Jung Bauermann @ 2018-03-28 23:51 UTC (permalink / raw)
To: Dave Hansen
Cc: Ram Pai, shuahkh, linux-kselftest, mpe, linuxppc-dev, linux-mm,
x86, linux-arch, linux-doc, linux-kernel, mingo, akpm, benh,
paulus, khandual, aneesh.kumar, bsingharora, hbabu, mhocko,
ebiederm, arnd
In-Reply-To: <34fd1ae9-9697-ac6c-d6bc-7c25b4515a25@intel.com>
Dave Hansen <dave.hansen@intel.com> writes:
> On 03/28/2018 01:47 PM, Thiago Jung Bauermann wrote:
>>>> if (flags)
>>>> - assert(rdpkey_reg() > orig_pkey_reg);
>>>> + assert(rdpkey_reg() < orig_pkey_reg);
>>>> }
>>>>
>>>> void pkey_write_allow(int pkey)
>>> This seems so horribly wrong that I wonder how it worked in the first
>>> place. Any idea?
>> The code simply wasn't used. pkey_disable_clear() is called by
>> pkey_write_allow() and pkey_access_allow(), but before this patch series
>> nothing called either of these functions.
>>
>
> Ahh, that explains it. Can that get stuck in the changelog, please?
Yes, will be in the next version.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] Extract initrd free logic from arch-specific code.
From: Oliver @ 2018-03-28 22:37 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Rob Landley, Shea Levy, linux-riscv, linux-kernel,
Christoph Hellwig, Richard Henderson, Ivan Kokshaysky,
Matt Turner, Vineet Gupta, Catalin Marinas, Will Deacon,
Mark Salter, Aurelien Jacquiot, Mikael Starvik, Jesper Nilsson,
Yoshinori Sato, Richard Kuo, Tony Luck, Fenghua Yu,
Geert Uytterhoeven, James Hogan, Michal Simek, Ralf Baechle,
David Howells, Ley Foon Tan, Jonas Bonn, Stefan Kristiansson,
Stafford Horne, James E.J. Bottomley, Helge Deller,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Palmer Dabbelt, Albert Ou, Martin Schwidefsky, Heiko Carstens,
Chen Liqin, Lennox Wu, Rich Felker, David S. Miller, Jeff Dike,
Richard Weinberger, Guan Xuetao, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, Chris Zankel, Max Filippov, Kate Stewart,
Greg Kroah-Hartman, Philippe Ombredanne, Eugeniy Paltsev, Al Viro,
Vladimir Murzin, Linus Walleij, Michal Hocko, Andrew Morton,
Sudip Mukherjee, Marc Zyngier, Rob Herring, Kees Cook,
Vlastimil Babka, Balbir Singh, Christophe Leroy, Joe Perches,
Dan Williams, Wei Yang, Christian König, Arnd Bergmann,
Deepa Dinamani, Daniel Thompson, Florian Fainelli, linux-alpha,
linux-snps-arc, linux-arm-kernel, adi-buildroot-devel,
linux-c6x-dev, linux-cris-kernel, uclinux-h8-devel, linux-hexagon,
linux-ia64, linux-m68k, linux-metag, linux-mips, linux-am33-list,
nios2-dev, openrisc, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, user-mode-linux-devel, user-mode-linux-user,
linux-xtensa, Nicholas Piggin
In-Reply-To: <20180328221401.GA14084@n2100.armlinux.org.uk>
On Thu, Mar 29, 2018 at 9:14 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Mar 28, 2018 at 02:04:22PM -0500, Rob Landley wrote:
>>
>>
>> On 03/28/2018 11:48 AM, Russell King - ARM Linux wrote:
>> > On Wed, Mar 28, 2018 at 10:58:51AM -0500, Rob Landley wrote:
>> >> On 03/28/2018 10:26 AM, Shea Levy wrote:
>> >>> Now only those architectures that have custom initrd free requirements
>> >>> need to define free_initrd_mem.
>> >> ...
>> >>> --- a/arch/arc/mm/init.c
>> >>> +++ b/arch/arc/mm/init.c
>> >>> @@ -229,10 +229,3 @@ void __ref free_initmem(void)
>> >>> {
>> >>> free_initmem_default(-1);
>> >>> }
>> >>> -
>> >>> -#ifdef CONFIG_BLK_DEV_INITRD
>> >>> -void __init free_initrd_mem(unsigned long start, unsigned long end)
>> >>> -{
>> >>> - free_reserved_area((void *)start, (void *)end, -1, "initrd");
>> >>> -}
>> >>> -#endif
>> >>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> >>> index 3f972e83909b..19d1c5594e2d 100644
>> >>> --- a/arch/arm/Kconfig
>> >>> +++ b/arch/arm/Kconfig
>> >>> @@ -47,6 +47,7 @@ config ARM
>> >>> select HARDIRQS_SW_RESEND
>> >>> select HAVE_ARCH_AUDITSYSCALL if (AEABI && !OABI_COMPAT)
>> >>> select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6
>> >>> + select HAVE_ARCH_FREE_INITRD_MEM
>> >>> select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
>> >>> select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
>> >>> select HAVE_ARCH_MMAP_RND_BITS if MMU
>> >>
>> >> Isn't this why weak symbols were invented?
>> >
>> > Weak symbols means that we end up with both the weakly-referenced code
>> > and the arch code in the kernel image. That's fine if the weak code
>> > is small.
>>
>> The kernel's been able to build with link time garbage collection since 2016:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b67067f1176d
>>
>> Wouldn't that remove the unused one?
>
> Probably, if anyone bothered to use that, which they don't.
>
> LD_DEAD_CODE_DATA_ELIMINATION is a symbol without a prompt, and from
> what I can see, nothing selects it. Therefore, the symbol is always
> disabled, and so the feature never gets used in mainline kernels.
>
> Brings up the obvious question - why is it there if it's completely
> unused? (Maybe to cause confusion, and allowing a justification
> for __weak ?)
IIRC Nick had some patches to do the arch enablement for powerpc, but
I'm not sure what happened to them though. I suspect it just fell down
Nick's ever growing TODO list.
^ permalink raw reply
* Re: [PATCH] Extract initrd free logic from arch-specific code.
From: Russell King - ARM Linux @ 2018-03-28 22:14 UTC (permalink / raw)
To: Rob Landley
Cc: Shea Levy, linux-riscv, linux-kernel, Christoph Hellwig,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Vineet Gupta,
Catalin Marinas, Will Deacon, Mark Salter, Aurelien Jacquiot,
Mikael Starvik, Jesper Nilsson, Yoshinori Sato, Richard Kuo,
Tony Luck, Fenghua Yu, Geert Uytterhoeven, James Hogan,
Michal Simek, Ralf Baechle, David Howells, Ley Foon Tan,
Jonas Bonn, Stefan Kristiansson, Stafford Horne,
James E.J. Bottomley, Helge Deller, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Palmer Dabbelt, Albert Ou,
Martin Schwidefsky, Heiko Carstens, Chen Liqin, Lennox Wu,
Rich Felker, David S. Miller, Jeff Dike, Richard Weinberger,
Guan Xuetao, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
Chris Zankel, Max Filippov, Kate Stewart, Greg Kroah-Hartman,
Philippe Ombredanne, Eugeniy Paltsev, Al Viro, Vladimir Murzin,
Linus Walleij, Michal Hocko, Andrew Morton, Sudip Mukherjee,
Marc Zyngier, Rob Herring, Kees Cook, Vlastimil Babka,
Balbir Singh, Christophe Leroy, Joe Perches,
Oliver O'Halloran, Dan Williams, Wei Yang,
Christian König, Arnd Bergmann, Deepa Dinamani,
Daniel Thompson, Florian Fainelli, linux-alpha, linux-snps-arc,
linux-arm-kernel, adi-buildroot-devel, linux-c6x-dev,
linux-cris-kernel, uclinux-h8-devel, linux-hexagon, linux-ia64,
linux-m68k, linux-metag, linux-mips, linux-am33-list, nios2-dev,
openrisc, linux-parisc, linuxppc-dev, linux-s390, linux-sh,
sparclinux, user-mode-linux-devel, user-mode-linux-user,
linux-xtensa
In-Reply-To: <de092e7f-0bc9-bb06-9798-12784930a6bd@landley.net>
On Wed, Mar 28, 2018 at 02:04:22PM -0500, Rob Landley wrote:
>
>
> On 03/28/2018 11:48 AM, Russell King - ARM Linux wrote:
> > On Wed, Mar 28, 2018 at 10:58:51AM -0500, Rob Landley wrote:
> >> On 03/28/2018 10:26 AM, Shea Levy wrote:
> >>> Now only those architectures that have custom initrd free requirements
> >>> need to define free_initrd_mem.
> >> ...
> >>> --- a/arch/arc/mm/init.c
> >>> +++ b/arch/arc/mm/init.c
> >>> @@ -229,10 +229,3 @@ void __ref free_initmem(void)
> >>> {
> >>> free_initmem_default(-1);
> >>> }
> >>> -
> >>> -#ifdef CONFIG_BLK_DEV_INITRD
> >>> -void __init free_initrd_mem(unsigned long start, unsigned long end)
> >>> -{
> >>> - free_reserved_area((void *)start, (void *)end, -1, "initrd");
> >>> -}
> >>> -#endif
> >>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >>> index 3f972e83909b..19d1c5594e2d 100644
> >>> --- a/arch/arm/Kconfig
> >>> +++ b/arch/arm/Kconfig
> >>> @@ -47,6 +47,7 @@ config ARM
> >>> select HARDIRQS_SW_RESEND
> >>> select HAVE_ARCH_AUDITSYSCALL if (AEABI && !OABI_COMPAT)
> >>> select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6
> >>> + select HAVE_ARCH_FREE_INITRD_MEM
> >>> select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
> >>> select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
> >>> select HAVE_ARCH_MMAP_RND_BITS if MMU
> >>
> >> Isn't this why weak symbols were invented?
> >
> > Weak symbols means that we end up with both the weakly-referenced code
> > and the arch code in the kernel image. That's fine if the weak code
> > is small.
>
> The kernel's been able to build with link time garbage collection since 2016:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b67067f1176d
>
> Wouldn't that remove the unused one?
Probably, if anyone bothered to use that, which they don't.
LD_DEAD_CODE_DATA_ELIMINATION is a symbol without a prompt, and from
what I can see, nothing selects it. Therefore, the symbol is always
disabled, and so the feature never gets used in mainline kernels.
Brings up the obvious question - why is it there if it's completely
unused? (Maybe to cause confusion, and allowing a justification
for __weak ?)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Nicholas Piggin @ 2018-03-28 22:09 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: David Miller, paulmck, arnd, linux-rdma, linuxppc-dev, linus971,
will.deacon, alexander.duyck, okaya, jgg, David.Laight, oohall,
netdev, alexander.h.duyck, torvalds
In-Reply-To: <1522272692.21446.42.camel@kernel.crashing.org>
On Thu, 29 Mar 2018 08:31:32 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Thu, 2018-03-29 at 02:23 +1000, Nicholas Piggin wrote:
> > On Wed, 28 Mar 2018 11:55:09 -0400 (EDT)
> > David Miller <davem@davemloft.net> wrote:
> >
> > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Date: Thu, 29 Mar 2018 02:13:16 +1100
> > >
> > > > Let's fix all archs, it's way easier than fixing all drivers. Half of
> > > > the archs are unused or dead anyway.
> > >
> > > Agreed.
> >
> > While we're making decrees here, can we do something about mmiowb?
> > The semantics are basically indecipherable.
>
> I was going to tackle that next :-)
>
> > This is a variation on the mandatory write barrier that causes writes to weakly
> > ordered I/O regions to be partially ordered. Its effects may go beyond the
> > CPU->Hardware interface and actually affect the hardware at some level.
> >
> > How can a driver writer possibly get that right?
> >
> > IIRC it was added for some big ia64 system that was really expensive
> > to implement the proper wmb() semantics on. So wmb() semantics were
> > quietly downgraded, then the subsequently broken drivers they cared
> > about were fixed by adding the stronger mmiowb().
> >
> > What should have happened was wmb and writel remained correct, sane, and
> > expensive, and they add an mmio_wmb() to order MMIO stores made by the
> > writel_relaxed accessors, then use that to speed up the few drivers they
> > care about.
> >
> > Now that ia64 doesn't matter too much, can we deprecate mmiowb and just
> > make wmb ordering talk about stores to the device, not to some
> > intermediate stage of the interconnect where it can be subsequently
> > reordered wrt the device? Drivers can be converted back to using wmb
> > or writel gradually.
>
> I was under the impression that mmiowb was specifically about ordering
> writel's with a subsequent spin_unlock, without it, MMIOs from
> different CPUs (within the same lock) would still arrive OO.
Yes more or less, and I think that until mmiowb was introduced, wmb
or writel was sufficient for this.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 00/41] cxlflash: OCXL transport support and miscellaneous fixes
From: Martin K. Petersen @ 2018-03-28 21:34 UTC (permalink / raw)
To: Uma Krishnan
Cc: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
Manoj N. Kumar, linuxppc-dev, Andrew Donnellan, Frederic Barrat,
Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>
Uma,
> This patch series adds OCXL support to the cxlflash driver. With this
> support, new devices using the OCXL transport will be supported by the
> cxlflash driver along with the existing CXL devices. An effort is made
> to keep this transport specific function independent of the existing
> core driver that communicates with the AFU.
>
> The first three patches contain a minor fix and staging improvements.
>
> This series is intended for 4.17 and is bisectable.
Something this big really needs to be submitted around rc2/rc3. It's way
too late in the cycle to ensure proper zeroday coverage for all these
commits.
I have started a 4.18/scsi-queue branch to hold this series for now.
The 4.18 branch will be rebased once 4.17rc1 is out in a few weeks. Your
changes won't show up in for-next until then either.
Thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 21:31 UTC (permalink / raw)
To: Nicholas Piggin, David Miller
Cc: paulmck, arnd, linux-rdma, linuxppc-dev, linus971, will.deacon,
alexander.duyck, okaya, jgg, David.Laight, oohall, netdev,
alexander.h.duyck, torvalds
In-Reply-To: <20180329022324.037c3f39@roar.ozlabs.ibm.com>
On Thu, 2018-03-29 at 02:23 +1000, Nicholas Piggin wrote:
> On Wed, 28 Mar 2018 11:55:09 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
>
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Date: Thu, 29 Mar 2018 02:13:16 +1100
> >
> > > Let's fix all archs, it's way easier than fixing all drivers. Half of
> > > the archs are unused or dead anyway.
> >
> > Agreed.
>
> While we're making decrees here, can we do something about mmiowb?
> The semantics are basically indecipherable.
I was going to tackle that next :-)
> This is a variation on the mandatory write barrier that causes writes to weakly
> ordered I/O regions to be partially ordered. Its effects may go beyond the
> CPU->Hardware interface and actually affect the hardware at some level.
>
> How can a driver writer possibly get that right?
>
> IIRC it was added for some big ia64 system that was really expensive
> to implement the proper wmb() semantics on. So wmb() semantics were
> quietly downgraded, then the subsequently broken drivers they cared
> about were fixed by adding the stronger mmiowb().
>
> What should have happened was wmb and writel remained correct, sane, and
> expensive, and they add an mmio_wmb() to order MMIO stores made by the
> writel_relaxed accessors, then use that to speed up the few drivers they
> care about.
>
> Now that ia64 doesn't matter too much, can we deprecate mmiowb and just
> make wmb ordering talk about stores to the device, not to some
> intermediate stage of the interconnect where it can be subsequently
> reordered wrt the device? Drivers can be converted back to using wmb
> or writel gradually.
I was under the impression that mmiowb was specifically about ordering
writel's with a subsequent spin_unlock, without it, MMIOs from
different CPUs (within the same lock) would still arrive OO.
If that's indeed the case, I would suggest ia64 switches to a similar
per-cpu flag trick powerpc uses.
Cheers,
Ben.
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH v9 09/24] mm: protect mremap() against SPF hanlder
From: David Rientjes @ 2018-03-28 21:21 UTC (permalink / raw)
To: Laurent Dufour
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <1fe7529a-947c-fdb2-12d2-b38bdd41bb04@linux.vnet.ibm.com>
On Wed, 28 Mar 2018, Laurent Dufour wrote:
> >> @@ -326,7 +336,10 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> >> mremap_userfaultfd_prep(new_vma, uf);
> >> arch_remap(mm, old_addr, old_addr + old_len,
> >> new_addr, new_addr + new_len);
> >> + if (vma != new_vma)
> >> + vm_raw_write_end(vma);
> >> }
> >> + vm_raw_write_end(new_vma);
> >
> > Just do
> >
> > vm_raw_write_end(vma);
> > vm_raw_write_end(new_vma);
> >
> > here.
>
> Are you sure ? we can have vma = new_vma done if (unlikely(err))
>
Sorry, what I meant was do
if (vma != new_vma)
vm_raw_write_end(vma);
vm_raw_write_end(new_vma);
after the conditional. Having the locking unnecessarily embedded in the
conditional has been an issue in the past with other areas of core code,
unless you have a strong reason for it.
^ permalink raw reply
* Re: [PATCH v9 01/24] mm: Introduce CONFIG_SPECULATIVE_PAGE_FAULT
From: David Rientjes @ 2018-03-28 21:18 UTC (permalink / raw)
To: Laurent Dufour
Cc: Thomas Gleixner, paulmck, peterz, akpm, kirill, ak, mhocko, dave,
jack, Matthew Wilcox, benh, mpe, paulus, Ingo Molnar, hpa,
Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <aa678038-9c5c-a8cb-0aed-ef19bde5d623@linux.vnet.ibm.com>
On Wed, 28 Mar 2018, Laurent Dufour wrote:
> > Putting this in mm/Kconfig is definitely the right way to go about it
> > instead of any generic option in arch/*.
> >
> > My question, though, was making this configurable by the user:
> >
> > config SPECULATIVE_PAGE_FAULT
> > bool "Speculative page faults"
> > depends on X86_64 || PPC
> > default y
> > help
> > ..
> >
> > It's a question about whether we want this always enabled on x86_64 and
> > power or whether the user should be able to disable it (right now they
> > can't). With a large feature like this, you may want to offer something
> > simple (disable CONFIG_SPECULATIVE_PAGE_FAULT) if someone runs into
> > regressions.
>
> I agree, but I think it would be important to get the per architecture
> enablement to avoid complex check here. For instance in the case of powerPC
> this is only supported for PPC_BOOK3S_64.
>
> To avoid exposing such per architecture define here, what do you think about
> having supporting architectures setting ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT
> and the SPECULATIVE_PAGE_FAULT depends on this, like this:
>
> In mm/Kconfig:
> config SPECULATIVE_PAGE_FAULT
> bool "Speculative page faults"
> depends on ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT && SMP
> default y
> help
> ...
>
> In arch/powerpc/Kconfig:
> config PPC
> ...
> select ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT if PPC_BOOK3S_64
>
> In arch/x86/Kconfig:
> config X86_64
> ...
> select ARCH_SUPPORTS_SPECULATIVE_PAGE_FAULT
>
>
Looks good to me! It feels like this will add more assurance that if
things regress for certain workloads that it can be disabled. I don't
feel strongly about the default value, I'm ok with it being enabled by
default.
^ permalink raw reply
* Re: [RFC PATCH v2 0/2] Randomization of address chosen by mmap.
From: Ilya Smith @ 2018-03-28 21:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kees Cook, Michal Hocko, Richard Henderson, ink, mattst88,
Vineet Gupta, Russell King, Tony Luck, Fenghua Yu, Ralf Baechle,
James E.J. Bottomley, Helge Deller, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Martin Schwidefsky,
Heiko Carstens, Yoshinori Sato, Rich Felker, David S. Miller,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML, nyc,
Al Viro, Arnd Bergmann, Greg KH, Deepa Dinamani, Hugh Dickins,
Kate Stewart, Philippe Ombredanne, Andrew Morton, Steve Capper,
Punit Agrawal, Aneesh Kumar K.V, Nick Piggin, Bhupesh Sharma,
Rik van Riel, nitin.m.gupta, Kirill A. Shutemov, Dan Williams,
Jan Kara, Ross Zwisler, Jerome Glisse, Andrea Arcangeli,
Oleg Nesterov, linux-alpha, LKML, linux-snps-arc, linux-ia64,
linux-metag, Linux MIPS Mailing List, linux-parisc, PowerPC,
linux-s390, linux-sh, sparclinux, Linux-MM
In-Reply-To: <20180327234904.GA27734@bombadil.infradead.org>
> On 28 Mar 2018, at 02:49, Matthew Wilcox <willy@infradead.org> wrote:
>=20
> On Tue, Mar 27, 2018 at 03:53:53PM -0700, Kees Cook wrote:
>> I agree: pushing this off to libc leaves a lot of things unprotected.
>> I think this should live in the kernel. The question I have is about
>> making it maintainable/readable/etc.
>>=20
>> The state-of-the-art for ASLR is moving to finer granularity (over
>> just base-address offset), so I'd really like to see this supported =
in
>> the kernel. We'll be getting there for other things in the future, =
and
>> I'd like to have a working production example for researchers to
>> study, etc.
>=20
> One thing we need is to limit the fragmentation of this approach.
> Even on 64-bit systems, we can easily get into a situation where there =
isn't
> space to map a contiguous terabyte.
As I wrote before, shift_random is introduced to be fragmentation limit. =
Even=20
without it, the main question here is =E2=80=98if we can=E2=80=99t =
allocate memory with N size=20
bytes, how many bytes we already allocated?=E2=80=99. =46rom these point =
of view I=20
already showed in previous version of patch that if application uses not =
so big=20
memory allocations, it will have enough memory to use. If it uses XX =
Gigabytes=20
or Terabytes memory, this application has all chances to be exploited =
with=20
fully randomization or without. Since it is much easier to find(or =
guess) any=20
usable pointer, etc. For the instance you have only 128 terabytes of =
memory for=20
user space, so probability to exploit this application is 1/128 what is =
not=20
secure at all. This is very rough estimate but I try to make things =
easier to=20
understand.
Best regards,
Ilya
^ permalink raw reply
* RE: [RFC PATCH v2 0/2] Randomization of address chosen by mmap.
From: Luck, Tony @ 2018-03-28 21:07 UTC (permalink / raw)
To: Rich Felker, Matthew Wilcox
Cc: Kees Cook, Ilya Smith, Michal Hocko, Richard Henderson,
ink@jurassic.park.msu.ru, mattst88@gmail.com, Vineet Gupta,
Russell King, Yu, Fenghua, Ralf Baechle, James E.J. Bottomley,
Helge Deller, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens,
Yoshinori Sato, David S. Miller, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, X86 ML, nyc@holomorphy.com, Al Viro,
Arnd Bergmann, Greg KH, Deepa Dinamani, Hugh Dickins,
Kate Stewart, Philippe Ombredanne, Andrew Morton, Steve Capper,
Punit Agrawal, Aneesh Kumar K.V, Nick Piggin, Bhupesh Sharma,
Rik van Riel, nitin.m.gupta@oracle.com, Kirill A. Shutemov,
Williams, Dan J, Jan Kara, Ross Zwisler, Jerome Glisse,
Andrea Arcangeli, Oleg Nesterov, linux-alpha@vger.kernel.org,
LKML, linux-snps-arc@lists.infradead.org,
linux-ia64@vger.kernel.org, linux-metag@vger.kernel.org,
Linux MIPS Mailing List, linux-parisc, PowerPC, linux-s390,
linux-sh, sparclinux, Linux-MM
In-Reply-To: <20180328000025.GM1436@brightrain.aerifal.cx>
> The default limit of only 65536 VMAs will also quickly come into play
> if consecutive anon mmaps don't get merged. Of course this can be
> raised, but it has significant resource and performance (fork) costs.
Could the random mmap address chooser look for how many existing
VMAs have space before/after and the right attributes to merge with the
new one you want to create? If this is above some threshold (100?) then
pick one of them randomly and allocate the new address so that it will
merge from below/above with an existing one.
That should still give you a very high degree of randomness, but prevent
out of control numbers of VMAs from being created.
-Tony
^ permalink raw reply
* Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()
From: Dave Hansen @ 2018-03-28 20:55 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Ram Pai, shuahkh, linux-kselftest, mpe, linuxppc-dev, linux-mm,
x86, linux-arch, linux-doc, linux-kernel, mingo, akpm, benh,
paulus, khandual, aneesh.kumar, bsingharora, hbabu, mhocko,
ebiederm, arnd
In-Reply-To: <87muys3p2v.fsf@morokweng.localdomain>
On 03/28/2018 01:47 PM, Thiago Jung Bauermann wrote:
>>> if (flags)
>>> - assert(rdpkey_reg() > orig_pkey_reg);
>>> + assert(rdpkey_reg() < orig_pkey_reg);
>>> }
>>>
>>> void pkey_write_allow(int pkey)
>> This seems so horribly wrong that I wonder how it worked in the first
>> place. Any idea?
> The code simply wasn't used. pkey_disable_clear() is called by
> pkey_write_allow() and pkey_access_allow(), but before this patch series
> nothing called either of these functions.
>
Ahh, that explains it. Can that get stuck in the changelog, please?
^ permalink raw reply
* Re: [PATCH v4 14/16] powerpc: Use generic free_initrd_mem.
From: Shea Levy @ 2018-03-28 20:53 UTC (permalink / raw)
To: Joe Perches, linux-riscv, linux-kernel
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Michal Hocko, Vlastimil Babka, Andrew Morton, Dan Williams,
Christophe Leroy, Oliver O'Halloran, linuxppc-dev
In-Reply-To: <1522269853.12357.135.camel@perches.com>
[-- Attachment #1: Type: text/plain, Size: 645 bytes --]
Joe Perches <joe@perches.com> writes:
> On Wed, 2018-03-28 at 16:36 -0400, Shea Levy wrote:
>> Signed-off-by: Shea Levy <shea@shealevy.com>
>
> Most people seem to want some form of commit message
> and not just your sign-off.
>
Ah, if the subject is insufficient I can add some more detail.
>
> And btw:
>
> It seems you used get_maintainer to determine who to
> send these patches to.
>
> I suggest you add --nogit and --nogit-fallback to the
> get_maintainer command line you use to avoid sending
> these patches to people like me that have done drive-by
> cleanup work on these files.
Whoops, thanks for the tip and sorry for the noise!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()
From: Thiago Jung Bauermann @ 2018-03-28 20:47 UTC (permalink / raw)
To: Dave Hansen
Cc: Ram Pai, shuahkh, linux-kselftest, mpe, linuxppc-dev, linux-mm,
x86, linux-arch, linux-doc, linux-kernel, mingo, akpm, benh,
paulus, khandual, aneesh.kumar, bsingharora, hbabu, mhocko,
ebiederm, arnd
In-Reply-To: <dc5ee0c8-afe3-78aa-001d-7b49b398337b@intel.com>
Dave Hansen <dave.hansen@intel.com> writes:
> On 02/21/2018 05:55 PM, Ram Pai wrote:
>> --- a/tools/testing/selftests/vm/protection_keys.c
>> +++ b/tools/testing/selftests/vm/protection_keys.c
>> @@ -461,7 +461,7 @@ void pkey_disable_clear(int pkey, int flags)
>> pkey, pkey, pkey_rights);
>> pkey_assert(pkey_rights >= 0);
>>
>> - pkey_rights |= flags;
>> + pkey_rights &= ~flags;
>>
>> ret = pkey_set(pkey, pkey_rights, 0);
>> /* pkey_reg and flags have the same format */
>> @@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
>> dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
>> pkey, rdpkey_reg());
>> if (flags)
>> - assert(rdpkey_reg() > orig_pkey_reg);
>> + assert(rdpkey_reg() < orig_pkey_reg);
>> }
>>
>> void pkey_write_allow(int pkey)
>
> This seems so horribly wrong that I wonder how it worked in the first
> place. Any idea?
The code simply wasn't used. pkey_disable_clear() is called by
pkey_write_allow() and pkey_access_allow(), but before this patch series
nothing called either of these functions.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v4 14/16] powerpc: Use generic free_initrd_mem.
From: Joe Perches @ 2018-03-28 20:44 UTC (permalink / raw)
To: Shea Levy, linux-riscv, linux-kernel
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Michal Hocko, Vlastimil Babka, Andrew Morton, Dan Williams,
Christophe Leroy, Oliver O'Halloran, linuxppc-dev
In-Reply-To: <20180328203659.18692-15-shea@shealevy.com>
On Wed, 2018-03-28 at 16:36 -0400, Shea Levy wrote:
> Signed-off-by: Shea Levy <shea@shealevy.com>
Most people seem to want some form of commit message
and not just your sign-off.
And btw:
It seems you used get_maintainer to determine who to
send these patches to.
I suggest you add --nogit and --nogit-fallback to the
get_maintainer command line you use to avoid sending
these patches to people like me that have done drive-by
cleanup work on these files.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox