[DTrace-devel] [PATCH] Handle unlabeled BPF_NOP instructions

[Kris Van Hees]

On Fri, Dec 11, 2020 at 06:41:04PM -0500, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Although BPF jump instructions take a relative distance, we emit BPF
> code that jumps to 1-based labels, which are relocated to relative
> distances later, during dt_as()'s second pass.
> 
> A special case is BPF_NOP instructions, which are jumps of 0 distance.
> These targets are interpreted by dt_as() as "label 0", which does not
> exist, which leads to incorrect code and possibly BPF verifier failures.
> 
> The problem has not been an issue so far since we have only emitted
> BPF_NOP instructions that are themselves labeled, and that particular
> case has been handled specially (by eliminating these NOPs during
> dt_as's first pass).

I think that the paragraph above are confusing and do not really add
information for the tiny commit.

> Add code to exclude BPF_NOP (jump 0) from jump-target relocation.

I would just write this as a short paragraph explaining that we emit all
branches with a label id stored in the offset field, to be resolved during
assembly.  Since "jmp 0" is used to encode a NOP in BPF, we need to exclude
jumps with offset 0 from the jump-target relocation.

> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>  libdtrace/dt_as.c  | 6 +++++-
>  libdtrace/dt_dis.c | 2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
> index 437a2d86..e33d40a5 100644
> --- a/libdtrace/dt_as.c
> +++ b/libdtrace/dt_as.c
> @@ -330,7 +330,7 @@ fail:
>  
>  	/*
>  	 * Make a second pass through the instructions, relocating each branch
> -	 * label to the index of the final instruction in the buffer and noting
> +	 * label to be a jump relative to the following instruction and noting
>  	 * any other instruction-specific DIFO flags such as dtdo_destructive.

Hm, I do not think this is correct.  Well, the original comment wasn't quite
accurate either of course.  I think it ought to be:

	Make a second pass through the instructions, relocating each branch
	target (a label ID) to the location of the label and noting
	any instruction-specific DIFO flags such as dtdo_destructive.

>  	 */
>  	for (i = 0; i < dp->dtdo_len; i++) {
> @@ -341,6 +341,10 @@ fail:
>  		if (BPF_CLASS(instr.code) != BPF_JMP)
>  			continue;
>  
> +		/* We ignore NOP (jmp 0). */
> +		if (BPF_IS_NOP(instr))
> +			continue;
> +

Good.

>  		/* We ignore function calls and function exits. */
>  		if (op == BPF_CALL || op == BPF_EXIT)
>  			continue;
> diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> index d047bde3..27e98c30 100644
> --- a/libdtrace/dt_dis.c
> +++ b/libdtrace/dt_dis.c
> @@ -346,7 +346,7 @@ dt_dis_jump(const dtrace_difo_t *dp, const char *name, uint_t addr,
>  	    const struct bpf_insn *in, const char *rname, FILE *fp)
>  {
>  	if (in->off == 0)
> -		fprintf(fp, "nop");
> +		fprintf(fp, "nop\n");

Good catch.  Never notived because we never generated NOPs that made it into
the final assembled code.

>  	else {
>  		int	n;
>  
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel