| Message ID | 20200903140542.156624-1-yauheni.kaliuta@redhat.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RFC] bpf: update current instruction on patching | expand |
Breaks #76/u bounds check mixed 32bit and 64bit arithmetic. test2 On Thu, Sep 3, 2020 at 6:02 PM Yauheni Kaliuta <yauheni.kaliuta@redhat.com> 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. > > 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; > -- > 2.26.2 >
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; >
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. > > > 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; > > >
On Thu, Sep 3, 2020 at 7:13 PM Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote: [...] > > 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. Nevermind, I missed that for x86 it's for 32 bits only. So, expected. [...]
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
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@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. Turns out it has been failing for quite some time. I stopped at commit 23ad04669f81f958e9a4121b0266228d2eb3c357 (HEAD) Author: Matteo Croce <mcroce@redhat.com> 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.
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?
Hi! On Mon, Sep 7, 2020 at 7:14 PM Ilya Leoshkevich <iii@linux.ibm.com> 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@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? Sounds as a plan!
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;
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. 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(-)