Re: [PATCH 3/3] powerpc/module_64: Use special stub for _mcount() with -mprofile-kernel

2020-04-23 Thread Qian Cai



> On Apr 21, 2020, at 1:35 PM, Naveen N. Rao  
> wrote:
> 
> Since commit c55d7b5e64265f ("powerpc: Remove STRICT_KERNEL_RWX
> incompatibility with RELOCATABLE"), powerpc kernels with
> -mprofile-kernel can crash in certain scenarios with a trace like below:
> 
>BUG: Unable to handle kernel instruction fetch (NULL pointer?)
>Faulting instruction address: 0x
>Oops: Kernel access of bad area, sig: 11 [#1]
>LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV
>
>NIP [] 0x0
>LR [c008102c0048] ext4_iomap_end+0x8/0x30 [ext4]
>Call Trace:
> iomap_apply+0x20c/0x920 (unreliable)
> iomap_bmap+0xfc/0x160
> ext4_bmap+0xa4/0x180 [ext4]
> bmap+0x4c/0x80
> jbd2_journal_init_inode+0x44/0x1a0 [jbd2]
> ext4_load_journal+0x440/0x860 [ext4]
> ext4_fill_super+0x342c/0x3ab0 [ext4]
> mount_bdev+0x25c/0x290
> ext4_mount+0x28/0x50 [ext4]
> legacy_get_tree+0x4c/0xb0
> vfs_get_tree+0x4c/0x130
> do_mount+0xa18/0xc50
> sys_mount+0x158/0x180
> system_call+0x5c/0x68
> 
> The NIP points to NULL, or a random location (data even), while the LR
> always points to the LEP of a function (with an offset of 8), indicating
> that something went wrong with ftrace. However, ftrace is not
> necessarily active when such crashes occur.
> 
> The kernel OOPS sometimes follows a warning from ftrace indicating that
> some module functions could not be patched with a nop. Other times, if a
> module is loaded early during boot, instruction patching can fail due to
> a separate bug, but the error is not reported due to missing error
> reporting.
> 
> In all the above cases when instruction patching fails, ftrace will be
> disabled but certain kernel module functions will be left with default
> calls to _mcount(). This is not a problem with ELFv1. However, with
> -mprofile-kernel, the default stub is problematic since it depends on a
> valid module TOC in r2. If the kernel (or a different module) calls into
> a function that does not use the TOC, the function won't have a prologue
> to setup the module TOC. When that function calls into _mcount(), we
> will end up in the relocation stub that will use the previous TOC, and
> end up trying to jump into a random location. From the above trace:
> 
>   iomap_apply+0x20c/0x920 [kernel TOC]
>   |
>   V
>   ext4_iomap_end+0x8/0x30 [no GEP == kernel TOC]
>   |
>   V
>   _mcount() stub
>   [uses kernel TOC -> random entry]
> 
> To address this, let's change over to using the special stub that is
> used for ftrace_[regs_]caller() for _mcount(). This ensures that we are
> not dependent on a valid module TOC in r2 for default _mcount()
> handling.
> 
> Reported-by: Qian Cai 
> Signed-off-by: Naveen N. Rao 

Feel free to add,

Tested-by: Qian Cai 

[PATCH 3/3] powerpc/module_64: Use special stub for _mcount() with -mprofile-kernel

2020-04-21 Thread Naveen N. Rao
Since commit c55d7b5e64265f ("powerpc: Remove STRICT_KERNEL_RWX
incompatibility with RELOCATABLE"), powerpc kernels with
-mprofile-kernel can crash in certain scenarios with a trace like below:

BUG: Unable to handle kernel instruction fetch (NULL pointer?)
Faulting instruction address: 0x
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV

NIP [] 0x0
LR [c008102c0048] ext4_iomap_end+0x8/0x30 [ext4]
Call Trace:
 iomap_apply+0x20c/0x920 (unreliable)
 iomap_bmap+0xfc/0x160
 ext4_bmap+0xa4/0x180 [ext4]
 bmap+0x4c/0x80
 jbd2_journal_init_inode+0x44/0x1a0 [jbd2]
 ext4_load_journal+0x440/0x860 [ext4]
 ext4_fill_super+0x342c/0x3ab0 [ext4]
 mount_bdev+0x25c/0x290
 ext4_mount+0x28/0x50 [ext4]
 legacy_get_tree+0x4c/0xb0
 vfs_get_tree+0x4c/0x130
 do_mount+0xa18/0xc50
 sys_mount+0x158/0x180
 system_call+0x5c/0x68

The NIP points to NULL, or a random location (data even), while the LR
always points to the LEP of a function (with an offset of 8), indicating
that something went wrong with ftrace. However, ftrace is not
necessarily active when such crashes occur.

The kernel OOPS sometimes follows a warning from ftrace indicating that
some module functions could not be patched with a nop. Other times, if a
module is loaded early during boot, instruction patching can fail due to
a separate bug, but the error is not reported due to missing error
reporting.

In all the above cases when instruction patching fails, ftrace will be
disabled but certain kernel module functions will be left with default
calls to _mcount(). This is not a problem with ELFv1. However, with
-mprofile-kernel, the default stub is problematic since it depends on a
valid module TOC in r2. If the kernel (or a different module) calls into
a function that does not use the TOC, the function won't have a prologue
to setup the module TOC. When that function calls into _mcount(), we
will end up in the relocation stub that will use the previous TOC, and
end up trying to jump into a random location. From the above trace:

iomap_apply+0x20c/0x920 [kernel TOC]
|
V
ext4_iomap_end+0x8/0x30 [no GEP == kernel TOC]
|
V
_mcount() stub
[uses kernel TOC -> random entry]

To address this, let's change over to using the special stub that is
used for ftrace_[regs_]caller() for _mcount(). This ensures that we are
not dependent on a valid module TOC in r2 for default _mcount()
handling.

Reported-by: Qian Cai 
Signed-off-by: Naveen N. Rao 
---
 arch/powerpc/kernel/module_64.c | 222 +++-
 1 file changed, 104 insertions(+), 118 deletions(-)

diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 7f4cc5346387..cc7e990e8376 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -348,6 +348,92 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
return 0;
 }
 
+#ifdef CONFIG_MPROFILE_KERNEL
+
+#define PACATOC offsetof(struct paca_struct, kernel_toc)
+
+/*
+ * ld  r12,PACATOC(r13)
+ * addis   r12,r12,
+ * addir12,r12,
+ * mtctr   r12
+ * bctr
+ */
+static u32 stub_insns[] = {
+   PPC_INST_LD | __PPC_RT(R12) | __PPC_RA(R13) | PACATOC,
+   PPC_INST_ADDIS | __PPC_RT(R12) | __PPC_RA(R12),
+   PPC_INST_ADDI | __PPC_RT(R12) | __PPC_RA(R12),
+   PPC_INST_MTCTR | __PPC_RS(R12),
+   PPC_INST_BCTR,
+};
+
+/*
+ * For mprofile-kernel we use a special stub for ftrace_caller() because we
+ * can't rely on r2 containing this module's TOC when we enter the stub.
+ *
+ * That can happen if the function calling us didn't need to use the toc. In
+ * that case it won't have setup r2, and the r2 value will be either the
+ * kernel's toc, or possibly another modules toc.
+ *
+ * To deal with that this stub uses the kernel toc, which is always accessible
+ * via the paca (in r13). The target (ftrace_caller()) is responsible for
+ * saving and restoring the toc before returning.
+ */
+static inline int create_ftrace_stub(struct ppc64_stub_entry *entry,
+   unsigned long addr,
+   struct module *me)
+{
+   long reladdr;
+
+   memcpy(entry->jump, stub_insns, sizeof(stub_insns));
+
+   /* Stub uses address relative to kernel toc (from the paca) */
+   reladdr = addr - kernel_toc_addr();
+   if (reladdr > 0x7FFF || reladdr < -(0x8000L)) {
+   pr_err("%s: Address of %ps out of range of kernel_toc.\n",
+   me->name, (void *)addr);
+   return 0;
+   }
+
+   entry->jump[1] |= PPC_HA(reladdr);
+   entry->jump[2] |= PPC_LO(reladdr);
+
+   /* Eventhough we don't use funcdata in the stub, it's