[PATCH 0/4] Add KFENCE support for LoongArch

[PATCH 0/4] Add KFENCE support for LoongArch

[Enze Li]

Hi all,

This patchset adds KFENCE support on LoongArch.

To run the testcases, you will need to enable the following options,

-> Kernel hacking
   [*] Tracers
       [*] Support for tracing block IO actions (NEW)
   -> Kernel Testing and Coverage
      <*> KUnit - Enable support for unit tests

and then,

-> Kernel hacking
   -> Memory Debugging
      [*] KFENCE: low-overhead sampling-based memory safety error detector (NEW)
          <*> KFENCE integration test suite (NEW)

With these options enabled, KFENCE will be tested during kernel startup.
And normally, you might get the following feedback,

========================================================
[   35.326363 ] # kfence: pass:23 fail:0 skip:2 total:25
[   35.326486 ] # Totals: pass:23 fail:0 skip:2 total:25
[   35.326621 ] ok 1 kfence
========================================================

you might notice that 2 testcases have been skipped.  If you tend to run
all testcases, please enable CONFIG_INIT_ON_FREE_DEFAULT_ON, you can
find it here,

-> Security options
   -> Kernel hardening options
      -> Memory initialization
         [*] Enable heap memory zeroing on free by default

and you might get all testcases passed.
========================================================
[   35.531860 ] # kfence: pass:25 fail:0 skip:0 total:25
[   35.531999 ] # Totals: pass:25 fail:0 skip:0 total:25
[   35.532135 ] ok 1 kfence
========================================================

Thanks,
Enze

Enze Li (4):
  LoongArch: mm: Add page table mapped mode support
  LoongArch: Get stack without NMI when providing regs parameter
  KFENCE: Deferring the assignment of the local variable addr
  LoongArch: Add KFENCE support

 arch/loongarch/Kconfig               |  1 +
 arch/loongarch/include/asm/kfence.h  | 62 ++++++++++++++++++++++++++++
 arch/loongarch/include/asm/page.h    | 10 +++++
 arch/loongarch/include/asm/pgtable.h | 12 ++++++
 arch/loongarch/kernel/stacktrace.c   | 16 ++++---
 arch/loongarch/mm/fault.c            | 22 ++++++----
 arch/loongarch/mm/pgtable.c          | 25 +++++++++++
 mm/kfence/core.c                     |  5 ++-
 8 files changed, 137 insertions(+), 16 deletions(-)
 create mode 100644 arch/loongarch/include/asm/kfence.h

-- 
2.34.1

[PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Enze Li]

According to LoongArch documentation online, there are two types of address
translation modes: direct mapped address translation mode (direct mapped mode)
and page table mapped address translation mode (page table mapped mode).

Currently, the upstream code only supports DMM (Direct Mapped Mode).
This patch adds a function that determines whether PTMM (Page Table
Mapped Mode) should be used, and also adds the corresponding handler
funcitons for both modes.

For more details on the two modes, see [1].

[1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 arch/loongarch/include/asm/page.h    | 10 ++++++++++
 arch/loongarch/include/asm/pgtable.h |  6 ++++++
 arch/loongarch/mm/pgtable.c          | 25 +++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
index 26e8dccb6619..05919be15801 100644
--- a/arch/loongarch/include/asm/page.h
+++ b/arch/loongarch/include/asm/page.h
@@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t;
 #define sym_to_pfn(x)		__phys_to_pfn(__pa_symbol(x))
 
 #define virt_to_pfn(kaddr)	PFN_DOWN(PHYSADDR(kaddr))
+
+#ifdef CONFIG_64BIT
+#define virt_to_page(kaddr)						\
+({									\
+	is_PTMM_addr((unsigned long)kaddr) ?				\
+	PTMM_virt_to_page((unsigned long)kaddr) :			\
+	DMM_virt_to_page((unsigned long)kaddr);				\
+})
+#else
 #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
+#endif
 
 extern int __virt_addr_valid(volatile void *kaddr);
 #define virt_addr_valid(kaddr)	__virt_addr_valid((volatile void *)(kaddr))
diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
index ed6a37bb55b5..0fc074b8bd48 100644
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
 #define PMD_T_LOG2	(__builtin_ffs(sizeof(pmd_t)) - 1)
 #define PTE_T_LOG2	(__builtin_ffs(sizeof(pte_t)) - 1)
 
+#ifdef CONFIG_64BIT
+struct page *DMM_virt_to_page(unsigned long kaddr);
+struct page *PTMM_virt_to_page(unsigned long kaddr);
+bool is_PTMM_addr(unsigned long kaddr);
+#endif
+
 extern pgd_t swapper_pg_dir[];
 extern pgd_t invalid_pg_dir[];
 
diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c
index 36a6dc0148ae..4c6448f996b6 100644
--- a/arch/loongarch/mm/pgtable.c
+++ b/arch/loongarch/mm/pgtable.c
@@ -9,6 +9,31 @@
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 
+#ifdef CONFIG_64BIT
+/* DMM stands for Direct Mapped Mode. */
+struct page *DMM_virt_to_page(unsigned long kaddr)
+{
+	return pfn_to_page(virt_to_pfn(kaddr));
+}
+EXPORT_SYMBOL_GPL(DMM_virt_to_page);
+
+/* PTMM stands for Page Table Mapped Mode. */
+struct page *PTMM_virt_to_page(unsigned long kaddr)
+{
+	return pte_page(*virt_to_kpte(kaddr));
+}
+EXPORT_SYMBOL_GPL(PTMM_virt_to_page);
+
+bool is_PTMM_addr(unsigned long kaddr)
+{
+	if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) ==
+		     GENMASK(BITS_PER_LONG - 1, cpu_vabits)))
+		return true;
+	return false;
+}
+EXPORT_SYMBOL_GPL(is_PTMM_addr);
+#endif
+
 pgd_t *pgd_alloc(struct mm_struct *mm)
 {
 	pgd_t *ret, *init;
-- 
2.34.1

[PATCH 2/4] LoongArch: Get stack without NMI when providing regs parameter

[Enze Li]

Currently, executing arch_stack_walk can only get the full stack
information including NMI.  This is because the implementation
of arch_stack_walk is forced to ignore the information passed by the
regs parameter and use the current stack information instead.

For some detection systems like KFENCE, only partial stack information
is needed.  In particular, the stack frame where the interrupt occurred.

To support KFENCE, this patch modifies the implementation of the
arch_stack_walk function so that if this function is called with the
regs argument passed, it retains all the stack information in regs and
uses it to provide accurate information.

Before the patch applied, I get,
[    1.531195 ] ==================================================================
[    1.531442 ] BUG: KFENCE: out-of-bounds read in stack_trace_save_regs+0x48/0x6c
[    1.531442 ]
[    1.531900 ] Out-of-bounds read at 0xffff800012267fff (1B left of kfence-#12):
[    1.532046 ]  stack_trace_save_regs+0x48/0x6c
[    1.532169 ]  kfence_report_error+0xa4/0x528
[    1.532276 ]  kfence_handle_page_fault+0x124/0x270
[    1.532388 ]  no_context+0x50/0x94
[    1.532453 ]  do_page_fault+0x1a8/0x36c
[    1.532524 ]  tlb_do_page_fault_0+0x118/0x1b4
[    1.532623 ]  test_out_of_bounds_read+0xa0/0x1d8
[    1.532745 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
[    1.532854 ]  kthread+0x124/0x130
[    1.532922 ]  ret_from_kernel_thread+0xc/0xa4
<snip>

With this patch applied, I get the correct stack information.
[    1.320220 ] ==================================================================
[    1.320401 ] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0xa8/0x1d8
[    1.320401 ]
[    1.320898 ] Out-of-bounds read at 0xffff800012257fff (1B left of kfence-#10):
[    1.321134 ]  test_out_of_bounds_read+0xa8/0x1d8
[    1.321264 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
[    1.321392 ]  kthread+0x124/0x130
[    1.321459 ]  ret_from_kernel_thread+0xc/0xa4
<snip>

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 arch/loongarch/kernel/stacktrace.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
index 2463d2fea21f..21f60811e26f 100644
--- a/arch/loongarch/kernel/stacktrace.c
+++ b/arch/loongarch/kernel/stacktrace.c
@@ -18,16 +18,20 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 	struct pt_regs dummyregs;
 	struct unwind_state state;
 
-	regs = &dummyregs;
-
 	if (task == current) {
-		regs->regs[3] = (unsigned long)__builtin_frame_address(0);
-		regs->csr_era = (unsigned long)__builtin_return_address(0);
+		if (regs)
+			memcpy(&dummyregs, regs, sizeof(*regs));
+		else {
+			dummyregs.regs[3] = (unsigned long)__builtin_frame_address(0);
+			dummyregs.csr_era = (unsigned long)__builtin_return_address(0);
+		}
 	} else {
-		regs->regs[3] = thread_saved_fp(task);
-		regs->csr_era = thread_saved_ra(task);
+		dummyregs.regs[3] = thread_saved_fp(task);
+		dummyregs.csr_era = thread_saved_ra(task);
 	}
 
+	regs = &dummyregs;
+
 	regs->regs[1] = 0;
 	for (unwind_start(&state, task, regs);
 	     !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {
-- 
2.34.1

[PATCH 3/4] KFENCE: Deferring the assignment of the local variable addr

[Enze Li]

The LoongArch architecture is different from other architectures.
It needs to update __kfence_pool during arch_kfence_init_pool.

This patch modifies the assignment location of the local variable addr
in the kfence_init_pool function to support the case of updating
__kfence_pool in arch_kfence_init_pool.

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 mm/kfence/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index dad3c0eb70a0..e124ffff489f 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -566,13 +566,14 @@ static void rcu_guarded_free(struct rcu_head *h)
  */
 static unsigned long kfence_init_pool(void)
 {
-	unsigned long addr = (unsigned long)__kfence_pool;
+	unsigned long addr;
 	struct page *pages;
 	int i;
 
 	if (!arch_kfence_init_pool())
-		return addr;
+		return (unsigned long)__kfence_pool;
 
+	addr = (unsigned long)__kfence_pool;
 	pages = virt_to_page(__kfence_pool);
 
 	/*
-- 
2.34.1

[PATCH 4/4] LoongArch: Add KFENCE support

[Enze Li]

The LoongArch architecture is quite different from other architectures.
When the allocating of KFENCE itself is done, it is mapped to the direct
mapping configuration window [1] by default on LoongArch.  It means that
it is not possible to use the page table mapped mode which required by
the KFENCE system and therefore it should be remapped to the appropriate
region.

This patch adds architecture specific implementation details for KFENCE.
In particular, this implements the required interface in <asm/kfence.h>.

Tested this patch by using the testcases and all passed.

[1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 arch/loongarch/Kconfig               |  1 +
 arch/loongarch/include/asm/kfence.h  | 62 ++++++++++++++++++++++++++++
 arch/loongarch/include/asm/pgtable.h |  6 +++
 arch/loongarch/mm/fault.c            | 22 ++++++----
 4 files changed, 83 insertions(+), 8 deletions(-)
 create mode 100644 arch/loongarch/include/asm/kfence.h

diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index 5411e3a4eb88..db27729003d3 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -93,6 +93,7 @@ config LOONGARCH
 	select HAVE_ARCH_JUMP_LABEL
 	select HAVE_ARCH_JUMP_LABEL_RELATIVE
 	select HAVE_ARCH_KASAN
+	select HAVE_ARCH_KFENCE if 64BIT
 	select HAVE_ARCH_MMAP_RND_BITS if MMU
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ARCH_TRACEHOOK
diff --git a/arch/loongarch/include/asm/kfence.h b/arch/loongarch/include/asm/kfence.h
new file mode 100644
index 000000000000..2a85acc2bc70
--- /dev/null
+++ b/arch/loongarch/include/asm/kfence.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KFENCE support for LoongArch.
+ *
+ * Author: Enze Li <lienze@kylinos.cn>
+ * Copyright (C) 2022-2023 KylinSoft Corporation.
+ */
+
+#ifndef _ASM_LOONGARCH_KFENCE_H
+#define _ASM_LOONGARCH_KFENCE_H
+
+#include <linux/kfence.h>
+#include <asm/pgtable.h>
+#include <asm/tlb.h>
+
+static inline char *arch_kfence_init_pool(void)
+{
+	char *__kfence_pool_orig = __kfence_pool;
+	struct vm_struct *area;
+	int err;
+
+	area = __get_vm_area_caller(KFENCE_POOL_SIZE, VM_IOREMAP,
+				    KFENCE_AREA_START, KFENCE_AREA_END,
+				    __builtin_return_address(0));
+	if (!area)
+		return NULL;
+
+	__kfence_pool = (char *)area->addr;
+	err = ioremap_page_range((unsigned long)__kfence_pool,
+				 (unsigned long)__kfence_pool + KFENCE_POOL_SIZE,
+				 virt_to_phys((void *)__kfence_pool_orig),
+				 PAGE_KERNEL);
+	if (err) {
+		free_vm_area(area);
+		return NULL;
+	}
+
+	return __kfence_pool;
+}
+
+/* Protect the given page and flush TLB. */
+static inline bool kfence_protect_page(unsigned long addr, bool protect)
+{
+	pte_t *pte = virt_to_kpte(addr);
+
+	if (WARN_ON(!pte) || pte_none(*pte))
+		return false;
+
+	if (protect)
+		set_pte(pte, __pte(pte_val(*pte) & ~(_PAGE_VALID | _PAGE_PRESENT)));
+	else
+		set_pte(pte, __pte(pte_val(*pte) | (_PAGE_VALID | _PAGE_PRESENT)));
+
+	/* Flush this CPU's TLB. */
+	preempt_disable();
+	local_flush_tlb_one(addr);
+	preempt_enable();
+
+	return true;
+}
+
+#endif /* _ASM_LOONGARCH_KFENCE_H */
diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
index 0fc074b8bd48..5a9c81298fe3 100644
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -85,7 +85,13 @@ extern unsigned long zero_page_mask;
 #define MODULES_VADDR	(vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
 #define MODULES_END	(MODULES_VADDR + SZ_256M)
 
+#ifdef CONFIG_KFENCE
+#define KFENCE_AREA_START	MODULES_END
+#define KFENCE_AREA_END		(KFENCE_AREA_START + SZ_512M)
+#define VMALLOC_START		KFENCE_AREA_END
+#else
 #define VMALLOC_START	MODULES_END
+#endif
 
 #ifndef CONFIG_KASAN
 #define VMALLOC_END	\
diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
index da5b6d518cdb..c0319128b221 100644
--- a/arch/loongarch/mm/fault.c
+++ b/arch/loongarch/mm/fault.c
@@ -23,6 +23,7 @@
 #include <linux/kprobes.h>
 #include <linux/perf_event.h>
 #include <linux/uaccess.h>
+#include <linux/kfence.h>
 
 #include <asm/branch.h>
 #include <asm/mmu_context.h>
@@ -30,7 +31,8 @@
 
 int show_unhandled_signals = 1;
 
-static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
+static void __kprobes no_context(struct pt_regs *regs, unsigned long address,
+				 unsigned long write)
 {
 	const int field = sizeof(unsigned long) * 2;
 
@@ -38,6 +40,9 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
 	if (fixup_exception(regs))
 		return;
 
+	if (kfence_handle_page_fault(address, write, regs))
+		return;
+
 	/*
 	 * Oops. The kernel tried to access some bad page. We'll have to
 	 * terminate things with extreme prejudice.
@@ -51,14 +56,15 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
 	die("Oops", regs);
 }
 
-static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address)
+static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address,
+				       unsigned long write)
 {
 	/*
 	 * We ran out of memory, call the OOM killer, and return the userspace
 	 * (which will retry the fault, or kill us if we got oom-killed).
 	 */
 	if (!user_mode(regs)) {
-		no_context(regs, address);
+		no_context(regs, address, write);
 		return;
 	}
 	pagefault_out_of_memory();
@@ -69,7 +75,7 @@ static void __kprobes do_sigbus(struct pt_regs *regs,
 {
 	/* Kernel mode? Handle exceptions or die */
 	if (!user_mode(regs)) {
-		no_context(regs, address);
+		no_context(regs, address, write);
 		return;
 	}
 
@@ -90,7 +96,7 @@ static void __kprobes do_sigsegv(struct pt_regs *regs,
 
 	/* Kernel mode? Handle exceptions or die */
 	if (!user_mode(regs)) {
-		no_context(regs, address);
+		no_context(regs, address, write);
 		return;
 	}
 
@@ -149,7 +155,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
 	 */
 	if (address & __UA_LIMIT) {
 		if (!user_mode(regs))
-			no_context(regs, address);
+			no_context(regs, address, write);
 		else
 			do_sigsegv(regs, write, address, si_code);
 		return;
@@ -211,7 +217,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
 
 	if (fault_signal_pending(fault, regs)) {
 		if (!user_mode(regs))
-			no_context(regs, address);
+			no_context(regs, address, write);
 		return;
 	}
 
@@ -232,7 +238,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
 	if (unlikely(fault & VM_FAULT_ERROR)) {
 		mmap_read_unlock(mm);
 		if (fault & VM_FAULT_OOM) {
-			do_out_of_memory(regs, address);
+			do_out_of_memory(regs, address, write);
 			return;
 		} else if (fault & VM_FAULT_SIGSEGV) {
 			do_sigsegv(regs, write, address, si_code);
-- 
2.34.1

Re: [PATCH 3/4] KFENCE: Deferring the assignment of the local variable addr

[Marco Elver]

On Wed, 19 Jul 2023 at 10:28, Enze Li <lienze@kylinos.cn> wrote:
>
> The LoongArch architecture is different from other architectures.
> It needs to update __kfence_pool during arch_kfence_init_pool.
>
> This patch modifies the assignment location of the local variable addr
> in the kfence_init_pool function to support the case of updating
> __kfence_pool in arch_kfence_init_pool.
>
> Signed-off-by: Enze Li <lienze@kylinos.cn>

I think it's fair to allow this use case.

However, please make sure that when your arch_kfence_init_pool()
fails, it is still possible to free the memblock allocated memory
properly.

Acked-by: Marco Elver <elver@google.com>

> ---
>  mm/kfence/core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index dad3c0eb70a0..e124ffff489f 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -566,13 +566,14 @@ static void rcu_guarded_free(struct rcu_head *h)
>   */
>  static unsigned long kfence_init_pool(void)
>  {
> -       unsigned long addr = (unsigned long)__kfence_pool;
> +       unsigned long addr;
>         struct page *pages;
>         int i;
>
>         if (!arch_kfence_init_pool())
> -               return addr;
> +               return (unsigned long)__kfence_pool;
>
> +       addr = (unsigned long)__kfence_pool;
>         pages = virt_to_page(__kfence_pool);
>
>         /*
> --
> 2.34.1
>

Re: [PATCH 3/4] KFENCE: Deferring the assignment of the local variable addr

[Huacai Chen]

Hi, Marco,

On Wed, Jul 19, 2023 at 6:55 PM Marco Elver <elver@google.com> wrote:
>
> On Wed, 19 Jul 2023 at 10:28, Enze Li <lienze@kylinos.cn> wrote:
> >
> > The LoongArch architecture is different from other architectures.
> > It needs to update __kfence_pool during arch_kfence_init_pool.
> >
> > This patch modifies the assignment location of the local variable addr
> > in the kfence_init_pool function to support the case of updating
> > __kfence_pool in arch_kfence_init_pool.
> >
> > Signed-off-by: Enze Li <lienze@kylinos.cn>
>
> I think it's fair to allow this use case.
>
> However, please make sure that when your arch_kfence_init_pool()
> fails, it is still possible to free the memblock allocated memory
> properly.
>
> Acked-by: Marco Elver <elver@google.com>
Does Acked-by means this patch can go through loongarch tree together
with other patches? If this patch should go through kfence tree, then
the others should wait for some time.

Huacai
>
> > ---
> >  mm/kfence/core.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> > index dad3c0eb70a0..e124ffff489f 100644
> > --- a/mm/kfence/core.c
> > +++ b/mm/kfence/core.c
> > @@ -566,13 +566,14 @@ static void rcu_guarded_free(struct rcu_head *h)
> >   */
> >  static unsigned long kfence_init_pool(void)
> >  {
> > -       unsigned long addr = (unsigned long)__kfence_pool;
> > +       unsigned long addr;
> >         struct page *pages;
> >         int i;
> >
> >         if (!arch_kfence_init_pool())
> > -               return addr;
> > +               return (unsigned long)__kfence_pool;
> >
> > +       addr = (unsigned long)__kfence_pool;
> >         pages = virt_to_page(__kfence_pool);
> >
> >         /*
> > --
> > 2.34.1
> >

Re: [PATCH 3/4] KFENCE: Deferring the assignment of the local variable addr

[Marco Elver]

On Wed, 19 Jul 2023 at 17:06, Huacai Chen <chenhuacai@kernel.org> wrote:
>
> Hi, Marco,
>
> On Wed, Jul 19, 2023 at 6:55 PM Marco Elver <elver@google.com> wrote:
> >
> > On Wed, 19 Jul 2023 at 10:28, Enze Li <lienze@kylinos.cn> wrote:
> > >
> > > The LoongArch architecture is different from other architectures.
> > > It needs to update __kfence_pool during arch_kfence_init_pool.
> > >
> > > This patch modifies the assignment location of the local variable addr
> > > in the kfence_init_pool function to support the case of updating
> > > __kfence_pool in arch_kfence_init_pool.
> > >
> > > Signed-off-by: Enze Li <lienze@kylinos.cn>
> >
> > I think it's fair to allow this use case.
> >
> > However, please make sure that when your arch_kfence_init_pool()
> > fails, it is still possible to free the memblock allocated memory
> > properly.
> >
> > Acked-by: Marco Elver <elver@google.com>
> Does Acked-by means this patch can go through loongarch tree together
> with other patches? If this patch should go through kfence tree, then
> the others should wait for some time.

It can go through loongarch tree. I don't think there are conflicts
with -mm around the patch's location right now.

Thanks,
-- Marco

Re: [PATCH 2/4] LoongArch: Get stack without NMI when providing regs parameter

[Huacai Chen]

Hi, Enze,

On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>
> Currently, executing arch_stack_walk can only get the full stack
> information including NMI.  This is because the implementation
> of arch_stack_walk is forced to ignore the information passed by the
> regs parameter and use the current stack information instead.
>
> For some detection systems like KFENCE, only partial stack information
> is needed.  In particular, the stack frame where the interrupt occurred.
>
> To support KFENCE, this patch modifies the implementation of the
> arch_stack_walk function so that if this function is called with the
> regs argument passed, it retains all the stack information in regs and
> uses it to provide accurate information.
>
> Before the patch applied, I get,
> [    1.531195 ] ==================================================================
> [    1.531442 ] BUG: KFENCE: out-of-bounds read in stack_trace_save_regs+0x48/0x6c
> [    1.531442 ]
> [    1.531900 ] Out-of-bounds read at 0xffff800012267fff (1B left of kfence-#12):
> [    1.532046 ]  stack_trace_save_regs+0x48/0x6c
> [    1.532169 ]  kfence_report_error+0xa4/0x528
> [    1.532276 ]  kfence_handle_page_fault+0x124/0x270
> [    1.532388 ]  no_context+0x50/0x94
> [    1.532453 ]  do_page_fault+0x1a8/0x36c
> [    1.532524 ]  tlb_do_page_fault_0+0x118/0x1b4
> [    1.532623 ]  test_out_of_bounds_read+0xa0/0x1d8
> [    1.532745 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
> [    1.532854 ]  kthread+0x124/0x130
> [    1.532922 ]  ret_from_kernel_thread+0xc/0xa4
> <snip>
>
> With this patch applied, I get the correct stack information.
> [    1.320220 ] ==================================================================
> [    1.320401 ] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0xa8/0x1d8
> [    1.320401 ]
> [    1.320898 ] Out-of-bounds read at 0xffff800012257fff (1B left of kfence-#10):
> [    1.321134 ]  test_out_of_bounds_read+0xa8/0x1d8
> [    1.321264 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
> [    1.321392 ]  kthread+0x124/0x130
> [    1.321459 ]  ret_from_kernel_thread+0xc/0xa4
> <snip>
>
> Signed-off-by: Enze Li <lienze@kylinos.cn>
> ---
>  arch/loongarch/kernel/stacktrace.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
> index 2463d2fea21f..21f60811e26f 100644
> --- a/arch/loongarch/kernel/stacktrace.c
> +++ b/arch/loongarch/kernel/stacktrace.c
> @@ -18,16 +18,20 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>         struct pt_regs dummyregs;
>         struct unwind_state state;
>
> -       regs = &dummyregs;
> -
>         if (task == current) {
> -               regs->regs[3] = (unsigned long)__builtin_frame_address(0);
> -               regs->csr_era = (unsigned long)__builtin_return_address(0);
> +               if (regs)
> +                       memcpy(&dummyregs, regs, sizeof(*regs));
> +               else {
> +                       dummyregs.regs[3] = (unsigned long)__builtin_frame_address(0);
> +                       dummyregs.csr_era = (unsigned long)__builtin_return_address(0);
> +               }
>         } else {
When "task != current", we don't need to handle the "regs != NULL" case?

Huacai

> -               regs->regs[3] = thread_saved_fp(task);
> -               regs->csr_era = thread_saved_ra(task);
> +               dummyregs.regs[3] = thread_saved_fp(task);
> +               dummyregs.csr_era = thread_saved_ra(task);
>         }
>
> +       regs = &dummyregs;
> +
>         regs->regs[1] = 0;
>         for (unwind_start(&state, task, regs);
>              !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {
> --
> 2.34.1
>
>

Re: [PATCH 4/4] LoongArch: Add KFENCE support

[Huacai Chen]

Hi, Enze,

On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>
> The LoongArch architecture is quite different from other architectures.
> When the allocating of KFENCE itself is done, it is mapped to the direct
> mapping configuration window [1] by default on LoongArch.  It means that
> it is not possible to use the page table mapped mode which required by
> the KFENCE system and therefore it should be remapped to the appropriate
> region.
>
> This patch adds architecture specific implementation details for KFENCE.
> In particular, this implements the required interface in <asm/kfence.h>.
>
> Tested this patch by using the testcases and all passed.
>
> [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
>
> Signed-off-by: Enze Li <lienze@kylinos.cn>
> ---
>  arch/loongarch/Kconfig               |  1 +
>  arch/loongarch/include/asm/kfence.h  | 62 ++++++++++++++++++++++++++++
>  arch/loongarch/include/asm/pgtable.h |  6 +++
>  arch/loongarch/mm/fault.c            | 22 ++++++----
>  4 files changed, 83 insertions(+), 8 deletions(-)
>  create mode 100644 arch/loongarch/include/asm/kfence.h
>
> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> index 5411e3a4eb88..db27729003d3 100644
> --- a/arch/loongarch/Kconfig
> +++ b/arch/loongarch/Kconfig
> @@ -93,6 +93,7 @@ config LOONGARCH
>         select HAVE_ARCH_JUMP_LABEL
>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
>         select HAVE_ARCH_KASAN
> +       select HAVE_ARCH_KFENCE if 64BIT
"if 64BIT" can be dropped here.

>         select HAVE_ARCH_MMAP_RND_BITS if MMU
>         select HAVE_ARCH_SECCOMP_FILTER
>         select HAVE_ARCH_TRACEHOOK
> diff --git a/arch/loongarch/include/asm/kfence.h b/arch/loongarch/include/asm/kfence.h
> new file mode 100644
> index 000000000000..2a85acc2bc70
> --- /dev/null
> +++ b/arch/loongarch/include/asm/kfence.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KFENCE support for LoongArch.
> + *
> + * Author: Enze Li <lienze@kylinos.cn>
> + * Copyright (C) 2022-2023 KylinSoft Corporation.
> + */
> +
> +#ifndef _ASM_LOONGARCH_KFENCE_H
> +#define _ASM_LOONGARCH_KFENCE_H
> +
> +#include <linux/kfence.h>
> +#include <asm/pgtable.h>
> +#include <asm/tlb.h>
> +
> +static inline char *arch_kfence_init_pool(void)
> +{
> +       char *__kfence_pool_orig = __kfence_pool;
I prefer kfence_pool than __kfence_pool_orig here.

> +       struct vm_struct *area;
> +       int err;
> +
> +       area = __get_vm_area_caller(KFENCE_POOL_SIZE, VM_IOREMAP,
> +                                   KFENCE_AREA_START, KFENCE_AREA_END,
> +                                   __builtin_return_address(0));
> +       if (!area)
> +               return NULL;
> +
> +       __kfence_pool = (char *)area->addr;
> +       err = ioremap_page_range((unsigned long)__kfence_pool,
> +                                (unsigned long)__kfence_pool + KFENCE_POOL_SIZE,
> +                                virt_to_phys((void *)__kfence_pool_orig),
> +                                PAGE_KERNEL);
> +       if (err) {
> +               free_vm_area(area);
> +               return NULL;
> +       }
> +
> +       return __kfence_pool;
> +}
> +
> +/* Protect the given page and flush TLB. */
> +static inline bool kfence_protect_page(unsigned long addr, bool protect)
> +{
> +       pte_t *pte = virt_to_kpte(addr);
> +
> +       if (WARN_ON(!pte) || pte_none(*pte))
> +               return false;
> +
> +       if (protect)
> +               set_pte(pte, __pte(pte_val(*pte) & ~(_PAGE_VALID | _PAGE_PRESENT)));
> +       else
> +               set_pte(pte, __pte(pte_val(*pte) | (_PAGE_VALID | _PAGE_PRESENT)));
> +
> +       /* Flush this CPU's TLB. */
> +       preempt_disable();
> +       local_flush_tlb_one(addr);
> +       preempt_enable();
> +
> +       return true;
> +}
> +
> +#endif /* _ASM_LOONGARCH_KFENCE_H */
> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> index 0fc074b8bd48..5a9c81298fe3 100644
> --- a/arch/loongarch/include/asm/pgtable.h
> +++ b/arch/loongarch/include/asm/pgtable.h
> @@ -85,7 +85,13 @@ extern unsigned long zero_page_mask;
>  #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
>  #define MODULES_END    (MODULES_VADDR + SZ_256M)
>
> +#ifdef CONFIG_KFENCE
> +#define KFENCE_AREA_START      MODULES_END
> +#define KFENCE_AREA_END                (KFENCE_AREA_START + SZ_512M)
Why you choose 512M here?

> +#define VMALLOC_START          KFENCE_AREA_END
> +#else
>  #define VMALLOC_START  MODULES_END
> +#endif
I don't like to put KFENCE_AREA between module and vmalloc range (it
may cause some problems), can we put it after vmemmap?

Huacai
>
>  #ifndef CONFIG_KASAN
>  #define VMALLOC_END    \
> diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
> index da5b6d518cdb..c0319128b221 100644
> --- a/arch/loongarch/mm/fault.c
> +++ b/arch/loongarch/mm/fault.c
> @@ -23,6 +23,7 @@
>  #include <linux/kprobes.h>
>  #include <linux/perf_event.h>
>  #include <linux/uaccess.h>
> +#include <linux/kfence.h>
>
>  #include <asm/branch.h>
>  #include <asm/mmu_context.h>
> @@ -30,7 +31,8 @@
>
>  int show_unhandled_signals = 1;
>
> -static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
> +static void __kprobes no_context(struct pt_regs *regs, unsigned long address,
> +                                unsigned long write)
>  {
>         const int field = sizeof(unsigned long) * 2;
>
> @@ -38,6 +40,9 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>         if (fixup_exception(regs))
>                 return;
>
> +       if (kfence_handle_page_fault(address, write, regs))
> +               return;
> +
>         /*
>          * Oops. The kernel tried to access some bad page. We'll have to
>          * terminate things with extreme prejudice.
> @@ -51,14 +56,15 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>         die("Oops", regs);
>  }
>
> -static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address)
> +static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address,
> +                                      unsigned long write)
>  {
>         /*
>          * We ran out of memory, call the OOM killer, and return the userspace
>          * (which will retry the fault, or kill us if we got oom-killed).
>          */
>         if (!user_mode(regs)) {
> -               no_context(regs, address);
> +               no_context(regs, address, write);
>                 return;
>         }
>         pagefault_out_of_memory();
> @@ -69,7 +75,7 @@ static void __kprobes do_sigbus(struct pt_regs *regs,
>  {
>         /* Kernel mode? Handle exceptions or die */
>         if (!user_mode(regs)) {
> -               no_context(regs, address);
> +               no_context(regs, address, write);
>                 return;
>         }
>
> @@ -90,7 +96,7 @@ static void __kprobes do_sigsegv(struct pt_regs *regs,
>
>         /* Kernel mode? Handle exceptions or die */
>         if (!user_mode(regs)) {
> -               no_context(regs, address);
> +               no_context(regs, address, write);
>                 return;
>         }
>
> @@ -149,7 +155,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>          */
>         if (address & __UA_LIMIT) {
>                 if (!user_mode(regs))
> -                       no_context(regs, address);
> +                       no_context(regs, address, write);
>                 else
>                         do_sigsegv(regs, write, address, si_code);
>                 return;
> @@ -211,7 +217,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>
>         if (fault_signal_pending(fault, regs)) {
>                 if (!user_mode(regs))
> -                       no_context(regs, address);
> +                       no_context(regs, address, write);
>                 return;
>         }
>
> @@ -232,7 +238,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>         if (unlikely(fault & VM_FAULT_ERROR)) {
>                 mmap_read_unlock(mm);
>                 if (fault & VM_FAULT_OOM) {
> -                       do_out_of_memory(regs, address);
> +                       do_out_of_memory(regs, address, write);
>                         return;
>                 } else if (fault & VM_FAULT_SIGSEGV) {
>                         do_sigsegv(regs, write, address, si_code);
> --
> 2.34.1
>
>

Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Huacai Chen]

Hi, Enze,

On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>
> According to LoongArch documentation online, there are two types of address
> translation modes: direct mapped address translation mode (direct mapped mode)
> and page table mapped address translation mode (page table mapped mode).
>
> Currently, the upstream code only supports DMM (Direct Mapped Mode).
> This patch adds a function that determines whether PTMM (Page Table
> Mapped Mode) should be used, and also adds the corresponding handler
> funcitons for both modes.
>
> For more details on the two modes, see [1].
>
> [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
>
> Signed-off-by: Enze Li <lienze@kylinos.cn>
> ---
>  arch/loongarch/include/asm/page.h    | 10 ++++++++++
>  arch/loongarch/include/asm/pgtable.h |  6 ++++++
>  arch/loongarch/mm/pgtable.c          | 25 +++++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
>
> diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
> index 26e8dccb6619..05919be15801 100644
> --- a/arch/loongarch/include/asm/page.h
> +++ b/arch/loongarch/include/asm/page.h
> @@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>  #define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))
>
>  #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
> +
> +#ifdef CONFIG_64BIT
> +#define virt_to_page(kaddr)                                            \
> +({                                                                     \
> +       is_PTMM_addr((unsigned long)kaddr) ?                            \
> +       PTMM_virt_to_page((unsigned long)kaddr) :                       \
> +       DMM_virt_to_page((unsigned long)kaddr);                         \
> +})
1, Rename these helpers to
is_dmw_addr()/dmw_virt_to_page()/tlb_virt_to_page() will be better.
2, These helpers are so simple so can be defined as inline function or
macros in page.h.
3, CONFIG_64BIT can be removed here.

Huacai

> +#else
>  #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
> +#endif
>
>  extern int __virt_addr_valid(volatile void *kaddr);
>  #define virt_addr_valid(kaddr) __virt_addr_valid((volatile void *)(kaddr))
> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> index ed6a37bb55b5..0fc074b8bd48 100644
> --- a/arch/loongarch/include/asm/pgtable.h
> +++ b/arch/loongarch/include/asm/pgtable.h
> @@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
>
> +#ifdef CONFIG_64BIT
> +struct page *DMM_virt_to_page(unsigned long kaddr);
> +struct page *PTMM_virt_to_page(unsigned long kaddr);
> +bool is_PTMM_addr(unsigned long kaddr);
> +#endif
> +
>  extern pgd_t swapper_pg_dir[];
>  extern pgd_t invalid_pg_dir[];
>
> diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c
> index 36a6dc0148ae..4c6448f996b6 100644
> --- a/arch/loongarch/mm/pgtable.c
> +++ b/arch/loongarch/mm/pgtable.c
> @@ -9,6 +9,31 @@
>  #include <asm/pgtable.h>
>  #include <asm/tlbflush.h>
>
> +#ifdef CONFIG_64BIT
> +/* DMM stands for Direct Mapped Mode. */
> +struct page *DMM_virt_to_page(unsigned long kaddr)
> +{
> +       return pfn_to_page(virt_to_pfn(kaddr));
> +}
> +EXPORT_SYMBOL_GPL(DMM_virt_to_page);
> +
> +/* PTMM stands for Page Table Mapped Mode. */
> +struct page *PTMM_virt_to_page(unsigned long kaddr)
> +{
> +       return pte_page(*virt_to_kpte(kaddr));
> +}
> +EXPORT_SYMBOL_GPL(PTMM_virt_to_page);
> +
> +bool is_PTMM_addr(unsigned long kaddr)
> +{
> +       if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) ==
> +                    GENMASK(BITS_PER_LONG - 1, cpu_vabits)))
> +               return true;
> +       return false;
> +}
> +EXPORT_SYMBOL_GPL(is_PTMM_addr);
> +#endif
> +
>  pgd_t *pgd_alloc(struct mm_struct *mm)
>  {
>         pgd_t *ret, *init;
> --
> 2.34.1
>
>

Re: [PATCH 2/4] LoongArch: Get stack without NMI when providing regs parameter

[Enze Li]

Hi Huacai,

Thanks for your review.

On Wed, Jul 19 2023 at 11:17:14 PM +0800, Huacai Chen wrote:

> Hi, Enze,
>
> On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>>
>> Currently, executing arch_stack_walk can only get the full stack
>> information including NMI.  This is because the implementation
>> of arch_stack_walk is forced to ignore the information passed by the
>> regs parameter and use the current stack information instead.
>>
>> For some detection systems like KFENCE, only partial stack information
>> is needed.  In particular, the stack frame where the interrupt occurred.
>>
>> To support KFENCE, this patch modifies the implementation of the
>> arch_stack_walk function so that if this function is called with the
>> regs argument passed, it retains all the stack information in regs and
>> uses it to provide accurate information.
>>
>> Before the patch applied, I get,
>> [    1.531195 ] ==================================================================
>> [    1.531442 ] BUG: KFENCE: out-of-bounds read in stack_trace_save_regs+0x48/0x6c
>> [    1.531442 ]
>> [    1.531900 ] Out-of-bounds read at 0xffff800012267fff (1B left of kfence-#12):
>> [    1.532046 ]  stack_trace_save_regs+0x48/0x6c
>> [    1.532169 ]  kfence_report_error+0xa4/0x528
>> [    1.532276 ]  kfence_handle_page_fault+0x124/0x270
>> [    1.532388 ]  no_context+0x50/0x94
>> [    1.532453 ]  do_page_fault+0x1a8/0x36c
>> [    1.532524 ]  tlb_do_page_fault_0+0x118/0x1b4
>> [    1.532623 ]  test_out_of_bounds_read+0xa0/0x1d8
>> [    1.532745 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [    1.532854 ]  kthread+0x124/0x130
>> [    1.532922 ]  ret_from_kernel_thread+0xc/0xa4
>> <snip>
>>
>> With this patch applied, I get the correct stack information.
>> [    1.320220 ] ==================================================================
>> [    1.320401 ] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0xa8/0x1d8
>> [    1.320401 ]
>> [    1.320898 ] Out-of-bounds read at 0xffff800012257fff (1B left of kfence-#10):
>> [    1.321134 ]  test_out_of_bounds_read+0xa8/0x1d8
>> [    1.321264 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
>> [    1.321392 ]  kthread+0x124/0x130
>> [    1.321459 ]  ret_from_kernel_thread+0xc/0xa4
>> <snip>
>>
>> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> ---
>>  arch/loongarch/kernel/stacktrace.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
>> index 2463d2fea21f..21f60811e26f 100644
>> --- a/arch/loongarch/kernel/stacktrace.c
>> +++ b/arch/loongarch/kernel/stacktrace.c
>> @@ -18,16 +18,20 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
>>         struct pt_regs dummyregs;
>>         struct unwind_state state;
>>
>> -       regs = &dummyregs;
>> -
>>         if (task == current) {
>> -               regs->regs[3] = (unsigned long)__builtin_frame_address(0);
>> -               regs->csr_era = (unsigned long)__builtin_return_address(0);
>> +               if (regs)
>> +                       memcpy(&dummyregs, regs, sizeof(*regs));
>> +               else {
>> +                       dummyregs.regs[3] = (unsigned long)__builtin_frame_address(0);
>> +                       dummyregs.csr_era = (unsigned long)__builtin_return_address(0);
>> +               }
>>         } else {
> When "task != current", we don't need to handle the "regs != NULL" case?
>
> Huacai
>

So far, I have not encountered this situation.  I'm not sure what
problems would arise from extending the modifications with "task !=
current".

However, these modifications now are sufficient for the KFENCE
system.  I would suggest that we don't modify other parts until we
encounter problems.  This way, we can forge ahead steadily.

Best Regards,
Enze

>> -               regs->regs[3] = thread_saved_fp(task);
>> -               regs->csr_era = thread_saved_ra(task);
>> +               dummyregs.regs[3] = thread_saved_fp(task);
>> +               dummyregs.csr_era = thread_saved_ra(task);
>>         }
>>
>> +       regs = &dummyregs;
>> +
>>         regs->regs[1] = 0;
>>         for (unwind_start(&state, task, regs);
>>              !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {
>> --
>> 2.34.1
>>
>>

Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Enze Li]

On Wed, Jul 19 2023 at 11:29:37 PM +0800, Huacai Chen wrote:

> Hi, Enze,
>
> On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>>
>> According to LoongArch documentation online, there are two types of address
>> translation modes: direct mapped address translation mode (direct mapped mode)
>> and page table mapped address translation mode (page table mapped mode).
>>
>> Currently, the upstream code only supports DMM (Direct Mapped Mode).
>> This patch adds a function that determines whether PTMM (Page Table
>> Mapped Mode) should be used, and also adds the corresponding handler
>> funcitons for both modes.
>>
>> For more details on the two modes, see [1].
>>
>> [1]
>> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
>>
>> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> ---
>>  arch/loongarch/include/asm/page.h    | 10 ++++++++++
>>  arch/loongarch/include/asm/pgtable.h |  6 ++++++
>>  arch/loongarch/mm/pgtable.c          | 25 +++++++++++++++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
>> index 26e8dccb6619..05919be15801 100644
>> --- a/arch/loongarch/include/asm/page.h
>> +++ b/arch/loongarch/include/asm/page.h
>> @@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>>  #define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))
>>
>>  #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
>> +
>> +#ifdef CONFIG_64BIT
>> +#define virt_to_page(kaddr)                                            \
>> +({                                                                     \
>> +       is_PTMM_addr((unsigned long)kaddr) ?                            \
>> +       PTMM_virt_to_page((unsigned long)kaddr) :                       \
>> +       DMM_virt_to_page((unsigned long)kaddr);                         \
>> +})
> 1, Rename these helpers to
> is_dmw_addr()/dmw_virt_to_page()/tlb_virt_to_page() will be better.
> 2, These helpers are so simple so can be defined as inline function or
> macros in page.h.

Hi Huacai,

Except for tlb_virt_to_page(), the remaining two modifications are easy.

I've run into a lot of problems when trying to make tlb_virt_to_page()
as a macro or inline function.  That's because we need to export this
symbol in order for it to be used by the module that called the
virt_to_page() function, other wise, we got the following errors,

-----------------------------------------------------------------------
  MODPOST Module.symvers
ERROR: modpost: "tlb_virt_to_page" [fs/hfsplus/hfsplus.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [fs/smb/client/cifs.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [crypto/gcm.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [crypto/ccm.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [crypto/essiv.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [lib/crypto/libchacha20poly1305.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/ttm/ttm.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/iscsi_tcp.ko] undefined!
ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/qla2xxx/qla2xxx.ko] undefined!
WARNING: modpost: suppressed 44 unresolved symbol warnings because there were too many)
-----------------------------------------------------------------------

It seems to me that wrapping it into a common function might be the only
way to successfully compile or link with this modification.

=========================================================
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -360,6 +360,8 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
 #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
 #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
 
+inline struct page *tlb_virt_to_page(unsigned long kaddr);
+

--- a/arch/loongarch/mm/pgtable.c
+++ b/arch/loongarch/mm/pgtable.c
@@ -9,6 +9,12 @@
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 
+inline struct page *tlb_virt_to_page(unsigned long kaddr)
+{
+       return pte_page(*virt_to_kpte(kaddr));
+}
+EXPORT_SYMBOL_GPL(tlb_virt_to_page);
=========================================================

WDYT?

Best Regards,
Enze

> 3, CONFIG_64BIT can be removed here.
>
> Huacai
>
>> +#else
>>  #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
>> +#endif
>>
>>  extern int __virt_addr_valid(volatile void *kaddr);
>>  #define virt_addr_valid(kaddr) __virt_addr_valid((volatile void *)(kaddr))
>> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
>> index ed6a37bb55b5..0fc074b8bd48 100644
>> --- a/arch/loongarch/include/asm/pgtable.h
>> +++ b/arch/loongarch/include/asm/pgtable.h
>> @@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
>>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
>>
>> +#ifdef CONFIG_64BIT
>> +struct page *DMM_virt_to_page(unsigned long kaddr);
>> +struct page *PTMM_virt_to_page(unsigned long kaddr);
>> +bool is_PTMM_addr(unsigned long kaddr);
>> +#endif
>> +
>>  extern pgd_t swapper_pg_dir[];
>>  extern pgd_t invalid_pg_dir[];
>>
>> diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c
>> index 36a6dc0148ae..4c6448f996b6 100644
>> --- a/arch/loongarch/mm/pgtable.c
>> +++ b/arch/loongarch/mm/pgtable.c
>> @@ -9,6 +9,31 @@
>>  #include <asm/pgtable.h>
>>  #include <asm/tlbflush.h>
>>
>> +#ifdef CONFIG_64BIT
>> +/* DMM stands for Direct Mapped Mode. */
>> +struct page *DMM_virt_to_page(unsigned long kaddr)
>> +{
>> +       return pfn_to_page(virt_to_pfn(kaddr));
>> +}
>> +EXPORT_SYMBOL_GPL(DMM_virt_to_page);
>> +
>> +/* PTMM stands for Page Table Mapped Mode. */
>> +struct page *PTMM_virt_to_page(unsigned long kaddr)
>> +{
>> +       return pte_page(*virt_to_kpte(kaddr));
>> +}
>> +EXPORT_SYMBOL_GPL(PTMM_virt_to_page);
>> +
>> +bool is_PTMM_addr(unsigned long kaddr)
>> +{
>> +       if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) ==
>> +                    GENMASK(BITS_PER_LONG - 1, cpu_vabits)))
>> +               return true;
>> +       return false;
>> +}
>> +EXPORT_SYMBOL_GPL(is_PTMM_addr);
>> +#endif
>> +
>>  pgd_t *pgd_alloc(struct mm_struct *mm)
>>  {
>>         pgd_t *ret, *init;
>> --
>> 2.34.1
>>
>>

Re: [PATCH 2/4] LoongArch: Get stack without NMI when providing regs parameter

[Huacai Chen]

On Fri, Jul 21, 2023 at 9:50 AM Enze Li <lienze@kylinos.cn> wrote:
>
> Hi Huacai,
>
> Thanks for your review.
>
> On Wed, Jul 19 2023 at 11:17:14 PM +0800, Huacai Chen wrote:
>
> > Hi, Enze,
> >
> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
> >>
> >> Currently, executing arch_stack_walk can only get the full stack
> >> information including NMI.  This is because the implementation
> >> of arch_stack_walk is forced to ignore the information passed by the
> >> regs parameter and use the current stack information instead.
> >>
> >> For some detection systems like KFENCE, only partial stack information
> >> is needed.  In particular, the stack frame where the interrupt occurred.
> >>
> >> To support KFENCE, this patch modifies the implementation of the
> >> arch_stack_walk function so that if this function is called with the
> >> regs argument passed, it retains all the stack information in regs and
> >> uses it to provide accurate information.
> >>
> >> Before the patch applied, I get,
> >> [    1.531195 ] ==================================================================
> >> [    1.531442 ] BUG: KFENCE: out-of-bounds read in stack_trace_save_regs+0x48/0x6c
> >> [    1.531442 ]
> >> [    1.531900 ] Out-of-bounds read at 0xffff800012267fff (1B left of kfence-#12):
> >> [    1.532046 ]  stack_trace_save_regs+0x48/0x6c
> >> [    1.532169 ]  kfence_report_error+0xa4/0x528
> >> [    1.532276 ]  kfence_handle_page_fault+0x124/0x270
> >> [    1.532388 ]  no_context+0x50/0x94
> >> [    1.532453 ]  do_page_fault+0x1a8/0x36c
> >> [    1.532524 ]  tlb_do_page_fault_0+0x118/0x1b4
> >> [    1.532623 ]  test_out_of_bounds_read+0xa0/0x1d8
> >> [    1.532745 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
> >> [    1.532854 ]  kthread+0x124/0x130
> >> [    1.532922 ]  ret_from_kernel_thread+0xc/0xa4
> >> <snip>
> >>
> >> With this patch applied, I get the correct stack information.
> >> [    1.320220 ] ==================================================================
> >> [    1.320401 ] BUG: KFENCE: out-of-bounds read in test_out_of_bounds_read+0xa8/0x1d8
> >> [    1.320401 ]
> >> [    1.320898 ] Out-of-bounds read at 0xffff800012257fff (1B left of kfence-#10):
> >> [    1.321134 ]  test_out_of_bounds_read+0xa8/0x1d8
> >> [    1.321264 ]  kunit_generic_run_threadfn_adapter+0x1c/0x28
> >> [    1.321392 ]  kthread+0x124/0x130
> >> [    1.321459 ]  ret_from_kernel_thread+0xc/0xa4
> >> <snip>
> >>
> >> Signed-off-by: Enze Li <lienze@kylinos.cn>
> >> ---
> >>  arch/loongarch/kernel/stacktrace.c | 16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/loongarch/kernel/stacktrace.c b/arch/loongarch/kernel/stacktrace.c
> >> index 2463d2fea21f..21f60811e26f 100644
> >> --- a/arch/loongarch/kernel/stacktrace.c
> >> +++ b/arch/loongarch/kernel/stacktrace.c
> >> @@ -18,16 +18,20 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
> >>         struct pt_regs dummyregs;
> >>         struct unwind_state state;
> >>
> >> -       regs = &dummyregs;
> >> -
> >>         if (task == current) {
> >> -               regs->regs[3] = (unsigned long)__builtin_frame_address(0);
> >> -               regs->csr_era = (unsigned long)__builtin_return_address(0);
> >> +               if (regs)
> >> +                       memcpy(&dummyregs, regs, sizeof(*regs));
> >> +               else {
> >> +                       dummyregs.regs[3] = (unsigned long)__builtin_frame_address(0);
> >> +                       dummyregs.csr_era = (unsigned long)__builtin_return_address(0);
> >> +               }
> >>         } else {
> > When "task != current", we don't need to handle the "regs != NULL" case?
> >
> > Huacai
> >
>
> So far, I have not encountered this situation.  I'm not sure what
> problems would arise from extending the modifications with "task !=
> current".
>
> However, these modifications now are sufficient for the KFENCE
> system.  I would suggest that we don't modify other parts until we
> encounter problems.  This way, we can forge ahead steadily.
I don't think so. In my opinion, "partial stack information" is a
clear requirement, whether the task is current or not.

So, if  the input regs is not NULL, we should always
memcpy(&dummyregs, regs, sizeof(*regs));

Or we may listen to Tiezhu's idea?

Huacai
>
> Best Regards,
> Enze
>
> >> -               regs->regs[3] = thread_saved_fp(task);
> >> -               regs->csr_era = thread_saved_ra(task);
> >> +               dummyregs.regs[3] = thread_saved_fp(task);
> >> +               dummyregs.csr_era = thread_saved_ra(task);
> >>         }
> >>
> >> +       regs = &dummyregs;
> >> +
> >>         regs->regs[1] = 0;
> >>         for (unwind_start(&state, task, regs);
> >>              !unwind_done(&state) && !unwind_error(&state); unwind_next_frame(&state)) {
> >> --
> >> 2.34.1
> >>
> >>

Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Huacai Chen]

On Fri, Jul 21, 2023 at 10:12 AM Enze Li <lienze@kylinos.cn> wrote:
>
> On Wed, Jul 19 2023 at 11:29:37 PM +0800, Huacai Chen wrote:
>
> > Hi, Enze,
> >
> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
> >>
> >> According to LoongArch documentation online, there are two types of address
> >> translation modes: direct mapped address translation mode (direct mapped mode)
> >> and page table mapped address translation mode (page table mapped mode).
> >>
> >> Currently, the upstream code only supports DMM (Direct Mapped Mode).
> >> This patch adds a function that determines whether PTMM (Page Table
> >> Mapped Mode) should be used, and also adds the corresponding handler
> >> funcitons for both modes.
> >>
> >> For more details on the two modes, see [1].
> >>
> >> [1]
> >> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
> >>
> >> Signed-off-by: Enze Li <lienze@kylinos.cn>
> >> ---
> >>  arch/loongarch/include/asm/page.h    | 10 ++++++++++
> >>  arch/loongarch/include/asm/pgtable.h |  6 ++++++
> >>  arch/loongarch/mm/pgtable.c          | 25 +++++++++++++++++++++++++
> >>  3 files changed, 41 insertions(+)
> >>
> >> diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
> >> index 26e8dccb6619..05919be15801 100644
> >> --- a/arch/loongarch/include/asm/page.h
> >> +++ b/arch/loongarch/include/asm/page.h
> >> @@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> >>  #define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))
> >>
> >>  #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
> >> +
> >> +#ifdef CONFIG_64BIT
> >> +#define virt_to_page(kaddr)                                            \
> >> +({                                                                     \
> >> +       is_PTMM_addr((unsigned long)kaddr) ?                            \
> >> +       PTMM_virt_to_page((unsigned long)kaddr) :                       \
> >> +       DMM_virt_to_page((unsigned long)kaddr);                         \
> >> +})
> > 1, Rename these helpers to
> > is_dmw_addr()/dmw_virt_to_page()/tlb_virt_to_page() will be better.
> > 2, These helpers are so simple so can be defined as inline function or
> > macros in page.h.
>
> Hi Huacai,
>
> Except for tlb_virt_to_page(), the remaining two modifications are easy.
>
> I've run into a lot of problems when trying to make tlb_virt_to_page()
> as a macro or inline function.  That's because we need to export this
> symbol in order for it to be used by the module that called the
> virt_to_page() function, other wise, we got the following errors,
>
> -----------------------------------------------------------------------
>   MODPOST Module.symvers
> ERROR: modpost: "tlb_virt_to_page" [fs/hfsplus/hfsplus.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [fs/smb/client/cifs.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [crypto/gcm.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [crypto/ccm.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [crypto/essiv.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [lib/crypto/libchacha20poly1305.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/ttm/ttm.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/iscsi_tcp.ko] undefined!
> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/qla2xxx/qla2xxx.ko] undefined!
> WARNING: modpost: suppressed 44 unresolved symbol warnings because there were too many)
> -----------------------------------------------------------------------
>
> It seems to me that wrapping it into a common function might be the only
> way to successfully compile or link with this modification.
>
> =========================================================
> --- a/arch/loongarch/include/asm/pgtable.h
> +++ b/arch/loongarch/include/asm/pgtable.h
> @@ -360,6 +360,8 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
>
> +inline struct page *tlb_virt_to_page(unsigned long kaddr);
> +
>
> --- a/arch/loongarch/mm/pgtable.c
> +++ b/arch/loongarch/mm/pgtable.c
> @@ -9,6 +9,12 @@
>  #include <asm/pgtable.h>
>  #include <asm/tlbflush.h>
>
> +inline struct page *tlb_virt_to_page(unsigned long kaddr)
> +{
> +       return pte_page(*virt_to_kpte(kaddr));
> +}
> +EXPORT_SYMBOL_GPL(tlb_virt_to_page);
> =========================================================
>
> WDYT?
>
> Best Regards,
> Enze
If you define "static inline" functions in page.h, there will be no problems.

Huacai
>
> > 3, CONFIG_64BIT can be removed here.
> >
> > Huacai
> >
> >> +#else
> >>  #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
> >> +#endif
> >>
> >>  extern int __virt_addr_valid(volatile void *kaddr);
> >>  #define virt_addr_valid(kaddr) __virt_addr_valid((volatile void *)(kaddr))
> >> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> >> index ed6a37bb55b5..0fc074b8bd48 100644
> >> --- a/arch/loongarch/include/asm/pgtable.h
> >> +++ b/arch/loongarch/include/asm/pgtable.h
> >> @@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
> >>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
> >>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
> >>
> >> +#ifdef CONFIG_64BIT
> >> +struct page *DMM_virt_to_page(unsigned long kaddr);
> >> +struct page *PTMM_virt_to_page(unsigned long kaddr);
> >> +bool is_PTMM_addr(unsigned long kaddr);
> >> +#endif
> >> +
> >>  extern pgd_t swapper_pg_dir[];
> >>  extern pgd_t invalid_pg_dir[];
> >>
> >> diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c
> >> index 36a6dc0148ae..4c6448f996b6 100644
> >> --- a/arch/loongarch/mm/pgtable.c
> >> +++ b/arch/loongarch/mm/pgtable.c
> >> @@ -9,6 +9,31 @@
> >>  #include <asm/pgtable.h>
> >>  #include <asm/tlbflush.h>
> >>
> >> +#ifdef CONFIG_64BIT
> >> +/* DMM stands for Direct Mapped Mode. */
> >> +struct page *DMM_virt_to_page(unsigned long kaddr)
> >> +{
> >> +       return pfn_to_page(virt_to_pfn(kaddr));
> >> +}
> >> +EXPORT_SYMBOL_GPL(DMM_virt_to_page);
> >> +
> >> +/* PTMM stands for Page Table Mapped Mode. */
> >> +struct page *PTMM_virt_to_page(unsigned long kaddr)
> >> +{
> >> +       return pte_page(*virt_to_kpte(kaddr));
> >> +}
> >> +EXPORT_SYMBOL_GPL(PTMM_virt_to_page);
> >> +
> >> +bool is_PTMM_addr(unsigned long kaddr)
> >> +{
> >> +       if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) ==
> >> +                    GENMASK(BITS_PER_LONG - 1, cpu_vabits)))
> >> +               return true;
> >> +       return false;
> >> +}
> >> +EXPORT_SYMBOL_GPL(is_PTMM_addr);
> >> +#endif
> >> +
> >>  pgd_t *pgd_alloc(struct mm_struct *mm)
> >>  {
> >>         pgd_t *ret, *init;
> >> --
> >> 2.34.1
> >>
> >>
>

Re: [PATCH 4/4] LoongArch: Add KFENCE support

[Enze Li]

On Wed, Jul 19 2023 at 11:27:50 PM +0800, Huacai Chen wrote:

> Hi, Enze,
>
> On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>>
>> The LoongArch architecture is quite different from other architectures.
>> When the allocating of KFENCE itself is done, it is mapped to the direct
>> mapping configuration window [1] by default on LoongArch.  It means that
>> it is not possible to use the page table mapped mode which required by
>> the KFENCE system and therefore it should be remapped to the appropriate
>> region.
>>
>> This patch adds architecture specific implementation details for KFENCE.
>> In particular, this implements the required interface in <asm/kfence.h>.
>>
>> Tested this patch by using the testcases and all passed.
>>
>> [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
>>
>> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> ---
>>  arch/loongarch/Kconfig               |  1 +
>>  arch/loongarch/include/asm/kfence.h  | 62 ++++++++++++++++++++++++++++
>>  arch/loongarch/include/asm/pgtable.h |  6 +++
>>  arch/loongarch/mm/fault.c            | 22 ++++++----
>>  4 files changed, 83 insertions(+), 8 deletions(-)
>>  create mode 100644 arch/loongarch/include/asm/kfence.h
>>
>> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
>> index 5411e3a4eb88..db27729003d3 100644
>> --- a/arch/loongarch/Kconfig
>> +++ b/arch/loongarch/Kconfig
>> @@ -93,6 +93,7 @@ config LOONGARCH
>>         select HAVE_ARCH_JUMP_LABEL
>>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
>>         select HAVE_ARCH_KASAN
>> +       select HAVE_ARCH_KFENCE if 64BIT
> "if 64BIT" can be dropped here.
>

Fixed.

>>         select HAVE_ARCH_MMAP_RND_BITS if MMU
>>         select HAVE_ARCH_SECCOMP_FILTER
>>         select HAVE_ARCH_TRACEHOOK
>> diff --git a/arch/loongarch/include/asm/kfence.h b/arch/loongarch/include/asm/kfence.h
>> new file mode 100644
>> index 000000000000..2a85acc2bc70
>> --- /dev/null
>> +++ b/arch/loongarch/include/asm/kfence.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * KFENCE support for LoongArch.
>> + *
>> + * Author: Enze Li <lienze@kylinos.cn>
>> + * Copyright (C) 2022-2023 KylinSoft Corporation.
>> + */
>> +
>> +#ifndef _ASM_LOONGARCH_KFENCE_H
>> +#define _ASM_LOONGARCH_KFENCE_H
>> +
>> +#include <linux/kfence.h>
>> +#include <asm/pgtable.h>
>> +#include <asm/tlb.h>
>> +
>> +static inline char *arch_kfence_init_pool(void)
>> +{
>> +       char *__kfence_pool_orig = __kfence_pool;
> I prefer kfence_pool than __kfence_pool_orig here.
>

Fixed.

>> +       struct vm_struct *area;
>> +       int err;
>> +
>> +       area = __get_vm_area_caller(KFENCE_POOL_SIZE, VM_IOREMAP,
>> +                                   KFENCE_AREA_START, KFENCE_AREA_END,
>> +                                   __builtin_return_address(0));
>> +       if (!area)
>> +               return NULL;
>> +
>> +       __kfence_pool = (char *)area->addr;
>> +       err = ioremap_page_range((unsigned long)__kfence_pool,
>> +                                (unsigned long)__kfence_pool + KFENCE_POOL_SIZE,
>> +                                virt_to_phys((void *)__kfence_pool_orig),
>> +                                PAGE_KERNEL);
>> +       if (err) {
>> +               free_vm_area(area);
>> +               return NULL;
>> +       }
>> +
>> +       return __kfence_pool;
>> +}
>> +
>> +/* Protect the given page and flush TLB. */
>> +static inline bool kfence_protect_page(unsigned long addr, bool protect)
>> +{
>> +       pte_t *pte = virt_to_kpte(addr);
>> +
>> +       if (WARN_ON(!pte) || pte_none(*pte))
>> +               return false;
>> +
>> +       if (protect)
>> +               set_pte(pte, __pte(pte_val(*pte) & ~(_PAGE_VALID | _PAGE_PRESENT)));
>> +       else
>> +               set_pte(pte, __pte(pte_val(*pte) | (_PAGE_VALID | _PAGE_PRESENT)));
>> +
>> +       /* Flush this CPU's TLB. */
>> +       preempt_disable();
>> +       local_flush_tlb_one(addr);
>> +       preempt_enable();
>> +
>> +       return true;
>> +}
>> +
>> +#endif /* _ASM_LOONGARCH_KFENCE_H */
>> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
>> index 0fc074b8bd48..5a9c81298fe3 100644
>> --- a/arch/loongarch/include/asm/pgtable.h
>> +++ b/arch/loongarch/include/asm/pgtable.h
>> @@ -85,7 +85,13 @@ extern unsigned long zero_page_mask;
>>  #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
>>  #define MODULES_END    (MODULES_VADDR + SZ_256M)
>>
>> +#ifdef CONFIG_KFENCE
>> +#define KFENCE_AREA_START      MODULES_END
>> +#define KFENCE_AREA_END                (KFENCE_AREA_START + SZ_512M)
> Why you choose 512M here?
>

One day I noticed that 512M can hold 16K (default 255) KFENCE objects,
which should be more than enough and I think this should be appropriate.

As far as I see, KFENCE system does not have the upper limit of this
value(CONFIG_KFENCE_NUM_OBJECTS), which could theoretically be any
number.  There's another way, how about setting this value to be
determined by the configuration, like this,

========================================================================
 +#define KFENCE_AREA_END \
 + (KFENCE_AREA_START + (CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)
========================================================================

>> +#define VMALLOC_START          KFENCE_AREA_END
>> +#else
>>  #define VMALLOC_START  MODULES_END
>> +#endif
> I don't like to put KFENCE_AREA between module and vmalloc range (it
> may cause some problems), can we put it after vmemmap?

I found that there is not enough space after vmemmap and that these
spaces are affected by KASAN. As follows,

Without KASAN
###### module 0xffff800002008000~0xffff800012008000
###### malloc 0xffff800032008000~0xfffffefffe000000                        
###### vmemmap 0xffffff0000000000~0xffffffffffffffff

With KASAN
###### module 0xffff800002008000~0xffff800012008000
###### malloc 0xffff800032008000~0xffffbefffe000000
###### vmemmap 0xffffbf0000000000~0xffffbfffffffffff

What about put it before MODULES_START?

============================================================================
--- a/arch/loongarch/include/asm/pgtable.h
+++ b/arch/loongarch/include/asm/pgtable.h
@@ -82,7 +82,14 @@ extern unsigned long zero_page_mask;
  * Avoid the first couple of pages so NULL pointer dereferences will
  * still reliably trap.
  */
+#ifdef CONFIG_KFENCE
+#define KFENCE_AREA_START      (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
+#define KFENCE_AREA_END        \
+       (KFENCE_AREA_START + (CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)
+#define MODULES_VADDR  KFENCE_AREA_END
+#else
 #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
+#endif
 #define MODULES_END    (MODULES_VADDR + SZ_256M)
============================================================================

Best Regards,
Enze

>>
>>  #ifndef CONFIG_KASAN
>>  #define VMALLOC_END    \
>> diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
>> index da5b6d518cdb..c0319128b221 100644
>> --- a/arch/loongarch/mm/fault.c
>> +++ b/arch/loongarch/mm/fault.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/kprobes.h>
>>  #include <linux/perf_event.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/kfence.h>
>>
>>  #include <asm/branch.h>
>>  #include <asm/mmu_context.h>
>> @@ -30,7 +31,8 @@
>>
>>  int show_unhandled_signals = 1;
>>
>> -static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>> +static void __kprobes no_context(struct pt_regs *regs, unsigned long address,
>> +                                unsigned long write)
>>  {
>>         const int field = sizeof(unsigned long) * 2;
>>
>> @@ -38,6 +40,9 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>>         if (fixup_exception(regs))
>>                 return;
>>
>> +       if (kfence_handle_page_fault(address, write, regs))
>> +               return;
>> +
>>         /*
>>          * Oops. The kernel tried to access some bad page. We'll have to
>>          * terminate things with extreme prejudice.
>> @@ -51,14 +56,15 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>>         die("Oops", regs);
>>  }
>>
>> -static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address)
>> +static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address,
>> +                                      unsigned long write)
>>  {
>>         /*
>>          * We ran out of memory, call the OOM killer, and return the userspace
>>          * (which will retry the fault, or kill us if we got oom-killed).
>>          */
>>         if (!user_mode(regs)) {
>> -               no_context(regs, address);
>> +               no_context(regs, address, write);
>>                 return;
>>         }
>>         pagefault_out_of_memory();
>> @@ -69,7 +75,7 @@ static void __kprobes do_sigbus(struct pt_regs *regs,
>>  {
>>         /* Kernel mode? Handle exceptions or die */
>>         if (!user_mode(regs)) {
>> -               no_context(regs, address);
>> +               no_context(regs, address, write);
>>                 return;
>>         }
>>
>> @@ -90,7 +96,7 @@ static void __kprobes do_sigsegv(struct pt_regs *regs,
>>
>>         /* Kernel mode? Handle exceptions or die */
>>         if (!user_mode(regs)) {
>> -               no_context(regs, address);
>> +               no_context(regs, address, write);
>>                 return;
>>         }
>>
>> @@ -149,7 +155,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>>          */
>>         if (address & __UA_LIMIT) {
>>                 if (!user_mode(regs))
>> -                       no_context(regs, address);
>> +                       no_context(regs, address, write);
>>                 else
>>                         do_sigsegv(regs, write, address, si_code);
>>                 return;
>> @@ -211,7 +217,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>>
>>         if (fault_signal_pending(fault, regs)) {
>>                 if (!user_mode(regs))
>> -                       no_context(regs, address);
>> +                       no_context(regs, address, write);
>>                 return;
>>         }
>>
>> @@ -232,7 +238,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>>         if (unlikely(fault & VM_FAULT_ERROR)) {
>>                 mmap_read_unlock(mm);
>>                 if (fault & VM_FAULT_OOM) {
>> -                       do_out_of_memory(regs, address);
>> +                       do_out_of_memory(regs, address, write);
>>                         return;
>>                 } else if (fault & VM_FAULT_SIGSEGV) {
>>                         do_sigsegv(regs, write, address, si_code);
>> --
>> 2.34.1
>>
>>

Re: [PATCH 4/4] LoongArch: Add KFENCE support

[Huacai Chen]

Hi, Enze,

On Fri, Jul 21, 2023 at 11:14 AM Enze Li <lienze@kylinos.cn> wrote:
>
> On Wed, Jul 19 2023 at 11:27:50 PM +0800, Huacai Chen wrote:
>
> > Hi, Enze,
> >
> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
> >>
> >> The LoongArch architecture is quite different from other architectures.
> >> When the allocating of KFENCE itself is done, it is mapped to the direct
> >> mapping configuration window [1] by default on LoongArch.  It means that
> >> it is not possible to use the page table mapped mode which required by
> >> the KFENCE system and therefore it should be remapped to the appropriate
> >> region.
> >>
> >> This patch adds architecture specific implementation details for KFENCE.
> >> In particular, this implements the required interface in <asm/kfence.h>.
> >>
> >> Tested this patch by using the testcases and all passed.
> >>
> >> [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
> >>
> >> Signed-off-by: Enze Li <lienze@kylinos.cn>
> >> ---
> >>  arch/loongarch/Kconfig               |  1 +
> >>  arch/loongarch/include/asm/kfence.h  | 62 ++++++++++++++++++++++++++++
> >>  arch/loongarch/include/asm/pgtable.h |  6 +++
> >>  arch/loongarch/mm/fault.c            | 22 ++++++----
> >>  4 files changed, 83 insertions(+), 8 deletions(-)
> >>  create mode 100644 arch/loongarch/include/asm/kfence.h
> >>
> >> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
> >> index 5411e3a4eb88..db27729003d3 100644
> >> --- a/arch/loongarch/Kconfig
> >> +++ b/arch/loongarch/Kconfig
> >> @@ -93,6 +93,7 @@ config LOONGARCH
> >>         select HAVE_ARCH_JUMP_LABEL
> >>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
> >>         select HAVE_ARCH_KASAN
> >> +       select HAVE_ARCH_KFENCE if 64BIT
> > "if 64BIT" can be dropped here.
> >
>
> Fixed.
>
> >>         select HAVE_ARCH_MMAP_RND_BITS if MMU
> >>         select HAVE_ARCH_SECCOMP_FILTER
> >>         select HAVE_ARCH_TRACEHOOK
> >> diff --git a/arch/loongarch/include/asm/kfence.h b/arch/loongarch/include/asm/kfence.h
> >> new file mode 100644
> >> index 000000000000..2a85acc2bc70
> >> --- /dev/null
> >> +++ b/arch/loongarch/include/asm/kfence.h
> >> @@ -0,0 +1,62 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * KFENCE support for LoongArch.
> >> + *
> >> + * Author: Enze Li <lienze@kylinos.cn>
> >> + * Copyright (C) 2022-2023 KylinSoft Corporation.
> >> + */
> >> +
> >> +#ifndef _ASM_LOONGARCH_KFENCE_H
> >> +#define _ASM_LOONGARCH_KFENCE_H
> >> +
> >> +#include <linux/kfence.h>
> >> +#include <asm/pgtable.h>
> >> +#include <asm/tlb.h>
> >> +
> >> +static inline char *arch_kfence_init_pool(void)
> >> +{
> >> +       char *__kfence_pool_orig = __kfence_pool;
> > I prefer kfence_pool than __kfence_pool_orig here.
> >
>
> Fixed.
>
> >> +       struct vm_struct *area;
> >> +       int err;
> >> +
> >> +       area = __get_vm_area_caller(KFENCE_POOL_SIZE, VM_IOREMAP,
> >> +                                   KFENCE_AREA_START, KFENCE_AREA_END,
> >> +                                   __builtin_return_address(0));
> >> +       if (!area)
> >> +               return NULL;
> >> +
> >> +       __kfence_pool = (char *)area->addr;
> >> +       err = ioremap_page_range((unsigned long)__kfence_pool,
> >> +                                (unsigned long)__kfence_pool + KFENCE_POOL_SIZE,
> >> +                                virt_to_phys((void *)__kfence_pool_orig),
> >> +                                PAGE_KERNEL);
> >> +       if (err) {
> >> +               free_vm_area(area);
> >> +               return NULL;
> >> +       }
> >> +
> >> +       return __kfence_pool;
> >> +}
> >> +
> >> +/* Protect the given page and flush TLB. */
> >> +static inline bool kfence_protect_page(unsigned long addr, bool protect)
> >> +{
> >> +       pte_t *pte = virt_to_kpte(addr);
> >> +
> >> +       if (WARN_ON(!pte) || pte_none(*pte))
> >> +               return false;
> >> +
> >> +       if (protect)
> >> +               set_pte(pte, __pte(pte_val(*pte) & ~(_PAGE_VALID | _PAGE_PRESENT)));
> >> +       else
> >> +               set_pte(pte, __pte(pte_val(*pte) | (_PAGE_VALID | _PAGE_PRESENT)));
> >> +
> >> +       /* Flush this CPU's TLB. */
> >> +       preempt_disable();
> >> +       local_flush_tlb_one(addr);
> >> +       preempt_enable();
> >> +
> >> +       return true;
> >> +}
> >> +
> >> +#endif /* _ASM_LOONGARCH_KFENCE_H */
> >> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> >> index 0fc074b8bd48..5a9c81298fe3 100644
> >> --- a/arch/loongarch/include/asm/pgtable.h
> >> +++ b/arch/loongarch/include/asm/pgtable.h
> >> @@ -85,7 +85,13 @@ extern unsigned long zero_page_mask;
> >>  #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
> >>  #define MODULES_END    (MODULES_VADDR + SZ_256M)
> >>
> >> +#ifdef CONFIG_KFENCE
> >> +#define KFENCE_AREA_START      MODULES_END
> >> +#define KFENCE_AREA_END                (KFENCE_AREA_START + SZ_512M)
> > Why you choose 512M here?
> >
>
> One day I noticed that 512M can hold 16K (default 255) KFENCE objects,
> which should be more than enough and I think this should be appropriate.
>
> As far as I see, KFENCE system does not have the upper limit of this
> value(CONFIG_KFENCE_NUM_OBJECTS), which could theoretically be any
> number.  There's another way, how about setting this value to be
> determined by the configuration, like this,
>
> ========================================================================
>  +#define KFENCE_AREA_END \
>  + (KFENCE_AREA_START + (CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)
> ========================================================================
How does other archs configure the size?

>
> >> +#define VMALLOC_START          KFENCE_AREA_END
> >> +#else
> >>  #define VMALLOC_START  MODULES_END
> >> +#endif
> > I don't like to put KFENCE_AREA between module and vmalloc range (it
> > may cause some problems), can we put it after vmemmap?
>
> I found that there is not enough space after vmemmap and that these
> spaces are affected by KASAN. As follows,
>
> Without KASAN
> ###### module 0xffff800002008000~0xffff800012008000
> ###### malloc 0xffff800032008000~0xfffffefffe000000
> ###### vmemmap 0xffffff0000000000~0xffffffffffffffff
>
> With KASAN
> ###### module 0xffff800002008000~0xffff800012008000
> ###### malloc 0xffff800032008000~0xffffbefffe000000
> ###### vmemmap 0xffffbf0000000000~0xffffbfffffffffff
>
> What about put it before MODULES_START?
I temporarily drop KASAN in linux-next for you. You can update a new
patch version without KASAN (still, put KFENCE after vmemmap), and
then we can improve further.

Huacai
>
> ============================================================================
> --- a/arch/loongarch/include/asm/pgtable.h
> +++ b/arch/loongarch/include/asm/pgtable.h
> @@ -82,7 +82,14 @@ extern unsigned long zero_page_mask;
>   * Avoid the first couple of pages so NULL pointer dereferences will
>   * still reliably trap.
>   */
> +#ifdef CONFIG_KFENCE
> +#define KFENCE_AREA_START      (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
> +#define KFENCE_AREA_END        \
> +       (KFENCE_AREA_START + (CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)
> +#define MODULES_VADDR  KFENCE_AREA_END
> +#else
>  #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
> +#endif
>  #define MODULES_END    (MODULES_VADDR + SZ_256M)
> ============================================================================
>
> Best Regards,
> Enze
>
> >>
> >>  #ifndef CONFIG_KASAN
> >>  #define VMALLOC_END    \
> >> diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
> >> index da5b6d518cdb..c0319128b221 100644
> >> --- a/arch/loongarch/mm/fault.c
> >> +++ b/arch/loongarch/mm/fault.c
> >> @@ -23,6 +23,7 @@
> >>  #include <linux/kprobes.h>
> >>  #include <linux/perf_event.h>
> >>  #include <linux/uaccess.h>
> >> +#include <linux/kfence.h>
> >>
> >>  #include <asm/branch.h>
> >>  #include <asm/mmu_context.h>
> >> @@ -30,7 +31,8 @@
> >>
> >>  int show_unhandled_signals = 1;
> >>
> >> -static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
> >> +static void __kprobes no_context(struct pt_regs *regs, unsigned long address,
> >> +                                unsigned long write)
> >>  {
> >>         const int field = sizeof(unsigned long) * 2;
> >>
> >> @@ -38,6 +40,9 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
> >>         if (fixup_exception(regs))
> >>                 return;
> >>
> >> +       if (kfence_handle_page_fault(address, write, regs))
> >> +               return;
> >> +
> >>         /*
> >>          * Oops. The kernel tried to access some bad page. We'll have to
> >>          * terminate things with extreme prejudice.
> >> @@ -51,14 +56,15 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
> >>         die("Oops", regs);
> >>  }
> >>
> >> -static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address)
> >> +static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address,
> >> +                                      unsigned long write)
> >>  {
> >>         /*
> >>          * We ran out of memory, call the OOM killer, and return the userspace
> >>          * (which will retry the fault, or kill us if we got oom-killed).
> >>          */
> >>         if (!user_mode(regs)) {
> >> -               no_context(regs, address);
> >> +               no_context(regs, address, write);
> >>                 return;
> >>         }
> >>         pagefault_out_of_memory();
> >> @@ -69,7 +75,7 @@ static void __kprobes do_sigbus(struct pt_regs *regs,
> >>  {
> >>         /* Kernel mode? Handle exceptions or die */
> >>         if (!user_mode(regs)) {
> >> -               no_context(regs, address);
> >> +               no_context(regs, address, write);
> >>                 return;
> >>         }
> >>
> >> @@ -90,7 +96,7 @@ static void __kprobes do_sigsegv(struct pt_regs *regs,
> >>
> >>         /* Kernel mode? Handle exceptions or die */
> >>         if (!user_mode(regs)) {
> >> -               no_context(regs, address);
> >> +               no_context(regs, address, write);
> >>                 return;
> >>         }
> >>
> >> @@ -149,7 +155,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> >>          */
> >>         if (address & __UA_LIMIT) {
> >>                 if (!user_mode(regs))
> >> -                       no_context(regs, address);
> >> +                       no_context(regs, address, write);
> >>                 else
> >>                         do_sigsegv(regs, write, address, si_code);
> >>                 return;
> >> @@ -211,7 +217,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> >>
> >>         if (fault_signal_pending(fault, regs)) {
> >>                 if (!user_mode(regs))
> >> -                       no_context(regs, address);
> >> +                       no_context(regs, address, write);
> >>                 return;
> >>         }
> >>
> >> @@ -232,7 +238,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> >>         if (unlikely(fault & VM_FAULT_ERROR)) {
> >>                 mmap_read_unlock(mm);
> >>                 if (fault & VM_FAULT_OOM) {
> >> -                       do_out_of_memory(regs, address);
> >> +                       do_out_of_memory(regs, address, write);
> >>                         return;
> >>                 } else if (fault & VM_FAULT_SIGSEGV) {
> >>                         do_sigsegv(regs, write, address, si_code);
> >> --
> >> 2.34.1
> >>
> >>
>

Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Enze Li]

On Fri, Jul 21 2023 at 10:21:38 AM +0800, Huacai Chen wrote:

> On Fri, Jul 21, 2023 at 10:12 AM Enze Li <lienze@kylinos.cn> wrote:
>>
>> On Wed, Jul 19 2023 at 11:29:37 PM +0800, Huacai Chen wrote:
>>
>> > Hi, Enze,
>> >
>> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>> >>
>> >> According to LoongArch documentation online, there are two types of address
>> >> translation modes: direct mapped address translation mode (direct mapped mode)
>> >> and page table mapped address translation mode (page table mapped mode).
>> >>
>> >> Currently, the upstream code only supports DMM (Direct Mapped Mode).
>> >> This patch adds a function that determines whether PTMM (Page Table
>> >> Mapped Mode) should be used, and also adds the corresponding handler
>> >> funcitons for both modes.
>> >>
>> >> For more details on the two modes, see [1].
>> >>
>> >> [1]
>> >> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
>> >>
>> >> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> >> ---
>> >>  arch/loongarch/include/asm/page.h    | 10 ++++++++++
>> >>  arch/loongarch/include/asm/pgtable.h |  6 ++++++
>> >>  arch/loongarch/mm/pgtable.c          | 25 +++++++++++++++++++++++++
>> >>  3 files changed, 41 insertions(+)
>> >>
>> >> diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
>> >> index 26e8dccb6619..05919be15801 100644
>> >> --- a/arch/loongarch/include/asm/page.h
>> >> +++ b/arch/loongarch/include/asm/page.h
>> >> @@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>> >>  #define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))
>> >>
>> >>  #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
>> >> +
>> >> +#ifdef CONFIG_64BIT
>> >> +#define virt_to_page(kaddr)                                            \
>> >> +({                                                                     \
>> >> +       is_PTMM_addr((unsigned long)kaddr) ?                            \
>> >> +       PTMM_virt_to_page((unsigned long)kaddr) :                       \
>> >> +       DMM_virt_to_page((unsigned long)kaddr);                         \
>> >> +})
>> > 1, Rename these helpers to
>> > is_dmw_addr()/dmw_virt_to_page()/tlb_virt_to_page() will be better.
>> > 2, These helpers are so simple so can be defined as inline function or
>> > macros in page.h.
>>
>> Hi Huacai,
>>
>> Except for tlb_virt_to_page(), the remaining two modifications are easy.
>>
>> I've run into a lot of problems when trying to make tlb_virt_to_page()
>> as a macro or inline function.  That's because we need to export this
>> symbol in order for it to be used by the module that called the
>> virt_to_page() function, other wise, we got the following errors,
>>
>> -----------------------------------------------------------------------
>>   MODPOST Module.symvers
>> ERROR: modpost: "tlb_virt_to_page" [fs/hfsplus/hfsplus.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [fs/smb/client/cifs.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [crypto/gcm.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [crypto/ccm.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [crypto/essiv.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [lib/crypto/libchacha20poly1305.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/ttm/ttm.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/iscsi_tcp.ko] undefined!
>> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/qla2xxx/qla2xxx.ko] undefined!
>> WARNING: modpost: suppressed 44 unresolved symbol warnings because there were too many)
>> -----------------------------------------------------------------------
>>
>> It seems to me that wrapping it into a common function might be the only
>> way to successfully compile or link with this modification.
>>
>> =========================================================
>> --- a/arch/loongarch/include/asm/pgtable.h
>> +++ b/arch/loongarch/include/asm/pgtable.h
>> @@ -360,6 +360,8 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
>>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
>>
>> +inline struct page *tlb_virt_to_page(unsigned long kaddr);
>> +
>>
>> --- a/arch/loongarch/mm/pgtable.c
>> +++ b/arch/loongarch/mm/pgtable.c
>> @@ -9,6 +9,12 @@
>>  #include <asm/pgtable.h>
>>  #include <asm/tlbflush.h>
>>
>> +inline struct page *tlb_virt_to_page(unsigned long kaddr)
>> +{
>> +       return pte_page(*virt_to_kpte(kaddr));
>> +}
>> +EXPORT_SYMBOL_GPL(tlb_virt_to_page);
>> =========================================================
>>
>> WDYT?
>>
>> Best Regards,
>> Enze
> If you define "static inline" functions in page.h, there will be no problems.
>

Hi Huacai,

After failed over and over and over again, I think I've found the reason
why we can't define tlb_virt_to_page as macro or inline function in            
asm/page.h or asm/pgtable.h. :)

I'll go through this step by step.

If I put tlb_virt_to_page in asm/page.h as following,

=====================================================
+static inline struct page *tlb_virt_to_page(unsigned long kaddr)
+{
+       return pte_page(*virt_to_kpte(kaddr));
+}
=====================================================

and compile kernel, gcc says to me the following error.

--------------------------------------------------------------------
  CC      arch/loongarch/kernel/asm-offsets.s
In file included from ./include/linux/shm.h:6,
                 from ./include/linux/sched.h:16,
                 from arch/loongarch/kernel/asm-offsets.c:8:
./arch/loongarch/include/asm/page.h: In function ‘tlb_virt_to_page’:
./arch/loongarch/include/asm/page.h:126:16: error: implicit declaration of function ‘pte_page’ [-Werror=implicit-function-declaration]
  126 |         return pte_page(*virt_to_kpte(kaddr));
      |                ^~~~~~~~
---------------------------------------------------------------------

"pte_page" is declared in asm/pgtable.h, so I put "#include
<asm/pgtable.h>" ahead, like this,

=====================================================
+#include <asm/pgtable.h>
+static inline struct page *tlb_virt_to_page(unsigned long kaddr)
+{
+       return pte_page(*virt_to_kpte(kaddr));
+}
=====================================================

then compile again, gcc says,

---------------------------------------------------------------------
  CC      arch/loongarch/kernel/asm-offsets.s                                                            
In file included from ./arch/loongarch/include/asm/page.h:98,                                            
                 from ./include/linux/shm.h:6,                                                           
                 from ./include/linux/sched.h:16,                                                        
                 from arch/loongarch/kernel/asm-offsets.c:8:                                             
./arch/loongarch/include/asm/page.h: In function ‘tlb_virt_to_page’:                                     
./arch/loongarch/include/asm/page.h:127:26: error: implicit declaration of function ‘virt_to_kpte’; did you mean ‘virt_to_pfn’? [-Werror=implicit-function-declaration]
  127 |         return pte_page(*virt_to_kpte(kaddr));
      |                          ^~~~~~~~~~~~
---------------------------------------------------------------------

"virt_to_kpte" is defined in linux/pgtable.h, consequently I add "#include
<linux/pgtable.h>" as well,

=====================================================
+#include <asm/pgtable.h>
+#include <linux/pgtable.h>
+static inline struct page *tlb_virt_to_page(unsigned long kaddr)
+{
+       return pte_page(*virt_to_kpte(kaddr));
+}
=====================================================

and continue,

---------------------------------------------------------------------
  CC      arch/loongarch/kernel/asm-offsets.s                                                            
  CALL    scripts/checksyscalls.sh                                                                       
  CC      arch/loongarch/vdso/vgetcpu.o                                                                  
  CC      arch/loongarch/vdso/vgettimeofday.o                                                            
In file included from ./arch/loongarch/include/asm/page.h:124,                                           
                 from ./include/linux/mm_types_task.h:16,                                                
                 from ./include/linux/mm_types.h:5,                                                      
                 from ./include/linux/mmzone.h:22,                                                       
                 from ./include/linux/gfp.h:7,                                                           
                 from ./include/linux/mm.h:7,                                                            
                 from ./arch/loongarch/include/asm/vdso.h:10,                                            
                 from arch/loongarch/vdso/vgetcpu.c:6:                                                   
./arch/loongarch/include/asm/pgtable.h: In function ‘pte_accessible’:                                    
./arch/loongarch/include/asm/pgtable.h:436:40: error: invalid use of undefined type ‘struct mm_struct’   
  436 |                         atomic_read(&mm->tlb_flush_pending))                                     
      |                                        ^~       
---------------------------------------------------------------------

The first line above shows that it compiled successfully for the
asm-offsets module.  That's fair enough.  Actually, the point is the
next one (invalid use of undefined type 'struct mm_struct').

As we all know, before the compiler compiles, it expands the header
files first.  For this example, it firstly expands from the header file
vdso.h, then the mm.h file and so on.  We can see that the line 436 of
asm/pgtable.h are using 'struct mm_struct'.  When we backtrack to a file
that has been previously expanded, it's obvious that the definition of
mm_struct does not appear in the expanded file.  Instead, it appears
afterward (mm_types.h).

To be clear, I'll exemplify this case with a cheap ASCII diagram.

                                                                 ... <-|
                    we're using 'mm_struct' here >>>   asm/pgtable.h <-|
                                                                 ... <-|
                                                                       |
                                                               |->...  |
                                                               |->asm/page.h
                                                               |->...
                                                       |->...  |
                                         |->...        |->mm_types_task.h
                             |->...      |->mm_types.h-|->...
                    |->...   |->mmzone.h-|->... |
            |->...  |->gfp.h-|->...             |
  |->...    |->mm.h-|->...            But 'mm_struct' is defined here.
  |->vdso.h-|->...
  |->...
vgetcpu.c

I've also tried to include mm_types.h in advance, but in this case that
doesn't work because the _LINUX_MM_TYPES_H macro already exists.
The "forward declaration" was also taken into account, in the end it was
found to be unavailable as well.

In summary, I'm afraid that rewriting tlb_virt_to_page in asm/page.h as
a macro or inline function is not possible.  The root case of this is
that both 'struct mm_struct' and 'virt_to_kpte' belong to high-level
data structures, and if they are referenced in asm/page.h at the
low-level, dependency problems arise.

Anyway, we can at least define it as a normal function in asm/pgtable.h,
is that Okay with you?

It may be a bit wordy, so please bear with me.  In addition, all of the
above is my understanding, am I missing something?

Best Regards,
Enze

>>
>> > 3, CONFIG_64BIT can be removed here.
>> >
>> > Huacai
>> >
>> >> +#else
>> >>  #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
>> >> +#endif
>> >>
>> >>  extern int __virt_addr_valid(volatile void *kaddr);
>> >>  #define virt_addr_valid(kaddr) __virt_addr_valid((volatile void *)(kaddr))
>> >> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
>> >> index ed6a37bb55b5..0fc074b8bd48 100644
>> >> --- a/arch/loongarch/include/asm/pgtable.h
>> >> +++ b/arch/loongarch/include/asm/pgtable.h
>> >> @@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>> >>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
>> >>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
>> >>
>> >> +#ifdef CONFIG_64BIT
>> >> +struct page *DMM_virt_to_page(unsigned long kaddr);
>> >> +struct page *PTMM_virt_to_page(unsigned long kaddr);
>> >> +bool is_PTMM_addr(unsigned long kaddr);
>> >> +#endif
>> >> +
>> >>  extern pgd_t swapper_pg_dir[];
>> >>  extern pgd_t invalid_pg_dir[];
>> >>
>> >> diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c
>> >> index 36a6dc0148ae..4c6448f996b6 100644
>> >> --- a/arch/loongarch/mm/pgtable.c
>> >> +++ b/arch/loongarch/mm/pgtable.c
>> >> @@ -9,6 +9,31 @@
>> >>  #include <asm/pgtable.h>
>> >>  #include <asm/tlbflush.h>
>> >>
>> >> +#ifdef CONFIG_64BIT
>> >> +/* DMM stands for Direct Mapped Mode. */
>> >> +struct page *DMM_virt_to_page(unsigned long kaddr)
>> >> +{
>> >> +       return pfn_to_page(virt_to_pfn(kaddr));
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(DMM_virt_to_page);
>> >> +
>> >> +/* PTMM stands for Page Table Mapped Mode. */
>> >> +struct page *PTMM_virt_to_page(unsigned long kaddr)
>> >> +{
>> >> +       return pte_page(*virt_to_kpte(kaddr));
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(PTMM_virt_to_page);
>> >> +
>> >> +bool is_PTMM_addr(unsigned long kaddr)
>> >> +{
>> >> +       if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) ==
>> >> +                    GENMASK(BITS_PER_LONG - 1, cpu_vabits)))
>> >> +               return true;
>> >> +       return false;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(is_PTMM_addr);
>> >> +#endif
>> >> +
>> >>  pgd_t *pgd_alloc(struct mm_struct *mm)
>> >>  {
>> >>         pgd_t *ret, *init;
>> >> --
>> >> 2.34.1
>> >>
>> >>
>>

Re: [PATCH 4/4] LoongArch: Add KFENCE support

[Enze Li]

On Fri, Jul 21 2023 at 11:19:10 AM +0800, Huacai Chen wrote:

> Hi, Enze,
>
> On Fri, Jul 21, 2023 at 11:14 AM Enze Li <lienze@kylinos.cn> wrote:
>>
>> On Wed, Jul 19 2023 at 11:27:50 PM +0800, Huacai Chen wrote:
>>
>> > Hi, Enze,
>> >
>> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
>> >>
>> >> The LoongArch architecture is quite different from other architectures.
>> >> When the allocating of KFENCE itself is done, it is mapped to the direct
>> >> mapping configuration window [1] by default on LoongArch.  It means that
>> >> it is not possible to use the page table mapped mode which required by
>> >> the KFENCE system and therefore it should be remapped to the appropriate
>> >> region.
>> >>
>> >> This patch adds architecture specific implementation details for KFENCE.
>> >> In particular, this implements the required interface in <asm/kfence.h>.
>> >>
>> >> Tested this patch by using the testcases and all passed.
>> >>
>> >> [1] https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
>> >>
>> >> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> >> ---
>> >>  arch/loongarch/Kconfig               |  1 +
>> >>  arch/loongarch/include/asm/kfence.h  | 62 ++++++++++++++++++++++++++++
>> >>  arch/loongarch/include/asm/pgtable.h |  6 +++
>> >>  arch/loongarch/mm/fault.c            | 22 ++++++----
>> >>  4 files changed, 83 insertions(+), 8 deletions(-)
>> >>  create mode 100644 arch/loongarch/include/asm/kfence.h
>> >>
>> >> diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
>> >> index 5411e3a4eb88..db27729003d3 100644
>> >> --- a/arch/loongarch/Kconfig
>> >> +++ b/arch/loongarch/Kconfig
>> >> @@ -93,6 +93,7 @@ config LOONGARCH
>> >>         select HAVE_ARCH_JUMP_LABEL
>> >>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
>> >>         select HAVE_ARCH_KASAN
>> >> +       select HAVE_ARCH_KFENCE if 64BIT
>> > "if 64BIT" can be dropped here.
>> >
>>
>> Fixed.
>>
>> >>         select HAVE_ARCH_MMAP_RND_BITS if MMU
>> >>         select HAVE_ARCH_SECCOMP_FILTER
>> >>         select HAVE_ARCH_TRACEHOOK
>> >> diff --git a/arch/loongarch/include/asm/kfence.h b/arch/loongarch/include/asm/kfence.h
>> >> new file mode 100644
>> >> index 000000000000..2a85acc2bc70
>> >> --- /dev/null
>> >> +++ b/arch/loongarch/include/asm/kfence.h
>> >> @@ -0,0 +1,62 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> +/*
>> >> + * KFENCE support for LoongArch.
>> >> + *
>> >> + * Author: Enze Li <lienze@kylinos.cn>
>> >> + * Copyright (C) 2022-2023 KylinSoft Corporation.
>> >> + */
>> >> +
>> >> +#ifndef _ASM_LOONGARCH_KFENCE_H
>> >> +#define _ASM_LOONGARCH_KFENCE_H
>> >> +
>> >> +#include <linux/kfence.h>
>> >> +#include <asm/pgtable.h>
>> >> +#include <asm/tlb.h>
>> >> +
>> >> +static inline char *arch_kfence_init_pool(void)
>> >> +{
>> >> +       char *__kfence_pool_orig = __kfence_pool;
>> > I prefer kfence_pool than __kfence_pool_orig here.
>> >
>>
>> Fixed.
>>
>> >> +       struct vm_struct *area;
>> >> +       int err;
>> >> +
>> >> +       area = __get_vm_area_caller(KFENCE_POOL_SIZE, VM_IOREMAP,
>> >> +                                   KFENCE_AREA_START, KFENCE_AREA_END,
>> >> +                                   __builtin_return_address(0));
>> >> +       if (!area)
>> >> +               return NULL;
>> >> +
>> >> +       __kfence_pool = (char *)area->addr;
>> >> +       err = ioremap_page_range((unsigned long)__kfence_pool,
>> >> +                                (unsigned long)__kfence_pool + KFENCE_POOL_SIZE,
>> >> +                                virt_to_phys((void *)__kfence_pool_orig),
>> >> +                                PAGE_KERNEL);
>> >> +       if (err) {
>> >> +               free_vm_area(area);
>> >> +               return NULL;
>> >> +       }
>> >> +
>> >> +       return __kfence_pool;
>> >> +}
>> >> +
>> >> +/* Protect the given page and flush TLB. */
>> >> +static inline bool kfence_protect_page(unsigned long addr, bool protect)
>> >> +{
>> >> +       pte_t *pte = virt_to_kpte(addr);
>> >> +
>> >> +       if (WARN_ON(!pte) || pte_none(*pte))
>> >> +               return false;
>> >> +
>> >> +       if (protect)
>> >> +               set_pte(pte, __pte(pte_val(*pte) & ~(_PAGE_VALID | _PAGE_PRESENT)));
>> >> +       else
>> >> +               set_pte(pte, __pte(pte_val(*pte) | (_PAGE_VALID | _PAGE_PRESENT)));
>> >> +
>> >> +       /* Flush this CPU's TLB. */
>> >> +       preempt_disable();
>> >> +       local_flush_tlb_one(addr);
>> >> +       preempt_enable();
>> >> +
>> >> +       return true;
>> >> +}
>> >> +
>> >> +#endif /* _ASM_LOONGARCH_KFENCE_H */
>> >> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
>> >> index 0fc074b8bd48..5a9c81298fe3 100644
>> >> --- a/arch/loongarch/include/asm/pgtable.h
>> >> +++ b/arch/loongarch/include/asm/pgtable.h
>> >> @@ -85,7 +85,13 @@ extern unsigned long zero_page_mask;
>> >>  #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
>> >>  #define MODULES_END    (MODULES_VADDR + SZ_256M)
>> >>
>> >> +#ifdef CONFIG_KFENCE
>> >> +#define KFENCE_AREA_START      MODULES_END
>> >> +#define KFENCE_AREA_END                (KFENCE_AREA_START + SZ_512M)
>> > Why you choose 512M here?
>> >
>>
>> One day I noticed that 512M can hold 16K (default 255) KFENCE objects,
>> which should be more than enough and I think this should be appropriate.
>>
>> As far as I see, KFENCE system does not have the upper limit of this
>> value(CONFIG_KFENCE_NUM_OBJECTS), which could theoretically be any
>> number.  There's another way, how about setting this value to be
>> determined by the configuration, like this,
>>
>> ========================================================================
>>  +#define KFENCE_AREA_END \
>>  + (KFENCE_AREA_START + (CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)
>> ========================================================================
> How does other archs configure the size?
>

They all use the same one with a macro named KFENCE_POOL_SIZE defined
like this during kernel startup,

#define KFENCE_POOL_SIZE ((CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)

For now, we do not need to consider the KASAN region, and get enough
address space after vmemmap, this will not be a problem.

>>
>> >> +#define VMALLOC_START          KFENCE_AREA_END
>> >> +#else
>> >>  #define VMALLOC_START  MODULES_END
>> >> +#endif
>> > I don't like to put KFENCE_AREA between module and vmalloc range (it
>> > may cause some problems), can we put it after vmemmap?
>>
>> I found that there is not enough space after vmemmap and that these
>> spaces are affected by KASAN. As follows,
>>
>> Without KASAN
>> ###### module 0xffff800002008000~0xffff800012008000
>> ###### malloc 0xffff800032008000~0xfffffefffe000000
>> ###### vmemmap 0xffffff0000000000~0xffffffffffffffff
>>
>> With KASAN
>> ###### module 0xffff800002008000~0xffff800012008000
>> ###### malloc 0xffff800032008000~0xffffbefffe000000
>> ###### vmemmap 0xffffbf0000000000~0xffffbfffffffffff
>>
>> What about put it before MODULES_START?
> I temporarily drop KASAN in linux-next for you. You can update a new
> patch version without KASAN (still, put KFENCE after vmemmap), and
> then we can improve further.
>
> Huacai

Thank you so much. :)

The v2 of the patchset is on the way.

Best Regards,
Enze

>>
>> ============================================================================
>> --- a/arch/loongarch/include/asm/pgtable.h
>> +++ b/arch/loongarch/include/asm/pgtable.h
>> @@ -82,7 +82,14 @@ extern unsigned long zero_page_mask;
>>   * Avoid the first couple of pages so NULL pointer dereferences will
>>   * still reliably trap.
>>   */
>> +#ifdef CONFIG_KFENCE
>> +#define KFENCE_AREA_START      (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
>> +#define KFENCE_AREA_END        \
>> +       (KFENCE_AREA_START + (CONFIG_KFENCE_NUM_OBJECTS + 1) * 2 * PAGE_SIZE)
>> +#define MODULES_VADDR  KFENCE_AREA_END
>> +#else
>>  #define MODULES_VADDR  (vm_map_base + PCI_IOSIZE + (2 * PAGE_SIZE))
>> +#endif
>>  #define MODULES_END    (MODULES_VADDR + SZ_256M)
>> ============================================================================
>>
>> Best Regards,
>> Enze
>>
>> >>
>> >>  #ifndef CONFIG_KASAN
>> >>  #define VMALLOC_END    \
>> >> diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
>> >> index da5b6d518cdb..c0319128b221 100644
>> >> --- a/arch/loongarch/mm/fault.c
>> >> +++ b/arch/loongarch/mm/fault.c
>> >> @@ -23,6 +23,7 @@
>> >>  #include <linux/kprobes.h>
>> >>  #include <linux/perf_event.h>
>> >>  #include <linux/uaccess.h>
>> >> +#include <linux/kfence.h>
>> >>
>> >>  #include <asm/branch.h>
>> >>  #include <asm/mmu_context.h>
>> >> @@ -30,7 +31,8 @@
>> >>
>> >>  int show_unhandled_signals = 1;
>> >>
>> >> -static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>> >> +static void __kprobes no_context(struct pt_regs *regs, unsigned long address,
>> >> +                                unsigned long write)
>> >>  {
>> >>         const int field = sizeof(unsigned long) * 2;
>> >>
>> >> @@ -38,6 +40,9 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>> >>         if (fixup_exception(regs))
>> >>                 return;
>> >>
>> >> +       if (kfence_handle_page_fault(address, write, regs))
>> >> +               return;
>> >> +
>> >>         /*
>> >>          * Oops. The kernel tried to access some bad page. We'll have to
>> >>          * terminate things with extreme prejudice.
>> >> @@ -51,14 +56,15 @@ static void __kprobes no_context(struct pt_regs *regs, unsigned long address)
>> >>         die("Oops", regs);
>> >>  }
>> >>
>> >> -static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address)
>> >> +static void __kprobes do_out_of_memory(struct pt_regs *regs, unsigned long address,
>> >> +                                      unsigned long write)
>> >>  {
>> >>         /*
>> >>          * We ran out of memory, call the OOM killer, and return the userspace
>> >>          * (which will retry the fault, or kill us if we got oom-killed).
>> >>          */
>> >>         if (!user_mode(regs)) {
>> >> -               no_context(regs, address);
>> >> +               no_context(regs, address, write);
>> >>                 return;
>> >>         }
>> >>         pagefault_out_of_memory();
>> >> @@ -69,7 +75,7 @@ static void __kprobes do_sigbus(struct pt_regs *regs,
>> >>  {
>> >>         /* Kernel mode? Handle exceptions or die */
>> >>         if (!user_mode(regs)) {
>> >> -               no_context(regs, address);
>> >> +               no_context(regs, address, write);
>> >>                 return;
>> >>         }
>> >>
>> >> @@ -90,7 +96,7 @@ static void __kprobes do_sigsegv(struct pt_regs *regs,
>> >>
>> >>         /* Kernel mode? Handle exceptions or die */
>> >>         if (!user_mode(regs)) {
>> >> -               no_context(regs, address);
>> >> +               no_context(regs, address, write);
>> >>                 return;
>> >>         }
>> >>
>> >> @@ -149,7 +155,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>> >>          */
>> >>         if (address & __UA_LIMIT) {
>> >>                 if (!user_mode(regs))
>> >> -                       no_context(regs, address);
>> >> +                       no_context(regs, address, write);
>> >>                 else
>> >>                         do_sigsegv(regs, write, address, si_code);
>> >>                 return;
>> >> @@ -211,7 +217,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>> >>
>> >>         if (fault_signal_pending(fault, regs)) {
>> >>                 if (!user_mode(regs))
>> >> -                       no_context(regs, address);
>> >> +                       no_context(regs, address, write);
>> >>                 return;
>> >>         }
>> >>
>> >> @@ -232,7 +238,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>> >>         if (unlikely(fault & VM_FAULT_ERROR)) {
>> >>                 mmap_read_unlock(mm);
>> >>                 if (fault & VM_FAULT_OOM) {
>> >> -                       do_out_of_memory(regs, address);
>> >> +                       do_out_of_memory(regs, address, write);
>> >>                         return;
>> >>                 } else if (fault & VM_FAULT_SIGSEGV) {
>> >>                         do_sigsegv(regs, write, address, si_code);
>> >> --
>> >> 2.34.1
>> >>
>> >>
>>

Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Huacai Chen]

On Sun, Jul 23, 2023 at 3:17 PM Enze Li <lienze@kylinos.cn> wrote:
>
> On Fri, Jul 21 2023 at 10:21:38 AM +0800, Huacai Chen wrote:
>
> > On Fri, Jul 21, 2023 at 10:12 AM Enze Li <lienze@kylinos.cn> wrote:
> >>
> >> On Wed, Jul 19 2023 at 11:29:37 PM +0800, Huacai Chen wrote:
> >>
> >> > Hi, Enze,
> >> >
> >> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:
> >> >>
> >> >> According to LoongArch documentation online, there are two types of address
> >> >> translation modes: direct mapped address translation mode (direct mapped mode)
> >> >> and page table mapped address translation mode (page table mapped mode).
> >> >>
> >> >> Currently, the upstream code only supports DMM (Direct Mapped Mode).
> >> >> This patch adds a function that determines whether PTMM (Page Table
> >> >> Mapped Mode) should be used, and also adds the corresponding handler
> >> >> funcitons for both modes.
> >> >>
> >> >> For more details on the two modes, see [1].
> >> >>
> >> >> [1]
> >> >> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode
> >> >>
> >> >> Signed-off-by: Enze Li <lienze@kylinos.cn>
> >> >> ---
> >> >>  arch/loongarch/include/asm/page.h    | 10 ++++++++++
> >> >>  arch/loongarch/include/asm/pgtable.h |  6 ++++++
> >> >>  arch/loongarch/mm/pgtable.c          | 25 +++++++++++++++++++++++++
> >> >>  3 files changed, 41 insertions(+)
> >> >>
> >> >> diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h
> >> >> index 26e8dccb6619..05919be15801 100644
> >> >> --- a/arch/loongarch/include/asm/page.h
> >> >> +++ b/arch/loongarch/include/asm/page.h
> >> >> @@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> >> >>  #define sym_to_pfn(x)          __phys_to_pfn(__pa_symbol(x))
> >> >>
> >> >>  #define virt_to_pfn(kaddr)     PFN_DOWN(PHYSADDR(kaddr))
> >> >> +
> >> >> +#ifdef CONFIG_64BIT
> >> >> +#define virt_to_page(kaddr)                                            \
> >> >> +({                                                                     \
> >> >> +       is_PTMM_addr((unsigned long)kaddr) ?                            \
> >> >> +       PTMM_virt_to_page((unsigned long)kaddr) :                       \
> >> >> +       DMM_virt_to_page((unsigned long)kaddr);                         \
> >> >> +})
> >> > 1, Rename these helpers to
> >> > is_dmw_addr()/dmw_virt_to_page()/tlb_virt_to_page() will be better.
> >> > 2, These helpers are so simple so can be defined as inline function or
> >> > macros in page.h.
> >>
> >> Hi Huacai,
> >>
> >> Except for tlb_virt_to_page(), the remaining two modifications are easy.
> >>
> >> I've run into a lot of problems when trying to make tlb_virt_to_page()
> >> as a macro or inline function.  That's because we need to export this
> >> symbol in order for it to be used by the module that called the
> >> virt_to_page() function, other wise, we got the following errors,
> >>
> >> -----------------------------------------------------------------------
> >>   MODPOST Module.symvers
> >> ERROR: modpost: "tlb_virt_to_page" [fs/hfsplus/hfsplus.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [fs/smb/client/cifs.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [crypto/gcm.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [crypto/ccm.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [crypto/essiv.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [lib/crypto/libchacha20poly1305.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/ttm/ttm.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/iscsi_tcp.ko] undefined!
> >> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/qla2xxx/qla2xxx.ko] undefined!
> >> WARNING: modpost: suppressed 44 unresolved symbol warnings because there were too many)
> >> -----------------------------------------------------------------------
> >>
> >> It seems to me that wrapping it into a common function might be the only
> >> way to successfully compile or link with this modification.
> >>
> >> =========================================================
> >> --- a/arch/loongarch/include/asm/pgtable.h
> >> +++ b/arch/loongarch/include/asm/pgtable.h
> >> @@ -360,6 +360,8 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
> >>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
> >>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
> >>
> >> +inline struct page *tlb_virt_to_page(unsigned long kaddr);
> >> +
> >>
> >> --- a/arch/loongarch/mm/pgtable.c
> >> +++ b/arch/loongarch/mm/pgtable.c
> >> @@ -9,6 +9,12 @@
> >>  #include <asm/pgtable.h>
> >>  #include <asm/tlbflush.h>
> >>
> >> +inline struct page *tlb_virt_to_page(unsigned long kaddr)
> >> +{
> >> +       return pte_page(*virt_to_kpte(kaddr));
> >> +}
> >> +EXPORT_SYMBOL_GPL(tlb_virt_to_page);
> >> =========================================================
> >>
> >> WDYT?
> >>
> >> Best Regards,
> >> Enze
> > If you define "static inline" functions in page.h, there will be no problems.
> >
>
> Hi Huacai,
>
> After failed over and over and over again, I think I've found the reason
> why we can't define tlb_virt_to_page as macro or inline function in
> asm/page.h or asm/pgtable.h. :)
>
> I'll go through this step by step.
>
> If I put tlb_virt_to_page in asm/page.h as following,
>
> =====================================================
> +static inline struct page *tlb_virt_to_page(unsigned long kaddr)
> +{
> +       return pte_page(*virt_to_kpte(kaddr));
> +}
> =====================================================
>
> and compile kernel, gcc says to me the following error.
>
> --------------------------------------------------------------------
>   CC      arch/loongarch/kernel/asm-offsets.s
> In file included from ./include/linux/shm.h:6,
>                  from ./include/linux/sched.h:16,
>                  from arch/loongarch/kernel/asm-offsets.c:8:
> ./arch/loongarch/include/asm/page.h: In function ‘tlb_virt_to_page’:
> ./arch/loongarch/include/asm/page.h:126:16: error: implicit declaration of function ‘pte_page’ [-Werror=implicit-function-declaration]
>   126 |         return pte_page(*virt_to_kpte(kaddr));
>       |                ^~~~~~~~
> ---------------------------------------------------------------------
>
> "pte_page" is declared in asm/pgtable.h, so I put "#include
> <asm/pgtable.h>" ahead, like this,
>
> =====================================================
> +#include <asm/pgtable.h>
> +static inline struct page *tlb_virt_to_page(unsigned long kaddr)
> +{
> +       return pte_page(*virt_to_kpte(kaddr));
> +}
> =====================================================
>
> then compile again, gcc says,
>
> ---------------------------------------------------------------------
>   CC      arch/loongarch/kernel/asm-offsets.s
> In file included from ./arch/loongarch/include/asm/page.h:98,
>                  from ./include/linux/shm.h:6,
>                  from ./include/linux/sched.h:16,
>                  from arch/loongarch/kernel/asm-offsets.c:8:
> ./arch/loongarch/include/asm/page.h: In function ‘tlb_virt_to_page’:
> ./arch/loongarch/include/asm/page.h:127:26: error: implicit declaration of function ‘virt_to_kpte’; did you mean ‘virt_to_pfn’? [-Werror=implicit-function-declaration]
>   127 |         return pte_page(*virt_to_kpte(kaddr));
>       |                          ^~~~~~~~~~~~
> ---------------------------------------------------------------------
>
> "virt_to_kpte" is defined in linux/pgtable.h, consequently I add "#include
> <linux/pgtable.h>" as well,
>
> =====================================================
> +#include <asm/pgtable.h>
> +#include <linux/pgtable.h>
> +static inline struct page *tlb_virt_to_page(unsigned long kaddr)
> +{
> +       return pte_page(*virt_to_kpte(kaddr));
> +}
> =====================================================
>
> and continue,
>
> ---------------------------------------------------------------------
>   CC      arch/loongarch/kernel/asm-offsets.s
>   CALL    scripts/checksyscalls.sh
>   CC      arch/loongarch/vdso/vgetcpu.o
>   CC      arch/loongarch/vdso/vgettimeofday.o
> In file included from ./arch/loongarch/include/asm/page.h:124,
>                  from ./include/linux/mm_types_task.h:16,
>                  from ./include/linux/mm_types.h:5,
>                  from ./include/linux/mmzone.h:22,
>                  from ./include/linux/gfp.h:7,
>                  from ./include/linux/mm.h:7,
>                  from ./arch/loongarch/include/asm/vdso.h:10,
>                  from arch/loongarch/vdso/vgetcpu.c:6:
> ./arch/loongarch/include/asm/pgtable.h: In function ‘pte_accessible’:
> ./arch/loongarch/include/asm/pgtable.h:436:40: error: invalid use of undefined type ‘struct mm_struct’
>   436 |                         atomic_read(&mm->tlb_flush_pending))
>       |                                        ^~
> ---------------------------------------------------------------------
>
> The first line above shows that it compiled successfully for the
> asm-offsets module.  That's fair enough.  Actually, the point is the
> next one (invalid use of undefined type 'struct mm_struct').
>
> As we all know, before the compiler compiles, it expands the header
> files first.  For this example, it firstly expands from the header file
> vdso.h, then the mm.h file and so on.  We can see that the line 436 of
> asm/pgtable.h are using 'struct mm_struct'.  When we backtrack to a file
> that has been previously expanded, it's obvious that the definition of
> mm_struct does not appear in the expanded file.  Instead, it appears
> afterward (mm_types.h).
>
> To be clear, I'll exemplify this case with a cheap ASCII diagram.
>
>                                                                  ... <-|
>                     we're using 'mm_struct' here >>>   asm/pgtable.h <-|
>                                                                  ... <-|
>                                                                        |
>                                                                |->...  |
>                                                                |->asm/page.h
>                                                                |->...
>                                                        |->...  |
>                                          |->...        |->mm_types_task.h
>                              |->...      |->mm_types.h-|->...
>                     |->...   |->mmzone.h-|->... |
>             |->...  |->gfp.h-|->...             |
>   |->...    |->mm.h-|->...            But 'mm_struct' is defined here.
>   |->vdso.h-|->...
>   |->...
> vgetcpu.c
>
> I've also tried to include mm_types.h in advance, but in this case that
> doesn't work because the _LINUX_MM_TYPES_H macro already exists.
> The "forward declaration" was also taken into account, in the end it was
> found to be unavailable as well.
>
> In summary, I'm afraid that rewriting tlb_virt_to_page in asm/page.h as
> a macro or inline function is not possible.  The root case of this is
> that both 'struct mm_struct' and 'virt_to_kpte' belong to high-level
> data structures, and if they are referenced in asm/page.h at the
> low-level, dependency problems arise.
>
> Anyway, we can at least define it as a normal function in asm/pgtable.h,
> is that Okay with you?
>
> It may be a bit wordy, so please bear with me.  In addition, all of the
> above is my understanding, am I missing something?
Well, you can define the helpers in .c files at present, but I have
another question.

Though other archs (e.g., RISC-V) have no DMW addresses, they still
have linear area. In other words, both LoongArch and RISC-V have
linear area and vmalloc-like areas. The only difference is LoongArch's
linear area is DMW-mapped but RISC-V's linear area is TLB-mapped.

For linear area, the translation is pfn_to_page(virt_to_pfn(kaddr)),
no matter LoongArch or RISC-V;
For vmalloc-like areas, the translation is
pte_page(*virt_to_kpte(kaddr)), no matter LoongArch or RISC-V.

My question is: why RISC-V only care about the linear area for
virt_to_page(), but you are caring about the vmalloc-like areas?

Huacai
>
> Best Regards,
> Enze
>
> >>
> >> > 3, CONFIG_64BIT can be removed here.
> >> >
> >> > Huacai
> >> >
> >> >> +#else
> >> >>  #define virt_to_page(kaddr)    pfn_to_page(virt_to_pfn(kaddr))
> >> >> +#endif
> >> >>
> >> >>  extern int __virt_addr_valid(volatile void *kaddr);
> >> >>  #define virt_addr_valid(kaddr) __virt_addr_valid((volatile void *)(kaddr))
> >> >> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h
> >> >> index ed6a37bb55b5..0fc074b8bd48 100644
> >> >> --- a/arch/loongarch/include/asm/pgtable.h
> >> >> +++ b/arch/loongarch/include/asm/pgtable.h
> >> >> @@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
> >> >>  #define PMD_T_LOG2     (__builtin_ffs(sizeof(pmd_t)) - 1)
> >> >>  #define PTE_T_LOG2     (__builtin_ffs(sizeof(pte_t)) - 1)
> >> >>
> >> >> +#ifdef CONFIG_64BIT
> >> >> +struct page *DMM_virt_to_page(unsigned long kaddr);
> >> >> +struct page *PTMM_virt_to_page(unsigned long kaddr);
> >> >> +bool is_PTMM_addr(unsigned long kaddr);
> >> >> +#endif
> >> >> +
> >> >>  extern pgd_t swapper_pg_dir[];
> >> >>  extern pgd_t invalid_pg_dir[];
> >> >>
> >> >> diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c
> >> >> index 36a6dc0148ae..4c6448f996b6 100644
> >> >> --- a/arch/loongarch/mm/pgtable.c
> >> >> +++ b/arch/loongarch/mm/pgtable.c
> >> >> @@ -9,6 +9,31 @@
> >> >>  #include <asm/pgtable.h>
> >> >>  #include <asm/tlbflush.h>
> >> >>
> >> >> +#ifdef CONFIG_64BIT
> >> >> +/* DMM stands for Direct Mapped Mode. */
> >> >> +struct page *DMM_virt_to_page(unsigned long kaddr)
> >> >> +{
> >> >> +       return pfn_to_page(virt_to_pfn(kaddr));
> >> >> +}
> >> >> +EXPORT_SYMBOL_GPL(DMM_virt_to_page);
> >> >> +
> >> >> +/* PTMM stands for Page Table Mapped Mode. */
> >> >> +struct page *PTMM_virt_to_page(unsigned long kaddr)
> >> >> +{
> >> >> +       return pte_page(*virt_to_kpte(kaddr));
> >> >> +}
> >> >> +EXPORT_SYMBOL_GPL(PTMM_virt_to_page);
> >> >> +
> >> >> +bool is_PTMM_addr(unsigned long kaddr)
> >> >> +{
> >> >> +       if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) ==
> >> >> +                    GENMASK(BITS_PER_LONG - 1, cpu_vabits)))
> >> >> +               return true;
> >> >> +       return false;
> >> >> +}
> >> >> +EXPORT_SYMBOL_GPL(is_PTMM_addr);
> >> >> +#endif
> >> >> +
> >> >>  pgd_t *pgd_alloc(struct mm_struct *mm)
> >> >>  {
> >> >>         pgd_t *ret, *init;
> >> >> --
> >> >> 2.34.1
> >> >>
> >> >>
> >>

Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Enze Li]

On Tue, Jul 25 2023 at 10:06:01 AM +0800, Huacai Chen wrote:

> On Sun, Jul 23, 2023 at 3:17 PM Enze Li <lienze@kylinos.cn> wrote:
>>
>> On Fri, Jul 21 2023 at 10:21:38 AM +0800, Huacai Chen wrote:
>>
>> > On Fri, Jul 21, 2023 at 10:12 AM Enze Li <lienze@kylinos.cn> wrote:
>> >>
>> >> On Wed, Jul 19 2023 at 11:29:37 PM +0800, Huacai Chen wrote:
>> >>
>> >> > Hi, Enze,
>> >> >
>> >> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@kylinos.cn> wrote:

<snip>

>> I've also tried to include mm_types.h in advance, but in this case that
>> doesn't work because the _LINUX_MM_TYPES_H macro already exists.
>> The "forward declaration" was also taken into account, in the end it was
>> found to be unavailable as well.
>>
>> In summary, I'm afraid that rewriting tlb_virt_to_page in asm/page.h as
>> a macro or inline function is not possible.  The root case of this is
>> that both 'struct mm_struct' and 'virt_to_kpte' belong to high-level
>> data structures, and if they are referenced in asm/page.h at the
>> low-level, dependency problems arise.
>>
>> Anyway, we can at least define it as a normal function in asm/pgtable.h,
>> is that Okay with you?
>>
>> It may be a bit wordy, so please bear with me.  In addition, all of the
>> above is my understanding, am I missing something?
> Well, you can define the helpers in .c files at present, but I have
> another question.
>
> Though other archs (e.g., RISC-V) have no DMW addresses, they still
> have linear area. In other words, both LoongArch and RISC-V have
> linear area and vmalloc-like areas. The only difference is LoongArch's
> linear area is DMW-mapped but RISC-V's linear area is TLB-mapped.
>
> For linear area, the translation is pfn_to_page(virt_to_pfn(kaddr)),
> no matter LoongArch or RISC-V;
> For vmalloc-like areas, the translation is
> pte_page(*virt_to_kpte(kaddr)), no matter LoongArch or RISC-V.
>

Hi Huacai,

Thanks for your reply.

> My question is: why RISC-V only care about the linear area for
> virt_to_page(), but you are caring about the vmalloc-like areas?

This patch is a preparation to make LoongArch support KFENCE.

One of the core principles of KFENCE is that pages are tagged in the PTE
and then synchronized to the TLB.  When the MMU detects an improper
access, it can generate an interrupt signal, which is subsequently
handled by a handler function (kfence_handle_page_fault) provided by
KFENCE.  In short, KFENCE requires the support of the TLB.

There's no need to take this into account on RISC-V because TLB mapping
is already supported in linear area.

Best Regards,
Enze

>
> Huacai

<snip>