[DTrace-devel] okay to emit unlabeled BPF_NOP()?

[Kris Van Hees]

On Thu, Dec 10, 2020 at 12:15:28PM -0800, Eugene Loh wrote:
> It appears that we can handle BPF_NOP() instructions only if they're 
> labeled.  Is that okay?  Known limitation?  Should we put a safeguard in 
> for unlabeled BPF_NOP() instructions?

That's a bug...

> BPF_NOP() instructions are encoded as "jump 0" instructions.  We 
> (dt_cg.c) encode other jump instructions as "jump label", where label is 
> a 1-based integer.  Then, during the "second pass" of dt_as() we 
> relocate jump targets, translating from labels to offsets (relative to 
> the immediately following instruction).  This means that a BPF_NOP() is 
> interpreted as a jump to label 0, which does not exist, and bad code is 
> produced, possibly causing the BPF verifier to fail.
> 
> This is not a big problem right now since we only generate BPF_NOP() 
> instructions when they are labeled, and there is special code to handle 
> that case.  But I'm wondering:
> 
> *)  Are we okay with that limitation?  (We cannot emit unlabeled 
> BPF_NOP() instructions.)
> 
> *)  Would it be okay to insert a safeguard -- say, in dt_irlist_append() 
> or dt_as() -- against unlabeled BPF_NOP() instructions?
> 
> Admittedly, it would seem ideal never to emit useless code. 
> Nevertheless, it's easy enough to insert a safeguard against some 
> unforeseen situation.

We should just make it work.  The following patch will do that as far as I
can see.  It ought to be correct because labels are always numbered from 1,
so a "jmp 0" instruction cannot be a valid jump to a label.  Note that the
verifier will strip these NOPs from the instruction stream anyhow.

diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
index 437a2d86..c8ef0c17 100644
--- a/libdtrace/dt_as.c
+++ b/libdtrace/dt_as.c
@@ -341,6 +341,10 @@ fail:
                if (BPF_CLASS(instr.code) != BPF_JMP)
                        continue;
 
+               /* We ignore NOP (jmp 0). */
+               if (BPF_IS_NOP(instr))
+                       continue;
+
                /* We ignore function calls and function exits. */
                if (op == BPF_CALL || op == BPF_EXIT)
                        continue;