* [PATCH 0/2] powerpc: Disable syscall emulation and stepping @ 2022-01-24 5:57 Nicholas Piggin 2022-01-24 5:57 ` [PATCH 1/2] powerpc/64: remove system call instruction emulation Nicholas Piggin ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Nicholas Piggin @ 2022-01-24 5:57 UTC (permalink / raw) To: linuxppc-dev; +Cc: Nicholas Piggin As discussed previously https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html I'm wondering whether PPC32 should be returning -1 for syscall instructions too here? That could be done in another patch anyway. Thanks, Nick Nicholas Piggin (2): powerpc/64: remove system call instruction emulation powerpc/uprobes: Reject uprobe on a system call instruction arch/powerpc/include/asm/ppc-opcode.h | 1 + arch/powerpc/kernel/interrupt_64.S | 10 ------- arch/powerpc/kernel/uprobes.c | 6 ++++ arch/powerpc/lib/sstep.c | 42 +++++++-------------------- 4 files changed, 18 insertions(+), 41 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] powerpc/64: remove system call instruction emulation 2022-01-24 5:57 [PATCH 0/2] powerpc: Disable syscall emulation and stepping Nicholas Piggin @ 2022-01-24 5:57 ` Nicholas Piggin 2022-01-24 5:57 ` [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction Nicholas Piggin 2022-01-24 6:39 ` [PATCH 0/2] powerpc: Disable syscall emulation and stepping Christophe Leroy 2 siblings, 0 replies; 12+ messages in thread From: Nicholas Piggin @ 2022-01-24 5:57 UTC (permalink / raw) To: linuxppc-dev; +Cc: Naveen N . Rao, Nicholas Piggin emulate_step instruction emulation including sc instruction emulation initially appeared in xmon. It then emulation code was then moved into sstep.c where kprobes could use it too, and later hw_breakpoint and uprobes started to use it. Until uprobes, the only instruction emulation users were for kernel mode instructions. - xmon only steps / breaks on kernel addresses. - kprobes is kernel only. - hw_breakpoint only emulates kernel instructions, single steps user. At one point there was support for the kernel to execute sc instructions, although that is long removed and it's not clear whether there was any in-tree code. So system call emulation is not required by the above users. uprobes uses emulate_step and it appears possible to emulate sc instruction in userspace. Userspace system call emulation is broken and it's not clear it ever worked well. The big complication is that userspace takes an interrupt to the kernel to emulate the instruction. The user->kernel interrupt sets up registers and interrupt stack frame expecting to return to userspace, then system call instruction emulation re-directs that stack frame to the kernel, early in the system call interrupt handler. This means the the interrupt return code takes the kernel->kernel restore path, which does not restore everything as the system call interrupt handler would expect coming from userspace. regs->iamr appears to get lost for example, because the kernel->kernel return does not restore the user iamr. Accounting such as irqflags tracing and CPU accounting does not get flipped back to user mode as the system call handler expects, so those appear to enter the kernel twice without returning to userspace. These things may be individually fixable with various complication, but it is a big complexity for unclear real benefit. This patch removes system call emulation and disables stepping system calls (because they don't work with trace interrupts, as commented). Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/kernel/interrupt_64.S | 10 ------- arch/powerpc/lib/sstep.c | 42 ++++++++---------------------- 2 files changed, 11 insertions(+), 41 deletions(-) diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S index 7bab2d7de372..6471034c7909 100644 --- a/arch/powerpc/kernel/interrupt_64.S +++ b/arch/powerpc/kernel/interrupt_64.S @@ -219,16 +219,6 @@ system_call_vectored common 0x3000 */ system_call_vectored sigill 0x7ff0 - -/* - * Entered via kernel return set up by kernel/sstep.c, must match entry regs - */ - .globl system_call_vectored_emulate -system_call_vectored_emulate: -_ASM_NOKPROBE_SYMBOL(system_call_vectored_emulate) - li r10,IRQS_ALL_DISABLED - stb r10,PACAIRQSOFTMASK(r13) - b system_call_vectored_common #endif /* CONFIG_PPC_BOOK3S */ .balign IFETCH_ALIGN_BYTES diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index a94b0cd0bdc5..5f317b12b2db 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -15,9 +15,6 @@ #include <asm/cputable.h> #include <asm/disassemble.h> -extern char system_call_common[]; -extern char system_call_vectored_emulate[]; - #ifdef CONFIG_PPC64 /* Bits in SRR1 that are copied from MSR */ #define MSR_MASK 0xffffffff87c0ffffUL @@ -3650,39 +3647,22 @@ int emulate_step(struct pt_regs *regs, ppc_inst_t instr) goto instr_done; #ifdef CONFIG_PPC64 - case SYSCALL: /* sc */ /* - * N.B. this uses knowledge about how the syscall - * entry code works. If that is changed, this will - * need to be changed also. + * Per ISA v3.1, section 7.5.15 'Trace Interrupt', we can't + * single step a system call instruction: + * + * Successful completion for an instruction means that the + * instruction caused no other interrupt. Thus a Trace + * interrupt never occurs for a System Call or System Call + * Vectored instruction, or for a Trap instruction that + * traps. */ - if (IS_ENABLED(CONFIG_PPC_FAST_ENDIAN_SWITCH) && - cpu_has_feature(CPU_FTR_REAL_LE) && - regs->gpr[0] == 0x1ebe) { - regs_set_return_msr(regs, regs->msr ^ MSR_LE); - goto instr_done; - } - regs->gpr[9] = regs->gpr[13]; - regs->gpr[10] = MSR_KERNEL; - regs->gpr[11] = regs->nip + 4; - regs->gpr[12] = regs->msr & MSR_MASK; - regs->gpr[13] = (unsigned long) get_paca(); - regs_set_return_ip(regs, (unsigned long) &system_call_common); - regs_set_return_msr(regs, MSR_KERNEL); - return 1; - + case SYSCALL: /* sc */ + return -1; #ifdef CONFIG_PPC_BOOK3S_64 case SYSCALL_VECTORED_0: /* scv 0 */ - regs->gpr[9] = regs->gpr[13]; - regs->gpr[10] = MSR_KERNEL; - regs->gpr[11] = regs->nip + 4; - regs->gpr[12] = regs->msr & MSR_MASK; - regs->gpr[13] = (unsigned long) get_paca(); - regs_set_return_ip(regs, (unsigned long) &system_call_vectored_emulate); - regs_set_return_msr(regs, MSR_KERNEL); - return 1; + return -1; #endif - case RFI: return -1; #endif -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction 2022-01-24 5:57 [PATCH 0/2] powerpc: Disable syscall emulation and stepping Nicholas Piggin 2022-01-24 5:57 ` [PATCH 1/2] powerpc/64: remove system call instruction emulation Nicholas Piggin @ 2022-01-24 5:57 ` Nicholas Piggin 2022-01-25 11:45 ` Michael Ellerman 2022-01-24 6:39 ` [PATCH 0/2] powerpc: Disable syscall emulation and stepping Christophe Leroy 2 siblings, 1 reply; 12+ messages in thread From: Nicholas Piggin @ 2022-01-24 5:57 UTC (permalink / raw) To: linuxppc-dev; +Cc: Naveen N . Rao, Nicholas Piggin Per the ISA, a Trace interrupt is not generated for a system call [vectored] instruction. Reject uprobes on such instructions as we are not emulating a system call [vectored] instruction anymore. Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> [np: Switch to pr_info_ratelimited] Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/include/asm/ppc-opcode.h | 1 + arch/powerpc/kernel/uprobes.c | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h index 9675303b724e..8bbe16ce5173 100644 --- a/arch/powerpc/include/asm/ppc-opcode.h +++ b/arch/powerpc/include/asm/ppc-opcode.h @@ -411,6 +411,7 @@ #define PPC_RAW_DCBFPS(a, b) (0x7c0000ac | ___PPC_RA(a) | ___PPC_RB(b) | (4 << 21)) #define PPC_RAW_DCBSTPS(a, b) (0x7c0000ac | ___PPC_RA(a) | ___PPC_RB(b) | (6 << 21)) #define PPC_RAW_SC() (0x44000002) +#define PPC_RAW_SCV() (0x44000001) #define PPC_RAW_SYNC() (0x7c0004ac) #define PPC_RAW_ISYNC() (0x4c00012c) diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c index c6975467d9ff..3779fde804bd 100644 --- a/arch/powerpc/kernel/uprobes.c +++ b/arch/powerpc/kernel/uprobes.c @@ -41,6 +41,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, if (addr & 0x03) return -EINVAL; + if (ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SC() || + ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SCV()) { + pr_info_ratelimited("Rejecting uprobe on system call instruction\n"); + return -EINVAL; + } + if (cpu_has_feature(CPU_FTR_ARCH_31) && ppc_inst_prefixed(ppc_inst_read(auprobe->insn)) && (addr & 0x3f) == 60) { -- 2.23.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction 2022-01-24 5:57 ` [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction Nicholas Piggin @ 2022-01-25 11:45 ` Michael Ellerman 2022-01-27 7:44 ` Nicholas Piggin 0 siblings, 1 reply; 12+ messages in thread From: Michael Ellerman @ 2022-01-25 11:45 UTC (permalink / raw) To: Nicholas Piggin, linuxppc-dev; +Cc: Naveen N . Rao, Nicholas Piggin Nicholas Piggin <npiggin@gmail.com> writes: > Per the ISA, a Trace interrupt is not generated for a system call > [vectored] instruction. Reject uprobes on such instructions as we are > not emulating a system call [vectored] instruction anymore. This should really be patch 1, otherwise there's a single commit window where we allow uprobes on sc but don't honour them. > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > [np: Switch to pr_info_ratelimited] > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/include/asm/ppc-opcode.h | 1 + > arch/powerpc/kernel/uprobes.c | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h > index 9675303b724e..8bbe16ce5173 100644 > --- a/arch/powerpc/include/asm/ppc-opcode.h > +++ b/arch/powerpc/include/asm/ppc-opcode.h > @@ -411,6 +411,7 @@ > #define PPC_RAW_DCBFPS(a, b) (0x7c0000ac | ___PPC_RA(a) | ___PPC_RB(b) | (4 << 21)) > #define PPC_RAW_DCBSTPS(a, b) (0x7c0000ac | ___PPC_RA(a) | ___PPC_RB(b) | (6 << 21)) > #define PPC_RAW_SC() (0x44000002) > +#define PPC_RAW_SCV() (0x44000001) > #define PPC_RAW_SYNC() (0x7c0004ac) > #define PPC_RAW_ISYNC() (0x4c00012c) > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index c6975467d9ff..3779fde804bd 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -41,6 +41,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > if (addr & 0x03) > return -EINVAL; > > + if (ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SC() || > + ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SCV()) { We should probably reject hypercall too? There's also a lot of reserved fields in `sc`, so doing an exact match like this risks missing instructions that are badly formed but the CPU will happily execute as `sc`. We'd obviously never expect to see those in compiler generated code, but it'd still be safer to mask. We could probably just reject opcode 17 entirely. And I guess for a subsequent patch, but we should be rejecting some others here as well shouldn't we? Like rfid etc. cheers > + pr_info_ratelimited("Rejecting uprobe on system call instruction\n"); > + return -EINVAL; > + } > + > if (cpu_has_feature(CPU_FTR_ARCH_31) && > ppc_inst_prefixed(ppc_inst_read(auprobe->insn)) && > (addr & 0x3f) == 60) { > -- > 2.23.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction 2022-01-25 11:45 ` Michael Ellerman @ 2022-01-27 7:44 ` Nicholas Piggin 2022-01-28 11:30 ` Naveen N. Rao 0 siblings, 1 reply; 12+ messages in thread From: Nicholas Piggin @ 2022-01-27 7:44 UTC (permalink / raw) To: linuxppc-dev, Michael Ellerman; +Cc: Naveen N . Rao Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm: > Nicholas Piggin <npiggin@gmail.com> writes: >> Per the ISA, a Trace interrupt is not generated for a system call >> [vectored] instruction. Reject uprobes on such instructions as we are >> not emulating a system call [vectored] instruction anymore. > > This should really be patch 1, otherwise there's a single commit window > where we allow uprobes on sc but don't honour them. Yep true. I also messed up Naveen's attribution! Will re-send (or maybe Naveen would take over the series). > >> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >> [np: Switch to pr_info_ratelimited] >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> arch/powerpc/include/asm/ppc-opcode.h | 1 + >> arch/powerpc/kernel/uprobes.c | 6 ++++++ >> 2 files changed, 7 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h >> index 9675303b724e..8bbe16ce5173 100644 >> --- a/arch/powerpc/include/asm/ppc-opcode.h >> +++ b/arch/powerpc/include/asm/ppc-opcode.h >> @@ -411,6 +411,7 @@ >> #define PPC_RAW_DCBFPS(a, b) (0x7c0000ac | ___PPC_RA(a) | ___PPC_RB(b) | (4 << 21)) >> #define PPC_RAW_DCBSTPS(a, b) (0x7c0000ac | ___PPC_RA(a) | ___PPC_RB(b) | (6 << 21)) >> #define PPC_RAW_SC() (0x44000002) >> +#define PPC_RAW_SCV() (0x44000001) >> #define PPC_RAW_SYNC() (0x7c0004ac) >> #define PPC_RAW_ISYNC() (0x4c00012c) >> >> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c >> index c6975467d9ff..3779fde804bd 100644 >> --- a/arch/powerpc/kernel/uprobes.c >> +++ b/arch/powerpc/kernel/uprobes.c >> @@ -41,6 +41,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, >> if (addr & 0x03) >> return -EINVAL; >> >> + if (ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SC() || >> + ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SCV()) { > > We should probably reject hypercall too? > > There's also a lot of reserved fields in `sc`, so doing an exact match > like this risks missing instructions that are badly formed but the CPU > will happily execute as `sc`. Yeah, scv as well has lev != 0 unsupported so should be excluded. > > We'd obviously never expect to see those in compiler generated code, but > it'd still be safer to mask. We could probably just reject opcode 17 > entirely. > > And I guess for a subsequent patch, but we should be rejecting some > others here as well shouldn't we? Like rfid etc. Traps under discussion I guess. For uprobe, rfid will be just another privilege fault. Is that dealt with somehow or do all privileged and illegal instructions also need to be excluded from stepping? (I assume we must handle that in a general way somehow) Thanks, Nick ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction 2022-01-27 7:44 ` Nicholas Piggin @ 2022-01-28 11:30 ` Naveen N. Rao 0 siblings, 0 replies; 12+ messages in thread From: Naveen N. Rao @ 2022-01-28 11:30 UTC (permalink / raw) To: Nicholas Piggin; +Cc: linuxppc-dev On 2022-01-27 13:14, Nicholas Piggin wrote: > Excerpts from Michael Ellerman's message of January 25, 2022 9:45 pm: >> Nicholas Piggin <npiggin@gmail.com> writes: >>> Per the ISA, a Trace interrupt is not generated for a system call >>> [vectored] instruction. Reject uprobes on such instructions as we are >>> not emulating a system call [vectored] instruction anymore. >> >> This should really be patch 1, otherwise there's a single commit >> window >> where we allow uprobes on sc but don't honour them. > > Yep true. I also messed up Naveen's attribution! Will re-send (or maybe > Naveen would take over the series). Yes, let me come up with a better, more complete patch for this. > >> >>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> >>> [np: Switch to pr_info_ratelimited] >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> arch/powerpc/include/asm/ppc-opcode.h | 1 + >>> arch/powerpc/kernel/uprobes.c | 6 ++++++ >>> 2 files changed, 7 insertions(+) >>> >>> diff --git a/arch/powerpc/include/asm/ppc-opcode.h >>> b/arch/powerpc/include/asm/ppc-opcode.h >>> index 9675303b724e..8bbe16ce5173 100644 >>> --- a/arch/powerpc/include/asm/ppc-opcode.h >>> +++ b/arch/powerpc/include/asm/ppc-opcode.h >>> @@ -411,6 +411,7 @@ >>> #define PPC_RAW_DCBFPS(a, b) (0x7c0000ac | ___PPC_RA(a) | >>> ___PPC_RB(b) | (4 << 21)) >>> #define PPC_RAW_DCBSTPS(a, b) (0x7c0000ac | ___PPC_RA(a) | >>> ___PPC_RB(b) | (6 << 21)) >>> #define PPC_RAW_SC() (0x44000002) >>> +#define PPC_RAW_SCV() (0x44000001) >>> #define PPC_RAW_SYNC() (0x7c0004ac) >>> #define PPC_RAW_ISYNC() (0x4c00012c) >>> >>> diff --git a/arch/powerpc/kernel/uprobes.c >>> b/arch/powerpc/kernel/uprobes.c >>> index c6975467d9ff..3779fde804bd 100644 >>> --- a/arch/powerpc/kernel/uprobes.c >>> +++ b/arch/powerpc/kernel/uprobes.c >>> @@ -41,6 +41,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe >>> *auprobe, >>> if (addr & 0x03) >>> return -EINVAL; >>> >>> + if (ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SC() || >>> + ppc_inst_val(ppc_inst_read(auprobe->insn)) == PPC_RAW_SCV()) { >> >> We should probably reject hypercall too? >> >> There's also a lot of reserved fields in `sc`, so doing an exact match >> like this risks missing instructions that are badly formed but the CPU >> will happily execute as `sc`. > > Yeah, scv as well has lev != 0 unsupported so should be excluded. >> >> We'd obviously never expect to see those in compiler generated code, >> but >> it'd still be safer to mask. We could probably just reject opcode 17 >> entirely. Indeed, thanks. >> >> And I guess for a subsequent patch, but we should be rejecting some >> others here as well shouldn't we? Like rfid etc. > > Traps under discussion I guess. For uprobe, rfid will be just another > privilege fault. Is that dealt with somehow or do all privileged and > illegal instructions also need to be excluded from stepping? (I assume > we must handle that in a general way somehow) Yes, this is all handled in our interrupt code if we emulate any of those privileged instructions. Otherwise, if a signal is generated, that would be caught by uprobe_deny_signal(). Thanks, Naveen ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping 2022-01-24 5:57 [PATCH 0/2] powerpc: Disable syscall emulation and stepping Nicholas Piggin 2022-01-24 5:57 ` [PATCH 1/2] powerpc/64: remove system call instruction emulation Nicholas Piggin 2022-01-24 5:57 ` [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction Nicholas Piggin @ 2022-01-24 6:39 ` Christophe Leroy 2022-01-25 3:04 ` Nicholas Piggin 2 siblings, 1 reply; 12+ messages in thread From: Christophe Leroy @ 2022-01-24 6:39 UTC (permalink / raw) To: Nicholas Piggin, linuxppc-dev Le 24/01/2022 à 06:57, Nicholas Piggin a écrit : > As discussed previously > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html > > I'm wondering whether PPC32 should be returning -1 for syscall > instructions too here? That could be done in another patch anyway. > The 'Programming Environments Manual for 32-Bit Implementations of the PowerPC™ Architecture' says: The following are not traced: • rfi instruction • sc and trap instructions that trap • Other instructions that cause interrupts (other than trace interrupts) • The first instruction of any interrupt handler • Instructions that are emulated by software So I think PPC32 should return -1 as well. Christophe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping 2022-01-24 6:39 ` [PATCH 0/2] powerpc: Disable syscall emulation and stepping Christophe Leroy @ 2022-01-25 3:04 ` Nicholas Piggin 2022-01-25 5:53 ` Christophe Leroy 0 siblings, 1 reply; 12+ messages in thread From: Nicholas Piggin @ 2022-01-25 3:04 UTC (permalink / raw) To: Christophe Leroy, linuxppc-dev; +Cc: Naveen N. Rao +Naveen (sorry missed cc'ing you at first) Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm: > > > Le 24/01/2022 à 06:57, Nicholas Piggin a écrit : >> As discussed previously >> >> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html >> >> I'm wondering whether PPC32 should be returning -1 for syscall >> instructions too here? That could be done in another patch anyway. >> > > The 'Programming Environments Manual for 32-Bit Implementations of the > PowerPC™ Architecture' says: > > The following are not traced: > • rfi instruction > • sc and trap instructions that trap > • Other instructions that cause interrupts (other than trace interrupts) > • The first instruction of any interrupt handler > • Instructions that are emulated by software > > > So I think PPC32 should return -1 as well. I agree. What about the trap instructions? analyse_instr returns 0 for them which falls through to return 0 for emulate_step, should they return -1 as well or am I missing something? Thanks, Nick ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping 2022-01-25 3:04 ` Nicholas Piggin @ 2022-01-25 5:53 ` Christophe Leroy [not found] ` <52b03748fdeff1bb2eb67f6038311e26@imap.linux.ibm.com> 2022-01-28 11:11 ` Naveen N. Rao 0 siblings, 2 replies; 12+ messages in thread From: Christophe Leroy @ 2022-01-25 5:53 UTC (permalink / raw) To: Nicholas Piggin, linuxppc-dev; +Cc: Naveen N. Rao Le 25/01/2022 à 04:04, Nicholas Piggin a écrit : > +Naveen (sorry missed cc'ing you at first) > > Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm: >> >> >> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit : >>> As discussed previously >>> >>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html >>> >>> I'm wondering whether PPC32 should be returning -1 for syscall >>> instructions too here? That could be done in another patch anyway. >>> >> >> The 'Programming Environments Manual for 32-Bit Implementations of the >> PowerPC™ Architecture' says: >> >> The following are not traced: >> • rfi instruction >> • sc and trap instructions that trap >> • Other instructions that cause interrupts (other than trace interrupts) >> • The first instruction of any interrupt handler >> • Instructions that are emulated by software >> >> >> So I think PPC32 should return -1 as well. > > I agree. > > What about the trap instructions? analyse_instr returns 0 for them > which falls through to return 0 for emulate_step, should they > return -1 as well or am I missing something? > For the traps I don't know. The manual says "trap instructions that trap" are not traced. It means that "trap instructions that _don't_ trap" are traced. Taking into account that trap instructions don't trap at least 99.9% of the time, not sure if returning -1 is needed. Allthought that'd probably be the safest. But then what happens with other instruction that will sparsely generate an exception like a DSI or so ? If we do it for the traps then we should do it for this as well, and then it becomes a non ending story. So at the end it's probably ok with return 0, both for them and for traps. Christophe ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <52b03748fdeff1bb2eb67f6038311e26@imap.linux.ibm.com>]
* Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping [not found] ` <52b03748fdeff1bb2eb67f6038311e26@imap.linux.ibm.com> @ 2022-01-27 7:39 ` Nicholas Piggin 2022-01-28 11:15 ` Naveen N. Rao 0 siblings, 1 reply; 12+ messages in thread From: Nicholas Piggin @ 2022-01-27 7:39 UTC (permalink / raw) To: Christophe Leroy, naverao1; +Cc: Naveen N. Rao, linuxppc-dev Excerpts from naverao1's message of January 25, 2022 8:48 pm: > On 2022-01-25 11:23, Christophe Leroy wrote: >> Le 25/01/2022 à 04:04, Nicholas Piggin a écrit : >>> +Naveen (sorry missed cc'ing you at first) >>> >>> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm: >>>> >>>> >>>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit : >>>>> As discussed previously >>>>> >>>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html >>>>> >>>>> I'm wondering whether PPC32 should be returning -1 for syscall >>>>> instructions too here? That could be done in another patch anyway. >>>>> >>>> >>>> The 'Programming Environments Manual for 32-Bit Implementations of >>>> the >>>> PowerPC™ Architecture' says: >>>> >>>> The following are not traced: >>>> • rfi instruction >>>> • sc and trap instructions that trap >>>> • Other instructions that cause interrupts (other than trace >>>> interrupts) >>>> • The first instruction of any interrupt handler >>>> • Instructions that are emulated by software >>>> >>>> >>>> So I think PPC32 should return -1 as well. >>> >>> I agree. >>> >>> What about the trap instructions? analyse_instr returns 0 for them >>> which falls through to return 0 for emulate_step, should they >>> return -1 as well or am I missing something? > > Yeah, good point about the trap instructions. > >>> >> >> For the traps I don't know. The manual says "trap instructions that >> trap" are not traced. It means that "trap instructions that _don't_ >> trap" are traced. Taking into account that trap instructions don't trap >> at least 99.9% of the time, not sure if returning -1 is needed. >> >> Allthought that'd probably be the safest. > > 'trap' is a special case since it is predominantly used by debuggers > and/or tracing infrastructure. Kprobes and Uprobes do not allow probes > on a trap instruction. But, xmon can be asked to step on a trap > instruction and that can interfere with kprobes in weird ways. > > So, I think it is best if we also exclude trap instructions from being > single stepped. > >> >> But then what happens with other instruction that will sparsely >> generate >> an exception like a DSI or so ? If we do it for the traps then we >> should >> do it for this as well, and then it becomes a non ending story. > > For a DSI, we restart the same instruction after handling the page > fault. > The single step exception is raised on the subsequent successful > completion of the instruction. Although it can cause a signal, and the signal handler can decide to resume somewhere else. Or kernel mode equivalent it can go to a fixup handler and resume somewhere else. How are those handled? Thanks, Nick > For most other interrupts (alignment, vsx > unavailable, ...), we end up emulating the single step exception itself > (see emulate_single_step()). So, those are ok if caused by an > instruction > being stepped. > > > - Naveen > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping 2022-01-27 7:39 ` Nicholas Piggin @ 2022-01-28 11:15 ` Naveen N. Rao 0 siblings, 0 replies; 12+ messages in thread From: Naveen N. Rao @ 2022-01-28 11:15 UTC (permalink / raw) To: Nicholas Piggin; +Cc: linuxppc-dev On 2022-01-27 13:09, Nicholas Piggin wrote: > Excerpts from naverao1's message of January 25, 2022 8:48 pm: >> On 2022-01-25 11:23, Christophe Leroy wrote: >>> Le 25/01/2022 à 04:04, Nicholas Piggin a écrit : >>>> +Naveen (sorry missed cc'ing you at first) >>>> >>>> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 >>>> pm: >>>>> >>>>> >>>>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit : >>>>>> As discussed previously >>>>>> >>>>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html >>>>>> >>>>>> I'm wondering whether PPC32 should be returning -1 for syscall >>>>>> instructions too here? That could be done in another patch anyway. >>>>>> >>>>> >>>>> The 'Programming Environments Manual for 32-Bit Implementations of >>>>> the >>>>> PowerPC™ Architecture' says: >>>>> >>>>> The following are not traced: >>>>> • rfi instruction >>>>> • sc and trap instructions that trap >>>>> • Other instructions that cause interrupts (other than trace >>>>> interrupts) >>>>> • The first instruction of any interrupt handler >>>>> • Instructions that are emulated by software >>>>> >>>>> >>>>> So I think PPC32 should return -1 as well. >>>> >>>> I agree. >>>> >>>> What about the trap instructions? analyse_instr returns 0 for them >>>> which falls through to return 0 for emulate_step, should they >>>> return -1 as well or am I missing something? >> >> Yeah, good point about the trap instructions. >> >>>> >>> >>> For the traps I don't know. The manual says "trap instructions that >>> trap" are not traced. It means that "trap instructions that _don't_ >>> trap" are traced. Taking into account that trap instructions don't >>> trap >>> at least 99.9% of the time, not sure if returning -1 is needed. >>> >>> Allthought that'd probably be the safest. >> >> 'trap' is a special case since it is predominantly used by debuggers >> and/or tracing infrastructure. Kprobes and Uprobes do not allow probes >> on a trap instruction. But, xmon can be asked to step on a trap >> instruction and that can interfere with kprobes in weird ways. >> >> So, I think it is best if we also exclude trap instructions from being >> single stepped. >> >>> >>> But then what happens with other instruction that will sparsely >>> generate >>> an exception like a DSI or so ? If we do it for the traps then we >>> should >>> do it for this as well, and then it becomes a non ending story. >> >> For a DSI, we restart the same instruction after handling the page >> fault. >> The single step exception is raised on the subsequent successful >> completion of the instruction. > > Although it can cause a signal, and the signal handler can decide > to resume somewhere else. If a signal is generated while we are single-stepping, we delay signal delivery (see uprobe_deny_signal()) until after the single stepping. For fatal signals, single stepping is disabled before we allow the signal to be delivered. > Or kernel mode equivalent it can go to a > fixup handler and resume somewhere else. For kprobes, we do not allow probing instructions that have an extable entry. - Naveen ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/2] powerpc: Disable syscall emulation and stepping 2022-01-25 5:53 ` Christophe Leroy [not found] ` <52b03748fdeff1bb2eb67f6038311e26@imap.linux.ibm.com> @ 2022-01-28 11:11 ` Naveen N. Rao 1 sibling, 0 replies; 12+ messages in thread From: Naveen N. Rao @ 2022-01-28 11:11 UTC (permalink / raw) To: Christophe Leroy, Nicholas Piggin; +Cc: linuxppc-dev [Sorry if you receive this in duplicate. Resending since this message didn't hit the list] On 2022-01-25 11:23, Christophe Leroy wrote: > Le 25/01/2022 à 04:04, Nicholas Piggin a écrit : >> +Naveen (sorry missed cc'ing you at first) >> >> Excerpts from Christophe Leroy's message of January 24, 2022 4:39 pm: >>> >>> >>> Le 24/01/2022 à 06:57, Nicholas Piggin a écrit : >>>> As discussed previously >>>> >>>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-January/238946.html >>>> >>>> I'm wondering whether PPC32 should be returning -1 for syscall >>>> instructions too here? That could be done in another patch anyway. >>>> >>> >>> The 'Programming Environments Manual for 32-Bit Implementations of >>> the >>> PowerPC™ Architecture' says: >>> >>> The following are not traced: >>> • rfi instruction >>> • sc and trap instructions that trap >>> • Other instructions that cause interrupts (other than trace >>> interrupts) >>> • The first instruction of any interrupt handler >>> • Instructions that are emulated by software >>> >>> >>> So I think PPC32 should return -1 as well. >> >> I agree. >> >> What about the trap instructions? analyse_instr returns 0 for them >> which falls through to return 0 for emulate_step, should they >> return -1 as well or am I missing something? Yeah, good point about the trap instructions. >> > > For the traps I don't know. The manual says "trap instructions that > trap" are not traced. It means that "trap instructions that _don't_ > trap" are traced. Taking into account that trap instructions don't trap > at least 99.9% of the time, not sure if returning -1 is needed. > > Allthought that'd probably be the safest. 'trap' is a special case since it is predominantly used by debuggers and/or tracing infrastructure. Kprobes and Uprobes do not allow probes on a trap instruction. But, xmon can be asked to step on a trap instruction and that can interfere with kprobes in weird ways. So, I think it is best if we also exclude trap instructions from being single stepped. > > But then what happens with other instruction that will sparsely > generate > an exception like a DSI or so ? If we do it for the traps then we > should > do it for this as well, and then it becomes a non ending story. For a DSI, we restart the same instruction after handling the page fault. The single step exception is raised on the subsequent successful completion of the instruction. For most other interrupts (alignment, vsx unavailable, ...), we end up emulating the single step exception itself (see emulate_single_step()). So, those are ok if caused by an instruction being stepped. - Naveen ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-01-28 11:31 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-24 5:57 [PATCH 0/2] powerpc: Disable syscall emulation and stepping Nicholas Piggin 2022-01-24 5:57 ` [PATCH 1/2] powerpc/64: remove system call instruction emulation Nicholas Piggin 2022-01-24 5:57 ` [PATCH 2/2] powerpc/uprobes: Reject uprobe on a system call instruction Nicholas Piggin 2022-01-25 11:45 ` Michael Ellerman 2022-01-27 7:44 ` Nicholas Piggin 2022-01-28 11:30 ` Naveen N. Rao 2022-01-24 6:39 ` [PATCH 0/2] powerpc: Disable syscall emulation and stepping Christophe Leroy 2022-01-25 3:04 ` Nicholas Piggin 2022-01-25 5:53 ` Christophe Leroy [not found] ` <52b03748fdeff1bb2eb67f6038311e26@imap.linux.ibm.com> 2022-01-27 7:39 ` Nicholas Piggin 2022-01-28 11:15 ` Naveen N. Rao 2022-01-28 11:11 ` Naveen N. Rao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).