LinuxLists.cc - [PATCH V2 0/3] riscv: Enable LOCKDEP

[permalink] [raw]

Subject: [PATCH V2 2/3] riscv: Fixup lockdep_assert_held with wrong param cpu_running

From: Zong Li <[email protected]>

The cpu_running is not a lock-class, it lacks the dep_map member in
completion. It causes the error as follow:

arch/riscv/kernel/smpboot.c: In function '__cpu_up':
./include/linux/lockdep.h:364:52: error: 'struct completion' has no member named 'dep_map'
364 | #define lockdep_is_held(lock) lock_is_held(&(lock)->dep_map)
| ^~
./include/asm-generic/bug.h:113:25: note: in definition of macro 'WARN_ON'
113 | int __ret_warn_on = !!(condition); \
| ^~~~~~~~~
./include/linux/lockdep.h:390:27: note: in expansion of macro 'lockdep_is_held'
390 | WARN_ON(debug_locks && !lockdep_is_held(l)); \
| ^~~~~~~~~~~~~~~
arch/riscv/kernel/smpboot.c:118:2: note: in expansion of macro 'lockdep_assert_held'
118 | lockdep_assert_held(&cpu_running);

There are a lot of archs which use cpu_running in smpboot.c (arm,
arm64, openrisc, xtensa, s390, x86, mips), but none of them try
lockdep_assert_held(&cpu_running.wait.lock). So Just remove it.

Signed-off-by: Zong Li <[email protected]>
Signed-off-by: Guo Ren <[email protected]>
---
arch/riscv/kernel/smpboot.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 4e99227..defc4e1 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -121,7 +121,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)

ret = start_secondary_cpu(cpu, tidle);
if (!ret) {
- lockdep_assert_held(&cpu_running);
wait_for_completion_timeout(&cpu_running,
msecs_to_jiffies(1000));

--
2.7.4

2020-06-27 13:59:48

[permalink] [raw]

Subject: [PATCH V2 3/3] riscv: Enable LOCKDEP_SUPPORT & fixup TRACE_IRQFLAGS_SUPPORT

From: Guo Ren <[email protected]>

Lockdep is needed by proving the spinlocks and rwlocks. To suupport
it, we need fixup TRACE_IRQFLAGS_SUPPORT in kernel/entry.S. This
patch follow Documentation/irqflags-tracing.txt.

Signed-off-by: Guo Ren <[email protected]>
Cc: Greentime Hu <[email protected]>
---
arch/riscv/Kconfig | 3 +++
arch/riscv/kernel/entry.S | 34 +++++++++++++++++++++++++++++++++-
2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 58d6f66..d5d5100 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -179,6 +179,9 @@ config PGTABLE_LEVELS
default 3 if 64BIT
default 2

+config LOCKDEP_SUPPORT
+ def_bool y
+
source "arch/riscv/Kconfig.socs"

menu "Platform type"
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index cae7e6d..45c81e4 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -97,19 +97,26 @@ _save_context:
la gp, __global_pointer$
.option pop

- la ra, ret_from_exception
+#ifdef CONFIG_TRACE_IRQFLAGS
+ call trace_hardirqs_off
+#endif
/*
* MSB of cause differentiates between
* interrupts and exceptions
*/
bge s4, zero, 1f

+ la ra, ret_from_exception
+
/* Handle interrupts */
move a0, sp /* pt_regs */
la a1, handle_arch_irq
REG_L a1, (a1)
jr a1
1:
+#ifdef CONFIG_TRACE_IRQFLAGS
+ call trace_hardirqs_on
+#endif
/*
* Exceptions run with interrupts enabled or disabled depending on the
* state of SR_PIE in m/sstatus.
@@ -119,6 +126,7 @@ _save_context:
csrs CSR_STATUS, SR_IE

1:
+ la ra, ret_from_exception
/* Handle syscalls */
li t0, EXC_SYSCALL
beq s4, t0, handle_syscall
@@ -137,6 +145,17 @@ _save_context:
tail do_trap_unknown

handle_syscall:
+#ifdef CONFIG_TRACE_IRQFLAGS
+ /* Recover a0 - a7 for system calls */
+ REG_L a0, PT_A0(sp)
+ REG_L a1, PT_A1(sp)
+ REG_L a2, PT_A2(sp)
+ REG_L a3, PT_A3(sp)
+ REG_L a4, PT_A4(sp)
+ REG_L a5, PT_A5(sp)
+ REG_L a6, PT_A6(sp)
+ REG_L a7, PT_A7(sp)
+#endif
/* save the initial A0 value (needed in signal handlers) */
REG_S a0, PT_ORIG_A0(sp)
/*
@@ -190,6 +209,9 @@ ret_from_syscall_rejected:
ret_from_exception:
REG_L s0, PT_STATUS(sp)
csrc CSR_STATUS, SR_IE
+#ifdef CONFIG_TRACE_IRQFLAGS
+ call trace_hardirqs_off
+#endif
#ifdef CONFIG_RISCV_M_MODE
/* the MPP value is too large to be used as an immediate arg for addi */
li t0, SR_MPP
@@ -216,6 +238,16 @@ resume_userspace:
csrw CSR_SCRATCH, tp

restore_all:
+#ifdef CONFIG_TRACE_IRQFLAGS
+ REG_L s1, PT_STATUS(sp)
+ andi t0, s1, SR_PIE
+ beqz t0, 1f
+ call trace_hardirqs_on
+ j 2f
+1:
+ call trace_hardirqs_off
+2:
+#endif
REG_L a0, PT_STATUS(sp)
/*
* The current load reservation is effectively part of the processor's
--
2.7.4

2020-06-27 14:01:10

[permalink] [raw]

Subject: [PATCH V2 1/3] riscv: Fixup static_obj() fail

From: Guo Ren <[email protected]>

When enable LOCKDEP, static_obj() will cause error. Because some
__initdata static variables is before _stext:

static int static_obj(const void *obj)
{
unsigned long start = (unsigned long) &_stext,
end = (unsigned long) &_end,
addr = (unsigned long) obj;

/*
* static variable?
*/
if ((addr >= start) && (addr < end))
return 1;

[ 0.067192] INFO: trying to register non-static key.
[ 0.067325] the code is fine but needs lockdep annotation.
[ 0.067449] turning off the locking correctness validator.
[ 0.067718] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc7-dirty #44
[ 0.067945] Call Trace:
[ 0.068369] [<ffffffe00020323c>] walk_stackframe+0x0/0xa4
[ 0.068506] [<ffffffe000203422>] show_stack+0x2a/0x34
[ 0.068631] [<ffffffe000521e4e>] dump_stack+0x94/0xca
[ 0.068757] [<ffffffe000255a4e>] register_lock_class+0x5b8/0x5bc
[ 0.068969] [<ffffffe000255abe>] __lock_acquire+0x6c/0x1d5c
[ 0.069101] [<ffffffe0002550fe>] lock_acquire+0xae/0x312
[ 0.069228] [<ffffffe000989a8e>] _raw_spin_lock_irqsave+0x40/0x5a
[ 0.069357] [<ffffffe000247c64>] complete+0x1e/0x50
[ 0.069479] [<ffffffe000984c38>] rest_init+0x1b0/0x28a
[ 0.069660] [<ffffffe0000016a2>] 0xffffffe0000016a2
[ 0.069779] [<ffffffe000001b84>] 0xffffffe000001b84
[ 0.069953] [<ffffffe000001092>] 0xffffffe000001092

static __initdata DECLARE_COMPLETION(kthreadd_done);

noinline void __ref rest_init(void)
{
...
complete(&kthreadd_done);

Signed-off-by: Guo Ren <[email protected]>
---
arch/riscv/kernel/vmlinux.lds.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
index e6f8016..f3586e3 100644
--- a/arch/riscv/kernel/vmlinux.lds.S
+++ b/arch/riscv/kernel/vmlinux.lds.S
@@ -22,6 +22,7 @@ SECTIONS
/* Beginning of code and text segment */
. = LOAD_OFFSET;
_start = .;
+ _stext = .;
HEAD_TEXT_SECTION
. = ALIGN(PAGE_SIZE);

@@ -54,7 +55,6 @@ SECTIONS
. = ALIGN(SECTION_ALIGN);
.text : {
_text = .;
- _stext = .;
TEXT_TEXT
SCHED_TEXT
CPUIDLE_TEXT
--
2.7.4

2020-07-09 22:08:20

[permalink] [raw]

Subject: Re: [PATCH V2 0/3] riscv: Enable LOCKDEP

On Sat, 27 Jun 2020 06:57:05 PDT (-0700), [email protected] wrote:
> From: Guo Ren <[email protected]>
>
> Lockdep is needed by proving the spinlocks and rwlocks. To support it,
> we need to add TRACE_IRQFLAGS codes in kernel/entry.S. These patches
> follow Documentation/irqflags-tracing.txt.
>
> Fixup 2 bugs that block the lockdep implementation.
>
> ---
> Changes in v2
> - Remove sX regs recovery codes which are unnecessary, because
> callee will handle them. Thx Greentime :)
>
> - Move "restore a0 - a7" to handle_syscall, but if _TIF_SYSCALL_WORK
> is set, "restore a1 - a7" is still duplicated. I prefer a C wrapper
> for syscall.
>
> Guo Ren (2):
> riscv: Fixup static_obj() fail
> riscv: Enable LOCKDEP_SUPPORT & fixup TRACE_IRQFLAGS_SUPPORT
>
> Zong Li (1):
> riscv: Fixup lockdep_assert_held with wrong param cpu_running
>
> arch/riscv/Kconfig | 3 +++
> arch/riscv/kernel/entry.S | 33 ++++++++++++++++++++++++++++++++-
> arch/riscv/kernel/smpboot.c | 1 -
> arch/riscv/kernel/vmlinux.lds.S | 2 +-
> 4 files changed, 36 insertions(+), 3 deletions(-)

These are on for-next. As far as I can tell lockdep is working, but I'm just
doing some simple boot tests.

Thanks!

2020-07-09 23:16:55

[permalink] [raw]

Subject: Re: [PATCH V2 0/3] riscv: Enable LOCKDEP

Thank you, Palmer

On Fri, Jul 10, 2020 at 6:06 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Sat, 27 Jun 2020 06:57:05 PDT (-0700), [email protected] wrote:
> > From: Guo Ren <[email protected]>
> >
> > Lockdep is needed by proving the spinlocks and rwlocks. To support it,
> > we need to add TRACE_IRQFLAGS codes in kernel/entry.S. These patches
> > follow Documentation/irqflags-tracing.txt.
> >
> > Fixup 2 bugs that block the lockdep implementation.
> >
> > ---
> > Changes in v2
> > - Remove sX regs recovery codes which are unnecessary, because
> > callee will handle them. Thx Greentime :)
> >
> > - Move "restore a0 - a7" to handle_syscall, but if _TIF_SYSCALL_WORK
> > is set, "restore a1 - a7" is still duplicated. I prefer a C wrapper
> > for syscall.
> >
> > Guo Ren (2):
> > riscv: Fixup static_obj() fail
> > riscv: Enable LOCKDEP_SUPPORT & fixup TRACE_IRQFLAGS_SUPPORT
> >
> > Zong Li (1):
> > riscv: Fixup lockdep_assert_held with wrong param cpu_running
> >
> > arch/riscv/Kconfig | 3 +++
> > arch/riscv/kernel/entry.S | 33 ++++++++++++++++++++++++++++++++-
> > arch/riscv/kernel/smpboot.c | 1 -
> > arch/riscv/kernel/vmlinux.lds.S | 2 +-
> > 4 files changed, 36 insertions(+), 3 deletions(-)
>
> These are on for-next. As far as I can tell lockdep is working, but I'm just
> doing some simple boot tests.
>
> Thanks!

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-09-11 21:03:07

by Aurelien Jarno

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

Hi,

On 2020-06-27 13:57, [email protected] wrote:
> From: Guo Ren <[email protected]>
>
> When enable LOCKDEP, static_obj() will cause error. Because some
> __initdata static variables is before _stext:
>
> static int static_obj(const void *obj)
> {
> unsigned long start = (unsigned long) &_stext,
> end = (unsigned long) &_end,
> addr = (unsigned long) obj;
>
> /*
> * static variable?
> */
> if ((addr >= start) && (addr < end))
> return 1;
>
> [ 0.067192] INFO: trying to register non-static key.
> [ 0.067325] the code is fine but needs lockdep annotation.
> [ 0.067449] turning off the locking correctness validator.
> [ 0.067718] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc7-dirty #44
> [ 0.067945] Call Trace:
> [ 0.068369] [<ffffffe00020323c>] walk_stackframe+0x0/0xa4
> [ 0.068506] [<ffffffe000203422>] show_stack+0x2a/0x34
> [ 0.068631] [<ffffffe000521e4e>] dump_stack+0x94/0xca
> [ 0.068757] [<ffffffe000255a4e>] register_lock_class+0x5b8/0x5bc
> [ 0.068969] [<ffffffe000255abe>] __lock_acquire+0x6c/0x1d5c
> [ 0.069101] [<ffffffe0002550fe>] lock_acquire+0xae/0x312
> [ 0.069228] [<ffffffe000989a8e>] _raw_spin_lock_irqsave+0x40/0x5a
> [ 0.069357] [<ffffffe000247c64>] complete+0x1e/0x50
> [ 0.069479] [<ffffffe000984c38>] rest_init+0x1b0/0x28a
> [ 0.069660] [<ffffffe0000016a2>] 0xffffffe0000016a2
> [ 0.069779] [<ffffffe000001b84>] 0xffffffe000001b84
> [ 0.069953] [<ffffffe000001092>] 0xffffffe000001092
>
> static __initdata DECLARE_COMPLETION(kthreadd_done);
>
> noinline void __ref rest_init(void)
> {
> ...
> complete(&kthreadd_done);
>
> Signed-off-by: Guo Ren <[email protected]>
> ---
> arch/riscv/kernel/vmlinux.lds.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> index e6f8016..f3586e3 100644
> --- a/arch/riscv/kernel/vmlinux.lds.S
> +++ b/arch/riscv/kernel/vmlinux.lds.S
> @@ -22,6 +22,7 @@ SECTIONS
> /* Beginning of code and text segment */
> . = LOAD_OFFSET;
> _start = .;
> + _stext = .;
> HEAD_TEXT_SECTION
> . = ALIGN(PAGE_SIZE);
>
> @@ -54,7 +55,6 @@ SECTIONS
> . = ALIGN(SECTION_ALIGN);
> .text : {
> _text = .;
> - _stext = .;
> TEXT_TEXT
> SCHED_TEXT
> CPUIDLE_TEXT

This patch has been backported to kernel 5.8.4. This causes the kernel
to crash when trying to execute the init process:

[ 3.484586] AppArmor: AppArmor sha1 policy hashing enabled
[ 4.749835] Freeing unused kernel memory: 492K
[ 4.752017] Run /init as init process
[ 4.753571] usercopy: Kernel memory overwrite attempt detected to kernel text (offset 507879, size 11)!
[ 4.754838] ------------[ cut here ]------------
[ 4.755651] kernel BUG at mm/usercopy.c:99!
[ 4.756445] Kernel BUG [#1]
[ 4.756815] Modules linked in:
[ 4.757542] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-1-riscv64 #1 Debian 5.8.7-1
[ 4.758372] epc: ffffffe0003b5120 ra : ffffffe0003b5120 sp : ffffffe07f783ca0
[ 4.758960] gp : ffffffe000cc7230 tp : ffffffe07f77cec0 t0 : ffffffe000cdafc0
[ 4.759772] t1 : 0000000000000064 t2 : 0000000000000000 s0 : ffffffe07f783cf0
[ 4.760534] s1 : ffffffe00095d780 a0 : 000000000000005b a1 : 0000000000000020
[ 4.761309] a2 : 0000000000000005 a3 : 0000000000000000 a4 : ffffffe000c1f340
[ 4.761848] a5 : ffffffe000c1f340 a6 : 0000000000000000 a7 : 0000000000000087
[ 4.762684] s2 : ffffffe000941848 s3 : 000000000007bfe7 s4 : 000000000000000b
[ 4.763500] s5 : 0000000000000000 s6 : ffffffe00091cc00 s7 : fffffffffffff000
[ 4.764376] s8 : 0000003ffffff000 s9 : ffffffe0769f3200 s10: 000000000000000b
[ 4.765208] s11: ffffffe07d548c40 t3 : 0000000000000000 t4 : 000000000001dcd0
[ 4.766059] t5 : ffffffe000cc8510 t6 : ffffffe000cd64aa
[ 4.766712] status: 0000000000000120 badaddr: 0000000000000000 cause: 0000000000000003
[ 4.768308] ---[ end trace 1f8e733e834d4c3e ]---
[ 4.769129] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[ 4.770070] SMP: stopping secondary CPUs
[ 4.771110] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

Note that this is with CONFIG_HARDENED_USERCOPY=y

Aurelien

--
Aurelien Jarno GPG: 4096R/1DDD8C9B
[email protected] http://www.aurel32.net

2020-09-12 02:44:28

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

It's come from mm/usercopy.c
/* Is this address range in the kernel text area? */
static inline void check_kernel_text_object(const unsigned long ptr,
unsigned long n, bool to_user)
{
unsigned long textlow = (unsigned long)_stext;
unsigned long texthigh = (unsigned long)_etext;
unsigned long textlow_linear, texthigh_linear;

if (overlaps(ptr, n, textlow, texthigh))
usercopy_abort("kernel text", NULL, to_user, ptr - textlow, n);

The __init_text/data areas will be freed after bootup, so I think it should be:
- unsigned long textlow = (unsigned long)_stext;
+ unsigned long textlow = (unsigned long)_text;

That means _stext should include init_text/data and _text is only for freeable.

On Sat, Sep 12, 2020 at 5:01 AM Aurelien Jarno <[email protected]> wrote:
>
> Hi,
>
> On 2020-06-27 13:57, [email protected] wrote:
> > From: Guo Ren <[email protected]>
> >
> > When enable LOCKDEP, static_obj() will cause error. Because some
> > __initdata static variables is before _stext:
> >
> > static int static_obj(const void *obj)
> > {
> > unsigned long start = (unsigned long) &_stext,
> > end = (unsigned long) &_end,
> > addr = (unsigned long) obj;
> >
> > /*
> > * static variable?
> > */
> > if ((addr >= start) && (addr < end))
> > return 1;
> >
> > [ 0.067192] INFO: trying to register non-static key.
> > [ 0.067325] the code is fine but needs lockdep annotation.
> > [ 0.067449] turning off the locking correctness validator.
> > [ 0.067718] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc7-dirty #44
> > [ 0.067945] Call Trace:
> > [ 0.068369] [<ffffffe00020323c>] walk_stackframe+0x0/0xa4
> > [ 0.068506] [<ffffffe000203422>] show_stack+0x2a/0x34
> > [ 0.068631] [<ffffffe000521e4e>] dump_stack+0x94/0xca
> > [ 0.068757] [<ffffffe000255a4e>] register_lock_class+0x5b8/0x5bc
> > [ 0.068969] [<ffffffe000255abe>] __lock_acquire+0x6c/0x1d5c
> > [ 0.069101] [<ffffffe0002550fe>] lock_acquire+0xae/0x312
> > [ 0.069228] [<ffffffe000989a8e>] _raw_spin_lock_irqsave+0x40/0x5a
> > [ 0.069357] [<ffffffe000247c64>] complete+0x1e/0x50
> > [ 0.069479] [<ffffffe000984c38>] rest_init+0x1b0/0x28a
> > [ 0.069660] [<ffffffe0000016a2>] 0xffffffe0000016a2
> > [ 0.069779] [<ffffffe000001b84>] 0xffffffe000001b84
> > [ 0.069953] [<ffffffe000001092>] 0xffffffe000001092
> >
> > static __initdata DECLARE_COMPLETION(kthreadd_done);
> >
> > noinline void __ref rest_init(void)
> > {
> > ...
> > complete(&kthreadd_done);
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > ---
> > arch/riscv/kernel/vmlinux.lds.S | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > index e6f8016..f3586e3 100644
> > --- a/arch/riscv/kernel/vmlinux.lds.S
> > +++ b/arch/riscv/kernel/vmlinux.lds.S
> > @@ -22,6 +22,7 @@ SECTIONS
> > /* Beginning of code and text segment */
> > . = LOAD_OFFSET;
> > _start = .;
> > + _stext = .;
> > HEAD_TEXT_SECTION
> > . = ALIGN(PAGE_SIZE);
> >
> > @@ -54,7 +55,6 @@ SECTIONS
> > . = ALIGN(SECTION_ALIGN);
> > .text : {
> > _text = .;
> > - _stext = .;
> > TEXT_TEXT
> > SCHED_TEXT
> > CPUIDLE_TEXT
>
>
> This patch has been backported to kernel 5.8.4. This causes the kernel
> to crash when trying to execute the init process:
>
> [ 3.484586] AppArmor: AppArmor sha1 policy hashing enabled
> [ 4.749835] Freeing unused kernel memory: 492K
> [ 4.752017] Run /init as init process
> [ 4.753571] usercopy: Kernel memory overwrite attempt detected to kernel text (offset 507879, size 11)!
> [ 4.754838] ------------[ cut here ]------------
> [ 4.755651] kernel BUG at mm/usercopy.c:99!
> [ 4.756445] Kernel BUG [#1]
> [ 4.756815] Modules linked in:
> [ 4.757542] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-1-riscv64 #1 Debian 5.8.7-1
> [ 4.758372] epc: ffffffe0003b5120 ra : ffffffe0003b5120 sp : ffffffe07f783ca0
> [ 4.758960] gp : ffffffe000cc7230 tp : ffffffe07f77cec0 t0 : ffffffe000cdafc0
> [ 4.759772] t1 : 0000000000000064 t2 : 0000000000000000 s0 : ffffffe07f783cf0
> [ 4.760534] s1 : ffffffe00095d780 a0 : 000000000000005b a1 : 0000000000000020
> [ 4.761309] a2 : 0000000000000005 a3 : 0000000000000000 a4 : ffffffe000c1f340
> [ 4.761848] a5 : ffffffe000c1f340 a6 : 0000000000000000 a7 : 0000000000000087
> [ 4.762684] s2 : ffffffe000941848 s3 : 000000000007bfe7 s4 : 000000000000000b
> [ 4.763500] s5 : 0000000000000000 s6 : ffffffe00091cc00 s7 : fffffffffffff000
> [ 4.764376] s8 : 0000003ffffff000 s9 : ffffffe0769f3200 s10: 000000000000000b
> [ 4.765208] s11: ffffffe07d548c40 t3 : 0000000000000000 t4 : 000000000001dcd0
> [ 4.766059] t5 : ffffffe000cc8510 t6 : ffffffe000cd64aa
> [ 4.766712] status: 0000000000000120 badaddr: 0000000000000000 cause: 0000000000000003
> [ 4.768308] ---[ end trace 1f8e733e834d4c3e ]---
> [ 4.769129] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [ 4.770070] SMP: stopping secondary CPUs
> [ 4.771110] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
>
> Note that this is with CONFIG_HARDENED_USERCOPY=y
>
> Aurelien
>
> --
> Aurelien Jarno GPG: 4096R/1DDD8C9B
> [email protected] http://www.aurel32.net

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-09-14 10:43:16

by Aurelien Jarno

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On 2020-09-12 10:39, Guo Ren wrote:
> It's come from mm/usercopy.c
> /* Is this address range in the kernel text area? */
> static inline void check_kernel_text_object(const unsigned long ptr,
> unsigned long n, bool to_user)
> {
> unsigned long textlow = (unsigned long)_stext;
> unsigned long texthigh = (unsigned long)_etext;
> unsigned long textlow_linear, texthigh_linear;
>
> if (overlaps(ptr, n, textlow, texthigh))
> usercopy_abort("kernel text", NULL, to_user, ptr - textlow, n);
>
> The __init_text/data areas will be freed after bootup, so I think it should be:
> - unsigned long textlow = (unsigned long)_stext;
> + unsigned long textlow = (unsigned long)_text;
>
> That means _stext should include init_text/data and _text is only for freeable.

I have no idea if it is the right thing to do or not, but I can confirm
this fixes the issue.

How should we proceed to get that fixed in time for 5.9? For the older
branches where it has been backported (so far 5.7 and 5.8), should we
just get that commit reverted instead?

Thanks,
Aurelien

--
Aurelien Jarno GPG: 4096R/1DDD8C9B
[email protected] http://www.aurel32.net

2020-09-24 07:40:45

by Andreas Schwab

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Sep 14 2020, Aurelien Jarno wrote:

> How should we proceed to get that fixed in time for 5.9? For the older
> branches where it has been backported (so far 5.7 and 5.8), should we
> just get that commit reverted instead?

Can this please be resolved ASAP?

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."

2020-09-24 16:21:17

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

How about this, revert the commit and don't free INIT_DATA_SECTION. I
think the solution is safe enough, but wast a little memory.

diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
index f3586e3..34d00d9 100644
--- a/arch/riscv/kernel/vmlinux.lds.S
+++ b/arch/riscv/kernel/vmlinux.lds.S
@@ -22,13 +22,11 @@ SECTIONS
/* Beginning of code and text segment */
. = LOAD_OFFSET;
_start = .;
- _stext = .;
HEAD_TEXT_SECTION
. = ALIGN(PAGE_SIZE);

__init_begin = .;
INIT_TEXT_SECTION(PAGE_SIZE)
- INIT_DATA_SECTION(16)
. = ALIGN(8);
__soc_early_init_table : {
__soc_early_init_table_start = .;
@@ -55,6 +53,7 @@ SECTIONS
. = ALIGN(SECTION_ALIGN);
.text : {
_text = .;
+ _stext = .;
TEXT_TEXT
SCHED_TEXT
CPUIDLE_TEXT
@@ -67,6 +66,8 @@ SECTIONS
_etext = .;
}

+ INIT_DATA_SECTION(16)
+
/* Start of data section */
_sdata = .;
RO_DATA(SECTION_ALIGN)

On Thu, Sep 24, 2020 at 3:36 PM Andreas Schwab <[email protected]> wrote:
>
> On Sep 14 2020, Aurelien Jarno wrote:
>
> > How should we proceed to get that fixed in time for 5.9? For the older
> > branches where it has been backported (so far 5.7 and 5.8), should we
> > just get that commit reverted instead?
>
> Can this please be resolved ASAP?
>
> Andreas.
>
> --
> Andreas Schwab, [email protected]
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-09-29 18:53:26

by Aurelien Jarno

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

Hi,

On 2020-09-25 00:19, Guo Ren wrote:
> How about this, revert the commit and don't free INIT_DATA_SECTION. I
> think the solution is safe enough, but wast a little memory.
>
> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> index f3586e3..34d00d9 100644
> --- a/arch/riscv/kernel/vmlinux.lds.S
> +++ b/arch/riscv/kernel/vmlinux.lds.S
> @@ -22,13 +22,11 @@ SECTIONS
> /* Beginning of code and text segment */
> . = LOAD_OFFSET;
> _start = .;
> - _stext = .;
> HEAD_TEXT_SECTION
> . = ALIGN(PAGE_SIZE);
>
> __init_begin = .;
> INIT_TEXT_SECTION(PAGE_SIZE)
> - INIT_DATA_SECTION(16)
> . = ALIGN(8);
> __soc_early_init_table : {
> __soc_early_init_table_start = .;
> @@ -55,6 +53,7 @@ SECTIONS
> . = ALIGN(SECTION_ALIGN);
> .text : {
> _text = .;
> + _stext = .;
> TEXT_TEXT
> SCHED_TEXT
> CPUIDLE_TEXT
> @@ -67,6 +66,8 @@ SECTIONS
> _etext = .;
> }
>
> + INIT_DATA_SECTION(16)
> +
> /* Start of data section */
> _sdata = .;
> RO_DATA(SECTION_ALIGN)
>

This patch doesn't apply, as tabs have been converted to space
somewhere. After fixing that, the patch applies and I confirm that it
fixes the problem.

Tested-by: Aurelien Jarno <[email protected]>

Thanks,
Aurelien

--
Aurelien Jarno GPG: 4096R/1DDD8C9B
[email protected] http://www.aurel32.net

2020-09-29 22:14:18

[permalink] [raw]

Subject: Re: [PATCH V2 2/3] riscv: Fixup lockdep_assert_held with wrong param cpu_running

On Sat, Jun 27, 2020 at 6:58 AM <[email protected]> wrote:
>
> From: Zong Li <[email protected]>
>
> The cpu_running is not a lock-class, it lacks the dep_map member in
> completion. It causes the error as follow:
>
> arch/riscv/kernel/smpboot.c: In function '__cpu_up':
> ./include/linux/lockdep.h:364:52: error: 'struct completion' has no member named 'dep_map'
> 364 | #define lockdep_is_held(lock) lock_is_held(&(lock)->dep_map)
> | ^~
> ./include/asm-generic/bug.h:113:25: note: in definition of macro 'WARN_ON'
> 113 | int __ret_warn_on = !!(condition); \
> | ^~~~~~~~~
> ./include/linux/lockdep.h:390:27: note: in expansion of macro 'lockdep_is_held'
> 390 | WARN_ON(debug_locks && !lockdep_is_held(l)); \
> | ^~~~~~~~~~~~~~~
> arch/riscv/kernel/smpboot.c:118:2: note: in expansion of macro 'lockdep_assert_held'
> 118 | lockdep_assert_held(&cpu_running);
>
> There are a lot of archs which use cpu_running in smpboot.c (arm,
> arm64, openrisc, xtensa, s390, x86, mips), but none of them try
> lockdep_assert_held(&cpu_running.wait.lock). So Just remove it.
>
> Signed-off-by: Zong Li <[email protected]>
> Signed-off-by: Guo Ren <[email protected]>
> ---
> arch/riscv/kernel/smpboot.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 4e99227..defc4e1 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -121,7 +121,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>
> ret = start_secondary_cpu(cpu, tidle);
> if (!ret) {
> - lockdep_assert_held(&cpu_running);
> wait_for_completion_timeout(&cpu_running,
> msecs_to_jiffies(1000));
>
> --
> 2.7.4
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Thanks for catching this.

Reviewed-by: Atish Patra <[email protected]>

--
Regards,
Atish

2020-10-05 08:27:00

by Andreas Schwab

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Sep 14 2020, Aurelien Jarno wrote:

> How should we proceed to get that fixed in time for 5.9? For the older
> branches where it has been backported (so far 5.7 and 5.8), should we
> just get that commit reverted instead?

Why is this still broken?

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."

2020-10-05 17:08:17

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Mon, 05 Oct 2020 01:25:22 PDT (-0700), [email protected] wrote:
> On Sep 14 2020, Aurelien Jarno wrote:
>
>> How should we proceed to get that fixed in time for 5.9? For the older
>> branches where it has been backported (so far 5.7 and 5.8), should we
>> just get that commit reverted instead?
>
> Why is this still broken?

Sorry, I hadn't seen this. I'm not seeing a boot failure on 5.9-rc8 with just
CONFIG_HARDENED_USERCPOY=y in addition to defconfig (on QEMU, though I doubt
that's relevant here). It looks like the fix is to essentially revert this,
which I'm fine with, but I'd prefer to have a failing test to make sure this
doesn't break again.

Guo: I don't see an actual patch (signed off and such) posted for this, do you
mind posting one? Otherwise I'll take a crack at constructing the revert
myself.

2020-10-05 19:16:25

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Thu, Sep 24, 2020 at 9:19 AM Guo Ren <[email protected]> wrote:
>
> How about this, revert the commit and don't free INIT_DATA_SECTION. I
> think the solution is safe enough, but wast a little memory.
>
> diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> index f3586e3..34d00d9 100644
> --- a/arch/riscv/kernel/vmlinux.lds.S
> +++ b/arch/riscv/kernel/vmlinux.lds.S
> @@ -22,13 +22,11 @@ SECTIONS
> /* Beginning of code and text segment */
> . = LOAD_OFFSET;
> _start = .;
> - _stext = .;
> HEAD_TEXT_SECTION
> . = ALIGN(PAGE_SIZE);
>
> __init_begin = .;
> INIT_TEXT_SECTION(PAGE_SIZE)
> - INIT_DATA_SECTION(16)
> . = ALIGN(8);
> __soc_early_init_table : {
> __soc_early_init_table_start = .;
> @@ -55,6 +53,7 @@ SECTIONS
> . = ALIGN(SECTION_ALIGN);
> .text : {
> _text = .;
> + _stext = .;
> TEXT_TEXT
> SCHED_TEXT
> CPUIDLE_TEXT
> @@ -67,6 +66,8 @@ SECTIONS
> _etext = .;
> }
>
> + INIT_DATA_SECTION(16)
> +

I think you need to move EXIT_DATA as well. Currently, we have init
data & text in one section.
In general it is better idea to separate those similar to ARM64.
Additionally, ARM64 applies different mapping for init data & text
as the init data section is marked as non-executable[1]

However, we don't modify any permission for any init sections. Should
we do that as well ?

[1] https://patchwork.kernel.org/patch/9572869/

> /* Start of data section */
> _sdata = .;
> RO_DATA(SECTION_ALIGN)
>
> On Thu, Sep 24, 2020 at 3:36 PM Andreas Schwab <[email protected]> wrote:
> >
> > On Sep 14 2020, Aurelien Jarno wrote:
> >
> > > How should we proceed to get that fixed in time for 5.9? For the older
> > > branches where it has been backported (so far 5.7 and 5.8), should we
> > > just get that commit reverted instead?
> >
> > Can this please be resolved ASAP?
> >
> > Andreas.
> >
> > --
> > Andreas Schwab, [email protected]
> > GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> > "And now for something completely different."
>
>
>
> --
> Best Regards
> Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Regards,
Atish

2020-10-05 19:48:42

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Mon, 05 Oct 2020 11:40:54 PDT (-0700), [email protected] wrote:
> On Okt 05 2020, Palmer Dabbelt wrote:
>
>> On Mon, 05 Oct 2020 01:25:22 PDT (-0700), [email protected] wrote:
>>> On Sep 14 2020, Aurelien Jarno wrote:
>>>
>>>> How should we proceed to get that fixed in time for 5.9? For the older
>>>> branches where it has been backported (so far 5.7 and 5.8), should we
>>>> just get that commit reverted instead?
>>>
>>> Why is this still broken?
>>
>> Sorry, I hadn't seen this. I'm not seeing a boot failure on 5.9-rc8 with just
>> CONFIG_HARDENED_USERCPOY=y in addition to defconfig (on QEMU, though I doubt
>> that's relevant here).
>
> I don't see a boot failure either, but eventually you will get crashes
> like this, and resources are not properly released:
>
> [ 4560.936645] usercopy: Kernel memory overwrite attempt detected to kernel text (offset 241626, size 16)!
> [ 4560.945324] ------------[ cut here ]------------
> [ 4560.949954] kernel BUG at mm/usercopy.c:99!
> [ 4560.954030] Kernel BUG [#1]
> [ 4560.956805] Modules linked in: nfsv3 nfs_acl rfkill mmc_block sf_pdma i2c_ocores virt_dma spi_sifive uio_pdrv_genirq uio loop drm drm_panel_orientation_quirks rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache af_packet mscc macsec macb ptp pps_core phylink of_mdio fixed_phy libphy pwm_sifive mmc_spi crc_itu_t crc7 of_mmc_spi mmc_core spi_bitbang sunrpc sg dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua
> [ 4560.995103] CPU: 2 PID: 23806 Comm: nis Not tainted 5.8.10-1-default #1 openSUSE Tumbleweed (unreleased)
> [ 4561.004563] epc: ffffffe00036140e ra : ffffffe00036140e sp : ffffffe004bc7d60
> [ 4561.011679] gp : ffffffe00127ee60 tp : ffffffe1b05d0000 t0 : ffffffe001297ca0
> [ 4561.018886] t1 : ffffffe001297c30 t2 : 0000000000000000 s0 : ffffffe004bc7d80
> [ 4561.026093] s1 : ffffffe00003afda a0 : 000000000000005b a1 : ffffffe1f7d67588
> [ 4561.033298] a2 : ffffffe1f7d6c108 a3 : 0000000000000000 a4 : ffffffe000043e80
> [ 4561.040506] a5 : ffffffe1f7d6be80 a6 : 0000000000000144 a7 : 0000000000000000
> [ 4561.047712] s2 : 0000000000000010 s3 : 0000000000000000 s4 : ffffffe00003afea
> [ 4561.054918] s5 : ffffffe1f7e00e80 s6 : 0000002af4a2c2e0 s7 : fffffffffffff000
> [ 4561.062124] s8 : 0000003ffffff000 s9 : ffffffe19f985400 s10: 0000000000000010
> [ 4561.069329] s11: ffffffe1f7e00e80 t3 : 0000000000038fa8 t4 : 0000000000038fa8
> [ 4561.076533] t5 : 0000000000000001 t6 : ffffffe00128e062
> [ 4561.081832] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> [ 4561.089821] ---[ end trace a7c93e7595e9c2cc ]---
> [ 4561.095589] BUG: Bad rss-counter state mm:00000000c54f4c29 type:MM_ANONPAGES val:1

Ah, I must have misunderstood. I guess I just assumed "init crashes" meant on
boot, not just at some time later. I just sent out a patch reverting this, LMK
if it fixes the issue. I have some work stuff to do, but I'll try to find some
time tonight to look into fixing both of the bugs -- otherwise I'll just take
the revert (assuming it does actually fix the issue for you and passes the
tests).

I saw Atish post after I started writing this: I agree we need to sort of the
kernel's memory map, I just think it's too late for 5.9.

> Andreas.

2020-10-05 20:33:44

by Andreas Schwab

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Okt 05 2020, Palmer Dabbelt wrote:

> On Mon, 05 Oct 2020 01:25:22 PDT (-0700), [email protected] wrote:
>> On Sep 14 2020, Aurelien Jarno wrote:
>>
>>> How should we proceed to get that fixed in time for 5.9? For the older
>>> branches where it has been backported (so far 5.7 and 5.8), should we
>>> just get that commit reverted instead?
>>
>> Why is this still broken?
>
> Sorry, I hadn't seen this. I'm not seeing a boot failure on 5.9-rc8 with just
> CONFIG_HARDENED_USERCPOY=y in addition to defconfig (on QEMU, though I doubt
> that's relevant here).

I don't see a boot failure either, but eventually you will get crashes
like this, and resources are not properly released:

[ 4560.936645] usercopy: Kernel memory overwrite attempt detected to kernel text (offset 241626, size 16)!
[ 4560.945324] ------------[ cut here ]------------
[ 4560.949954] kernel BUG at mm/usercopy.c:99!
[ 4560.954030] Kernel BUG [#1]
[ 4560.956805] Modules linked in: nfsv3 nfs_acl rfkill mmc_block sf_pdma i2c_ocores virt_dma spi_sifive uio_pdrv_genirq uio loop drm drm_panel_orientation_quirks rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache af_packet mscc macsec macb ptp pps_core phylink of_mdio fixed_phy libphy pwm_sifive mmc_spi crc_itu_t crc7 of_mmc_spi mmc_core spi_bitbang sunrpc sg dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua
[ 4560.995103] CPU: 2 PID: 23806 Comm: nis Not tainted 5.8.10-1-default #1 openSUSE Tumbleweed (unreleased)
[ 4561.004563] epc: ffffffe00036140e ra : ffffffe00036140e sp : ffffffe004bc7d60
[ 4561.011679] gp : ffffffe00127ee60 tp : ffffffe1b05d0000 t0 : ffffffe001297ca0
[ 4561.018886] t1 : ffffffe001297c30 t2 : 0000000000000000 s0 : ffffffe004bc7d80
[ 4561.026093] s1 : ffffffe00003afda a0 : 000000000000005b a1 : ffffffe1f7d67588
[ 4561.033298] a2 : ffffffe1f7d6c108 a3 : 0000000000000000 a4 : ffffffe000043e80
[ 4561.040506] a5 : ffffffe1f7d6be80 a6 : 0000000000000144 a7 : 0000000000000000
[ 4561.047712] s2 : 0000000000000010 s3 : 0000000000000000 s4 : ffffffe00003afea
[ 4561.054918] s5 : ffffffe1f7e00e80 s6 : 0000002af4a2c2e0 s7 : fffffffffffff000
[ 4561.062124] s8 : 0000003ffffff000 s9 : ffffffe19f985400 s10: 0000000000000010
[ 4561.069329] s11: ffffffe1f7e00e80 t3 : 0000000000038fa8 t4 : 0000000000038fa8
[ 4561.076533] t5 : 0000000000000001 t6 : ffffffe00128e062
[ 4561.081832] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
[ 4561.089821] ---[ end trace a7c93e7595e9c2cc ]---
[ 4561.095589] BUG: Bad rss-counter state mm:00000000c54f4c29 type:MM_ANONPAGES val:1

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."

2020-10-05 23:17:14

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Mon, Oct 5, 2020 at 12:46 PM Palmer Dabbelt <[email protected]> wrote:
>
> On Mon, 05 Oct 2020 11:40:54 PDT (-0700), [email protected] wrote:
> > On Okt 05 2020, Palmer Dabbelt wrote:
> >
> >> On Mon, 05 Oct 2020 01:25:22 PDT (-0700), [email protected] wrote:
> >>> On Sep 14 2020, Aurelien Jarno wrote:
> >>>
> >>>> How should we proceed to get that fixed in time for 5.9? For the older
> >>>> branches where it has been backported (so far 5.7 and 5.8), should we
> >>>> just get that commit reverted instead?
> >>>
> >>> Why is this still broken?
> >>
> >> Sorry, I hadn't seen this. I'm not seeing a boot failure on 5.9-rc8 with just
> >> CONFIG_HARDENED_USERCPOY=y in addition to defconfig (on QEMU, though I doubt
> >> that's relevant here).
> >
> > I don't see a boot failure either, but eventually you will get crashes
> > like this, and resources are not properly released:
> >
> > [ 4560.936645] usercopy: Kernel memory overwrite attempt detected to kernel text (offset 241626, size 16)!
> > [ 4560.945324] ------------[ cut here ]------------
> > [ 4560.949954] kernel BUG at mm/usercopy.c:99!
> > [ 4560.954030] Kernel BUG [#1]
> > [ 4560.956805] Modules linked in: nfsv3 nfs_acl rfkill mmc_block sf_pdma i2c_ocores virt_dma spi_sifive uio_pdrv_genirq uio loop drm drm_panel_orientation_quirks rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache af_packet mscc macsec macb ptp pps_core phylink of_mdio fixed_phy libphy pwm_sifive mmc_spi crc_itu_t crc7 of_mmc_spi mmc_core spi_bitbang sunrpc sg dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua
> > [ 4560.995103] CPU: 2 PID: 23806 Comm: nis Not tainted 5.8.10-1-default #1 openSUSE Tumbleweed (unreleased)
> > [ 4561.004563] epc: ffffffe00036140e ra : ffffffe00036140e sp : ffffffe004bc7d60
> > [ 4561.011679] gp : ffffffe00127ee60 tp : ffffffe1b05d0000 t0 : ffffffe001297ca0
> > [ 4561.018886] t1 : ffffffe001297c30 t2 : 0000000000000000 s0 : ffffffe004bc7d80
> > [ 4561.026093] s1 : ffffffe00003afda a0 : 000000000000005b a1 : ffffffe1f7d67588
> > [ 4561.033298] a2 : ffffffe1f7d6c108 a3 : 0000000000000000 a4 : ffffffe000043e80
> > [ 4561.040506] a5 : ffffffe1f7d6be80 a6 : 0000000000000144 a7 : 0000000000000000
> > [ 4561.047712] s2 : 0000000000000010 s3 : 0000000000000000 s4 : ffffffe00003afea
> > [ 4561.054918] s5 : ffffffe1f7e00e80 s6 : 0000002af4a2c2e0 s7 : fffffffffffff000
> > [ 4561.062124] s8 : 0000003ffffff000 s9 : ffffffe19f985400 s10: 0000000000000010
> > [ 4561.069329] s11: ffffffe1f7e00e80 t3 : 0000000000038fa8 t4 : 0000000000038fa8
> > [ 4561.076533] t5 : 0000000000000001 t6 : ffffffe00128e062
> > [ 4561.081832] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
> > [ 4561.089821] ---[ end trace a7c93e7595e9c2cc ]---
> > [ 4561.095589] BUG: Bad rss-counter state mm:00000000c54f4c29 type:MM_ANONPAGES val:1
>
> Ah, I must have misunderstood. I guess I just assumed "init crashes" meant on
> boot, not just at some time later. I just sent out a patch reverting this, LMK
> if it fixes the issue. I have some work stuff to do, but I'll try to find some
> time tonight to look into fixing both of the bugs -- otherwise I'll just take
> the revert (assuming it does actually fix the issue for you and passes the
> tests).
>
> I saw Atish post after I started writing this: I agree we need to sort of the
> kernel's memory map, I just think it's too late for 5.9.
>

Yes. It is definitely a for-next material. I will try to take a stab
at this if nobody else has an objection.

> > Andreas.
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Regards,
Atish

2020-10-05 23:17:57

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Mon, 05 Oct 2020 14:12:44 PDT (-0700), [email protected] wrote:
> On Mon, Oct 5, 2020 at 12:46 PM Palmer Dabbelt <[email protected]> wrote:
>>
>> On Mon, 05 Oct 2020 11:40:54 PDT (-0700), [email protected] wrote:
>> > On Okt 05 2020, Palmer Dabbelt wrote:
>> >
>> >> On Mon, 05 Oct 2020 01:25:22 PDT (-0700), [email protected] wrote:
>> >>> On Sep 14 2020, Aurelien Jarno wrote:
>> >>>
>> >>>> How should we proceed to get that fixed in time for 5.9? For the older
>> >>>> branches where it has been backported (so far 5.7 and 5.8), should we
>> >>>> just get that commit reverted instead?
>> >>>
>> >>> Why is this still broken?
>> >>
>> >> Sorry, I hadn't seen this. I'm not seeing a boot failure on 5.9-rc8 with just
>> >> CONFIG_HARDENED_USERCPOY=y in addition to defconfig (on QEMU, though I doubt
>> >> that's relevant here).
>> >
>> > I don't see a boot failure either, but eventually you will get crashes
>> > like this, and resources are not properly released:
>> >
>> > [ 4560.936645] usercopy: Kernel memory overwrite attempt detected to kernel text (offset 241626, size 16)!
>> > [ 4560.945324] ------------[ cut here ]------------
>> > [ 4560.949954] kernel BUG at mm/usercopy.c:99!
>> > [ 4560.954030] Kernel BUG [#1]
>> > [ 4560.956805] Modules linked in: nfsv3 nfs_acl rfkill mmc_block sf_pdma i2c_ocores virt_dma spi_sifive uio_pdrv_genirq uio loop drm drm_panel_orientation_quirks rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache af_packet mscc macsec macb ptp pps_core phylink of_mdio fixed_phy libphy pwm_sifive mmc_spi crc_itu_t crc7 of_mmc_spi mmc_core spi_bitbang sunrpc sg dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua
>> > [ 4560.995103] CPU: 2 PID: 23806 Comm: nis Not tainted 5.8.10-1-default #1 openSUSE Tumbleweed (unreleased)
>> > [ 4561.004563] epc: ffffffe00036140e ra : ffffffe00036140e sp : ffffffe004bc7d60
>> > [ 4561.011679] gp : ffffffe00127ee60 tp : ffffffe1b05d0000 t0 : ffffffe001297ca0
>> > [ 4561.018886] t1 : ffffffe001297c30 t2 : 0000000000000000 s0 : ffffffe004bc7d80
>> > [ 4561.026093] s1 : ffffffe00003afda a0 : 000000000000005b a1 : ffffffe1f7d67588
>> > [ 4561.033298] a2 : ffffffe1f7d6c108 a3 : 0000000000000000 a4 : ffffffe000043e80
>> > [ 4561.040506] a5 : ffffffe1f7d6be80 a6 : 0000000000000144 a7 : 0000000000000000
>> > [ 4561.047712] s2 : 0000000000000010 s3 : 0000000000000000 s4 : ffffffe00003afea
>> > [ 4561.054918] s5 : ffffffe1f7e00e80 s6 : 0000002af4a2c2e0 s7 : fffffffffffff000
>> > [ 4561.062124] s8 : 0000003ffffff000 s9 : ffffffe19f985400 s10: 0000000000000010
>> > [ 4561.069329] s11: ffffffe1f7e00e80 t3 : 0000000000038fa8 t4 : 0000000000038fa8
>> > [ 4561.076533] t5 : 0000000000000001 t6 : ffffffe00128e062
>> > [ 4561.081832] status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
>> > [ 4561.089821] ---[ end trace a7c93e7595e9c2cc ]---
>> > [ 4561.095589] BUG: Bad rss-counter state mm:00000000c54f4c29 type:MM_ANONPAGES val:1
>>
>> Ah, I must have misunderstood. I guess I just assumed "init crashes" meant on
>> boot, not just at some time later. I just sent out a patch reverting this, LMK
>> if it fixes the issue. I have some work stuff to do, but I'll try to find some
>> time tonight to look into fixing both of the bugs -- otherwise I'll just take
>> the revert (assuming it does actually fix the issue for you and passes the
>> tests).
>>
>> I saw Atish post after I started writing this: I agree we need to sort of the
>> kernel's memory map, I just think it's too late for 5.9.
>>
>
> Yes. It is definitely a for-next material. I will try to take a stab
> at this if nobody else has an objection.

WFM. Thanks!

>
>> > Andreas.
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

2020-10-06 16:48:38

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Tue, Oct 6, 2020 at 3:14 AM Atish Patra <[email protected]> wrote:
>
> On Thu, Sep 24, 2020 at 9:19 AM Guo Ren <[email protected]> wrote:
> >
> > How about this, revert the commit and don't free INIT_DATA_SECTION. I
> > think the solution is safe enough, but wast a little memory.
> >
> > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > index f3586e3..34d00d9 100644
> > --- a/arch/riscv/kernel/vmlinux.lds.S
> > +++ b/arch/riscv/kernel/vmlinux.lds.S
> > @@ -22,13 +22,11 @@ SECTIONS
> > /* Beginning of code and text segment */
> > . = LOAD_OFFSET;
> > _start = .;
> > - _stext = .;
> > HEAD_TEXT_SECTION
> > . = ALIGN(PAGE_SIZE);
> >
> > __init_begin = .;
> > INIT_TEXT_SECTION(PAGE_SIZE)
> > - INIT_DATA_SECTION(16)
> > . = ALIGN(8);
> > __soc_early_init_table : {
> > __soc_early_init_table_start = .;
> > @@ -55,6 +53,7 @@ SECTIONS
> > . = ALIGN(SECTION_ALIGN);
> > .text : {
> > _text = .;
> > + _stext = .;
> > TEXT_TEXT
> > SCHED_TEXT
> > CPUIDLE_TEXT
> > @@ -67,6 +66,8 @@ SECTIONS
> > _etext = .;
> > }
> >
> > + INIT_DATA_SECTION(16)
> > +
>
> I think you need to move EXIT_DATA as well. Currently, we have init
> data & text in one section.
It's not related to this issue. There is two check code problem:
1. static int static_obj(const void *obj)
{
unsigned long start = (unsigned long) &_stext,
end = (unsigned long) &_end,
addr = (unsigned long) obj;

/*
* static variable?
*/
if ((addr >= start) && (addr < end))
return 1;

2. /* Is this address range in the kernel text area? */
static inline void check_kernel_text_object(const unsigned long ptr,
unsigned long n, bool to_user)
{
unsigned long textlow = (unsigned long)_stext;
unsigned long texthigh = (unsigned long)_etext;
unsigned long textlow_linear, texthigh_linear;

if (overlaps(ptr, n, textlow, texthigh))
usercopy_abort("kernel text", NULL, to_user, ptr -
textlow, n);

The patch of commit: a0fa4027dc911 (riscv: Fixup static_obj() fail) broke 2th.

> In general it is better idea to separate those similar to ARM64.
> Additionally, ARM64 applies different mapping for init data & text
> as the init data section is marked as non-executable[1]
Yes, it's safer to protect init text & init data, but it's should be
another patch.

>
> However, we don't modify any permission for any init sections. Should
> we do that as well ?
Agree, we should do that.

>
> [1] https://patchwork.kernel.org/patch/9572869/
>
> > /* Start of data section */
> > _sdata = .;
> > RO_DATA(SECTION_ALIGN)
> >
> > On Thu, Sep 24, 2020 at 3:36 PM Andreas Schwab <[email protected]> wrote:
> > >
> > > On Sep 14 2020, Aurelien Jarno wrote:
> > >
> > > > How should we proceed to get that fixed in time for 5.9? For the older
> > > > branches where it has been backported (so far 5.7 and 5.8), should we
> > > > just get that commit reverted instead?
> > >
> > > Can this please be resolved ASAP?
> > >
> > > Andreas.
> > >
> > > --
> > > Andreas Schwab, [email protected]
> > > GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> > > "And now for something completely different."
> >
> >
> >
> > --
> > Best Regards
> > Guo Ren
> >
> > ML: https://lore.kernel.org/linux-csky/
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> --
> Regards,
> Atish

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-10-06 16:56:58

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Tue, Oct 6, 2020 at 12:39 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Mon, 05 Oct 2020 01:25:22 PDT (-0700), [email protected] wrote:
> > On Sep 14 2020, Aurelien Jarno wrote:
> >
> >> How should we proceed to get that fixed in time for 5.9? For the older
> >> branches where it has been backported (so far 5.7 and 5.8), should we
> >> just get that commit reverted instead?
> >
> > Why is this still broken?
>
> Sorry, I hadn't seen this. I'm not seeing a boot failure on 5.9-rc8 with just
> CONFIG_HARDENED_USERCPOY=y in addition to defconfig (on QEMU, though I doubt
> that's relevant here). It looks like the fix is to essentially revert this,
> which I'm fine with, but I'd prefer to have a failing test to make sure this
> doesn't break again.
>
> Guo: I don't see an actual patch (signed off and such) posted for this, do you
> mind posting one? Otherwise I'll take a crack at constructing the revert
> myself.

Please have a look:
https://lore.kernel.org/linux-riscv/[email protected]/T/#u

The only revert couldn't solve the static_obj problem.

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/

2020-10-06 20:41:30

[permalink] [raw]

Subject: Re: [PATCH V2 1/3] riscv: Fixup static_obj() fail

On Tue, Oct 6, 2020 at 9:46 AM Guo Ren <[email protected]> wrote:
>
> On Tue, Oct 6, 2020 at 3:14 AM Atish Patra <[email protected]> wrote:
> >
> > On Thu, Sep 24, 2020 at 9:19 AM Guo Ren <[email protected]> wrote:
> > >
> > > How about this, revert the commit and don't free INIT_DATA_SECTION. I
> > > think the solution is safe enough, but wast a little memory.
> > >
> > > diff --git a/arch/riscv/kernel/vmlinux.lds.S b/arch/riscv/kernel/vmlinux.lds.S
> > > index f3586e3..34d00d9 100644
> > > --- a/arch/riscv/kernel/vmlinux.lds.S
> > > +++ b/arch/riscv/kernel/vmlinux.lds.S
> > > @@ -22,13 +22,11 @@ SECTIONS
> > > /* Beginning of code and text segment */
> > > . = LOAD_OFFSET;
> > > _start = .;
> > > - _stext = .;
> > > HEAD_TEXT_SECTION
> > > . = ALIGN(PAGE_SIZE);
> > >
> > > __init_begin = .;
> > > INIT_TEXT_SECTION(PAGE_SIZE)
> > > - INIT_DATA_SECTION(16)
> > > . = ALIGN(8);
> > > __soc_early_init_table : {
> > > __soc_early_init_table_start = .;
> > > @@ -55,6 +53,7 @@ SECTIONS
> > > . = ALIGN(SECTION_ALIGN);
> > > .text : {
> > > _text = .;
> > > + _stext = .;
> > > TEXT_TEXT
> > > SCHED_TEXT
> > > CPUIDLE_TEXT
> > > @@ -67,6 +66,8 @@ SECTIONS
> > > _etext = .;
> > > }
> > >
> > > + INIT_DATA_SECTION(16)
> > > +
> >
> > I think you need to move EXIT_DATA as well. Currently, we have init
> > data & text in one section.
> It's not related to this issue. There is two check code problem:

Yes. But we shouldn't move only INIT_DATA_SECTION out of __init section
while leaving percpu & exit data in the __init section. Here is what I
have in mind.

diff --git a/arch/riscv/kernel/vmlinux.lds.S
b/arch/riscv/kernel/vmlinux.lds.S
index 9795359cb9da..4432cef8184e 100644
--- a/arch/riscv/kernel/vmlinux.lds.S
+++ b/arch/riscv/kernel/vmlinux.lds.S
@@ -26,13 +26,11 @@ SECTIONS
/* Beginning of code and text segment */
. = LOAD_OFFSET;
_start = .;
_start = .;
- _stext = .;
HEAD_TEXT_SECTION
. = ALIGN(PAGE_SIZE);

__init_begin = .;
INIT_TEXT_SECTION(PAGE_SIZE)
- INIT_DATA_SECTION(16)
. = ALIGN(8);
__soc_early_init_table : {
__soc_early_init_table_start = .;
@@ -49,16 +47,13 @@ SECTIONS
{
EXIT_TEXT
}
- .exit.data :
- {
- EXIT_DATA
- }
- PERCPU_SECTION(L1_CACHE_BYTES)
+
__init_end = .;

. = ALIGN(SECTION_ALIGN);
.text : {
_text = .;
+ _stext = .;
TEXT_TEXT
SCHED_TEXT
CPUIDLE_TEXT
@@ -77,6 +72,17 @@ SECTIONS
#endif

/* Start of data section */
+ __init_data_begin = .;
+ INIT_DATA_SECTION(16)
+ .exit.data :
+ {
+ EXIT_DATA
+ }
+
+ PERCPU_SECTION(L1_CACHE_BYTES)
+
+ __init_data_end = .;
+

As you correctly pointed out, this wastes around ~200K init memory
that is wasted.
That is not an ideal solution.

The other alternative is to move __init_text section after _text as
well similar to other architectures. But that won't work
for RISC-V as we jump from _start to __start_kernel(in __init section)
in head.S. A JAL instruction can't be fit because
__start_kernel is now too far. We can't replace JAL with a JALR
because that would require an additional
instruction and violates image header format.

Any other ideas to solve this problem without wasting memory ?

> 1. static int static_obj(const void *obj)
> {
> unsigned long start = (unsigned long) &_stext,
> end = (unsigned long) &_end,
> addr = (unsigned long) obj;
>
> /*
> * static variable?
> */
> if ((addr >= start) && (addr < end))
> return 1;
>
> 2. /* Is this address range in the kernel text area? */
> static inline void check_kernel_text_object(const unsigned long ptr,
> unsigned long n, bool to_user)
> {
> unsigned long textlow = (unsigned long)_stext;
> unsigned long texthigh = (unsigned long)_etext;
> unsigned long textlow_linear, texthigh_linear;
>
> if (overlaps(ptr, n, textlow, texthigh))
> usercopy_abort("kernel text", NULL, to_user, ptr -
> textlow, n);
>
> The patch of commit: a0fa4027dc911 (riscv: Fixup static_obj() fail) broke 2th.
>
> > In general it is better idea to separate those similar to ARM64.
> > Additionally, ARM64 applies different mapping for init data & text
> > as the init data section is marked as non-executable[1]
> Yes, it's safer to protect init text & init data, but it's should be
> another patch.
>

Yes. I will send the patch based on this fix.

> >
> > However, we don't modify any permission for any init sections. Should
> > we do that as well ?
> Agree, we should do that.
>
> >
> > [1] https://patchwork.kernel.org/patch/9572869/
> >
> > > /* Start of data section */
> > > _sdata = .;
> > > RO_DATA(SECTION_ALIGN)
> > >
> > > On Thu, Sep 24, 2020 at 3:36 PM Andreas Schwab <[email protected]> wrote:
> > > >
> > > > On Sep 14 2020, Aurelien Jarno wrote:
> > > >
> > > > > How should we proceed to get that fixed in time for 5.9? For the older
> > > > > branches where it has been backported (so far 5.7 and 5.8), should we
> > > > > just get that commit reverted instead?
> > > >
> > > > Can this please be resolved ASAP?
> > > >
> > > > Andreas.
> > > >
> > > > --
> > > > Andreas Schwab, [email protected]
> > > > GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> > > > "And now for something completely different."
> > >
> > >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
> > >
> > > ML: https://lore.kernel.org/linux-csky/
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> >
> >
> > --
> > Regards,
> > Atish
>
>
>
> --
> Best Regards
> Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/

--
Regards,
Atish

2020-10-07 16:07:05