Re: [PATCH V5 5/7] x86/alternative: Batch of patch operations

[Peter Zijlstra]

On Mon, Apr 01, 2019 at 10:58:17AM +0200, Daniel Bristot de Oliveira wrote:
> +	ip = (void *) regs->ip - sizeof(unsigned char);

That one confused me mightily. Does that want to be:

	ip = (void *)regs->ip - LEN_INT3;

or something? Even just a naked 1 would've been less confusing.

>  
> +	/*
> +	 * Skip the binary search if there is a single member in the vector.
> +	 */
> +	if (unlikely(bp_patching.nr_entries == 1))
> +		goto single_poke;
> +
> +	tp = bsearch(ip, bp_patching.vec, bp_patching.nr_entries,
> +		     sizeof(struct text_patch_loc),
> +		     patch_cmp);
> +	if (!tp)
> +		return 0;
> +
> +	/* set up the specified breakpoint detour */
> +	regs->ip = (unsigned long) tp->detour;
>  	return 1;
> +single_poke:
> +	if (ip == bp_patching.vec->addr) {
> +		regs->ip = (unsigned long) bp_patching.vec->detour;
> +		return 1;
> +	}
> +
> +	return 0;

	if (bp_patching.nr_entries > 1) {
		tp = bsearch(ip, bp_patching.vec, bp_patching.nr_entries,
			     sizeof(struct text_patch_loc), patch_cmp);
		if (!tp)
			return 0;
	} else {
		tp = bp_patching.vec;
		if (tp->addr != ip)
			return 0;
	}

	regs->ip = (unsigned long)tp->detour;
	return 1;
>  }
>  NOKPROBE_SYMBOL(poke_int3_handler);

> +void text_poke_bp_batch(struct text_patch_loc *tp, unsigned int nr_entries)
>  {
> +	int patched_all_but_first = 0;
>  	unsigned char int3 = 0xcc;
> +	unsigned int i;
>  
>  	lockdep_assert_held(&text_mutex);
>  
> +	bp_patching.vec = tp;
> +	bp_patching.nr_entries = nr_entries;
> +	bp_patching.in_progress = true;
>  	/*
>  	 * Corresponding read barrier in int3 notifier for making sure the
>  	 * in_progress and handler are correctly ordered wrt. patching.
>  	 */
>  	smp_wmb();
>  
> +	/*
> +	 * First step: add a int3 trap to the address that will be patched.
> +	 */
> +	for (i = 0; i < nr_entries; i++)
> +		text_poke(tp[i].addr, &int3, sizeof(int3));
>  
>  	on_each_cpu(do_sync_core, NULL, 1);
>  
> +	/*
> +	 * Second step: update all but the first byte of the patched range.
> +	 */
> +	for (i = 0; i < nr_entries; i++) {
> +		if (tp[i].len - sizeof(int3) > 0) {
> +			text_poke((char *)tp[i].addr + sizeof(int3),
> +				  (const char *)tp[i].opcode + sizeof(int3),
> +				  tp[i].len - sizeof(int3));
> +			patched_all_but_first++;
> +		}
> +	}
> +
> +	if (patched_all_but_first) {
>  		/*
>  		 * According to Intel, this core syncing is very likely
>  		 * not necessary and we'd be safe even without it. But
> @@ -821,15 +874,52 @@ void *text_poke_bp(void *addr, const void *opcode, size_t len, void *handler)
>  		on_each_cpu(do_sync_core, NULL, 1);
>  	}
>  
> +	/*
> +	 * Third step: replace the first byte (int3) by the first byte of
> +	 * replacing opcode.
> +	 */
> +	for (i = 0; i < nr_entries; i++)
> +		text_poke(tp[i].addr, tp[i].opcode, sizeof(int3));
>  
>  	on_each_cpu(do_sync_core, NULL, 1);
>  	/*
>  	 * sync_core() implies an smp_mb() and orders this store against
>  	 * the writing of the new instruction.
>  	 */
> +	bp_patching.vec = NULL;
> +	bp_patching.nr_entries = 0;
> +	bp_patching.in_progress = false;
> +}