Re: [PATCH v4 bpf-next 2/9] bpf: Adjust BPF_JMP that jumps to the 1st insn of the prologue

[Martin KaFai Lau <martin.lau@linux.dev>]

On 8/28/24 11:43 AM, Alexei Starovoitov wrote:
> On Wed, Aug 28, 2024 at 10:44 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>
>>>> -       for (i = 0; i < insn_cnt; i++, insn++) {
>>>> +       for (i = skip_cnt; i < insn_cnt; i++, insn++) {
>>>
>>> Do we really need to add this argument?
>>>
>>>> -               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
>>>> +               WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));
>>>
>>> We can always do for (i = delta; ...
>>>
>>> The above case of skip_cnt == 0 is lucky to work this way.
>>> It would be less surprising to skip all insns in the patch.
>>> Maybe I'm missing something.
>>
>> For subprog_start case, tgt_idx (where the patch started) may not be 0. How
>> about this:
>>
>>          for (i = 0; i < insn_cnt; i++, insn++) {
>>                  if (tgt_idx <= i && i < tgt_idx + delta)
>>                          continue;
> 
> Yeah. Right. Same idea, but certainly your way is more correct
> instead of my buggy proposal.
> 
> In that sense the "for (i = skip_cnt" approach
> is also a bit buggy, if tgt_idx != 0.

Yep. Adding skip_cnt like this patch 2 is not right. I didn't think hard enough 
what to do with the existing adjust_jmp_off().

I will remove skip_cnt in the next respin.

Thanks for the review!