lkml.org 
[lkml]   [2020]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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();
}
\
 
 \ /
  Last update: 2020-03-25 23:07    [W:0.066 / U:13.940 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site