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

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

On Thu, 2020-09-03 at 19:13 +0300, Yauheni Kaliuta wrote:
> On Thu, Sep 3, 2020 at 6:10 PM Daniel Borkmann <daniel@iogearbox.net>
> 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().
> 
> The code is
> Disassembly of section classifier/test:
> 
> 0000000000000000 test_cls:
>        0:       85 01 00 00 ff ff ff ff call -1
>                 0000000000000000:  R_BPF_64_32  f7
>        1:       95 00 00 00 00 00 00 00 exit
> 0000000000000000 f1:
>        0:       61 01 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
>        1:       95 00 00 00 00 00 00 00 exit
> [...]
> 00000000000000a8 f7:
>       21:       85 01 00 00 ff ff ff ff call -1
>                 00000000000000a8:  R_BPF_64_32  f6
>       22:       95 00 00 00 00 00 00 00 exit
> 
> Before the patching the bytecode is:
> 
> 00000000: 85 01 00 00 00 00 00 16 95 00 00 00 00 00 00 00
> 00000010: 61 01 00 00 00 00 00 00 95 00 00 00 00 00 00 00
> [...]
> 
> It becomes
> 
> 
> 00000000: 85 01 00 00 00 00 00 2b bc 00 00 00 00 00 00 01
> 00000010: 95 00 00 00 00 00 00 00 61 01 00 80 00 00 00 00
> 
> at the end, the 2b offset is incorrect.
> 
> With that zext patching the code "85 01 00 00 00 00 00 16" is
> replaced
> with "85 01 00 00 00 00 00 16 bc 00 00 00 00 00 00 01", 0x16 is not
> changed, but the real offset has changed.
> 
> > 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?
> 
> I have not investigated why on s390 it is zext'ed, but on x86 not,
> it's related to the size of the register when it returns 32bit value.
> There may be a bug there as well.
> 
> I did think a bit more on your words, making the zext patching code
> specially check jumps and adjust the offset in the patchlet looks
> more
> correct. But duplicates the existing code. I should spend more time
> on
> that.

I guess copying the existing insn into the patchlet was introduced
because there is nothing like bpf_insert_insns()? I.e. we can replace
an existing insn with a patchlet, but cannot append anything to it.
Would introducing such function solve this problem?