Re: [RESEND][PATCH v3 14/17] static_call: Add static_cond_call()

[Peter Zijlstra <peterz@infradead.org>]

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();
 	}