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?
