[PATCH v2 2/2] PR 34558: bpf: don't mis-assemble `gotol' with signed offset

[Vineet Gupta]

On 9/4/26 17:30, Jose E. Marchesi wrote:
>>> In any case, wouldn't simply reordering the opcodes table like below fix
>>> the existing broken issues without having to add special casing to the
>>> generic template-based parser?  Something like this passes the new test:
>>>
>>> diff --git a/opcodes/bpf-opc.c b/opcodes/bpf-opc.c
>>> index 4babf6e8649..df0b90ffb90 100644
>>> --- a/opcodes/bpf-opc.c
>>> +++ b/opcodes/bpf-opc.c
>>> @@ -247,6 +247,10 @@ const struct bpf_opcode bpf_opcodes[] =
>>>      {BPF_INSN_STXDWI, "stdw%W[ %dr %o16 ] , %i32", "* ( u64 * ) ( %dr %o16 ) = %i32",
>>>       BPF_V1, BPF_CODE, BPF_CLASS_ST|BPF_SIZE_DW|BPF_MODE_MEM},
>>>    +  /* 32-bit jump-always.  */
>>> +  {BPF_INSN_JAL, "jal%W%d32", "gotol%w%d32",
>>> +   BPF_V4, BPF_CODE, BPF_CLASS_JMP32|BPF_CODE_JA|BPF_SRC_K},
>>> +
>>>      /* Compare-and-jump instructions (reg OP reg).  */
>>>      {BPF_INSN_JAR, "ja%W%d16", "goto%w%d16",
>>>       BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JA|BPF_SRC_K},
>>> @@ -303,10 +307,6 @@ const struct bpf_opcode bpf_opcodes[] =
>>>      {BPF_INSN_JNEI, "jne%W%dr , %i32 , %d16", "if%w%dr != %i32%wgoto%w%d16",
>>>       BPF_V1, BPF_CODE, BPF_CLASS_JMP|BPF_CODE_JNE|BPF_SRC_K},
>>>    -  /* 32-bit jump-always.  */
>>> -  {BPF_INSN_JAL, "jal%W%d32", "gotol%w%d32",
>>> -   BPF_V4, BPF_CODE, BPF_CLASS_JMP32|BPF_CODE_JA|BPF_SRC_K},
>>> -
>>>      /* 32-bit compare-and-jump instructions (reg OP reg).  */
>>>      {BPF_INSN_JEQ32R, "jeq32%W%dr , %sr , %d16", "if%w%dw == %sw%wgoto%w%d16",
>>>       BPF_V3, BPF_CODE, BPF_CLASS_JMP32|BPF_CODE_JEQ|BPF_SRC_X},
>> I gave this a try and indeed it fixes the issue at hand, but
>> mis-compiles two other cases
>>
>> 1. For -misa-spec=v1 build gotol +1 assembles to goto 0 + 'l' because
>> JAL is not available and JAR wins regardless of the order (md_assemble
>> skips the opcodes not covered by the isa-spec).
>> 2. conditional jumps such as if%w%dr > %sr%wgoto%w%d16 embeds the same
>> goto%w prefix, so if r1 > r2 gotol +1 leads to same issue.
> It would be nice to have test cases for these.

Good point, will do.

>> I understand the need to not add special casings to the parser so I'm
>> open to trying any other solutions to fix this. I have an orthogonal
>> patch which improves the diagnostics for general gotol related
>> miscompiles.
> What about introducing a new mark (such as %t, for tab) to explicitly
> match the end of a "mnemonic" in the template:
>
>     "gotol%t%w%d32"
>     "goto%t%w%d32"
>
> Where %t basically implements the flexible spacing rules, emitting an
> error if the next character in the stream doesn't conform to these
> rules.  This way %w will stay to simply be zero or more whitespaces
> regardless of the context where it appears.  Note this still requires
> the usual ordering of the entries by mnemonic.

Sound like a nice idea indeed. Let me give that a spin.

Thx,
-Vineet