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

[Eugene Loh]

On 01/04/2021 02:18 PM, Kris Van Hees wrote:

> 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.

"Paragraph" or "paragraphs"?  I like the paragraph that explains why 
this wasn't
previously a problem, since it anticipates a natural question. 
Nevertheless, v2
of the patch (coming out in a second) tosses it out since I'm guessing 
that's
what you're suggesting.

>> 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.
>> 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.

Again, not real sure about your feedback, but I've tried to incorporate 
it in v2,
which might be the next email in your inbox.