* [PATCH v3 3/7] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
From: Christophe Leroy @ 2020-02-03 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
From: Russell Currey <ruscur@russell.cc>
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v3: no change
v2: no change
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 0b063830eea8..e37960ef68c6 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -370,7 +370,7 @@ config PPC_PTDUMP
If you are unsure, say N.
config PPC_DEBUG_WX
- bool "Warn on W+X mappings at boot"
+ bool "Warn on W+X mappings at boot & enable manual checks at runtime"
depends on PPC_PTDUMP && STRICT_KERNEL_RWX
help
Generate a warning if any W+X mappings are found at boot.
@@ -384,7 +384,9 @@ config PPC_DEBUG_WX
of other unfixed kernel bugs easier.
There is no runtime or memory usage effect of this option
- once the kernel has booted up - it's a one time check.
+ once the kernel has booted up, it only automatically checks once.
+
+ Enables the "check_wx_pages" debugfs entry for checking at runtime.
If in doubt, say "Y".
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index 206156255247..a15e19a3b14e 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -4,7 +4,7 @@
*
* This traverses the kernel pagetables and dumps the
* information about the used sections of memory to
- * /sys/kernel/debug/kernel_pagetables.
+ * /sys/kernel/debug/kernel_page_tables.
*
* Derived from the arm64 implementation:
* Copyright (c) 2014, The Linux Foundation, Laura Abbott.
@@ -413,6 +413,25 @@ void ptdump_check_wx(void)
else
pr_info("Checked W+X mappings: passed, no W+X pages found\n");
}
+
+static int check_wx_debugfs_set(void *data, u64 val)
+{
+ if (val != 1ULL)
+ return -EINVAL;
+
+ ptdump_check_wx();
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n");
+
+static int ptdump_check_wx_init(void)
+{
+ return debugfs_create_file("check_wx_pages", 0200, NULL,
+ NULL, &check_wx_fops) ? 0 : -ENOMEM;
+}
+device_initcall(ptdump_check_wx_init);
#endif
static int ptdump_init(void)
--
2.25.0
^ permalink raw reply related
* [PATCH v3 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Christophe Leroy @ 2020-02-03 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <80ebd9075cd7c8b412c6d5d05f7542f9026642ef.1580713729.git.christophe.leroy@c-s.fr>
With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one
W+X page at boot by default. This can be tested with
CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
kernel log during boot.
powerpc doesn't implement its own alloc() for kprobes like other
architectures do, but we couldn't immediately mark RO anyway since we do
a memcpy to the page we allocate later. After that, nothing should be
allowed to modify the page, and write permissions are removed well
before the kprobe is armed.
The memcpy() would fail if >1 probes were allocated, so use
patch_instruction() instead which is safe for RO.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: copied alloc_insn_page() from arm64, set_memory_ro() is now called there.
v2: removed the redundant flush
---
arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 2d27ec4feee4..bfab91ded234 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -24,6 +24,8 @@
#include <asm/sstep.h>
#include <asm/sections.h>
#include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/vmalloc.h>
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
return addr;
}
+void *alloc_insn_page(void)
+{
+ void *page = vmalloc_exec(PAGE_SIZE);
+
+ if (page)
+ set_memory_ro((unsigned long)page, 1);
+
+ return page;
+}
+
int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
@@ -124,11 +136,8 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- memcpy(p->ainsn.insn, p->addr,
- MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+ patch_instruction(p->ainsn.insn, *p->addr);
p->opcode = *p->addr;
- flush_icache_range((unsigned long)p->ainsn.insn,
- (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
}
p->ainsn.boostable = 0;
--
2.25.0
^ permalink raw reply related
* [PATCH v3 1/7] powerpc/mm: Implement set_memory() routines
From: Christophe Leroy @ 2020-02-03 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, ruscur
Cc: linuxppc-dev, linux-kernel
The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
and are generally useful primitives to have. This implementation is
designed to be completely generic across powerpc's many MMUs.
It's possible that this could be optimised to be faster for specific
MMUs, but the focus is on having a generic and safe implementation for
now.
This implementation does not handle cases where the caller is attempting
to change the mapping of the page it is executing from, or if another
CPU is concurrently using the page being altered. These cases likely
shouldn't happen, but a more complex implementation with MMU-specific code
could safely handle them, so that is left as a TODO for now.
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3:
- Changes 'action' from int to long to avoid build failure on PPC64 when casting to/from void*
- Move pageattr.o into obj-y in Makefile
v2:
- use integers instead of pointers for action
- drop action check, nobody should call change_memory_attr() directly.
Should it happen, the function will just do nothing.
- Renamed confusing 'pte_val' var to 'pte' as pte_val() is already a function.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 32 ++++++++++++
arch/powerpc/mm/Makefile | 2 +-
arch/powerpc/mm/pageattr.c | 74 +++++++++++++++++++++++++++
4 files changed, 108 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 730c06668f22..d0c6e7b7a62d 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -129,6 +129,7 @@ config PPC
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_MEMBARRIER_CALLBACKS
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
+ select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
new file mode 100644
index 000000000000..64011ea444b4
--- /dev/null
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_SET_MEMORY_H
+#define _ASM_POWERPC_SET_MEMORY_H
+
+#define SET_MEMORY_RO 0
+#define SET_MEMORY_RW 1
+#define SET_MEMORY_NX 2
+#define SET_MEMORY_X 3
+
+int change_memory_attr(unsigned long addr, int numpages, long action);
+
+static inline int set_memory_ro(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RO);
+}
+
+static inline int set_memory_rw(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RW);
+}
+
+static inline int set_memory_nx(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_NX);
+}
+
+static inline int set_memory_x(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_X);
+}
+
+#endif
diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
index 5e147986400d..a998fdac52f9 100644
--- a/arch/powerpc/mm/Makefile
+++ b/arch/powerpc/mm/Makefile
@@ -5,7 +5,7 @@
ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
-obj-y := fault.o mem.o pgtable.o mmap.o \
+obj-y := fault.o mem.o pgtable.o mmap.o pageattr.o \
init_$(BITS).o pgtable_$(BITS).o \
pgtable-frag.o ioremap.o ioremap_$(BITS).o \
init-common.o mmu_context.o drmem.o
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
new file mode 100644
index 000000000000..2b573768a7f7
--- /dev/null
+++ b/arch/powerpc/mm/pageattr.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * MMU-generic set_memory implementation for powerpc
+ *
+ * Copyright 2019, IBM Corporation.
+ */
+
+#include <linux/mm.h>
+#include <linux/set_memory.h>
+
+#include <asm/mmu.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
+
+
+/*
+ * Updates the attributes of a page in three steps:
+ *
+ * 1. invalidate the page table entry
+ * 2. flush the TLB
+ * 3. install the new entry with the updated attributes
+ *
+ * This is unsafe if the caller is attempting to change the mapping of the
+ * page it is executing from, or if another CPU is concurrently using the
+ * page being altered.
+ *
+ * TODO make the implementation resistant to this.
+ */
+static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ long action = (long)data;
+ pte_t pte;
+
+ spin_lock(&init_mm.page_table_lock);
+
+ /* invalidate the PTE so it's safe to modify */
+ pte = ptep_get_and_clear(&init_mm, addr, ptep);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+
+ /* modify the PTE bits as desired, then apply */
+ switch (action) {
+ case SET_MEMORY_RO:
+ pte = pte_wrprotect(pte);
+ break;
+ case SET_MEMORY_RW:
+ pte = pte_mkwrite(pte);
+ break;
+ case SET_MEMORY_NX:
+ pte = pte_exprotect(pte);
+ break;
+ case SET_MEMORY_X:
+ pte = pte_mkexec(pte);
+ break;
+ default:
+ break;
+ }
+
+ set_pte_at(&init_mm, addr, ptep, pte);
+ spin_unlock(&init_mm.page_table_lock);
+
+ return 0;
+}
+
+int change_memory_attr(unsigned long addr, int numpages, long action)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long sz = numpages * PAGE_SIZE;
+
+ if (!numpages)
+ return 0;
+
+ return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
+}
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v6 1/5] powerpc/mm: Implement set_memory() routines
From: Christophe Leroy @ 2020-02-03 7:06 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel, dja
In-Reply-To: <8675c11631ac027a78e00d4fe2c20736496b1e97.camel@russell.cc>
Le 03/02/2020 à 01:46, Russell Currey a écrit :
> On Wed, 2020-01-08 at 13:52 +0100, Christophe Leroy wrote:
>>
>> Le 24/12/2019 à 06:55, Russell Currey a écrit :
>>> diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
>>> index 5e147986400d..d0a0bcbc9289 100644
>>> --- a/arch/powerpc/mm/Makefile
>>> +++ b/arch/powerpc/mm/Makefile
>>> @@ -20,3 +20,4 @@ obj-$(CONFIG_HIGHMEM) += highmem.o
>>> obj-$(CONFIG_PPC_COPRO_BASE) += copro_fault.o
>>> obj-$(CONFIG_PPC_PTDUMP) += ptdump/
>>> obj-$(CONFIG_KASAN) += kasan/
>>> +obj-$(CONFIG_ARCH_HAS_SET_MEMORY) += pageattr.o
>>
>> CONFIG_ARCH_HAS_SET_MEMORY is set inconditionnally, I think you
>> should
>> add pageattr.o to obj-y instead. CONFIG_ARCH_HAS_XXX are almost
>> never
>> used in Makefiles
>
> Fair enough, will keep that in mind
I forgot I commented that. I'll do it in v3.
>>> + pte_t pte_val;
>>> +
>>> + // invalidate the PTE so it's safe to modify
>>> + pte_val = ptep_get_and_clear(&init_mm, addr, ptep);
>>> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>>
>> Why flush a range for a single page ? On most targets this will do a
>> tlbia which is heavy, while a tlbie would suffice.
>>
>> I think flush_tlb_kernel_range() should be replaced by something
>> flushing only a single page.
>
> You might be able to help me out here, I wanted to do that but the only
> functions I could find that flushed single pages needed a
> vm_area_struct, which I can't get.
I sent out two patches for that, one for book3s/32 and one for nohash:
https://patchwork.ozlabs.org/patch/1231983/
https://patchwork.ozlabs.org/patch/1232223/
Maybe one for book3s/64 would be needed as well ? Can you do it if needed ?
>
>>
>>> +
>>> + // modify the PTE bits as desired, then apply
>>> + switch (action) {
>>> + case SET_MEMORY_RO:
>>> + pte_val = pte_wrprotect(pte_val);
>>> + break;
>>> + case SET_MEMORY_RW:
>>> + pte_val = pte_mkwrite(pte_val);
>>> + break;
>>> + case SET_MEMORY_NX:
>>> + pte_val = pte_exprotect(pte_val);
>>> + break;
>>> + case SET_MEMORY_X:
>>> + pte_val = pte_mkexec(pte_val);
>>> + break;
>>> + default:
>>> + WARN_ON(true);
>>> + return -EINVAL;
>>
>> Is it worth checking that the action is valid for each page ? I
>> think
>> validity of action should be checked in change_memory_attr(). All
>> other
>> functions are static so you know they won't be called from outside.
>>
>> Once done, you can squash __change_page_attr() into
>> change_page_attr(),
>> remove the ret var and return 0 all the time.
>
> Makes sense to fold things into a single function, but in terms of
> performance it shouldn't make a difference, right? I still have to
> check the action to determine what to change (unless I replace passing
> SET_MEMORY_RO into apply_to_page_range() with a function pointer to
> pte_wrprotect() for example).
pte_wrprotect() is a static inline.
>
>>
>>> + }
>>> +
>>> + set_pte_at(&init_mm, addr, ptep, pte_val);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int change_page_attr(pte_t *ptep, unsigned long addr, void
>>> *data)
>>> +{
>>> + int ret;
>>> +
>>> + spin_lock(&init_mm.page_table_lock);
>>> + ret = __change_page_attr(ptep, addr, data);
>>> + spin_unlock(&init_mm.page_table_lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +int change_memory_attr(unsigned long addr, int numpages, int
>>> action)
>>> +{
>>> + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
>>> + unsigned long size = numpages * PAGE_SIZE;
>>> +
>>> + if (!numpages)
>>> + return 0;
>>> +
>>> + return apply_to_page_range(&init_mm, start, size,
>>> change_page_attr, &action);
>>
>> Use (void*)action instead of &action (see upper comment)
>
> To get this to work I had to use (void *)(size_t)action to stop the
> compiler from complaining about casting an int to a void*, is there a
> better way to go about it? Works fine, just looks gross.
Yes, use long instead (see my v3)
Christophe
^ permalink raw reply
* Re: [PATCH v2 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Christophe Leroy @ 2020-02-03 7:00 UTC (permalink / raw)
To: Russell Currey, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <3078df74c232e54aef3e8bb3523587a3053ab0ec.camel@russell.cc>
Le 03/02/2020 à 05:50, Russell Currey a écrit :
> On Fri, 2020-01-31 at 13:34 +0000, Christophe Leroy wrote:
>> With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be
>> one
>> W+X page at boot by default. This can be tested with
>> CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
>> kernel log during boot.
>>
>> powerpc doesn't implement its own alloc() for kprobes like other
>> architectures do, but we couldn't immediately mark RO anyway since we
>> do
>> a memcpy to the page we allocate later. After that, nothing should
>> be
>> allowed to modify the page, and write permissions are removed well
>> before the kprobe is armed.
>>
>> The memcpy() would fail if >1 probes were allocated, so use
>> patch_instruction() instead which is safe for RO.
>>
>> Reviewed-by: Daniel Axtens <dja@axtens.net>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> v2: removed the redundant flush
>> ---
>> arch/powerpc/kernel/kprobes.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/kprobes.c
>> b/arch/powerpc/kernel/kprobes.c
>> index 2d27ec4feee4..d3e594e6094c 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -24,6 +24,7 @@
>> #include <asm/sstep.h>
>> #include <asm/sections.h>
>> #include <linux/uaccess.h>
>> +#include <linux/set_memory.h>
>>
>> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
>> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>> @@ -124,13 +125,12 @@ int arch_prepare_kprobe(struct kprobe *p)
>> }
>>
>> if (!ret) {
>> - memcpy(p->ainsn.insn, p->addr,
>> - MAX_INSN_SIZE *
>> sizeof(kprobe_opcode_t));
>> + patch_instruction(p->ainsn.insn, *p->addr);
>> p->opcode = *p->addr;
>> - flush_icache_range((unsigned long)p->ainsn.insn,
>> - (unsigned long)p->ainsn.insn +
>> sizeof(kprobe_opcode_t));
>> }
>>
>> + set_memory_ro((unsigned long)p->ainsn.insn, 1);
>> +
>
>
> Since this can be called multiple times on the same page, can avoid by
> implementing:
>
> void *alloc_insn_page(void)
> {
> void *page;
>
> page = vmalloc_exec(PAGE_SIZE);
> if (page)
> set_memory_ro((unsigned long)page, 1);
>
> return page;
> }
>
> Which is pretty much the same as what's in arm64. Works for me and
> passes ftracetest, I was originally doing this but cut it because it
> broke with the memcpy, but works with patch_instruction().
>
>> p->ainsn.boostable = 0;
>> return ret;
>> }
Ok. I'll send out v3 as patch 1 fails on PPC64, so I'll take that in.
Christophe
^ permalink raw reply
* Re: [linuxppc-dev] Patch notification: 1 patch updated
From: Christophe Leroy @ 2020-02-03 6:21 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20200203010001.21489.11114@bilbo.ozlabs.org>
Le 03/02/2020 à 02:00, Patchwork a écrit :
> Hello,
>
> The following patch (submitted by you) has been updated in Patchwork:
>
> * linuxppc-dev: powerpc/nohash: Don't flush all TLBs when flushing one page
> - http://patchwork.ozlabs.org/patch/1231983/
> - for: Linux PPC development
> was: New
> now: Superseded
Superseded ? By what ?
I sent a v2 for book3s/32, but this one is for nohash.
Christophe
^ permalink raw reply
* Re: [PATCH v2 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Russell Currey @ 2020-02-03 4:50 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <01fdf1b7375b3e1e43a634bf6719b576c4c5db11.1580477672.git.christophe.leroy@c-s.fr>
On Fri, 2020-01-31 at 13:34 +0000, Christophe Leroy wrote:
> With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be
> one
> W+X page at boot by default. This can be tested with
> CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
> kernel log during boot.
>
> powerpc doesn't implement its own alloc() for kprobes like other
> architectures do, but we couldn't immediately mark RO anyway since we
> do
> a memcpy to the page we allocate later. After that, nothing should
> be
> allowed to modify the page, and write permissions are removed well
> before the kprobe is armed.
>
> The memcpy() would fail if >1 probes were allocated, so use
> patch_instruction() instead which is safe for RO.
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> v2: removed the redundant flush
> ---
> arch/powerpc/kernel/kprobes.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c
> b/arch/powerpc/kernel/kprobes.c
> index 2d27ec4feee4..d3e594e6094c 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -24,6 +24,7 @@
> #include <asm/sstep.h>
> #include <asm/sections.h>
> #include <linux/uaccess.h>
> +#include <linux/set_memory.h>
>
> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> @@ -124,13 +125,12 @@ int arch_prepare_kprobe(struct kprobe *p)
> }
>
> if (!ret) {
> - memcpy(p->ainsn.insn, p->addr,
> - MAX_INSN_SIZE *
> sizeof(kprobe_opcode_t));
> + patch_instruction(p->ainsn.insn, *p->addr);
> p->opcode = *p->addr;
> - flush_icache_range((unsigned long)p->ainsn.insn,
> - (unsigned long)p->ainsn.insn +
> sizeof(kprobe_opcode_t));
> }
>
> + set_memory_ro((unsigned long)p->ainsn.insn, 1);
> +
Since this can be called multiple times on the same page, can avoid by
implementing:
void *alloc_insn_page(void)
{
void *page;
page = vmalloc_exec(PAGE_SIZE);
if (page)
set_memory_ro((unsigned long)page, 1);
return page;
}
Which is pretty much the same as what's in arm64. Works for me and
passes ftracetest, I was originally doing this but cut it because it
broke with the memcpy, but works with patch_instruction().
> p->ainsn.boostable = 0;
> return ret;
> }
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/pseries: Account for SPURR ticks on idle CPUs
From: Gautham R Shenoy @ 2020-02-03 4:45 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R. Shenoy, Tyrel Datwyler, linux-kernel, Kamalesh Babulal,
Naveen N. Rao, Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <87o8wnu3t7.fsf@linux.ibm.com>
Hello Nathan,
On Wed, Dec 04, 2019 at 04:24:52PM -0600, Nathan Lynch wrote:
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
> > diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> > index a36fd05..708ec68 100644
> > --- a/arch/powerpc/kernel/idle.c
> > +++ b/arch/powerpc/kernel/idle.c
> > @@ -33,6 +33,8 @@
> > unsigned long cpuidle_disable = IDLE_NO_OVERRIDE;
> > EXPORT_SYMBOL(cpuidle_disable);
> >
> > +DEFINE_PER_CPU(u64, idle_spurr_cycles);
> > +
>
> Does idle_spurr_cycles need any special treatment for CPU
> online/offline?
If offline uses extended cede, then we need to take a snapshot of the
idle_spurr_cycles before going offline and add the delta once we are
back online. However, since the plan is to deprecate the use of
extended cede for CPU-Offline and use only rtas-stop-self, we don't
need any special handling there.
>
> > static int __init powersave_off(char *arg)
> > {
> > ppc_md.power_save = NULL;
> > diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> > index 74c2479..45e2be4 100644
> > --- a/drivers/cpuidle/cpuidle-pseries.c
> > +++ b/drivers/cpuidle/cpuidle-pseries.c
> > @@ -30,11 +30,14 @@ struct cpuidle_driver pseries_idle_driver = {
> > static struct cpuidle_state *cpuidle_state_table __read_mostly;
> > static u64 snooze_timeout __read_mostly;
> > static bool snooze_timeout_en __read_mostly;
> > +DECLARE_PER_CPU(u64, idle_spurr_cycles);
>
> This belongs in a header...
Will move it to the header file.
>
>
> > -static inline void idle_loop_prolog(unsigned long *in_purr)
> > +static inline void idle_loop_prolog(unsigned long *in_purr,
> > + unsigned long *in_spurr)
> > {
> > ppc64_runlatch_off();
> > *in_purr = mfspr(SPRN_PURR);
> > + *in_spurr = mfspr(SPRN_SPURR);
> > /*
> > * Indicate to the HV that we are idle. Now would be
> > * a good time to find other work to dispatch.
> > @@ -42,13 +45,16 @@ static inline void idle_loop_prolog(unsigned long *in_purr)
> > get_lppaca()->idle = 1;
> > }
> >
> > -static inline void idle_loop_epilog(unsigned long in_purr)
> > +static inline void idle_loop_epilog(unsigned long in_purr,
> > + unsigned long in_spurr)
> > {
> > u64 wait_cycles;
> > + u64 *idle_spurr_cycles_ptr = this_cpu_ptr(&idle_spurr_cycles);
> >
> > wait_cycles = be64_to_cpu(get_lppaca()->wait_state_cycles);
> > wait_cycles += mfspr(SPRN_PURR) - in_purr;
> > get_lppaca()->wait_state_cycles = cpu_to_be64(wait_cycles);
> > + *idle_spurr_cycles_ptr += mfspr(SPRN_SPURR) - in_spurr;
>
> ... and the sampling and increment logic probably should be further
> encapsulated in accessor functions that can be used in both the cpuidle
> driver and the default/generic idle implementation. Or is there some
> reason this is specific to the pseries cpuidle driver?
I am not sure if we use SPURR and PURR for performing accounting on
Bare-Metal systems. IIUC, the patches proposed by Kamalesh is only to
use idle_[s]purr and [s]purr on POWERVM LPARs. This is why I coded the
sampling/increment logic in the pseries cpuidle driver. But you are
right, in the absence of cpuidle, when we use the default idle
implementation, we will still need to note the value of
idle_purr/spurr.
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/sysfs: Show idle_purr and idle_spurr for every CPU
From: Gautham R Shenoy @ 2020-02-03 4:50 UTC (permalink / raw)
To: Naveen N. Rao
Cc: Nathan Lynch, Gautham R. Shenoy, Tyrel Datwyler, linux-kernel,
Kamalesh Babulal, Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <1575564547.si4rk0s96p.naveen@linux.ibm.com>
Hi Naveen,
On Thu, Dec 05, 2019 at 10:23:58PM +0530, Naveen N. Rao wrote:
> >diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> >index 80a676d..42ade55 100644
> >--- a/arch/powerpc/kernel/sysfs.c
> >+++ b/arch/powerpc/kernel/sysfs.c
> >@@ -1044,6 +1044,36 @@ static ssize_t show_physical_id(struct device *dev,
> > }
> > static DEVICE_ATTR(physical_id, 0444, show_physical_id, NULL);
> >
> >+static ssize_t idle_purr_show(struct device *dev,
> >+ struct device_attribute *attr, char *buf)
> >+{
> >+ struct cpu *cpu = container_of(dev, struct cpu, dev);
> >+ unsigned int cpuid = cpu->dev.id;
> >+ struct lppaca *cpu_lppaca_ptr = paca_ptrs[cpuid]->lppaca_ptr;
> >+ u64 idle_purr_cycles = be64_to_cpu(cpu_lppaca_ptr->wait_state_cycles);
> >+
> >+ return sprintf(buf, "%llx\n", idle_purr_cycles);
> >+}
> >+static DEVICE_ATTR_RO(idle_purr);
> >+
> >+DECLARE_PER_CPU(u64, idle_spurr_cycles);
> >+static ssize_t idle_spurr_show(struct device *dev,
> >+ struct device_attribute *attr, char *buf)
> >+{
> >+ struct cpu *cpu = container_of(dev, struct cpu, dev);
> >+ unsigned int cpuid = cpu->dev.id;
> >+ u64 *idle_spurr_cycles_ptr = per_cpu_ptr(&idle_spurr_cycles, cpuid);
>
> Is it possible for a user to read stale values if a particular cpu is in an
> extended cede? Is it possible to use smp_call_function_single() to force the
> cpu out of idle?
Yes, if the CPU whose idle_spurr cycle is being read is still in idle,
then we will miss reporting the delta spurr cycles for this last
idle-duration. Yes, we can use an smp_call_function_single(), though
that will introduce IPI noise. How often will idle_[s]purr be read ?
>
> - Naveen
>
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/sysfs: Show idle_purr and idle_spurr for every CPU
From: Gautham R Shenoy @ 2020-02-03 4:47 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R. Shenoy, Tyrel Datwyler, linux-kernel, Kamalesh Babulal,
Naveen N. Rao, Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <87pnh3u3ts.fsf@linux.ibm.com>
Hello Nathan,
On Wed, Dec 04, 2019 at 04:24:31PM -0600, Nathan Lynch wrote:
> "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
> > @@ -1067,6 +1097,8 @@ static int __init topology_init(void)
> > register_cpu(c, cpu);
> >
> > device_create_file(&c->dev, &dev_attr_physical_id);
> > + if (firmware_has_feature(FW_FEATURE_SPLPAR))
> > + create_idle_purr_spurr_sysfs_entry(&c->dev);
>
> Architecturally speaking PURR/SPURR aren't strongly linked to the PAPR
> SPLPAR option, are they? I'm not sure it's right for these attributes to
> be absent if the platform does not support shared processor mode.
Doesn't FW_FEATURE_SPLPAR refer to all Pseries guests ? It is perhaps
incorrectly named, but from the other uses in the kernel, it seems to
indicate that we are running as a guest instead of on a bare-metal
system.
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH v2 1/7] powerpc/mm: Implement set_memory() routines
From: kbuild test robot @ 2020-02-03 3:45 UTC (permalink / raw)
To: Christophe Leroy; +Cc: kbuild-all, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <84be5ad6a996adf5693260749dcb4d8c69182073.1580477672.git.christophe.leroy@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 3739 bytes --]
Hi Christophe,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20200131]
[cannot apply to v5.5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-mm-Implement-set_memory-routines/20200203-060234
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ppc64_defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/powerpc/mm/pageattr.c: In function 'change_page_attr':
>> arch/powerpc/mm/pageattr.c:32:15: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
int action = (int)data;
^
arch/powerpc/mm/pageattr.c: In function 'change_memory_attr':
>> arch/powerpc/mm/pageattr.c:73:68: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
^
cc1: all warnings being treated as errors
vim +32 arch/powerpc/mm/pageattr.c
15
16
17 /*
18 * Updates the attributes of a page in three steps:
19 *
20 * 1. invalidate the page table entry
21 * 2. flush the TLB
22 * 3. install the new entry with the updated attributes
23 *
24 * This is unsafe if the caller is attempting to change the mapping of the
25 * page it is executing from, or if another CPU is concurrently using the
26 * page being altered.
27 *
28 * TODO make the implementation resistant to this.
29 */
30 static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
31 {
> 32 int action = (int)data;
33 pte_t pte;
34
35 spin_lock(&init_mm.page_table_lock);
36
37 /* invalidate the PTE so it's safe to modify */
38 pte = ptep_get_and_clear(&init_mm, addr, ptep);
39 flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
40
41 /* modify the PTE bits as desired, then apply */
42 switch (action) {
43 case SET_MEMORY_RO:
44 pte = pte_wrprotect(pte);
45 break;
46 case SET_MEMORY_RW:
47 pte = pte_mkwrite(pte);
48 break;
49 case SET_MEMORY_NX:
50 pte = pte_exprotect(pte);
51 break;
52 case SET_MEMORY_X:
53 pte = pte_mkexec(pte);
54 break;
55 default:
56 break;
57 }
58
59 set_pte_at(&init_mm, addr, ptep, pte);
60 spin_unlock(&init_mm.page_table_lock);
61
62 return 0;
63 }
64
65 int change_memory_attr(unsigned long addr, int numpages, int action)
66 {
67 unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
68 unsigned long sz = numpages * PAGE_SIZE;
69
70 if (!numpages)
71 return 0;
72
> 73 return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25677 bytes --]
^ permalink raw reply
* Re: [PATCH v6 1/5] powerpc/mm: Implement set_memory() routines
From: Russell Currey @ 2020-02-03 0:46 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel, dja
In-Reply-To: <8f8940e2-c6ab-fca2-ab8a-61b80b2edd22@c-s.fr>
On Wed, 2020-01-08 at 13:52 +0100, Christophe Leroy wrote:
>
> Le 24/12/2019 à 06:55, Russell Currey a écrit :
> > The set_memory_{ro/rw/nx/x}() functions are required for
> > STRICT_MODULE_RWX,
> > and are generally useful primitives to have. This implementation
> > is
> > designed to be completely generic across powerpc's many MMUs.
> >
> > It's possible that this could be optimised to be faster for
> > specific
> > MMUs, but the focus is on having a generic and safe implementation
> > for
> > now.
> >
> > This implementation does not handle cases where the caller is
> > attempting
> > to change the mapping of the page it is executing from, or if
> > another
> > CPU is concurrently using the page being altered. These cases
> > likely
> > shouldn't happen, but a more complex implementation with MMU-
> > specific code
> > could safely handle them, so that is left as a TODO for now.
> >
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> > arch/powerpc/Kconfig | 1 +
> > arch/powerpc/include/asm/set_memory.h | 32 +++++++++++
> > arch/powerpc/mm/Makefile | 1 +
> > arch/powerpc/mm/pageattr.c | 83
> > +++++++++++++++++++++++++++
> > 4 files changed, 117 insertions(+)
> > create mode 100644 arch/powerpc/include/asm/set_memory.h
> > create mode 100644 arch/powerpc/mm/pageattr.c
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 1ec34e16ed65..f0b9b47b5353 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -133,6 +133,7 @@ config PPC
> > select ARCH_HAS_PTE_SPECIAL
> > select ARCH_HAS_MEMBARRIER_CALLBACKS
> > select ARCH_HAS_SCALED_CPUTIME if
> > VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
> > + select ARCH_HAS_SET_MEMORY
> > select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 ||
> > PPC32) && !RELOCATABLE && !HIBERNATION)
> > select ARCH_HAS_TICK_BROADCAST if
> > GENERIC_CLOCKEVENTS_BROADCAST
> > select ARCH_HAS_UACCESS_FLUSHCACHE
> > diff --git a/arch/powerpc/include/asm/set_memory.h
> > b/arch/powerpc/include/asm/set_memory.h
> > new file mode 100644
> > index 000000000000..5230ddb2fefd
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/set_memory.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_POWERPC_SET_MEMORY_H
> > +#define _ASM_POWERPC_SET_MEMORY_H
> > +
> > +#define SET_MEMORY_RO 1
> > +#define SET_MEMORY_RW 2
> > +#define SET_MEMORY_NX 3
> > +#define SET_MEMORY_X 4
>
> Maybe going from 0 to 3 would be better than 1 to 4
>
> > +
> > +int change_memory_attr(unsigned long addr, int numpages, int
> > action);
>
> action could be unsigned.
>
> > +
> > +static inline int set_memory_ro(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_RO);
> > +}
> > +
> > +static inline int set_memory_rw(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_RW);
> > +}
> > +
> > +static inline int set_memory_nx(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_NX);
> > +}
> > +
> > +static inline int set_memory_x(unsigned long addr, int numpages)
> > +{
> > + return change_memory_attr(addr, numpages, SET_MEMORY_X);
> > +}
> > +
> > +#endif
> > diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
> > index 5e147986400d..d0a0bcbc9289 100644
> > --- a/arch/powerpc/mm/Makefile
> > +++ b/arch/powerpc/mm/Makefile
> > @@ -20,3 +20,4 @@ obj-$(CONFIG_HIGHMEM) += highmem.o
> > obj-$(CONFIG_PPC_COPRO_BASE) += copro_fault.o
> > obj-$(CONFIG_PPC_PTDUMP) += ptdump/
> > obj-$(CONFIG_KASAN) += kasan/
> > +obj-$(CONFIG_ARCH_HAS_SET_MEMORY) += pageattr.o
>
> CONFIG_ARCH_HAS_SET_MEMORY is set inconditionnally, I think you
> should
> add pageattr.o to obj-y instead. CONFIG_ARCH_HAS_XXX are almost
> never
> used in Makefiles
Fair enough, will keep that in mind
>
> > diff --git a/arch/powerpc/mm/pageattr.c
> > b/arch/powerpc/mm/pageattr.c
> > new file mode 100644
> > index 000000000000..15d5fb04f531
> > --- /dev/null
> > +++ b/arch/powerpc/mm/pageattr.c
> > @@ -0,0 +1,83 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * MMU-generic set_memory implementation for powerpc
> > + *
> > + * Copyright 2019, IBM Corporation.
> > + */
> > +
> > +#include <linux/mm.h>
> > +#include <linux/set_memory.h>
> > +
> > +#include <asm/mmu.h>
> > +#include <asm/page.h>
> > +#include <asm/pgtable.h>
> > +
> > +
> > +/*
> > + * Updates the attributes of a page in three steps:
> > + *
> > + * 1. invalidate the page table entry
> > + * 2. flush the TLB
> > + * 3. install the new entry with the updated attributes
> > + *
> > + * This is unsafe if the caller is attempting to change the
> > mapping of the
> > + * page it is executing from, or if another CPU is concurrently
> > using the
> > + * page being altered.
> > + *
> > + * TODO make the implementation resistant to this.
> > + */
> > +static int __change_page_attr(pte_t *ptep, unsigned long addr,
> > void *data)
> > +{
> > + int action = *((int *)data);
>
> Don't use pointers for so simple things, pointers forces the compiler
> to
> setup a stack frame and save the data into stack. Instead do:
>
> int action = (int)data;
>
> > + pte_t pte_val;
> > +
> > + // invalidate the PTE so it's safe to modify
> > + pte_val = ptep_get_and_clear(&init_mm, addr, ptep);
> > + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>
> Why flush a range for a single page ? On most targets this will do a
> tlbia which is heavy, while a tlbie would suffice.
>
> I think flush_tlb_kernel_range() should be replaced by something
> flushing only a single page.
You might be able to help me out here, I wanted to do that but the only
functions I could find that flushed single pages needed a
vm_area_struct, which I can't get.
>
> > +
> > + // modify the PTE bits as desired, then apply
> > + switch (action) {
> > + case SET_MEMORY_RO:
> > + pte_val = pte_wrprotect(pte_val);
> > + break;
> > + case SET_MEMORY_RW:
> > + pte_val = pte_mkwrite(pte_val);
> > + break;
> > + case SET_MEMORY_NX:
> > + pte_val = pte_exprotect(pte_val);
> > + break;
> > + case SET_MEMORY_X:
> > + pte_val = pte_mkexec(pte_val);
> > + break;
> > + default:
> > + WARN_ON(true);
> > + return -EINVAL;
>
> Is it worth checking that the action is valid for each page ? I
> think
> validity of action should be checked in change_memory_attr(). All
> other
> functions are static so you know they won't be called from outside.
>
> Once done, you can squash __change_page_attr() into
> change_page_attr(),
> remove the ret var and return 0 all the time.
Makes sense to fold things into a single function, but in terms of
performance it shouldn't make a difference, right? I still have to
check the action to determine what to change (unless I replace passing
SET_MEMORY_RO into apply_to_page_range() with a function pointer to
pte_wrprotect() for example).
>
> > + }
> > +
> > + set_pte_at(&init_mm, addr, ptep, pte_val);
> > +
> > + return 0;
> > +}
> > +
> > +static int change_page_attr(pte_t *ptep, unsigned long addr, void
> > *data)
> > +{
> > + int ret;
> > +
> > + spin_lock(&init_mm.page_table_lock);
> > + ret = __change_page_attr(ptep, addr, data);
> > + spin_unlock(&init_mm.page_table_lock);
> > +
> > + return ret;
> > +}
> > +
> > +int change_memory_attr(unsigned long addr, int numpages, int
> > action)
> > +{
> > + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> > + unsigned long size = numpages * PAGE_SIZE;
> > +
> > + if (!numpages)
> > + return 0;
> > +
> > + return apply_to_page_range(&init_mm, start, size,
> > change_page_attr, &action);
>
> Use (void*)action instead of &action (see upper comment)
To get this to work I had to use (void *)(size_t)action to stop the
compiler from complaining about casting an int to a void*, is there a
better way to go about it? Works fine, just looks gross.
>
> > +}
> >
>
> Christophe
>
^ permalink raw reply
* Re: Latest Git kernel: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
From: Christian Zigotzky @ 2020-02-02 15:02 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, DTML, Darren Stevens,
Linux Kernel Mailing List, linuxppc-dev, contact@a-eon.com,
R.T.Dickinson, Christoph Hellwig, mad skateman,
netdev@vger.kernel.org
In-Reply-To: <75aab3c9-1cb6-33bf-5de1-e05bbd98b6fb@c-s.fr>
[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]
On 02 February 2020 at 09:19 am, Christophe Leroy wrote:
> Hello,
>
> Le 02/02/2020 à 01:08, Christian Zigotzky a écrit :
>> Hello,
>>
>> We regularly compile and test Linux kernels every day during the
>> merge window. Since Thuesday we have very high CPU loads because of
>> the avahi daemon on our desktop Linux systems (Ubuntu, Debian etc).
>>
>> Error message: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for
>> device
>
> Do you know which ioctl, on which device ?
> Can you take a trace of running avahi-daemon with 'strace' ?
>
> Can you bisect ?
>
> Christophe
Hi Christophe,
Hi All,
I figured out that the avahi-daemon has a problem with the IPv6 address
of a network interface since the Git kernel from Thursday. (Log attached)
This generates high CPU usage because the avahi-daemon tries to access
the IPv6 address again and again and thereby it produces a lot of log
messages.
We figured out that the networking updates aren't responsible for this
issue because we created a test kernel on Wednesday. The issue is
somewhere in the commits from Wednesday night to Thursday (CET).
Please compile the latest Git kernel and test it with a desktop linux
distribution for example Ubuntu. In my point of view there are many
desktop machines affected. Many server systems don't use the avahi
daemon so they aren't affected.
It's possible to deactivate the access to the IPv6 address with the
following line in the file "/etc/avahi/avahi-daemon.conf":
use-ipv6=no
After a reboot the CPU usage is normal again. This is only a temporary
solution.
Unfortunately I don't have the time for bisecting next week. I have a
lot of other work to do. In my point of view it is very important that
you also compile the latest Git kernels. Then you will see the issue and
then you have a better possibility to fix the issue.
Thanks,
Christian
[-- Attachment #2: avahi_log --]
[-- Type: text/plain, Size: 8831 bytes --]
Kernel 5.5.0: journalctl | grep -i avahi
Feb 02 13:57:05 DC1 systemd[1]: Listening on Avahi mDNS/DNS-SD Stack Activation Socket.
Feb 02 13:57:05 DC1 systemd[1]: Starting Avahi mDNS/DNS-SD Stack...
Feb 02 13:57:05 DC1 avahi-daemon[4314]: Found user 'avahi' (UID 112) and group 'avahi' (GID 122).
Feb 02 13:57:05 DC1 avahi-daemon[4314]: Successfully dropped root privileges.
Feb 02 13:57:05 DC1 avahi-daemon[4314]: avahi-daemon 0.6.32-rc starting up.
Feb 02 13:57:06 DC1 systemd[1]: Started Avahi DNS Configuration Daemon.
Feb 02 13:57:06 DC1 avahi-daemon[4314]: Successfully called chroot().
Feb 02 13:57:06 DC1 avahi-daemon[4314]: Successfully dropped remaining capabilities.
Feb 02 13:57:06 DC1 avahi-daemon[4314]: No service file found in /etc/avahi/services.
Feb 02 13:57:06 DC1 avahi-daemon[4314]: Network interface enumeration completed.
Feb 02 13:57:06 DC1 avahi-daemon[4314]: Server startup complete. Host name is DC1.local. Local service cookie is 3202921551.
Feb 02 13:57:06 DC1 avahi-daemon[4314]: Failed to parse address 'localhost', ignoring.
Feb 02 13:57:06 DC1 avahi-dnsconfd[4487]: Successfully connected to Avahi daemon.
Feb 02 13:57:06 DC1 systemd[1]: Started Avahi mDNS/DNS-SD Stack.
Feb 02 13:57:07 DC1 root[4749]: /etc/dhcp/dhclient-enter-hooks.d/avahi-autoipd returned non-zero exit status 1
Feb 02 13:57:07 DC1 avahi-daemon[4314]: Joining mDNS multicast group on interface enP4096p4s4.IPv4 with address 192.168.178.47.
Feb 02 13:57:07 DC1 avahi-daemon[4314]: New relevant interface enP4096p4s4.IPv4 for mDNS.
Feb 02 13:57:07 DC1 avahi-daemon[4314]: Registering new address record for 192.168.178.47 on enP4096p4s4.IPv4.
Feb 02 13:57:09 DC1 avahi-daemon[4314]: Joining mDNS multicast group on interface enP4096p4s4.IPv6 with address fe80::250:fcff:fecb:5181.
Feb 02 13:57:09 DC1 avahi-daemon[4314]: New relevant interface enP4096p4s4.IPv6 for mDNS.
Feb 02 13:57:09 DC1 avahi-daemon[4314]: Registering new address record for fe80::250:fcff:fecb:5181 on enP4096p4s4.*.
Feb 02 13:57:10 DC1 avahi-daemon[4314]: Leaving mDNS multicast group on interface enP4096p4s4.IPv6 with address fe80::250:fcff:fecb:5181.
Feb 02 13:57:10 DC1 avahi-daemon[4314]: Joining mDNS multicast group on interface enP4096p4s4.IPv6 with address 2a02:8109:89c0:ebfc:250:fcff:fecb:5181.
Feb 02 13:57:10 DC1 avahi-daemon[4314]: Registering new address record for 2a02:8109:89c0:ebfc:250:fcff:fecb:5181 on enP4096p4s4.*.
Feb 02 13:57:10 DC1 avahi-daemon[4314]: Withdrawing address record for fe80::250:fcff:fecb:5181 on enP4096p4s4.
------
Latest Git kernel (5.6): journalctl | grep -i avahi
Feb 02 14:04:04 DC1 systemd[1]: Listening on Avahi mDNS/DNS-SD Stack Activation Socket.
Feb 02 14:04:05 DC1 systemd[1]: Started Avahi DNS Configuration Daemon.
Feb 02 14:04:05 DC1 systemd[1]: Starting Avahi mDNS/DNS-SD Stack...
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Found user 'avahi' (UID 112) and group 'avahi' (GID 122).
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Successfully dropped root privileges.
Feb 02 14:04:05 DC1 avahi-daemon[4573]: avahi-daemon 0.6.32-rc starting up.
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Successfully called chroot().
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Successfully dropped remaining capabilities.
Feb 02 14:04:05 DC1 avahi-daemon[4573]: No service file found in /etc/avahi/services.
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Network interface enumeration completed.
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Server startup complete. Host name is DC1.local. Local service cookie is 285370789.
Feb 02 14:04:05 DC1 avahi-daemon[4573]: Failed to parse address 'localhost', ignoring.
Feb 02 14:04:05 DC1 avahi-dnsconfd[4425]: Successfully connected to Avahi daemon.
Feb 02 14:04:05 DC1 root[4642]: /etc/dhcp/dhclient-enter-hooks.d/avahi-autoipd returned non-zero exit status 1
Feb 02 14:04:06 DC1 systemd[1]: Started Avahi mDNS/DNS-SD Stack.
Feb 02 14:04:06 DC1 avahi-daemon[4573]: Joining mDNS multicast group on interface enP4096p4s4.IPv4 with address 192.168.178.47.
Feb 02 14:04:06 DC1 avahi-daemon[4573]: New relevant interface enP4096p4s4.IPv4 for mDNS.
Feb 02 14:04:06 DC1 avahi-daemon[4573]: Registering new address record for 192.168.178.47 on enP4096p4s4.IPv4.
Feb 02 14:04:08 DC1 avahi-daemon[4573]: Joining mDNS multicast group on interface enP4096p4s4.IPv6 with address fe80::250:fcff:fecb:5181.
Feb 02 14:04:08 DC1 avahi-daemon[4573]: New relevant interface enP4096p4s4.IPv6 for mDNS.
Feb 02 14:04:08 DC1 avahi-daemon[4573]: Registering new address record for fe80::250:fcff:fecb:5181 on enP4096p4s4.*.
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
...
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:08 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:09 DC1 avahi-daemon[4573]: Leaving mDNS multicast group on interface enP4096p4s4.IPv6 with address fe80::250:fcff:fecb:5181.
Feb 02 14:04:09 DC1 avahi-daemon[4573]: Joining mDNS multicast group on interface enP4096p4s4.IPv6 with address 2a02:8109:89c0:ebfc:250:fcff:fecb:5181.
Feb 02 14:04:09 DC1 avahi-daemon[4573]: Registering new address record for 2a02:8109:89c0:ebfc:250:fcff:fecb:5181 on enP4096p4s4.*.
Feb 02 14:04:09 DC1 avahi-daemon[4573]: Withdrawing address record for fe80::250:fcff:fecb:5181 on enP4096p4s4.
Feb 02 14:04:28 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:28 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:28 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:28 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:28 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:28 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
...
Feb 02 14:04:29 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:29 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:29 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:35 DC1 systemd-journald[2489]: Suppressed 513915 messages from /system.slice/avahi-daemon.service
Feb 02 14:04:35 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:35 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:04:35 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
------
Latest Git kernel (5.6): systemctl status avahi-daemon
● avahi-daemon.service - Avahi mDNS/DNS-SD Stack
Loaded: loaded (/lib/systemd/system/avahi-daemon.service; enabled; vendor preset: enabled)
Active: active (running) since Sun 2020-02-02 14:04:06 CET; 11min ago
Main PID: 4573 (avahi-daemon)
Status: "avahi-daemon 0.6.32-rc starting up."
CGroup: /system.slice/avahi-daemon.service
├─4573 avahi-daemon: running [DC1.local]
└─4581 avahi-daemon: chroot helper
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
Feb 02 14:15:34 DC1 avahi-daemon[4573]: ioctl(): Inappropriate ioctl for device
------
^ permalink raw reply
* Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
From: Qian Cai @ 2020-02-02 11:26 UTC (permalink / raw)
To: Christophe Leroy
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Tetsuo Handa, Heiko Carstens, Michal Hocko, linux-mm,
Paul Mackerras, kasan-dev, sparclinux, Thomas Gleixner,
linux-s390, x86, Russell King - ARM Linux, Matthew Wilcox,
Steven Price, Jason Gunthorpe, Gerald Schaefer, linux-snps-arc,
Ingo Molnar, Kees Cook, Anshuman Khandual, Masahiro Yamada,
Mark Brown, Kirill A . Shutemov, Dan Williams, Vlastimil Babka,
linux-arm-kernel, Sri Krishna chowdary, Ard Biesheuvel,
Greg Kroah-Hartman, Dave Hansen, linux-mips, Ralf Baechle,
linux-kernel, Paul Burton, Mike Rapoport, Vineet Gupta,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <473d8198-3ac4-af3b-e2ec-c0698a3565d3@c-s.fr>
> On Jan 30, 2020, at 9:13 AM, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
> config DEBUG_VM_PGTABLE
> bool "Debug arch page table for semantics compliance" if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
> depends on MMU
> default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE
> default 'y' if DEBUG_VM
Does it really necessary to potentially force all bots to run this? Syzbot, kernel test robot etc? Does it ever pay off for all their machine times there?
^ permalink raw reply
* Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
From: Christophe Leroy @ 2020-02-02 8:31 UTC (permalink / raw)
To: Anshuman Khandual, linux-mm
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, Michal Hocko, Dave Hansen, Paul Mackerras,
sparclinux, Thomas Gleixner, linux-s390, Jason Gunthorpe, x86,
Russell King - ARM Linux, Matthew Wilcox, Steven Price,
Tetsuo Handa, Gerald Schaefer, linux-snps-arc, Ingo Molnar,
Kees Cook, Masahiro Yamada, Mark Brown, Kirill A . Shutemov,
Dan Williams, Vlastimil Babka, linux-arm-kernel,
Sri Krishna chowdary, Ard Biesheuvel, Greg Kroah-Hartman,
linux-mips, Ralf Baechle, linux-kernel, Paul Burton,
Mike Rapoport, Vineet Gupta, Martin Schwidefsky, Andrew Morton,
linuxppc-dev, David S. Miller
In-Reply-To: <ad5ad414-3524-2efa-df16-1ee357e6e2e4@arm.com>
Le 02/02/2020 à 08:18, Anshuman Khandual a écrit :
> On 01/30/2020 07:43 PM, Christophe Leroy wrote:
>>
>>
>> Le 30/01/2020 à 14:04, Anshuman Khandual a écrit :
>>>
>>> On 01/28/2020 10:35 PM, Christophe Leroy wrote:
>>
>>>
>>>> I think we could make it standalone and 'default y if DEBUG_VM' instead.
>>>
>>> Which will yield the same result like before but in a different way. But
>>> yes, this test could go about either way but unless there is a good enough
>>> reason why change the current one.
>>
>> I think if we want people to really use it on other architectures it must be possible to activate it without having to modify Kconfig. Otherwise people won't even know the test exists and the architecture fails the test.
>>
>> The purpose of a test suite is to detect bugs. If you can't run the test until you have fixed the bugs, I guess nobody will ever detect the bugs and they will never be fixed.
>>
>> So I think:
>> - the test should be 'default y' when ARCH_HAS_DEBUG_VM_PGTABLE is selected
>> - the test should be 'default n' when ARCH_HAS_DEBUG_VM_PGTABLE is not selected, and it should be user selectable if EXPERT is selected.
>>
>> Something like:
>>
>> config DEBUG_VM_PGTABLE
>> bool "Debug arch page table for semantics compliance" if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
>> depends on MMU
>
> (ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT) be moved along side MMU on the same line ?
Yes could also go along side MMU, or could be a depend by itself:
depends on ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
>
>> default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE
>> default 'y' if DEBUG_VM
>
> This looks good, at least until we get all platforms enabled. Will do all these
> changes along with s390 enablement and re-spin.
Christophe
^ permalink raw reply
* Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
From: Anshuman Khandual @ 2020-02-02 8:26 UTC (permalink / raw)
To: Christophe Leroy, linux-mm
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, Michal Hocko, Dave Hansen, Paul Mackerras,
sparclinux, Thomas Gleixner, linux-s390, Jason Gunthorpe, x86,
Russell King - ARM Linux, Matthew Wilcox, Steven Price,
Tetsuo Handa, Gerald Schaefer, linux-snps-arc, Ingo Molnar,
Kees Cook, Masahiro Yamada, Mark Brown, Kirill A . Shutemov,
Dan Williams, Vlastimil Babka, linux-arm-kernel,
Sri Krishna chowdary, Ard Biesheuvel, Greg Kroah-Hartman,
linux-mips, Ralf Baechle, linux-kernel, Paul Burton,
Mike Rapoport, Vineet Gupta, Martin Schwidefsky, Andrew Morton,
linuxppc-dev, David S. Miller
In-Reply-To: <49754f74-53a7-0e4a-bb16-53617f8c902c@arm.com>
On 01/30/2020 06:34 PM, Anshuman Khandual wrote:
> On 01/28/2020 10:35 PM, Christophe Leroy wrote:
>>
>> Le 28/01/2020 à 02:27, Anshuman Khandual a écrit :
>>> This adds tests which will validate architecture page table helpers and
>>> other accessors in their compliance with expected generic MM semantics.
>>> This will help various architectures in validating changes to existing
>>> page table helpers or addition of new ones.
>>>
>>> This test covers basic page table entry transformations including but not
>>> limited to old, young, dirty, clean, write, write protect etc at various
>>> level along with populating intermediate entries with next page table page
>>> and validating them.
>>>
>>> Test page table pages are allocated from system memory with required size
>>> and alignments. The mapped pfns at page table levels are derived from a
>>> real pfn representing a valid kernel text symbol. This test gets called
>>> right after page_alloc_init_late().
>>>
>>> This gets build and run when CONFIG_DEBUG_VM_PGTABLE is selected along with
>>> CONFIG_VM_DEBUG. Architectures willing to subscribe this test also need to
>>> select CONFIG_ARCH_HAS_DEBUG_VM_PGTABLE which for now is limited to x86 and
>>> arm64. Going forward, other architectures too can enable this after fixing
>>> build or runtime problems (if any) with their page table helpers.
>>>
>>> Folks interested in making sure that a given platform's page table helpers
>>> conform to expected generic MM semantics should enable the above config
>>> which will just trigger this test during boot. Any non conformity here will
>>> be reported as an warning which would need to be fixed. This test will help
>>> catch any changes to the agreed upon semantics expected from generic MM and
>>> enable platforms to accommodate it thereafter.
>>>
>> [...]
>>
>>> Tested-by: Christophe Leroy <christophe.leroy@c-s.fr> #PPC32
>> Also tested on PPC64 (under QEMU): book3s/64 64k pages, book3s/64 4k pages and book3e/64
> Hmm but earlier Michael Ellerman had reported some problems while
> running these tests on PPC64, a soft lock up in hash__pte_update()
> and a kernel BUG (radix MMU). Are those problems gone away now ?
>
> Details in this thread - https://patchwork.kernel.org/patch/11214603/
>
It is always better to have more platforms enabled than not. But lets keep
this test disabled on PPC64 for now, if there is any inconsistency between
results while running this under QEMU and on actual systems.
^ permalink raw reply
* Re: Latest Git kernel: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
From: Christophe Leroy @ 2020-02-02 8:19 UTC (permalink / raw)
To: Christian Zigotzky, Michael Ellerman
Cc: DTML, Darren Stevens, Linux Kernel Mailing List, linuxppc-dev,
contact@a-eon.com, R.T.Dickinson, Christoph Hellwig
In-Reply-To: <58a6d45c-0712-18df-1b14-2f04cf12a1cb@xenosoft.de>
Hello,
Le 02/02/2020 à 01:08, Christian Zigotzky a écrit :
> Hello,
>
> We regularly compile and test Linux kernels every day during the merge
> window. Since Thuesday we have very high CPU loads because of the avahi
> daemon on our desktop Linux systems (Ubuntu, Debian etc).
>
> Error message: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
Do you know which ioctl, on which device ?
Can you take a trace of running avahi-daemon with 'strace' ?
Can you bisect ?
Christophe
^ permalink raw reply
* Re: [PATCH V12] mm/debug: Add tests validating architecture page table helpers
From: Anshuman Khandual @ 2020-02-02 7:18 UTC (permalink / raw)
To: Christophe Leroy, linux-mm
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, Michal Hocko, Dave Hansen, Paul Mackerras,
sparclinux, Thomas Gleixner, linux-s390, Jason Gunthorpe, x86,
Russell King - ARM Linux, Matthew Wilcox, Steven Price,
Tetsuo Handa, Gerald Schaefer, linux-snps-arc, Ingo Molnar,
Kees Cook, Masahiro Yamada, Mark Brown, Kirill A . Shutemov,
Dan Williams, Vlastimil Babka, linux-arm-kernel,
Sri Krishna chowdary, Ard Biesheuvel, Greg Kroah-Hartman,
linux-mips, Ralf Baechle, linux-kernel, Paul Burton,
Mike Rapoport, Vineet Gupta, Martin Schwidefsky, Andrew Morton,
linuxppc-dev, David S. Miller
In-Reply-To: <473d8198-3ac4-af3b-e2ec-c0698a3565d3@c-s.fr>
On 01/30/2020 07:43 PM, Christophe Leroy wrote:
>
>
> Le 30/01/2020 à 14:04, Anshuman Khandual a écrit :
>>
>> On 01/28/2020 10:35 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 28/01/2020 à 02:27, Anshuman Khandual a écrit :
>>>> diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
>>>> index 0b6c4042942a..fb0e76d254b3 100644
>>>> --- a/arch/x86/include/asm/pgtable_64.h
>>>> +++ b/arch/x86/include/asm/pgtable_64.h
>>>> @@ -53,6 +53,12 @@ static inline void sync_initial_page_table(void) { }
>>>> struct mm_struct;
>>>> +#define mm_p4d_folded mm_p4d_folded
>>>> +static inline bool mm_p4d_folded(struct mm_struct *mm)
>>>> +{
>>>> + return !pgtable_l5_enabled();
>>>> +}
>>>> +
>>>
>>> For me this should be part of another patch, it is not directly linked to the tests.
>>
>> We did discuss about this earlier and Kirril mentioned its not worth
>> a separate patch.
>>
>> https://lore.kernel.org/linux-arm-kernel/20190913091305.rkds4f3fqv3yjhjy@box/
>
> For me it would make sense to not mix this patch which implement tests, and changes that are needed for the test to work (or even build) on the different architectures.
>
> But that's up to you.
>
>>
>>>
>>>> void set_pte_vaddr_p4d(p4d_t *p4d_page, unsigned long vaddr, pte_t new_pte);
>>>> void set_pte_vaddr_pud(pud_t *pud_page, unsigned long vaddr, pte_t new_pte);
>>>> diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
>>>> index 798ea36a0549..e0b04787e789 100644
>>>> --- a/include/asm-generic/pgtable.h
>>>> +++ b/include/asm-generic/pgtable.h
>>>> @@ -1208,6 +1208,12 @@ static inline bool arch_has_pfn_modify_check(void)
>>>> # define PAGE_KERNEL_EXEC PAGE_KERNEL
>>>> #endif
>>>> +#ifdef CONFIG_DEBUG_VM_PGTABLE
>>>
>>> Not sure it is a good idea to put that in include/asm-generic/pgtable.h
>>
>> Logically that is the right place, as it is related to page table but
>> not something platform related.
>
> I can't see any debug related features in that file.
>
>>
>>>
>>> By doing this you are forcing a rebuild of almost all files, whereas only init/main.o and mm/debug_vm_pgtable.o should be rebuilt when activating this config option.
>>
>> I agreed but whats the alternative ? We could move these into init/main.c
>> to make things simpler but will that be a right place, given its related
>> to generic page table.
>
> What about linux/mmdebug.h instead ? (I have not checked if it would reduce the impact, but that's where things related to CONFIG_DEBUG_VM seems to be).
>
> Otherwise, you can just create new file, for instance <linux/mmdebug-pgtable.h> and include that file only in the init/main.c and mm/debug_vm_pgtable.c
IMHO it might not be wise to add yet another header file for this purpose.
Instead lets use <linux/mmdebug.h> in line with DEBUG_VM, DEBUG_VM_PGFLAGS,
DEBUG_VIRTUAL (which is also a stand alone test). A simple grep shows that
the impact of mmdebug.h would be less than generic pgtable.h header.
>
>
>
>>
>>>
>>>> +extern void debug_vm_pgtable(void);
>>>
>>> Please don't use the 'extern' keyword, it is useless and not to be used for functions declaration.
>>
>> Really ? But, there are tons of examples doing the same thing both in
>> generic and platform code as well.
>
> Yes, but how can we improve if we blindly copy the errors from the past ? Having tons of 'extern' doesn't mean we must add more.
>
> I think checkpatch.pl usually complains when a patch brings a new unreleval extern symbol.
Sure np, will drop it. But checkpatch.pl never complained.
>
>>
>>>
>>>> +#else
>>>> +static inline void debug_vm_pgtable(void) { }
>>>> +#endif
>>>> +
>>>> #endif /* !__ASSEMBLY__ */
>>>> #ifndef io_remap_pfn_range
>>>> diff --git a/init/main.c b/init/main.c
>>>> index da1bc0b60a7d..5e59e6ac0780 100644
>>>> --- a/init/main.c
>>>> +++ b/init/main.c
>>>> @@ -1197,6 +1197,7 @@ static noinline void __init kernel_init_freeable(void)
>>>> sched_init_smp();
>>>> page_alloc_init_late();
>>>> + debug_vm_pgtable();
>>>
>>> Wouldn't it be better to call debug_vm_pgtable() in kernel_init() between the call to async_synchronise_full() and ftrace_free_init_mem() ?
>>
>> IIRC, proposed location is the earliest we could call debug_vm_pgtable().
>> Is there any particular benefit or reason to move it into kernel_init() ?
>
> It would avoid having it lost in the middle of drivers logs, would be close to the end of init, at a place we can't miss it, close to the result of other tests like CONFIG_DEBUG_RODATA_TEST for instance.
>
> At the moment, you have to look for it to be sure the test is done and what the result is.
Sure, will move it.
>
>>
>>>
>>>> /* Initialize page ext after all struct pages are initialized. */
>>>> page_ext_init();
>>>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>>>> index 5ffe144c9794..7cceae923c05 100644
>>>> --- a/lib/Kconfig.debug
>>>> +++ b/lib/Kconfig.debug
>>>> @@ -653,6 +653,12 @@ config SCHED_STACK_END_CHECK
>>>> data corruption or a sporadic crash at a later stage once the region
>>>> is examined. The runtime overhead introduced is minimal.
>>>> +config ARCH_HAS_DEBUG_VM_PGTABLE
>>>> + bool
>>>> + help
>>>> + An architecture should select this when it can successfully
>>>> + build and run DEBUG_VM_PGTABLE.
>>>> +
>>>> config DEBUG_VM
>>>> bool "Debug VM"
>>>> depends on DEBUG_KERNEL
>>>> @@ -688,6 +694,22 @@ config DEBUG_VM_PGFLAGS
>>>> If unsure, say N.
>>>> +config DEBUG_VM_PGTABLE
>>>> + bool "Debug arch page table for semantics compliance"
>>>> + depends on MMU
>>>> + depends on DEBUG_VM
>>>
>>> Does it really need to depend on DEBUG_VM ?
>>
>> No. It seemed better to package this test along with DEBUG_VM (although I
>> dont remember the conversation around it) and hence this dependency.
>
> Yes but it perfectly work as standalone. The more easy it is to activate and the more people will use it. DEBUG_VM obliges to rebuild the kernel entirely and could modify the behaviour. Could the helpers we are testing behave differently when DEBUG_VM is not set ? I think it's good the test things as close as possible to final config.
Makes sense. There is no functional dependency for the individual tests
here on DEBUG_VM.
>
>>
>>> I think we could make it standalone and 'default y if DEBUG_VM' instead.
>>
>> Which will yield the same result like before but in a different way. But
>> yes, this test could go about either way but unless there is a good enough
>> reason why change the current one.
>
> I think if we want people to really use it on other architectures it must be possible to activate it without having to modify Kconfig. Otherwise people won't even know the test exists and the architecture fails the test.
>
> The purpose of a test suite is to detect bugs. If you can't run the test until you have fixed the bugs, I guess nobody will ever detect the bugs and they will never be fixed.
>
> So I think:
> - the test should be 'default y' when ARCH_HAS_DEBUG_VM_PGTABLE is selected
> - the test should be 'default n' when ARCH_HAS_DEBUG_VM_PGTABLE is not selected, and it should be user selectable if EXPERT is selected.
>
> Something like:
>
> config DEBUG_VM_PGTABLE
> bool "Debug arch page table for semantics compliance" if ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT
> depends on MMU
(ARCH_HAS_DEBUG_VM_PGTABLE || EXPERT) be moved along side MMU on the same line ?
> default 'n' if !ARCH_HAS_DEBUG_VM_PGTABLE
> default 'y' if DEBUG_VM
This looks good, at least until we get all platforms enabled. Will do all these
changes along with s390 enablement and re-spin.
>
>
>>
>>>
>>>> + depends on ARCH_HAS_DEBUG_VM_PGTABLE
>>>> + default y
>>>> + help
>>>> + This option provides a debug method which can be used to test
>>>> + architecture page table helper functions on various platforms in
>>>> + verifying if they comply with expected generic MM semantics. This
>>>> + will help architecture code in making sure that any changes or
>>>> + new additions of these helpers still conform to expected
>>>> + semantics of the generic MM.
>>>> +
>>>> + If unsure, say N.
>>>> +
>>>
>>> Does it make sense to make it 'default y' and say 'If unsure, say N' ?
>>
>> No it does. Not when it defaults 'y' unconditionally. Will drop the last
>> sentence "If unsure, say N". Nice catch, thank you.
>
> Well I was not asking if 'default y' was making sense but only if 'If unsure say N' was making sense due to the 'default y'. You got it.
>
> Christophe
>
>
^ permalink raw reply
* Re: Latest Git kernel: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
From: Randy Dunlap @ 2020-02-02 4:37 UTC (permalink / raw)
To: Christian Zigotzky, Michael Ellerman
Cc: DTML, Darren Stevens, netdev@vger.kernel.org,
Linux Kernel Mailing List, linuxppc-dev, contact@a-eon.com,
R.T.Dickinson, Christoph Hellwig
In-Reply-To: <58a6d45c-0712-18df-1b14-2f04cf12a1cb@xenosoft.de>
[might be network related, so adding netdev mailing list]
On 2/1/20 4:08 PM, Christian Zigotzky wrote:
> Hello,
>
> We regularly compile and test Linux kernels every day during the merge window. Since Thuesday we have very high CPU loads because of the avahi daemon on our desktop Linux systems (Ubuntu, Debian etc).
>
> Error message: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
>
> Could you please test the latest Git kernel?
>
> It is possible to deactivate the avahi daemon with the following lines in the file "/etc/avahi/avahi-daemon.conf":
>
> use-ipv4=no
> use-ipv6=no
>
> But this is only a temporary solution.
>
> Thanks,
> Christian
--
~Randy
^ permalink raw reply
* Latest Git kernel: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
From: Christian Zigotzky @ 2020-02-02 0:08 UTC (permalink / raw)
To: Michael Ellerman
Cc: DTML, Darren Stevens, Linux Kernel Mailing List, linuxppc-dev,
contact@a-eon.com, R.T.Dickinson, Christoph Hellwig
In-Reply-To: <CAPDyKFrbYmV6_nV6psVLq6VRKMXf0PXpemBbj48yjOr3P130BA@mail.gmail.com>
Hello,
We regularly compile and test Linux kernels every day during the merge
window. Since Thuesday we have very high CPU loads because of the avahi
daemon on our desktop Linux systems (Ubuntu, Debian etc).
Error message: avahi-daemon[2410]: ioctl(): Inappropriate ioctl for device
Could you please test the latest Git kernel?
It is possible to deactivate the avahi daemon with the following lines
in the file "/etc/avahi/avahi-daemon.conf":
use-ipv4=no
use-ipv6=no
But this is only a temporary solution.
Thanks,
Christian
^ permalink raw reply
* [powerpc:next] BUILD SUCCESS 41196224883a64e56e0ef237c19eb837058df071
From: kbuild test robot @ 2020-02-01 17:10 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
branch HEAD: 41196224883a64e56e0ef237c19eb837058df071 powerpc/32s: Fix kasan_early_hash_table() for CONFIG_VMAP_STACK
elapsed time: 3111m
configs tested: 241
configs skipped: 25
The following configs have been built successfully.
More configs may be tested in the coming days.
arm allmodconfig
arm allnoconfig
arm allyesconfig
arm at91_dt_defconfig
arm efm32_defconfig
arm exynos_defconfig
arm multi_v5_defconfig
arm multi_v7_defconfig
arm shmobile_defconfig
arm sunxi_defconfig
arm64 allmodconfig
arm64 allnoconfig
arm64 allyesconfig
arm64 defconfig
sparc allyesconfig
parisc defconfig
arc defconfig
um x86_64_defconfig
arc allyesconfig
riscv rv32_defconfig
sparc64 allyesconfig
parisc b180_defconfig
m68k sun3_defconfig
s390 defconfig
microblaze mmu_defconfig
um i386_defconfig
ia64 defconfig
powerpc defconfig
c6x evmc6678_defconfig
microblaze nommu_defconfig
powerpc allnoconfig
alpha defconfig
mips allmodconfig
i386 alldefconfig
i386 allnoconfig
i386 allyesconfig
i386 defconfig
ia64 alldefconfig
ia64 allmodconfig
ia64 allnoconfig
ia64 allyesconfig
xtensa common_defconfig
openrisc or1ksim_defconfig
nios2 3c120_defconfig
xtensa iss_defconfig
c6x allyesconfig
nios2 10m50_defconfig
openrisc simple_smp_defconfig
csky defconfig
nds32 allnoconfig
nds32 defconfig
h8300 edosk2674_defconfig
h8300 h8300h-sim_defconfig
h8300 h8s-sim_defconfig
m68k allmodconfig
m68k m5475evb_defconfig
m68k multi_defconfig
powerpc ppc64_defconfig
powerpc rhel-kconfig
mips 32r2_defconfig
mips 64r6el_defconfig
mips allnoconfig
mips allyesconfig
mips fuloong2e_defconfig
mips malta_kvm_defconfig
parisc allnoconfig
parisc allyesonfig
parisc c3000_defconfig
x86_64 randconfig-a001-20200130
x86_64 randconfig-a002-20200130
x86_64 randconfig-a003-20200130
i386 randconfig-a001-20200130
i386 randconfig-a002-20200130
i386 randconfig-a003-20200130
x86_64 randconfig-a001-20200129
x86_64 randconfig-a002-20200129
x86_64 randconfig-a003-20200129
i386 randconfig-a001-20200129
i386 randconfig-a002-20200129
i386 randconfig-a003-20200129
x86_64 randconfig-a001-20200131
x86_64 randconfig-a002-20200131
x86_64 randconfig-a003-20200131
i386 randconfig-a001-20200131
i386 randconfig-a002-20200131
i386 randconfig-a003-20200131
alpha randconfig-a001-20200130
m68k randconfig-a001-20200130
mips randconfig-a001-20200130
nds32 randconfig-a001-20200130
parisc randconfig-a001-20200130
riscv randconfig-a001-20200130
alpha randconfig-a001-20200131
m68k randconfig-a001-20200131
mips randconfig-a001-20200131
nds32 randconfig-a001-20200131
parisc randconfig-a001-20200131
c6x randconfig-a001-20200130
h8300 randconfig-a001-20200130
microblaze randconfig-a001-20200130
nios2 randconfig-a001-20200130
sparc64 randconfig-a001-20200130
c6x randconfig-a001-20200131
h8300 randconfig-a001-20200131
microblaze randconfig-a001-20200131
nios2 randconfig-a001-20200131
sparc64 randconfig-a001-20200131
csky randconfig-a001-20200130
openrisc randconfig-a001-20200130
s390 randconfig-a001-20200130
sh randconfig-a001-20200130
xtensa randconfig-a001-20200130
x86_64 randconfig-b001-20200131
x86_64 randconfig-b002-20200131
x86_64 randconfig-b003-20200131
i386 randconfig-b001-20200131
i386 randconfig-b002-20200131
i386 randconfig-b003-20200131
x86_64 randconfig-b001-20200129
x86_64 randconfig-b002-20200129
x86_64 randconfig-b003-20200129
i386 randconfig-b001-20200129
i386 randconfig-b002-20200129
i386 randconfig-b003-20200129
x86_64 randconfig-c001-20200131
x86_64 randconfig-c002-20200131
x86_64 randconfig-c003-20200131
i386 randconfig-c001-20200131
i386 randconfig-c002-20200131
i386 randconfig-c003-20200131
x86_64 randconfig-c001-20200129
x86_64 randconfig-c002-20200129
x86_64 randconfig-c003-20200129
i386 randconfig-c001-20200129
i386 randconfig-c002-20200129
i386 randconfig-c003-20200129
x86_64 randconfig-d001-20200129
x86_64 randconfig-d002-20200129
x86_64 randconfig-d003-20200129
i386 randconfig-d001-20200129
i386 randconfig-d002-20200129
i386 randconfig-d003-20200129
x86_64 randconfig-d001-20200130
x86_64 randconfig-d002-20200130
x86_64 randconfig-d003-20200130
i386 randconfig-d001-20200130
i386 randconfig-d002-20200130
i386 randconfig-d003-20200130
x86_64 randconfig-d001-20200131
x86_64 randconfig-d002-20200131
x86_64 randconfig-d003-20200131
i386 randconfig-d001-20200131
i386 randconfig-d002-20200131
i386 randconfig-d003-20200131
x86_64 randconfig-e001-20200129
x86_64 randconfig-e002-20200129
x86_64 randconfig-e003-20200129
i386 randconfig-e001-20200129
i386 randconfig-e002-20200129
i386 randconfig-e003-20200129
x86_64 randconfig-e001-20200130
x86_64 randconfig-e002-20200130
x86_64 randconfig-e003-20200130
i386 randconfig-e001-20200130
i386 randconfig-e002-20200130
i386 randconfig-e003-20200130
x86_64 randconfig-f001-20200130
x86_64 randconfig-f002-20200130
x86_64 randconfig-f003-20200130
i386 randconfig-f001-20200130
i386 randconfig-f002-20200130
i386 randconfig-f003-20200130
x86_64 randconfig-f001-20200129
x86_64 randconfig-f002-20200129
x86_64 randconfig-f003-20200129
i386 randconfig-f001-20200129
i386 randconfig-f002-20200129
i386 randconfig-f003-20200129
x86_64 randconfig-g001-20200129
x86_64 randconfig-g002-20200129
x86_64 randconfig-g003-20200129
i386 randconfig-g001-20200129
i386 randconfig-g002-20200129
i386 randconfig-g003-20200129
x86_64 randconfig-g001-20200130
x86_64 randconfig-g002-20200130
x86_64 randconfig-g003-20200130
i386 randconfig-g001-20200130
i386 randconfig-g002-20200130
i386 randconfig-g003-20200130
x86_64 randconfig-h001-20200129
x86_64 randconfig-h002-20200129
x86_64 randconfig-h003-20200129
i386 randconfig-h001-20200129
i386 randconfig-h002-20200129
i386 randconfig-h003-20200129
arc randconfig-a001-20200131
arm randconfig-a001-20200131
arm64 randconfig-a001-20200131
ia64 randconfig-a001-20200131
powerpc randconfig-a001-20200131
sparc randconfig-a001-20200131
arm64 randconfig-a001-20200130
ia64 randconfig-a001-20200130
sparc randconfig-a001-20200130
arm randconfig-a001-20200130
arc randconfig-a001-20200130
powerpc randconfig-a001-20200130
arc randconfig-a001-20200129
arm randconfig-a001-20200129
arm64 randconfig-a001-20200129
ia64 randconfig-a001-20200129
powerpc randconfig-a001-20200129
sparc randconfig-a001-20200129
riscv defconfig
riscv nommu_virt_defconfig
riscv allmodconfig
riscv allnoconfig
riscv allyesconfig
s390 alldefconfig
s390 allmodconfig
s390 allnoconfig
s390 allyesconfig
s390 debug_defconfig
s390 zfcpdump_defconfig
sh allmodconfig
sh allnoconfig
sh rsk7269_defconfig
sh sh7785lcr_32bit_defconfig
sh titan_defconfig
sparc defconfig
sparc64 allmodconfig
sparc64 allnoconfig
sparc64 defconfig
um defconfig
x86_64 fedora-25
x86_64 kexec
x86_64 lkp
x86_64 rhel
x86_64 rhel-7.2-clear
x86_64 rhel-7.6
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
^ permalink raw reply
* Re: [PATCH] lkdtm: Test KUAP directional user access unlocks on powerpc
From: Kees Cook @ 2020-02-01 16:40 UTC (permalink / raw)
To: Russell Currey; +Cc: kernel-hardening, linux-kernel, linuxppc-dev, dja
In-Reply-To: <0b016861756cbe27e66651b5c21229a06558cb57.camel@russell.cc>
On Fri, Jan 31, 2020 at 05:53:14PM +1100, Russell Currey wrote:
> Correct, the ACCESS_USERSPACE test does the same thing. Splitting this
> into separate R and W tests makes sense, even if it is unlikely that
> one would be broken without the other.
That would be my preference too -- the reason it wasn't separated before
was because it was one big toggle before. I just had both directions in
the test out of a desire for completeness.
Splitting into WRITE_USERSPACE and READ_USERSPACE seems good. Though if
you want to test functionality (read while only write disabled), then
I'm not sure what that should look like. Does the new
user_access_begin() API provide a way to query existing state? I'll go
read the series...
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] powerpc/32s: Don't flush all TLBs when flushing one page
From: Christophe Leroy @ 2020-02-01 16:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b30b2eae6960502eaf0d9e36c60820b839693c33.1580542939.git.christophe.leroy@c-s.fr>
Le 01/02/2020 à 09:04, Christophe Leroy a écrit :
> When flushing any memory range, the flushing function
> flushes all TLBs.
>
> When (start) and (end - 1) are in the same memory page,
> flush that page instead.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
> ---
> v2: Reworked the test as the previous one was always false (end - start was PAGE_SIZE - 1 for a single page)
> ---
> arch/powerpc/mm/book3s32/tlb.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/mm/book3s32/tlb.c b/arch/powerpc/mm/book3s32/tlb.c
> index 2fcd321040ff..724c0490fb17 100644
> --- a/arch/powerpc/mm/book3s32/tlb.c
> +++ b/arch/powerpc/mm/book3s32/tlb.c
> @@ -79,11 +79,14 @@ static void flush_range(struct mm_struct *mm, unsigned long start,
> int count;
> unsigned int ctx = mm->context.id;
>
> + start &= PAGE_MASK;
> if (!Hash) {
> - _tlbia();
> + if (end - start <= PAGE_SIZE)
> + _tlbie(start);
> + else
> + _tlbia();
> return;
> }
> - start &= PAGE_MASK;
> if (start >= end)
> return;
> end = (end - 1) | ~PAGE_MASK;
>
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Don't flush all TLBs when flushing one page
From: Segher Boessenkool @ 2020-02-01 16:17 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <96671e01-6206-8952-a498-942b42e98ef0@c-s.fr>
On Sat, Feb 01, 2020 at 03:53:12PM +0100, Christophe Leroy wrote:
> >>No, in end the low bits are set, that's a BIT OR with ~PAGE_MASK, so it
> >>sets all low bits to 1.
> >
> >Oh, wow, yes, I cannot read apparently.
> >
> >Maybe there are some ROUND_DOWN and ROUND_UP macros you could use?
>
> Yes but my intention was to modify the existing code as less as possible.
> What do you think about version v2 of the patch ?
It looked fine to me.
Add my
Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>
if you want.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc/32s: Don't flush all TLBs when flushing one page
From: Christophe Leroy @ 2020-02-01 14:53 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <20200201140629.GM22482@gate.crashing.org>
Le 01/02/2020 à 15:06, Segher Boessenkool a écrit :
> On Sat, Feb 01, 2020 at 08:27:03AM +0100, Christophe Leroy wrote:
>> Le 31/01/2020 à 20:38, Segher Boessenkool a écrit :
>>> On Fri, Jan 31, 2020 at 05:15:20PM +0100, Christophe Leroy wrote:
>>>> Le 31/01/2020 à 16:51, Segher Boessenkool a écrit :
>>>>> On Fri, Jan 31, 2020 at 03:37:34PM +0000, Christophe Leroy wrote:
>>>>>> When the range is a single page, do a page flush instead.
>>>>>
>>>>>> + start &= PAGE_MASK;
>>>>>> + end = (end - 1) | ~PAGE_MASK;
>>>>>> if (!Hash) {
>>>>>> - _tlbia();
>>>>>> + if (end - start == PAGE_SIZE)
>>>>>> + _tlbie(start);
>>>>>> + else
>>>>>> + _tlbia();
>>>>>> return;
>>>>>> }
>>>>>
>>>>> For just one page, you get end - start == 0 actually?
>>>>
>>>> Oops, good catch.
>>>>
>>>> Indeed you don't get PAGE_SIZE but (PAGE_SIZE - 1) for just one page.
>>>
>>> You have all low bits masked off in both start and end, so you get zero.
>>> You could make the condion read "if (start == end)?
>>
>> No, in end the low bits are set, that's a BIT OR with ~PAGE_MASK, so it
>> sets all low bits to 1.
>
> Oh, wow, yes, I cannot read apparently.
>
> Maybe there are some ROUND_DOWN and ROUND_UP macros you could use?
>
Yes but my intention was to modify the existing code as less as possible.
What do you think about version v2 of the patch ?
Christophe
^ 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