On Thu, 2020-09-03 at 17:10 +0200, Daniel Borkmann wrote: > 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; > > Hi! Last time I ran selftests against bpf-next ~1 month ago, and I don't remember seeing any test_progs failures. Now I tried it again, and I see the same problem as Yauheni. So this must be relatively new - I'll try to bisect the commit that caused this. Best regards, Ilya
