On Fri, 2020-09-04 at 17:17 +0200, Ilya Leoshkevich wrote:
> 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.
Turns out it has been failing for quite some time. I stopped at
commit 23ad04669f81f958e9a4121b0266228d2eb3c357 (HEAD)
Author: Matteo Croce <mcroce@xxxxxxxxxx>
Date: Mon May 11 13:32:34 2020 +0200
samples: bpf: Fix build error
because I can't build earlier commits on my new Debian 10 VM due to
linker errors. The asm in the failing test is quite simple, so it's
unlikely that LLVM ouput has changed in the meantime. So I must have
simply missed it. Sorry!
I'll double check whether there have been more unnoticed failures.
