Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, May 01, 2019 at 09:33:28PM +0200, Jiri Kosina wrote: > On Wed, 1 May 2019, Steven Rostedt wrote: > > > I never tested the 32 bit version of this. And we could just not > > implement it (I don't think there's live kernel patching for it > > either). > > That's correct, there is no livepatching on x86_32 (and no plans for > it). CONFIG_LIVEPATCH is not available for 32bit builds at all. We still want this for static_call(), even on 32bit.
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, 1 May 2019, Steven Rostedt wrote: > I never tested the 32 bit version of this. And we could just not > implement it (I don't think there's live kernel patching for it > either). That's correct, there is no livepatching on x86_32 (and no plans for it). CONFIG_LIVEPATCH is not available for 32bit builds at all. -- Jiri Kosina SUSE Labs
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, May 01, 2019 at 12:03:52PM -0700, Linus Torvalds wrote: > On Wed, May 1, 2019 at 6:11 AM Peter Zijlstra wrote: > > > > Here goes, compile tested only... > > Ugh, two different threads. This has the same bug (same source) as the > one Steven posted: This is what Steve started from; lets continue in the other thread. > > --- a/arch/x86/entry/entry_32.S > > +++ b/arch/x86/entry/entry_32.S > > @@ -1479,6 +1479,13 @@ ENTRY(int3) > > ASM_CLAC > > pushl $-1 # mark this as an int > > > > + testl $SEGMENT_RPL_MASK, PT_CS(%esp) > > + jnz .Lfrom_usermode_no_gap > > + .rept 6 > > + pushl 5*4(%esp) > > + .endr > > +.Lfrom_usermode_no_gap: > > This will corrupt things horribly if you still use vm86 mode. Checking > CS RPL is simply not correct. I'll go fix; I never really understood that vm86 crud and I cobbled this 32bit thing together based on the 64bit version (that Josh did a while ago).
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, 1 May 2019 12:03:52 -0700 Linus Torvalds wrote: > On Wed, May 1, 2019 at 6:11 AM Peter Zijlstra wrote: > > > > Here goes, compile tested only... > > Ugh, two different threads. This has the same bug (same source) as the > one Steven posted: > > > --- a/arch/x86/entry/entry_32.S > > +++ b/arch/x86/entry/entry_32.S > > @@ -1479,6 +1479,13 @@ ENTRY(int3) > > ASM_CLAC > > pushl $-1 # mark this as an int > > > > + testl $SEGMENT_RPL_MASK, PT_CS(%esp) > > + jnz .Lfrom_usermode_no_gap > > + .rept 6 > > + pushl 5*4(%esp) > > + .endr > > +.Lfrom_usermode_no_gap: > > This will corrupt things horribly if you still use vm86 mode. Checking > CS RPL is simply not correct. I never tested the 32 bit version of this. And we could just not implement it (I don't think there's live kernel patching for it either). But this doesn't make it any worse than my version, because under the full testing of my patch with the trampolines, I would easily crash the 32 bit version. That was one reason I made my last patch only support 64 bit. Under light load, 32 bit works, but when I stress it (running perf and ftrace together) it blows up. Could be an NMI issue. -- Steve
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, May 1, 2019 at 6:11 AM Peter Zijlstra wrote: > > Here goes, compile tested only... Ugh, two different threads. This has the same bug (same source) as the one Steven posted: > --- a/arch/x86/entry/entry_32.S > +++ b/arch/x86/entry/entry_32.S > @@ -1479,6 +1479,13 @@ ENTRY(int3) > ASM_CLAC > pushl $-1 # mark this as an int > > + testl $SEGMENT_RPL_MASK, PT_CS(%esp) > + jnz .Lfrom_usermode_no_gap > + .rept 6 > + pushl 5*4(%esp) > + .endr > +.Lfrom_usermode_no_gap: This will corrupt things horribly if you still use vm86 mode. Checking CS RPL is simply not correct. Linus
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, May 01, 2019 at 02:58:24PM -0400, Steven Rostedt wrote:
> > + if (ftrace_location(ip)) {
> > + int3_emulate_call(regs, ftrace_update_func_call);
>
> Should be:
>
> int3_emulate_call(regs, (unsigned long)ftrace_regs_caller);
Ah, I lost the plot a little there.
> > + return 1;
> > + } else if (is_ftrace_caller(ip)) {
> > + if (!ftrace_update_func_call) {
> > + int3_emulate_jmp(regs, regs->ip - INT3_INSN_SIZE +
> > CALL_INSN_SIZE);
>
> I see what you did here, but I think:
>
> int3_emulate_jmp(regs, ip + CALL_INSN_SIZE);
>
> looks better. But that said, we could in the beginning do:
>
> ip = regs->ip - INT3_INSN_SIZE;
>
> instead of
>
> ip = regs->ip - 1;
>
> I made these updates and posted them to Linus.
I was actually considering:
static inline void int3_emulate_nop(struct pt_regs *regs, unsigned long size)
{
int3_emulate_jmp(regs, regs->ip - INT3_INSN_SIZE + size);
}
And then the above becomes:
int3_emulate_nop(regs, CALL_INSN_SIZE);
Hmm?
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Wed, 1 May 2019 15:11:17 +0200
Peter Zijlstra wrote:
> On Tue, Apr 30, 2019 at 11:33:21AM -0700, Linus Torvalds wrote:
> > Anyway, since Andy really likes the entry code change, can we have
> > that patch in parallel and judge the difference that way? Iirc, that
> > was x86-64 specific too.
>
> Here goes, compile tested only...
>
> It obviously needs a self-test, but that shoulnd't be too hard to
> arrange.
>
I was able to get it applied (with slight tweaking) but it then
crashed. But that was due to incorrect updates in the
ftrace_int3_handler().
> ---
> arch/x86/entry/entry_32.S| 7 +++
> arch/x86/entry/entry_64.S| 14 --
> arch/x86/include/asm/text-patching.h | 20
> arch/x86/kernel/ftrace.c | 24 +++-
> 4 files changed, 58 insertions(+), 7 deletions(-)
> #endif /* _ASM_X86_TEXT_PATCHING_H */
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index ef49517f6bb2..90d319687d7e 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #ifdef CONFIG_DYNAMIC_FTRACE
>
> @@ -231,6 +232,7 @@ int ftrace_modify_call(struct dyn_ftrace *rec,
> unsigned long old_addr, }
>
> static unsigned long ftrace_update_func;
> +static unsigned long ftrace_update_func_call;
>
> static int update_ftrace_func(unsigned long ip, void *new)
> {
> @@ -259,6 +261,8 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
> unsigned char *new;
> int ret;
>
> + ftrace_update_func_call = (unsigned long)func;
> +
> new = ftrace_call_replace(ip, (unsigned long)func);
> ret = update_ftrace_func(ip, new);
>
> @@ -295,12 +299,19 @@ int ftrace_int3_handler(struct pt_regs *regs)
> return 0;
>
> ip = regs->ip - 1;
> - if (!ftrace_location(ip) && !is_ftrace_caller(ip))
> - return 0;
> -
> - regs->ip += MCOUNT_INSN_SIZE - 1;
> + if (ftrace_location(ip)) {
> + int3_emulate_call(regs, ftrace_update_func_call);
Should be:
int3_emulate_call(regs, (unsigned long)ftrace_regs_caller);
> + return 1;
> + } else if (is_ftrace_caller(ip)) {
> + if (!ftrace_update_func_call) {
> + int3_emulate_jmp(regs, regs->ip - INT3_INSN_SIZE +
> CALL_INSN_SIZE);
I see what you did here, but I think:
int3_emulate_jmp(regs, ip + CALL_INSN_SIZE);
looks better. But that said, we could in the beginning do:
ip = regs->ip - INT3_INSN_SIZE;
instead of
ip = regs->ip - 1;
I made these updates and posted them to Linus.
-- Steve
> + return 1;
> + }
> + int3_emulate_call(regs, ftrace_update_func_call);
> + return 1;
> + }
>
> - return 1;
> + return 0;
> }
> NOKPROBE_SYMBOL(ftrace_int3_handler);
>
> @@ -859,6 +870,8 @@ void arch_ftrace_update_trampoline(struct
> ftrace_ops *ops)
> func = ftrace_ops_get_func(ops);
>
> + ftrace_update_func_call = (unsigned long)func;
> +
> /* Do a safe modify in case the trampoline is executing */
> new = ftrace_call_replace(ip, (unsigned long)func);
> ret = update_ftrace_func(ip, new);
> @@ -960,6 +973,7 @@ static int ftrace_mod_jmp(unsigned long ip, void
> *func) {
> unsigned char *new;
>
> + ftrace_update_func_call = 0UL;
> new = ftrace_jmp_replace(ip, (unsigned long)func);
>
> return update_ftrace_func(ip, new);
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Tue, Apr 30, 2019 at 11:33:21AM -0700, Linus Torvalds wrote:
> Anyway, since Andy really likes the entry code change, can we have
> that patch in parallel and judge the difference that way? Iirc, that
> was x86-64 specific too.
Here goes, compile tested only...
It obviously needs a self-test, but that shoulnd't be too hard to
arrange.
---
arch/x86/entry/entry_32.S| 7 +++
arch/x86/entry/entry_64.S| 14 --
arch/x86/include/asm/text-patching.h | 20
arch/x86/kernel/ftrace.c | 24 +++-
4 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 7b23431be5cb..d246302085a3 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -1479,6 +1479,13 @@ ENTRY(int3)
ASM_CLAC
pushl $-1 # mark this as an int
+ testl $SEGMENT_RPL_MASK, PT_CS(%esp)
+ jnz .Lfrom_usermode_no_gap
+ .rept 6
+ pushl 5*4(%esp)
+ .endr
+.Lfrom_usermode_no_gap:
+
SAVE_ALL switch_stacks=1
ENCODE_FRAME_POINTER
TRACE_IRQS_OFF
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 20e45d9b4e15..268cd9affe04 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -878,7 +878,7 @@ apicinterrupt IRQ_WORK_VECTOR
irq_work_interrupt smp_irq_work_interrupt
* @paranoid == 2 is special: the stub will never switch stacks. This is for
* #DF: if the thread stack is somehow unusable, we'll still get a useful OOPS.
*/
-.macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1
ist_offset=0
+.macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1
ist_offset=0 create_gap=0
ENTRY(\sym)
UNWIND_HINT_IRET_REGS offset=\has_error_code*8
@@ -898,6 +898,16 @@ ENTRY(\sym)
jnz .Lfrom_usermode_switch_stack_\@
.endif
+ .if \create_gap == 1
+ testb $3, CS-ORIG_RAX(%rsp)
+ jnz .Lfrom_usermode_no_gap_\@
+ .rept 6
+ pushq 5*8(%rsp)
+ .endr
+ UNWIND_HINT_IRET_REGS offset=8
+.Lfrom_usermode_no_gap_\@:
+ .endif
+
.if \paranoid
callparanoid_entry
.else
@@ -1129,7 +1139,7 @@ apicinterrupt3 HYPERV_STIMER0_VECTOR \
#endif /* CONFIG_HYPERV */
idtentry debug do_debughas_error_code=0
paranoid=1 shift_ist=IST_INDEX_DB ist_offset=DB_STACK_OFFSET
-idtentry int3 do_int3 has_error_code=0
+idtentry int3 do_int3 has_error_code=0
create_gap=1
idtentry stack_segment do_stack_segmenthas_error_code=1
#ifdef CONFIG_XEN_PV
diff --git a/arch/x86/include/asm/text-patching.h
b/arch/x86/include/asm/text-patching.h
index e85ff65c43c3..ba275b6292db 100644
--- a/arch/x86/include/asm/text-patching.h
+++ b/arch/x86/include/asm/text-patching.h
@@ -39,4 +39,24 @@ extern int poke_int3_handler(struct pt_regs *regs);
extern void *text_poke_bp(void *addr, const void *opcode, size_t len, void
*handler);
extern int after_bootmem;
+static inline void int3_emulate_push(struct pt_regs *regs, unsigned long val)
+{
+ regs->sp -= sizeof(unsigned long);
+ *(unsigned long *)regs->sp = val;
+}
+
+static inline void int3_emulate_jmp(struct pt_regs *regs, unsigned long ip)
+{
+ regs->ip = ip;
+}
+
+#define INT3_INSN_SIZE 1
+#define CALL_INSN_SIZE 5
+
+static inline void int3_emulate_call(struct pt_regs *regs, unsigned long func)
+{
+ int3_emulate_push(regs, regs->ip - INT3_INSN_SIZE + CALL_INSN_SIZE);
+ int3_emulate_jmp(regs, func);
+}
+
#endif /* _ASM_X86_TEXT_PATCHING_H */
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index ef49517f6bb2..90d319687d7e 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_DYNAMIC_FTRACE
@@ -231,6 +232,7 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned
long old_addr,
}
static unsigned long ftrace_update_func;
+static unsigned long ftrace_update_func_call;
static int update_ftrace_func(unsigned long ip, void *new)
{
@@ -259,6 +261,8 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
unsigned char *new;
int ret;
+ ftrace_update_func_call = (unsigned long)func;
+
new = ftrace_call_replace(ip, (unsigned long)func);
ret = update_ftrace_func(ip, new);
@@ -295,12 +299,19 @@ int ftrace_int3_handler(struct pt_regs *regs)
return 0;
ip = regs->ip - 1;
- if (!ftrace_location(ip) && !is_ftrace_caller(ip))
- return 0;
-
- regs->ip += MCOUNT_INSN_SIZE - 1;
+ if (ftrace_location(ip)) {
+ int3_emulate_call(regs, ftrace_update_func_call);
+ return 1;
+ } else if
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Tue, 30 Apr 2019 11:33:21 -0700
Linus Torvalds wrote:
> > + "ftrace_emulate_call_update_irqoff:\n\t"
> > + "push %gs:ftrace_bp_call_return\n\t"
> > + "sti\n\t"
> > + "jmp *ftrace_update_func_call\n"
>
> .. and this should then use the "push push sti ret" model instead.
>
> Plus get updated for objtool complaints.
And unfortunately, this blows up on lockdep. Lockdep notices that the
return from the breakpoint handler has interrupts enabled, and will not
enable them in its shadow irqs disabled variable. But then we enabled
them in the trampoline, without telling lockdep and we trigger
something likes this:
[ cut here ]
IRQs not enabled as expected
WARNING: CPU: 2 PID: 0 at kernel/time/tick-sched.c:979
tick_nohz_idle_enter+0x44/0x8c
Modules linked in:
CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.1.0-rc3-test+ #123
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
EIP: tick_nohz_idle_enter+0x44/0x8c
Code: f0 05 00 00 00 75 26 83 b8 c4 05 00 00 00 75 1d 80 3d 5f 0f 43 c1 00 75
14 68 72 74 16 c1 c6 05 5f 0f 43 c1 01 e8 33 d7 f8 ff <0f> 0b 58 fa e8 4e 2c 04
00 bb e0 36 6b c1 64 03 1d 28 81 56 c1 8b
EAX: 001c EBX: ee769f84 ECX: EDX: 0006
ESI: EDI: 0002 EBP: ee769f50 ESP: ee769f48
DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00210292
CR0: 80050033 CR2: CR3: 016c4000 CR4: 001406f0
Call Trace:
do_idle+0x2a/0x1fc
cpu_startup_entry+0x1e/0x20
start_secondary+0x1d3/0x1ec
startup_32_smp+0x164/0x168
I have to fool lockdep with the following:
if (regs->flags & X86_EFLAGS_IF) {
regs->flags &= ~X86_EFLAGS_IF;
regs->ip = (unsigned long) ftrace_emulate_call_irqoff;
/* Tell lockdep here we are enabling interrupts */
trace_hardirqs_on();
} else {
regs->ip = (unsigned long) ftrace_emulate_call_irqon;
}
-- Steve
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Tue, 30 Apr 2019 11:33:21 -0700 Linus Torvalds wrote: > On Tue, Apr 30, 2019 at 10:49 AM Steven Rostedt wrote: > > > > + > > +asm( > > + ".text\n" > > + > > + /* Trampoline for function update with interrupts enabled */ > > + ".global ftrace_emulate_call_irqoff\n" > > + ".type ftrace_emulate_call_irqoff, @function\n" > > + "ftrace_emulate_call_irqoff:\n\t" > > + "push %gs:ftrace_bp_call_return\n\t" > > Well, as mentioned in my original suggestion, this won't work on > 32-bit, or on UP. They have different models for per-cpu data (32-bti > uses %fs, and UP doesn't use a segment override at all). Ah, yeah, I forgot about 32-bit. I could easily make this use fs as well, and for UP, just use a static variable. > > Maybe we just don't care about UP at all for this code, of course. > > And maybe we can make the decision to also make 32-bit just not use > this either - so maybe the code is ok per se, just needs to make sure > it never triggers for the cases that it's not written for.. > > > + "ftrace_emulate_call_update_irqoff:\n\t" > > + "push %gs:ftrace_bp_call_return\n\t" > > + "sti\n\t" > > + "jmp *ftrace_update_func_call\n" > > .. and this should then use the "push push sti ret" model instead. > > Plus get updated for objtool complaints. Yeah, I see that now. Somehow it disappeared when I looked for it after making some other changes. I can update it. > > Anyway, since Andy really likes the entry code change, can we have > that patch in parallel and judge the difference that way? Iirc, that > was x86-64 specific too. Note, I don't think live kernel patching supports 32 bit anyway, so that may not be an issue. Josh, When you come back to the office, can you look into that method? -- Steve
Re: [RFC][PATCH] ftrace/x86: Emulate call function while updating in breakpoint handler
On Tue, Apr 30, 2019 at 10:49 AM Steven Rostedt wrote: > > + > +asm( > + ".text\n" > + > + /* Trampoline for function update with interrupts enabled */ > + ".global ftrace_emulate_call_irqoff\n" > + ".type ftrace_emulate_call_irqoff, @function\n" > + "ftrace_emulate_call_irqoff:\n\t" > + "push %gs:ftrace_bp_call_return\n\t" Well, as mentioned in my original suggestion, this won't work on 32-bit, or on UP. They have different models for per-cpu data (32-bti uses %fs, and UP doesn't use a segment override at all). Maybe we just don't care about UP at all for this code, of course. And maybe we can make the decision to also make 32-bit just not use this either - so maybe the code is ok per se, just needs to make sure it never triggers for the cases that it's not written for.. > + "ftrace_emulate_call_update_irqoff:\n\t" > + "push %gs:ftrace_bp_call_return\n\t" > + "sti\n\t" > + "jmp *ftrace_update_func_call\n" .. and this should then use the "push push sti ret" model instead. Plus get updated for objtool complaints. Anyway, since Andy really likes the entry code change, can we have that patch in parallel and judge the difference that way? Iirc, that was x86-64 specific too. Linus
