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 9:48 AM, Alexei Starovoitov wrote:
> On Tue, Aug 27, 2024 at 12:53 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> The next patch will add a ctx ptr saving instruction
>> "(r1 = *(u64 *)(r10 -8)" at the beginning for the main prog
>> when there is an epilogue patch (by the .gen_epilogue() verifier
>> ops added in the next patch).
>>
>> There is one corner case if the bpf prog has a BPF_JMP that jumps
>> to the 1st instruction. It needs an adjustment such that
>> those BPF_JMP instructions won't jump to the newly added
>> ctx saving instruction.
>> The commit 5337ac4c9b80 ("bpf: Fix the corner case with may_goto and jump to the 1st insn.")
>> has the details on this case.
>>
>> Note that the jump back to 1st instruction is not limited to the
>> ctx ptr saving instruction. The same also applies to the prologue.
>> A later test, pro_epilogue_goto_start.c, has a test for the prologue
>> only case.
>>
>> Thus, this patch does one adjustment after gen_prologue and
>> the future ctx ptr saving. It is done by
>> adjust_jmp_off(env->prog, 0, delta) where delta has the total
>> number of instructions in the prologue and
>> the future ctx ptr saving instruction.
>>
>> The adjust_jmp_off(env->prog, 0, delta) assumes that the
>> prologue does not have a goto 1st instruction itself.
>> To accommodate the prologue might have a goto 1st insn itself,
>> adjust_jmp_off() needs to skip the prologue instructions. This patch
>> adds a skip_cnt argument to the adjust_jmp_off(). The skip_cnt is the
>> number of instructions at the beginning that does not need adjustment.
>> adjust_jmp_off(prog, 0, delta, delta) is used in this patch.
>>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
>>   kernel/bpf/verifier.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index b408692a12d7..8714b83c5fb8 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19277,14 +19277,14 @@ static struct bpf_prog *bpf_patch_insn_data(struct bpf_verifier_env *env, u32 of
>>    * For all jmp insns in a given 'prog' that point to 'tgt_idx' insn adjust the
>>    * jump offset by 'delta'.
>>    */
>> -static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta)
>> +static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta, u32 skip_cnt)
>>   {
>> -       struct bpf_insn *insn = prog->insnsi;
>> +       struct bpf_insn *insn = prog->insnsi + skip_cnt;
>>          u32 insn_cnt = prog->len, i;
>>          s32 imm;
>>          s16 off;
>>
>> -       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;

		/* ... */
	}