| Message ID | 20250103160214.657508-1-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: misaligned: disable pagefault before accessing user memory | expand |
Hi Clément, On 03/01/2025 17:02, Clément Léger wrote: > Calling copy_{from/to}_user() in interrupt context might actually sleep > and display a BUG message: > > [ 10.377019] BUG: sleeping function called from invalid context at include/linux/uaccess.h:162 > [ 10.379868] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 88, name: ssh-keygen > [ 10.380009] preempt_count: 0, expected: 0 > [ 10.380324] CPU: 0 UID: 0 PID: 88 Comm: ssh-keygen Not tainted 6.13.0-rc5-00013-g3435cd5f1331-dirty #19 > [ 10.380639] Hardware name: riscv-virtio,qemu (DT) > [ 10.380798] Call Trace: > [ 10.381108] [<ffffffff80013d30>] dump_backtrace+0x1c/0x24 > [ 10.381690] [<ffffffff800022d8>] show_stack+0x28/0x34 > [ 10.381812] [<ffffffff8000ee1c>] dump_stack_lvl+0x4a/0x68 > [ 10.381958] [<ffffffff8000ee4e>] dump_stack+0x14/0x1c > [ 10.382047] [<ffffffff80065e0a>] __might_resched+0xfa/0x104 > [ 10.382172] [<ffffffff80065e56>] __might_sleep+0x42/0x66 > [ 10.382267] [<ffffffff801b0c5e>] __might_fault+0x1c/0x24 > [ 10.382363] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2 > [ 10.382459] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc > [ 10.382565] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee > [ 10.382714] [<ffffffff80a0dc66>] handle_exception+0x146/0x152 > > In order to safely handle user memory access from this context, disable > page fault while copying user memory. Although this might lead to copy > failure in some cases (offlined page), this is the best we can try to be > safe. > > Fixes: b686ecdeacf6 ("riscv: misaligned: Restrict user access to kernel memory") > Signed-off-by: Clément Léger <cleger@rivosinc.com> > > --- > arch/riscv/kernel/traps_misaligned.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c > index 7cc108aed74e..75a08ed20070 100644 > --- a/arch/riscv/kernel/traps_misaligned.c > +++ b/arch/riscv/kernel/traps_misaligned.c > @@ -355,7 +355,7 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs) > { > union reg_data val; > unsigned long epc = regs->epc; > - unsigned long insn; > + unsigned long insn, copy_len; > unsigned long addr = regs->badaddr; > int fp = 0, shift = 0, len = 0; > > @@ -441,7 +441,16 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs) > > val.data_u64 = 0; > if (user_mode(regs)) { > - if (copy_from_user(&val, (u8 __user *)addr, len)) > + /* > + * We can not sleep in exception context. Disable pagefault to > + * avoid a potential sleep while accessing user memory. Side > + * effect is that if it would have sleep, then the copy will > + * fail. > + */ > + pagefault_disable(); > + copy_len = copy_from_user(&val, (u8 __user *)addr, len); > + pagefault_enable(); > + if (copy_len) > return -1; > } else { > memcpy(&val, (u8 *)addr, len); > @@ -463,7 +472,7 @@ static int handle_scalar_misaligned_store(struct pt_regs *regs) > { > union reg_data val; > unsigned long epc = regs->epc; > - unsigned long insn; > + unsigned long insn, copy_len; > unsigned long addr = regs->badaddr; > int len = 0, fp = 0; > > @@ -539,7 +548,16 @@ static int handle_scalar_misaligned_store(struct pt_regs *regs) > return -EOPNOTSUPP; > > if (user_mode(regs)) { > - if (copy_to_user((u8 __user *)addr, &val, len)) > + /* > + * We can not sleep in exception context. Disable pagefault to > + * avoid a potential sleep while accessing user memory. Side > + * effect is that if it would have sleep, then the copy will > + * fail. > + */ > + pagefault_disable(); > + copy_len = copy_to_user((u8 __user *)addr, &val, len); > + pagefault_enable(); > + if (copy_len) > return -1; > } else { > memcpy((u8 *)addr, &val, len); I'm wondering why the irqs are disabled here? What prevents us from enabling them? IIUC, if for some reason the user page is not in memory, we will fail the misaligned access and kill the process, so such failures could be completely random right? But if we go for this solution, I think the same needs to be done in __read_insn(). Let me know if I misunderstood something. Thanks, Alex
On 06/01/2025 15:37, Alexandre Ghiti wrote: > Hi Clément, > > On 03/01/2025 17:02, Clément Léger wrote: >> Calling copy_{from/to}_user() in interrupt context might actually sleep >> and display a BUG message: >> >> [ 10.377019] BUG: sleeping function called from invalid context at >> include/linux/uaccess.h:162 >> [ 10.379868] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: >> 88, name: ssh-keygen >> [ 10.380009] preempt_count: 0, expected: 0 >> [ 10.380324] CPU: 0 UID: 0 PID: 88 Comm: ssh-keygen Not tainted >> 6.13.0-rc5-00013-g3435cd5f1331-dirty #19 >> [ 10.380639] Hardware name: riscv-virtio,qemu (DT) >> [ 10.380798] Call Trace: >> [ 10.381108] [<ffffffff80013d30>] dump_backtrace+0x1c/0x24 >> [ 10.381690] [<ffffffff800022d8>] show_stack+0x28/0x34 >> [ 10.381812] [<ffffffff8000ee1c>] dump_stack_lvl+0x4a/0x68 >> [ 10.381958] [<ffffffff8000ee4e>] dump_stack+0x14/0x1c >> [ 10.382047] [<ffffffff80065e0a>] __might_resched+0xfa/0x104 >> [ 10.382172] [<ffffffff80065e56>] __might_sleep+0x42/0x66 >> [ 10.382267] [<ffffffff801b0c5e>] __might_fault+0x1c/0x24 >> [ 10.382363] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2 >> [ 10.382459] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc >> [ 10.382565] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee >> [ 10.382714] [<ffffffff80a0dc66>] handle_exception+0x146/0x152 >> >> In order to safely handle user memory access from this context, disable >> page fault while copying user memory. Although this might lead to copy >> failure in some cases (offlined page), this is the best we can try to be >> safe. >> >> Fixes: b686ecdeacf6 ("riscv: misaligned: Restrict user access to >> kernel memory") >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> >> --- >> arch/riscv/kernel/traps_misaligned.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/ >> traps_misaligned.c >> index 7cc108aed74e..75a08ed20070 100644 >> --- a/arch/riscv/kernel/traps_misaligned.c >> +++ b/arch/riscv/kernel/traps_misaligned.c >> @@ -355,7 +355,7 @@ static int handle_scalar_misaligned_load(struct >> pt_regs *regs) >> { >> union reg_data val; >> unsigned long epc = regs->epc; >> - unsigned long insn; >> + unsigned long insn, copy_len; >> unsigned long addr = regs->badaddr; >> int fp = 0, shift = 0, len = 0; >> @@ -441,7 +441,16 @@ static int handle_scalar_misaligned_load(struct >> pt_regs *regs) >> val.data_u64 = 0; >> if (user_mode(regs)) { >> - if (copy_from_user(&val, (u8 __user *)addr, len)) >> + /* >> + * We can not sleep in exception context. Disable pagefault to >> + * avoid a potential sleep while accessing user memory. Side >> + * effect is that if it would have sleep, then the copy will >> + * fail. >> + */ >> + pagefault_disable(); >> + copy_len = copy_from_user(&val, (u8 __user *)addr, len); >> + pagefault_enable(); >> + if (copy_len) >> return -1; >> } else { >> memcpy(&val, (u8 *)addr, len); >> @@ -463,7 +472,7 @@ static int handle_scalar_misaligned_store(struct >> pt_regs *regs) >> { >> union reg_data val; >> unsigned long epc = regs->epc; >> - unsigned long insn; >> + unsigned long insn, copy_len; >> unsigned long addr = regs->badaddr; >> int len = 0, fp = 0; >> @@ -539,7 +548,16 @@ static int >> handle_scalar_misaligned_store(struct pt_regs *regs) >> return -EOPNOTSUPP; >> if (user_mode(regs)) { >> - if (copy_to_user((u8 __user *)addr, &val, len)) >> + /* >> + * We can not sleep in exception context. Disable pagefault to >> + * avoid a potential sleep while accessing user memory. Side >> + * effect is that if it would have sleep, then the copy will >> + * fail. >> + */ >> + pagefault_disable(); >> + copy_len = copy_to_user((u8 __user *)addr, &val, len); >> + pagefault_enable(); >> + if (copy_len) >> return -1; >> } else { >> memcpy((u8 *)addr, &val, len); > > > I'm wondering why the irqs are disabled here? What prevents us from > enabling them? IIUC, if for some reason the user page is not in memory, > we will fail the misaligned access and kill the process, so such > failures could be completely random right? Hi Alex, This code is called from interrupt context as a result from an exception. AFAIK, interrupts are disabled during the whole exception handling and re-enabled when returning to user mode in do_notify_resume(). So that's why it triggers this warning. Clément > > But if we go for this solution, I think the same needs to be done in > __read_insn(). > > Let me know if I misunderstood something. > > Thanks, > > Alex >
On 06/01/2025 15:37, Alexandre Ghiti wrote: > Hi Clément, > > On 03/01/2025 17:02, Clément Léger wrote: >> Calling copy_{from/to}_user() in interrupt context might actually sleep >> and display a BUG message: >> >> [ 10.377019] BUG: sleeping function called from invalid context at >> include/linux/uaccess.h:162 >> [ 10.379868] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: >> 88, name: ssh-keygen >> [ 10.380009] preempt_count: 0, expected: 0 >> [ 10.380324] CPU: 0 UID: 0 PID: 88 Comm: ssh-keygen Not tainted >> 6.13.0-rc5-00013-g3435cd5f1331-dirty #19 >> [ 10.380639] Hardware name: riscv-virtio,qemu (DT) >> [ 10.380798] Call Trace: >> [ 10.381108] [<ffffffff80013d30>] dump_backtrace+0x1c/0x24 >> [ 10.381690] [<ffffffff800022d8>] show_stack+0x28/0x34 >> [ 10.381812] [<ffffffff8000ee1c>] dump_stack_lvl+0x4a/0x68 >> [ 10.381958] [<ffffffff8000ee4e>] dump_stack+0x14/0x1c >> [ 10.382047] [<ffffffff80065e0a>] __might_resched+0xfa/0x104 >> [ 10.382172] [<ffffffff80065e56>] __might_sleep+0x42/0x66 >> [ 10.382267] [<ffffffff801b0c5e>] __might_fault+0x1c/0x24 >> [ 10.382363] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2 >> [ 10.382459] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc >> [ 10.382565] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee >> [ 10.382714] [<ffffffff80a0dc66>] handle_exception+0x146/0x152 >> >> In order to safely handle user memory access from this context, disable >> page fault while copying user memory. Although this might lead to copy >> failure in some cases (offlined page), this is the best we can try to be >> safe. >> >> Fixes: b686ecdeacf6 ("riscv: misaligned: Restrict user access to >> kernel memory") >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> >> --- >> arch/riscv/kernel/traps_misaligned.c | 26 ++++++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/ >> traps_misaligned.c >> index 7cc108aed74e..75a08ed20070 100644 >> --- a/arch/riscv/kernel/traps_misaligned.c >> +++ b/arch/riscv/kernel/traps_misaligned.c >> @@ -355,7 +355,7 @@ static int handle_scalar_misaligned_load(struct >> pt_regs *regs) >> { >> union reg_data val; >> unsigned long epc = regs->epc; >> - unsigned long insn; >> + unsigned long insn, copy_len; >> unsigned long addr = regs->badaddr; >> int fp = 0, shift = 0, len = 0; >> @@ -441,7 +441,16 @@ static int handle_scalar_misaligned_load(struct >> pt_regs *regs) >> val.data_u64 = 0; >> if (user_mode(regs)) { >> - if (copy_from_user(&val, (u8 __user *)addr, len)) >> + /* >> + * We can not sleep in exception context. Disable pagefault to >> + * avoid a potential sleep while accessing user memory. Side >> + * effect is that if it would have sleep, then the copy will >> + * fail. >> + */ >> + pagefault_disable(); >> + copy_len = copy_from_user(&val, (u8 __user *)addr, len); >> + pagefault_enable(); >> + if (copy_len) >> return -1; >> } else { >> memcpy(&val, (u8 *)addr, len); >> @@ -463,7 +472,7 @@ static int handle_scalar_misaligned_store(struct >> pt_regs *regs) >> { >> union reg_data val; >> unsigned long epc = regs->epc; >> - unsigned long insn; >> + unsigned long insn, copy_len; >> unsigned long addr = regs->badaddr; >> int len = 0, fp = 0; >> @@ -539,7 +548,16 @@ static int >> handle_scalar_misaligned_store(struct pt_regs *regs) >> return -EOPNOTSUPP; >> if (user_mode(regs)) { >> - if (copy_to_user((u8 __user *)addr, &val, len)) >> + /* >> + * We can not sleep in exception context. Disable pagefault to >> + * avoid a potential sleep while accessing user memory. Side >> + * effect is that if it would have sleep, then the copy will >> + * fail. >> + */ >> + pagefault_disable(); >> + copy_len = copy_to_user((u8 __user *)addr, &val, len); >> + pagefault_enable(); >> + if (copy_len) >> return -1; >> } else { >> memcpy((u8 *)addr, &val, len); > > > I'm wondering why the irqs are disabled here? What prevents us from > enabling them? IIUC, if for some reason the user page is not in memory, > we will fail the misaligned access and kill the process, so such > failures could be completely random right? > > But if we go for this solution, I think the same needs to be done in > __read_insn(). Forgot to answer to that question: Indeed, it should be modified the same way though It did not triggered any warning since in uses the "raw" access so there is no check done before accessing the memory (ie no call to might_sleep()). I'll send a V2. Thanks, Clément > > Let me know if I misunderstood something. > > Thanks, > > Alex >
On Mon, 6 Jan 2025, Clément Léger wrote: > > I'm wondering why the irqs are disabled here? What prevents us from > > enabling them? IIUC, if for some reason the user page is not in memory, > > we will fail the misaligned access and kill the process, so such > > failures could be completely random right? > > Hi Alex, > > This code is called from interrupt context as a result from an > exception. AFAIK, interrupts are disabled during the whole exception > handling and re-enabled when returning to user mode in > do_notify_resume(). So that's why it triggers this warning. This feels outright wrong. We have analogous emulation code in Alpha and MIPS ports and we stay away from quick sands by enabling interrupts in the relevant exception handlers as soon as it is safe to do so; in particular when emulating user-mode unaligned accesses. What causes the RISC-V port to be different here? See e.g. the MIPS exception dispatcher in arch/mips/kernel/genex.S where BUILD_HANDLER invocations explicitly enable interrupts for most of the handlers (via `sti' -> `__build_clear_sti'). Then unaligned emulation is run with the interrupt enable setting preserved from the context the fault happened in (via `ade' -> `__build_clear_ade'), clearly because it can in principle trigger from kernel code run with interrupts disabled. But such code is not supposed to access user memory in the first place and traps from the user mode will have had interrupts enabled. I think the RISC-V port has to do the same, i.e. run the handler with the supervisor interrupt enable bit set according to whether interrupts were enabled or not for the mode the fault has triggered in. Maciej
On 07/01/2025 03:33, Maciej W. Rozycki wrote: > On Mon, 6 Jan 2025, Clément Léger wrote: > >>> I'm wondering why the irqs are disabled here? What prevents us from >>> enabling them? IIUC, if for some reason the user page is not in memory, >>> we will fail the misaligned access and kill the process, so such >>> failures could be completely random right? >> >> Hi Alex, >> >> This code is called from interrupt context as a result from an >> exception. AFAIK, interrupts are disabled during the whole exception >> handling and re-enabled when returning to user mode in >> do_notify_resume(). So that's why it triggers this warning. > > This feels outright wrong. We have analogous emulation code in Alpha and > MIPS ports and we stay away from quick sands by enabling interrupts in the > relevant exception handlers as soon as it is safe to do so; in particular > when emulating user-mode unaligned accesses. What causes the RISC-V port > to be different here? Hi Maciej, I probably did not clarify that enough. The current path for misaligned accesses does not re-enable irqs (whatever the mode we enter from). Some other path does (page_fault) as it's indeed safe. > > See e.g. the MIPS exception dispatcher in arch/mips/kernel/genex.S where > BUILD_HANDLER invocations explicitly enable interrupts for most of the > handlers (via `sti' -> `__build_clear_sti'). Then unaligned emulation is > run with the interrupt enable setting preserved from the context the fault > happened in (via `ade' -> `__build_clear_ade'), clearly because it can in > principle trigger from kernel code run with interrupts disabled. But such > code is not supposed to access user memory in the first place and traps > from the user mode will have had interrupts enabled. > > I think the RISC-V port has to do the same, i.e. run the handler with the > supervisor interrupt enable bit set according to whether interrupts were > enabled or not for the mode the fault has triggered in. It seems like as you say, it would actually be safe to re-enable interrupts in the misaligned load/store path as well as do_trap()/do_trap_break() which might also benefit from this improvement. After reviewing, the code, there is also a bunch of irqentry_nmi_enter() that seems a bit weird, maybe they could be replaced with irqentry_enter(). Thanks for the review, Clément > > Maciej
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c index 7cc108aed74e..75a08ed20070 100644 --- a/arch/riscv/kernel/traps_misaligned.c +++ b/arch/riscv/kernel/traps_misaligned.c @@ -355,7 +355,7 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs) { union reg_data val; unsigned long epc = regs->epc; - unsigned long insn; + unsigned long insn, copy_len; unsigned long addr = regs->badaddr; int fp = 0, shift = 0, len = 0; @@ -441,7 +441,16 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs) val.data_u64 = 0; if (user_mode(regs)) { - if (copy_from_user(&val, (u8 __user *)addr, len)) + /* + * We can not sleep in exception context. Disable pagefault to + * avoid a potential sleep while accessing user memory. Side + * effect is that if it would have sleep, then the copy will + * fail. + */ + pagefault_disable(); + copy_len = copy_from_user(&val, (u8 __user *)addr, len); + pagefault_enable(); + if (copy_len) return -1; } else { memcpy(&val, (u8 *)addr, len); @@ -463,7 +472,7 @@ static int handle_scalar_misaligned_store(struct pt_regs *regs) { union reg_data val; unsigned long epc = regs->epc; - unsigned long insn; + unsigned long insn, copy_len; unsigned long addr = regs->badaddr; int len = 0, fp = 0; @@ -539,7 +548,16 @@ static int handle_scalar_misaligned_store(struct pt_regs *regs) return -EOPNOTSUPP; if (user_mode(regs)) { - if (copy_to_user((u8 __user *)addr, &val, len)) + /* + * We can not sleep in exception context. Disable pagefault to + * avoid a potential sleep while accessing user memory. Side + * effect is that if it would have sleep, then the copy will + * fail. + */ + pagefault_disable(); + copy_len = copy_to_user((u8 __user *)addr, &val, len); + pagefault_enable(); + if (copy_len) return -1; } else { memcpy((u8 *)addr, &val, len);
Calling copy_{from/to}_user() in interrupt context might actually sleep and display a BUG message: [ 10.377019] BUG: sleeping function called from invalid context at include/linux/uaccess.h:162 [ 10.379868] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 88, name: ssh-keygen [ 10.380009] preempt_count: 0, expected: 0 [ 10.380324] CPU: 0 UID: 0 PID: 88 Comm: ssh-keygen Not tainted 6.13.0-rc5-00013-g3435cd5f1331-dirty #19 [ 10.380639] Hardware name: riscv-virtio,qemu (DT) [ 10.380798] Call Trace: [ 10.381108] [<ffffffff80013d30>] dump_backtrace+0x1c/0x24 [ 10.381690] [<ffffffff800022d8>] show_stack+0x28/0x34 [ 10.381812] [<ffffffff8000ee1c>] dump_stack_lvl+0x4a/0x68 [ 10.381958] [<ffffffff8000ee4e>] dump_stack+0x14/0x1c [ 10.382047] [<ffffffff80065e0a>] __might_resched+0xfa/0x104 [ 10.382172] [<ffffffff80065e56>] __might_sleep+0x42/0x66 [ 10.382267] [<ffffffff801b0c5e>] __might_fault+0x1c/0x24 [ 10.382363] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2 [ 10.382459] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc [ 10.382565] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee [ 10.382714] [<ffffffff80a0dc66>] handle_exception+0x146/0x152 In order to safely handle user memory access from this context, disable page fault while copying user memory. Although this might lead to copy failure in some cases (offlined page), this is the best we can try to be safe. Fixes: b686ecdeacf6 ("riscv: misaligned: Restrict user access to kernel memory") Signed-off-by: Clément Léger <cleger@rivosinc.com> --- arch/riscv/kernel/traps_misaligned.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)