Messages in this thread Patch in this message | | | Date | Wed, 25 Mar 2020 23:07:00 +0100 | From | Peter Zijlstra <> | Subject | Re: [RESEND][PATCH v3 14/17] static_call: Add static_cond_call() |
| |
On Wed, Mar 25, 2020 at 01:52:00PM -0700, Linus Torvalds wrote: > On Wed, Mar 25, 2020 at 12:35 PM <hpa@zytor.com> wrote: > > > > "movl $0,%eax" is five bytes, the same length as a call. Doesn't work for a tailcall, still, although if the sequence: > > > > jmp tailcall > > retq > > > > ... can be generated at the tailcall site then the jmp can get patched out. > > No, the problem is literally that the whole approach depends on the > compiler just generating normal code for the static calls. > > And the tailcall is the only interesting case. The normal call thing > can be trivially just a single instruction (a mov like you say, but > also easily just a xor padded with prefixes).
So I got the text poking bit written, and that turned out to be the simple part :/ Find below.
Then we can do:
#define static_void_call(name) if (STATIC_CALL_NAME(name).func) \ ((typeof(STATIC_CALL_TRAMP(name))*)STATIC_CALL_NAME(name).func)
Which works, as evidenced by if being the current static_cond_call(), but it is non-optimal code-gen for the case where func will never be NULL, and also there is no way to write a !void version of the same.
The best I can come up with is something like:
#define static_call(name, args...) ({ \ typeof(STATIC_CALL_TRAMP(name)(args)) __ret = (typeof(STATIC_CALL_TRAMP(name)(args)))0; \ if (STATIC_CALL_NAME(name).func) \ __ret = ((typeof(STATIC_CALL_TRAMP(name))*)STATIC_CALL_NAME(name).func)(args); \ __ret; })
Which has a different (and IMO less natural) syntax.
That then brings us to the HAVE_STATIC_CALL variant; there we need to somehow make the void vs !void thing persistent, and there I ran out of ideas.
Initially I figured we could do something like:
#define annotate_void_call() ({ \ asm volatile("%c0:\n\t" \ ".pushsection .discard.void_call\n\t" \ ".long %c0b - .\n\t" \ ".popsection\n\t" : : "i" (__COUNTER__)); \ })
#define static_void_call(name) \ annotate_void_call(); \ STATIC_CALL_TRAMP(name)
But that doesn't actually work for something like:
static_void_call(foo)(static_call(bar)());
Where the argument setup of the call, include another static call. Arguably this is quite insane, and we could just say: "don't-do-that-then", but it does show how fragile this is.
Anyway, let me ponder this a little more... brain is starting to give out anyway. More tomorrow.
--- diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index 346c98d5261e..240996338f66 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -947,6 +947,7 @@ struct text_poke_loc { s32 rel32; u8 opcode; const u8 text[POKE_MAX_OPCODE_SIZE]; + u8 multi; };
struct bp_patching_desc { @@ -1103,8 +1104,8 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries .refs = ATOMIC_INIT(1), }; unsigned char int3 = INT3_INSN_OPCODE; + int do_sync, do_multi = 0; unsigned int i; - int do_sync;
lockdep_assert_held(&text_mutex);
@@ -1119,11 +1120,24 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries /* * First step: add a int3 trap to the address that will be patched. */ - for (i = 0; i < nr_entries; i++) + for (i = 0; i < nr_entries; i++) { text_poke(text_poke_addr(&tp[i]), &int3, INT3_INSN_SIZE); + do_multi |= tp[i].multi; + }
text_poke_sync();
+ if (do_multi) { + /* + * In case the 'old' text consisted of multiple instructions + * we need to wait for an rcu_tasks quiescence period to ensure + * all potentially preempted tasks have normally scheduled. + * This ensures no tasks still have their instruction pointer + * pointed at what will become the middle of an instruction. + */ + synchronize_rcu_tasks(); + } + /* * Second step: update all but the first byte of the patched range. */ @@ -1176,10 +1190,28 @@ static void text_poke_loc_init(struct text_poke_loc *tp, void *addr, { struct insn insn;
+ /* + * Determine if the 'old' text at @addr consists of multiple + * instructions. Make an exception for INT3 and RET, since + * they don't (necessarily) continue to execute the following + * instructions. + */ + kernel_insn_init(&insn, addr, MAX_INSN_SIZE); + insn_get_length(&insn); + tp.multi = (insn.legnth < len) && + (insn.opcode.bytes[0] != RET_INSN_OPCODE || + insn.opcode.bytes[0] != INT3_INSN_OPCODE); + + /* + * Copy the 'new' text into the text_poke vector. + */ memcpy((void *)tp->text, opcode, len); if (!emulate) emulate = opcode;
+ /* + * Decode the instruction poke_int3_handler() needs to emulate. + */ kernel_insn_init(&insn, emulate, MAX_INSN_SIZE); insn_get_length(&insn);
diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c index 1e82e2486e76..2055e2d3674d 100644 --- a/arch/x86/kernel/static_call.c +++ b/arch/x86/kernel/static_call.c @@ -9,8 +9,13 @@ enum insn_type { nop = 1, /* site cond-call */ jmp = 2, /* tramp / site tail-call */ ret = 3, /* tramp / site cond-tail-call */ + null = 4, + null_ret = 5, };
+static const u8 null_insn[5] = { 0xb8, 0x00, 0x00, 0x00, 0x00 }; /* movl $0, %eax */ +static const u8 null_ret_insn[5] = { 0x31, 0xc0, 0xc3, 0x90, 0x90 }; /* xorl %eax, %eax; ret; nop; nop; */ + static void __ref __static_call_transform(void *insn, enum insn_type type, void *func) { int size = CALL_INSN_SIZE; @@ -34,6 +39,14 @@ static void __ref __static_call_transform(void *insn, enum insn_type type, void size = RET_INSN_SIZE; break;
+ case null: + code = null_insi; + break; + + case null_ret: + code = null_ret_insn; + break; + default: /* GCC is a moron -- it figures @code can be uninitialized below */ BUG(); }
| |