On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
On code patching it may require to update branch destinations if the
code size changed. bpf_adj_delta_to_imm/off increments offset only
if the patched area is after the branch instruction. But it's
possible, that the patched area itself is a branch instruction and
requires destination update.
Could you provide a concrete example and walk us through? I'm probably
missing something but if the patchlet contains a branch instruction, then
it should be 'self-contained'. In the sense that the patchlet is a 'black
box' that replaces 1 insns with n insns but there is no awareness what's
inside these insns and hence no fixup for that inside bpf_patch_insn_data().
So, if we take an existing branch insns from the code, move it into the
patchlet and extend beginning or end, then it feels more like a bug to the
one that called bpf_patch_insn_data(), aka zext code here. Bit puzzled why
this is only seen now, my impression was that Ilya was running s390x the
BPF selftests quite recently?
The problem was triggered by bpf selftest
test_progs -t global_funcs
on s390, where the very first "call" instruction is patched from
verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
The patch includes current instruction to the condition check.
Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@xxxxxxxxxx>
---
kernel/bpf/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ed0b3578867c..b0a9a22491a5 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
s32 delta = end_new - end_old;
s64 imm = insn->imm;
- if (curr < pos && curr + imm + 1 >= end_old)
+ if (curr <= pos && curr + imm + 1 >= end_old)
imm += delta;
else if (curr >= end_new && curr + imm + 1 < end_new)
imm -= delta;
@@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
s32 delta = end_new - end_old;
s32 off = insn->off;
- if (curr < pos && curr + off + 1 >= end_old)
+ if (curr <= pos && curr + off + 1 >= end_old)
off += delta;
else if (curr >= end_new && curr + off + 1 < end_new)
off -= delta;
