Hi! On Mon, Sep 7, 2020 at 7:14 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > > On Thu, 2020-09-03 at 19:13 +0300, Yauheni Kaliuta wrote: > > On Thu, Sep 3, 2020 at 6:10 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> > > 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? Sounds as a plan! -- WBR, Yauheni
