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

Commit Message

Martin KaFai Lau Aug. 29, 2024, 9:08 p.m. UTC

From: Martin KaFai Lau <[e-mail]>

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,
this patch changes the adjust_jmp_off() to skip considering
the instructions between [tgt_idx, tgt_idx + delta).

Signed-off-by: Martin KaFai Lau <[e-mail]>
---
 kernel/bpf/verifier.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Eduard Zingerman Aug. 30, 2024, 12:47 a.m. UTC | #1

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...

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

Martin KaFai Lau Aug. 30, 2024, 1:10 a.m. UTC | #2

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;
>>   
> 
>

Patch

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));
+
 	if (bpf_prog_is_offloaded(env->prog->aux))
 		return 0;