Re: [PATCH v5 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/29/24 5:47 PM, Eduard Zingerman wrote:
> On Thu, 2024-08-29 at 14:08 -0700, Martin KaFai Lau wrote:
> 
> [...]
> 
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 261849384ea8..03e974129c05 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19286,6 +19286,9 @@ static int adjust_jmp_off(struct bpf_prog *prog, u32 tgt_idx, u32 delta)
>>   	for (i = 0; i < insn_cnt; i++, insn++) {
>>   		u8 code = insn->code;
>>   
>> +		if (tgt_idx <= i && i < tgt_idx + delta)
>> +			continue;
>> +
>>   		if ((BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) ||
>>   		    BPF_OP(code) == BPF_CALL || BPF_OP(code) == BPF_EXIT)
>>   			continue;
>> @@ -19704,6 +19707,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>>   		}
>>   	}
>>   
>> +	if (delta)
>> +		WARN_ON(adjust_jmp_off(env->prog, 0, delta));
> 
> Just noticed this.
> Suppose prologue is three instructions long and no epilogue,
> then cnt == 3 and delta == 2, adjust_jmp_off() would skip instructions
> in range [0..2), while inserted instructions range is [0..2].
> So, this would work only if the last statement in the prologue/epilogue
> generator is:
> 
> 	*insn++ = prog->insnsi[0];
> 
> which seems to be true for prologue generators in the tree,
> but looks a bit unintuitive...

Right, it is the current requirement/setup for the existing gen_prologue. It 
should be obvious to spot if the gen_prologue does not do this and more unlikely 
also somehow needs to jump back to itself.

Thanks for looking at the patches!

> 
>> +
>>   	if (bpf_prog_is_offloaded(env->prog->aux))
>>   		return 0;
>>   
> 
>