Re: [PATCH RFC] bpf: update current instruction on patching

[Ilya Leoshkevich <iii@linux.ibm.com>]

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@redhat.com>
> > ---
> >   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